From 6765f66c3faddb6dfddacafd120375e340ca7e63 Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Thu, 2 Jan 2025 21:29:15 +0530 Subject: [PATCH 1/7] notif test: Clean up `TestPlatformDispatcher.defaultRouteNameTestValue` --- test/notifications/display_test.dart | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 32d8254d6d..b72e97ed01 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -1112,11 +1112,12 @@ 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(); + addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString(); // Now start the app. From 05628cab9cda744068d911560288de6513155fe7 Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Mon, 27 Jan 2025 14:06:03 +0530 Subject: [PATCH 2/7] app: Query initial-account-id while handling initial routes This avoids a potential race if the queried account is logged out between the invocation of this Builder callback and `MaterialApp.onGenerateInitialRoutes` (if such a race is possible). --- lib/widgets/app.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index f20e9f8fcc..f248a1a600 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -178,8 +178,6 @@ class _ZulipAppState extends State with WidgetsBindingObserver { 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( onGenerateTitle: (BuildContext context) { return ZulipLocalizations.of(context).zulipAppTitle; @@ -209,6 +207,8 @@ class _ZulipAppState extends State with WidgetsBindingObserver { onGenerateRoute: (_) => null, onGenerateInitialRoutes: (_) { + // TODO(#524) choose initial account as last one used + final initialAccountId = globalStore.accounts.firstOrNull?.id; return [ if (initialAccountId == null) MaterialWidgetRoute(page: const ChooseAccountPage()) From 4ece284be147c4b20e371c263a3eaf171d1df601 Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Fri, 7 Feb 2025 18:02:18 +0530 Subject: [PATCH 3/7] app [nfc]: Reorder _ZulipAppState methods --- lib/widgets/app.dart | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index f248a1a600..8d32bac9e0 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -139,26 +139,6 @@ class ZulipApp extends StatefulWidget { } class _ZulipAppState extends State with WidgetsBindingObserver { - @override - Future didPushRouteInformation(routeInformation) async { - switch (routeInformation.uri) { - case Uri(scheme: 'zulip', host: 'login') && var url: - await LoginPage.handleWebAuthUrl(url); - return true; - case Uri(scheme: 'zulip', host: 'notification') && var url: - await NotificationDisplayManager.navigateForNotification(url); - return true; - } - return super.didPushRouteInformation(routeInformation); - } - - Future _handleInitialRoute() async { - final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName); - if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { - await NotificationDisplayManager.navigateForNotification(initialRouteUrl); - } - } - @override void initState() { super.initState(); @@ -172,6 +152,26 @@ class _ZulipAppState extends State with WidgetsBindingObserver { super.dispose(); } + Future _handleInitialRoute() async { + final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName); + if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { + await NotificationDisplayManager.navigateForNotification(initialRouteUrl); + } + } + + @override + Future didPushRouteInformation(routeInformation) async { + switch (routeInformation.uri) { + case Uri(scheme: 'zulip', host: 'login') && var url: + await LoginPage.handleWebAuthUrl(url); + return true; + case Uri(scheme: 'zulip', host: 'notification') && var url: + await NotificationDisplayManager.navigateForNotification(url); + return true; + } + return super.didPushRouteInformation(routeInformation); + } + @override Widget build(BuildContext context) { final themeData = zulipThemeData(context); From 6d7c751ea23161ca203ee8eb620b9629084590e1 Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Thu, 2 Jan 2025 21:36:35 +0530 Subject: [PATCH 4/7] app: Pull out `_handleGenerateInitialRoutes` And remove the use of Builder widget, which is unncessary after this refactor. --- lib/widgets/app.dart | 87 +++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 41 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 8d32bac9e0..b6786ea3d1 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -152,6 +152,23 @@ class _ZulipAppState extends State with WidgetsBindingObserver { super.dispose(); } + List> _handleGenerateInitialRoutes(String initialRoute) { + // The `_ZulipAppState.context` lacks the required ancestors. Instead + // we use the Navigator which should be available when this callback is + // called and it's context should have the required ancestors. + final context = ZulipApp.navigatorKey.currentContext!; + final globalStore = GlobalStoreWidget.of(context); + + // 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), + ]; + } + Future _handleInitialRoute() async { final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName); if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { @@ -176,47 +193,35 @@ class _ZulipAppState extends State with WidgetsBindingObserver { Widget build(BuildContext context) { final themeData = zulipThemeData(context); return GlobalStoreWidget( - child: Builder(builder: (context) { - final globalStore = GlobalStoreWidget.of(context); - return MaterialApp( - onGenerateTitle: (BuildContext context) { - return ZulipLocalizations.of(context).zulipAppTitle; - }, - localizationsDelegates: ZulipLocalizations.localizationsDelegates, - supportedLocales: ZulipLocalizations.supportedLocales, - theme: themeData, - - navigatorKey: ZulipApp.navigatorKey, - navigatorObservers: widget.navigatorObservers ?? const [], - builder: (BuildContext context, Widget? child) { - if (!ZulipApp.ready.value) { - SchedulerBinding.instance.addPostFrameCallback( - (_) => widget._declareReady()); - } - GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); - return child!; - }, - - // We use onGenerateInitialRoutes for the real work of specifying the - // initial nav state. To do that we need [MaterialApp] to decide to - // build a [Navigator]... which means specifying either `home`, `routes`, - // `onGenerateRoute`, or `onUnknownRoute`. Make it `onGenerateRoute`. - // It never actually gets called, though: `onGenerateInitialRoutes` - // handles startup, and then we always push whole routes with methods - // like [Navigator.push], never mere names as with [Navigator.pushNamed]. - onGenerateRoute: (_) => null, - - onGenerateInitialRoutes: (_) { - // 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), - ]; - }); - })); + child: MaterialApp( + onGenerateTitle: (BuildContext context) { + return ZulipLocalizations.of(context).zulipAppTitle; + }, + localizationsDelegates: ZulipLocalizations.localizationsDelegates, + supportedLocales: ZulipLocalizations.supportedLocales, + theme: themeData, + + navigatorKey: ZulipApp.navigatorKey, + navigatorObservers: widget.navigatorObservers ?? const [], + builder: (BuildContext context, Widget? child) { + if (!ZulipApp.ready.value) { + SchedulerBinding.instance.addPostFrameCallback( + (_) => widget._declareReady()); + } + GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); + return child!; + }, + + // We use onGenerateInitialRoutes for the real work of specifying the + // initial nav state. To do that we need [MaterialApp] to decide to + // build a [Navigator]... which means specifying either `home`, `routes`, + // `onGenerateRoute`, or `onUnknownRoute`. Make it `onGenerateRoute`. + // It never actually gets called, though: `onGenerateInitialRoutes` + // handles startup, and then we always push whole routes with methods + // like [Navigator.push], never mere names as with [Navigator.pushNamed]. + onGenerateRoute: (_) => null, + + onGenerateInitialRoutes: _handleGenerateInitialRoutes)); } } From bd70287c0a95b68d7eb0bc95bc5c01e6de04898d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 30 Jan 2025 14:56:21 -0800 Subject: [PATCH 5/7] page [nfc]: Add interface to get account ID for most of our routes --- lib/widgets/home.dart | 2 +- lib/widgets/message_list.dart | 2 +- lib/widgets/page.dart | 10 +++++++++- lib/widgets/profile.dart | 2 +- test/widgets/page_checks.dart | 2 +- 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/widgets/home.dart b/lib/widgets/home.dart index ad70b57c32..d7b1585022 100644 --- a/lib/widgets/home.dart +++ b/lib/widgets/home.dart @@ -31,7 +31,7 @@ enum _HomePageTab { class HomePage extends StatefulWidget { const HomePage({super.key}); - static Route buildRoute({required int accountId}) { + static AccountRoute buildRoute({required int accountId}) { return MaterialAccountWidgetRoute(accountId: accountId, loadingPlaceholderPage: _LoadingPlaceholderPage(accountId: accountId), page: const HomePage()); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 4263041dbc..25993efa30 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -188,7 +188,7 @@ abstract class MessageListPageState { class MessageListPage extends StatefulWidget { const MessageListPage({super.key, required this.initNarrow}); - static Route buildRoute({int? accountId, BuildContext? context, + static AccountRoute buildRoute({int? accountId, BuildContext? context, required Narrow narrow}) { return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: MessageListPage(initNarrow: narrow)); diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart index 0ba65fb3d4..a2c6fe52a1 100644 --- a/lib/widgets/page.dart +++ b/lib/widgets/page.dart @@ -35,6 +35,12 @@ abstract class WidgetRoute extends PageRoute { Widget get page; } +/// A page route that specifies a particular Zulip account to use, by ID. +abstract class AccountRoute extends PageRoute { + /// The [Account.id] of the account to use for this page. + int get accountId; +} + /// A [MaterialPageRoute] that always builds the same widget. /// /// This is useful for making the route more transparent for a test to inspect. @@ -56,8 +62,10 @@ class MaterialWidgetRoute extends MaterialPageRoute implem } /// A mixin for providing a given account's per-account store on a page route. -mixin AccountPageRouteMixin on PageRoute { +mixin AccountPageRouteMixin on PageRoute implements AccountRoute { + @override int get accountId; + Widget? get loadingPlaceholderPage; @override diff --git a/lib/widgets/profile.dart b/lib/widgets/profile.dart index c4f8970ff0..327910f6c0 100644 --- a/lib/widgets/profile.dart +++ b/lib/widgets/profile.dart @@ -30,7 +30,7 @@ class ProfilePage extends StatelessWidget { final int userId; - static Route buildRoute({int? accountId, BuildContext? context, + static AccountRoute buildRoute({int? accountId, BuildContext? context, required int userId}) { return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: ProfilePage(userId: userId)); diff --git a/test/widgets/page_checks.dart b/test/widgets/page_checks.dart index 412a59fc49..a3692273bf 100644 --- a/test/widgets/page_checks.dart +++ b/test/widgets/page_checks.dart @@ -6,6 +6,6 @@ extension WidgetRouteChecks on Subject> { Subject get page => has((x) => x.page, 'page'); } -extension AccountPageRouteMixinChecks on Subject> { +extension AccountRouteChecks on Subject> { Subject get accountId => has((x) => x.accountId, 'accountId'); } From 4b2f51e0f9370252cc2959bfb5e584a83fb1233e Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Mon, 30 Dec 2024 23:28:20 +0530 Subject: [PATCH 6/7] notif: Use associated account as initial account, if opened from background Previously, when two accounts (Account-1 and Account-2) were logged in, the app always defaulted to showing the home page of Account-1 on launch. If the app was closed and the user opened a notification from Account-2, the navigation stack would be: HomePage(Account-1) -> MessageListPage(Account-2) This commit fixes that behaviour, now when a notification is opened while the app is closed, the home page will correspond to the account associated with the notification's conversation. This addresses #1210 for background notifications. --- lib/notifications/display.dart | 52 +++++++++++++++++++--------- lib/widgets/app.dart | 27 ++++++++++----- test/notifications/display_test.dart | 35 +++++++++++++++++-- 3 files changed, 87 insertions(+), 27 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 1b85ccaebc..210a08ee57 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -453,6 +453,39 @@ 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 and shows an error dialog if the associated account is not + /// found in the global store. + static AccountRoute? routeForNotification({ + required BuildContext context, + required Uri url, + }) { + final globalStore = GlobalStoreWidget.of(context); + + 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) { // TODO(log) + final zulipLocalizations = ZulipLocalizations.of(context); + showErrorDialog(context: context, + title: zulipLocalizations.errorNotificationOpenTitle, + message: zulipLocalizations.errorNotificationOpenAccountMissing); + return null; + } + + return MessageListPage.buildRoute( + accountId: account.id, + // TODO(#82): Open at specific message, not just conversation + narrow: payload.narrow); + } + /// Navigates to the [MessageListPage] of the specific conversation /// given the `zulip://notification/…` Android intent data URL, /// generated with [NotificationOpenPayload.buildUrl] while creating @@ -460,29 +493,16 @@ class NotificationDisplayManager { static Future 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) - showErrorDialog(context: context, - title: zulipLocalizations.errorNotificationOpenTitle, - message: zulipLocalizations.errorNotificationOpenAccountMissing); - return; - } + final route = routeForNotification(context: context, url: url); + if (route == null) return; // TODO(log) // TODO(nav): Better interact with existing nav stack on notif open - unawaited(navigator.push(MaterialAccountWidgetRoute(accountId: account.id, - // TODO(#82): Open at specific message, not just conversation - page: MessageListPage(initNarrow: payload.narrow)))); + unawaited(navigator.push(route)); } static Future _fetchBitmap(Uri url) async { diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index b6786ea3d1..9fa82144a1 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -143,7 +143,6 @@ class _ZulipAppState extends State with WidgetsBindingObserver { void initState() { super.initState(); WidgetsBinding.instance.addObserver(this); - _handleInitialRoute(); } @override @@ -157,8 +156,25 @@ class _ZulipAppState extends State with WidgetsBindingObserver { // we use the Navigator which should be available when this callback is // called and it's context should have the required ancestors. final context = ZulipApp.navigatorKey.currentContext!; - final globalStore = GlobalStoreWidget.of(context); + final initialRouteUrl = Uri.tryParse(initialRoute); + if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { + final route = NotificationDisplayManager.routeForNotification( + context: context, + url: initialRouteUrl); + + if (route != null) { + return [ + HomePage.buildRoute(accountId: route.accountId), + route, + ]; + } else { + // The account didn't match any existing accounts, + // fall through to show the default route below. + } + } + + final globalStore = GlobalStoreWidget.of(context); // TODO(#524) choose initial account as last one used final initialAccountId = globalStore.accounts.firstOrNull?.id; return [ @@ -169,13 +185,6 @@ class _ZulipAppState extends State with WidgetsBindingObserver { ]; } - Future _handleInitialRoute() async { - final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName); - if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) { - await NotificationDisplayManager.navigateForNotification(initialRouteUrl); - } - } - @override Future didPushRouteInformation(routeInformation) async { switch (routeInformation.uri) { diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index b72e97ed01..51ec661930 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -960,11 +960,12 @@ void main() { group('NotificationDisplayManager open', () { late List> pushedRoutes; - void takeStartingRoutes({bool withAccount = true}) { + void takeStartingRoutes({Account? account, bool withAccount = true}) { + account ??= eg.selfAccount; final expected = >[ if (withAccount) (it) => it.isA() - ..accountId.equals(eg.selfAccount.id) + ..accountId.equals(account!.id) ..page.isA() else (it) => it.isA().page.isA(), @@ -1130,6 +1131,36 @@ void main() { takeStartingRoutes(); matchesNavigation(check(pushedRoutes).single, account, message); }); + + testWidgets('uses associated account as initial account; if initial route', (tester) async { + addTearDown(testBinding.reset); + + 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(); + addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); + tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString(); + + await prepare(tester, early: true); + check(pushedRoutes).isEmpty(); // GlobalStore hasn't loaded yet + + await tester.pump(); + takeStartingRoutes(account: accountB); + matchesNavigation(check(pushedRoutes).single, accountB, message); + }); }); group('NotificationOpenPayload', () { From fb1b97fb3c970d3b96b4bdfad550cf35d20c702e Mon Sep 17 00:00:00 2001 From: Rajesh Malviya Date: Tue, 28 Jan 2025 20:17:04 +0530 Subject: [PATCH 7/7] notif: Query account by realm URL origin, not full URL This fixes a potential bug, in case the server returned `realm_url` contains a trailing `/`. --- lib/notifications/display.dart | 5 +++-- test/notifications/display_test.dart | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/notifications/display.dart b/lib/notifications/display.dart index 210a08ee57..1d74db6854 100644 --- a/lib/notifications/display.dart +++ b/lib/notifications/display.dart @@ -470,8 +470,9 @@ class NotificationDisplayManager { 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); + final account = globalStore.accounts.firstWhereOrNull( + (account) => account.realmUrl.origin == payload.realmUrl.origin + && account.userId == payload.userId); if (account == null) { // TODO(log) final zulipLocalizations = ZulipLocalizations.of(context); showErrorDialog(context: context, diff --git a/test/notifications/display_test.dart b/test/notifications/display_test.dart index 51ec661930..b1c56b55b1 100644 --- a/test/notifications/display_test.dart +++ b/test/notifications/display_test.dart @@ -1037,6 +1037,21 @@ void main() { eg.dmMessage(from: eg.otherUser, to: [eg.selfUser])); }); + testWidgets('account queried by realmUrl origin component', (tester) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add( + eg.selfAccount.copyWith(realmUrl: Uri.parse('http://chat.example')), + eg.initialSnapshot()); + await prepare(tester); + + await checkOpenNotification(tester, + eg.selfAccount.copyWith(realmUrl: Uri.parse('http://chat.example/')), + eg.streamMessage()); + await checkOpenNotification(tester, + eg.selfAccount.copyWith(realmUrl: Uri.parse('http://chat.example')), + eg.streamMessage()); + }); + testWidgets('no accounts', (tester) async { await prepare(tester, withAccount: false); await openNotification(tester, eg.selfAccount, eg.streamMessage());