Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notif: Use associated account as initial account; if opened from background #1240

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions lib/notifications/display.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ import '../log.dart';
import '../model/binding.dart';
import '../model/localizations.dart';
import '../model/narrow.dart';
import '../model/store.dart';
import '../widgets/app.dart';
import '../widgets/color.dart';
import '../widgets/dialog.dart';
import '../widgets/message_list.dart';
import '../widgets/page.dart';
import '../widgets/store.dart';
import '../widgets/theme.dart';

Expand Down Expand Up @@ -456,36 +456,58 @@ class NotificationDisplayManager {

static String _personKey(Uri realmUrl, int userId) => "$realmUrl|$userId";

/// Provides the route and the account ID by parsing the notification URL.
///
/// The URL must have been generated using [NotificationOpenPayload.buildUrl]
/// while creating the notification.
///
/// Returns null if the associated account is not found in the global
/// store.
static ({Route<void> route, int accountId})? routeForNotification({
required GlobalStore globalStore,
required Uri url,
}) {
assert(debugLog('got notif: url: $url'));
assert(url.scheme == 'zulip' && url.host == 'notification');
final payload = NotificationOpenPayload.parseUrl(url);

final account = globalStore.accounts.firstWhereOrNull((account) =>
account.realmUrl == payload.realmUrl && account.userId == payload.userId);
if (account == null) return null;

final route = MessageListPage.buildRoute(
accountId: account.id,
// TODO(#82): Open at specific message, not just conversation
narrow: payload.narrow);
return (route: route, accountId: account.id);
}

/// Navigates to the [MessageListPage] of the specific conversation
/// given the `zulip://notification/…` Android intent data URL,
/// generated with [NotificationOpenPayload.buildUrl] while creating
/// the notification.
static Future<void> navigateForNotification(Uri url) async {
assert(debugLog('opened notif: url: $url'));

assert(url.scheme == 'zulip' && url.host == 'notification');
final payload = NotificationOpenPayload.parseUrl(url);

NavigatorState navigator = await ZulipApp.navigator;
final context = navigator.context;
assert(context.mounted);
if (!context.mounted) return; // TODO(linter): this is impossible as there's no actual async gap, but the use_build_context_synchronously lint doesn't see that

final zulipLocalizations = ZulipLocalizations.of(context);
final globalStore = GlobalStoreWidget.of(context);
final account = globalStore.accounts.firstWhereOrNull((account) =>
account.realmUrl == payload.realmUrl && account.userId == payload.userId);
if (account == null) { // TODO(log)

final notificationResult =
routeForNotification(globalStore: globalStore, url: url);
if (notificationResult == null) { // TODO(log)
showErrorDialog(context: context,
title: zulipLocalizations.errorNotificationOpenTitle,
message: zulipLocalizations.errorNotificationOpenAccountMissing);
return;
}

// TODO(nav): Better interact with existing nav stack on notif open
unawaited(navigator.push(MaterialAccountWidgetRoute<void>(accountId: account.id,
// TODO(#82): Open at specific message, not just conversation
page: MessageListPage(initNarrow: payload.narrow))));
unawaited(navigator.push(notificationResult.route));
}

static Future<Uint8List?> _fetchBitmap(Uri url) async {
Expand Down
60 changes: 44 additions & 16 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -152,18 +152,56 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
return super.didPushRouteInformation(routeInformation);
}

Future<void> _handleInitialRoute() async {
final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName);
if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) {
await NotificationDisplayManager.navigateForNotification(initialRouteUrl);
InitialRouteListFactory _handleGenerateInitialRoutes(BuildContext context) {
final globalStore = GlobalStoreWidget.of(context);

void showNotificationErrorDialog() async {
final navigator = await ZulipApp.navigator;
final navigatorContext = navigator.context;
assert(navigatorContext.mounted);
// TODO(linter): this is impossible as there's no actual async gap, but
// the use_build_context_synchronously lint doesn't see that.
if (!navigatorContext.mounted) return;

final zulipLocalizations = ZulipLocalizations.of(navigatorContext);
showErrorDialog(context: navigatorContext,
title: zulipLocalizations.errorNotificationOpenTitle,
message: zulipLocalizations.errorNotificationOpenAccountMissing);
}

return (String initialRoute) {
final initialRouteUrl = Uri.parse(initialRoute);
if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) {
final notificationResult = NotificationDisplayManager.routeForNotification(
globalStore: globalStore,
url: initialRouteUrl);

if (notificationResult != null) {
return [
HomePage.buildRoute(accountId: notificationResult.accountId),
notificationResult.route,
];
} else {
showNotificationErrorDialog();
// Fallthrough to show default route below.
}
}

// TODO(#524) choose initial account as last one used
final initialAccountId = globalStore.accounts.firstOrNull?.id;
return [
if (initialAccountId == null)
MaterialWidgetRoute(page: const ChooseAccountPage())
else
HomePage.buildRoute(accountId: initialAccountId),
];
};
}

@override
void initState() {
super.initState();
WidgetsBinding.instance.addObserver(this);
_handleInitialRoute();
}

@override
Expand All @@ -177,9 +215,6 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
final themeData = zulipThemeData(context);
return GlobalStoreWidget(
child: Builder(builder: (context) {
final globalStore = GlobalStoreWidget.of(context);
// TODO(#524) choose initial account as last one used
final initialAccountId = globalStore.accounts.firstOrNull?.id;
return MaterialApp(
title: 'Zulip',
localizationsDelegates: ZulipLocalizations.localizationsDelegates,
Expand All @@ -206,14 +241,7 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
// like [Navigator.push], never mere names as with [Navigator.pushNamed].
onGenerateRoute: (_) => null,

onGenerateInitialRoutes: (_) {
return [
if (initialAccountId == null)
MaterialWidgetRoute(page: const ChooseAccountPage())
else
HomePage.buildRoute(accountId: initialAccountId),
];
});
onGenerateInitialRoutes: _handleGenerateInitialRoutes(context));
}));
}
}
Expand Down
50 changes: 45 additions & 5 deletions test/notifications/display_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,7 @@ void main() {

testWidgets('at app launch', (tester) async {
addTearDown(testBinding.reset);
addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be best in its own commit, since it's about an unrelated test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this and the formating change in this test to a separate commit.

// Set up a value for `PlatformDispatcher.defaultRouteName` to return,
// for determining the intial route.
final account = eg.selfAccount;
Expand All @@ -1079,11 +1080,11 @@ void main() {
realmUrl: data.realmUrl,
userId: data.userId,
narrow: switch (data.recipient) {
FcmMessageChannelRecipient(:var streamId, :var topic) =>
TopicNarrow(streamId, topic),
FcmMessageDmRecipient(:var allRecipientIds) =>
DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
}).buildUrl();
FcmMessageChannelRecipient(:var streamId, :var topic) =>
TopicNarrow(streamId, topic),
FcmMessageDmRecipient(:var allRecipientIds) =>
DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
}).buildUrl();
tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();

// Now start the app.
Expand All @@ -1096,6 +1097,45 @@ void main() {
takeStartingRoutes();
matchesNavigation(check(pushedRoutes).single, account, message);
});

testWidgets('uses associated account as initial account; if initial route', (tester) async {
addTearDown(testBinding.reset);
addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);

final accountA = eg.selfAccount;
final accountB = eg.otherAccount;
final message = eg.streamMessage();
final data = messageFcmMessage(message, account: accountB);
await testBinding.globalStore.add(accountA, eg.initialSnapshot());
await testBinding.globalStore.add(accountB, eg.initialSnapshot());

final intentDataUrl = NotificationOpenPayload(
realmUrl: data.realmUrl,
userId: data.userId,
narrow: switch (data.recipient) {
FcmMessageChannelRecipient(:var streamId, :var topic) =>
TopicNarrow(streamId, topic),
FcmMessageDmRecipient(:var allRecipientIds) =>
DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
}).buildUrl();
tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it needs a corresponding teardown, with clearDefaultRouteNameTestValue. (Same with the existing place we set this, actually)


await prepare(tester, early: true);
check(pushedRoutes).isEmpty(); // GlobalStore hasn't loaded yet

await tester.pump();
check(pushedRoutes).deepEquals(<Condition<Object?>>[
(it) => it.isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountB.id)
..page.isA<HomePage>(),
(it) => it.isA<MaterialAccountWidgetRoute>()
..accountId.equals(accountB.id)
..page.isA<MessageListPage>()
.initNarrow.equals(SendableNarrow.ofMessage(message,
selfUserId: accountB.userId))
]);
pushedRoutes.clear();
});
});

group('NotificationOpenPayload', () {
Expand Down