From e4d75fe617f4051ab0f2ef775cce39b333d7c93e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 11:57:43 -0800 Subject: [PATCH 01/16] store [nfc]: Rename LoadingPlaceholder widget, from LoadingPage This doesn't particularly have a "page" nature; it'd work just as well as one thing on a larger page, if it came to that. --- lib/widgets/store.dart | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index f69a2c7d6b..cf4494522f 100644 --- a/lib/widgets/store.dart +++ b/lib/widgets/store.dart @@ -65,8 +65,8 @@ class _GlobalStoreWidgetState extends State { @override Widget build(BuildContext context) { final store = this.store; - // TODO: factor out the use of LoadingPage to be configured by the widget, like [widget.child] is - if (store == null) return const LoadingPage(); + // TODO: factor out the use of LoadingPlaceholder to be configured by the widget, like [widget.child] is + if (store == null) return const LoadingPlaceholder(); return _GlobalStoreInheritedWidget(store: store, child: widget.child); } } @@ -220,8 +220,8 @@ class _PerAccountStoreWidgetState extends State { @override Widget build(BuildContext context) { - // TODO: factor out the use of LoadingPage to be configured by the widget, like [widget.child] is - if (store == null) return const LoadingPage(); + // TODO: factor out the use of LoadingPlaceholder to be configured by the widget, like [widget.child] is + if (store == null) return const LoadingPlaceholder(); return _PerAccountStoreInheritedWidget(store: store!, child: widget.child); } } @@ -242,8 +242,8 @@ class _PerAccountStoreInheritedWidget extends InheritedNotifier store != oldWidget.store; } -class LoadingPage extends StatelessWidget { - const LoadingPage({super.key}); +class LoadingPlaceholder extends StatelessWidget { + const LoadingPlaceholder({super.key}); @override Widget build(BuildContext context) { From e76e5ce10f412f1208c184b6b50f2af7d27c0bb8 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 12:03:57 -0800 Subject: [PATCH 02/16] store [nfc]: Make placeholder configurable on store widgets --- lib/widgets/store.dart | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index cf4494522f..1929a225f6 100644 --- a/lib/widgets/store.dart +++ b/lib/widgets/store.dart @@ -12,8 +12,13 @@ import '../model/store.dart'; /// * [PerAccountStoreWidget], for the user's data associated with a /// particular Zulip account. class GlobalStoreWidget extends StatefulWidget { - const GlobalStoreWidget({super.key, required this.child}); + const GlobalStoreWidget({ + super.key, + this.placeholder = const LoadingPlaceholder(), + required this.child, + }); + final Widget placeholder; final Widget child; /// The app's global data store. @@ -65,8 +70,7 @@ class _GlobalStoreWidgetState extends State { @override Widget build(BuildContext context) { final store = this.store; - // TODO: factor out the use of LoadingPlaceholder to be configured by the widget, like [widget.child] is - if (store == null) return const LoadingPlaceholder(); + if (store == null) return widget.placeholder; return _GlobalStoreInheritedWidget(store: store, child: widget.child); } } @@ -107,10 +111,12 @@ class PerAccountStoreWidget extends StatefulWidget { const PerAccountStoreWidget({ super.key, required this.accountId, + this.placeholder = const LoadingPlaceholder(), required this.child, }); final int accountId; + final Widget placeholder; final Widget child; /// The user's data for the relevant Zulip account for this widget. @@ -220,8 +226,7 @@ class _PerAccountStoreWidgetState extends State { @override Widget build(BuildContext context) { - // TODO: factor out the use of LoadingPlaceholder to be configured by the widget, like [widget.child] is - if (store == null) return const LoadingPlaceholder(); + if (store == null) return widget.placeholder; return _PerAccountStoreInheritedWidget(store: store!, child: widget.child); } } From 5435296843ea5191687aea8530b5814449fd60ec Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 12:28:01 -0800 Subject: [PATCH 03/16] nav [nfc]: Write full docs for MaterialAccountWidgetRoute and friends I was rereading some of the code that uses these classes, and found I had to reread their implementation to refresh myself on what they each did. --- lib/widgets/page.dart | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart index bd2072fb4a..0857d750b8 100644 --- a/lib/widgets/page.dart +++ b/lib/widgets/page.dart @@ -14,6 +14,10 @@ abstract class WidgetRoute extends PageRoute { /// A [MaterialPageRoute] that always builds the same widget. /// /// This is useful for making the route more transparent for a test to inspect. +/// +/// See also: +/// * [MaterialAccountWidgetRoute], a subclass which automates reusing a +/// per-account store on the new route. class MaterialWidgetRoute extends MaterialPageRoute implements WidgetRoute { MaterialWidgetRoute({ required this.page, @@ -27,6 +31,7 @@ class MaterialWidgetRoute extends MaterialPageRoute implements WidgetRoute final Widget page; } +/// A mixin for providing a given account's per-account store on a page route. mixin AccountPageRouteMixin on PageRoute { int get accountId; @@ -38,6 +43,16 @@ mixin AccountPageRouteMixin on PageRoute { } } +/// A [MaterialPageRoute] that reuses the given context's per-account store. +/// +/// This reuse is the desired behavior for any navigation that's meant to stay +/// within a given account. +/// +/// See also: +/// * [MaterialAccountWidgetRoute], a subclass which is more transparent +/// for tests. +/// * [AccountPageRouteBuilder], for defining one-off page routes +/// in terms of callbacks. class MaterialAccountPageRoute extends MaterialPageRoute with AccountPageRouteMixin { MaterialAccountPageRoute({ required BuildContext context, @@ -52,9 +67,20 @@ class MaterialAccountPageRoute extends MaterialPageRoute with AccountPageR final int accountId; } -/// A [MaterialAccountPageRoute] that always builds the same widget. +/// A [MaterialPageRoute] that reuses the given context's per-account store +/// and always builds the same widget. /// -/// This is useful for making the route more transparent for a test to inspect. +/// This is the [PageRoute] subclass to use for most navigation in the app. +/// +/// The reuse of the per-account store is the desired behavior for any +/// navigation that's meant to stay within a given account. +/// +/// Always building the same widget is useful for making the route +/// more transparent for a test to inspect. +/// +/// See also: +/// * [MaterialWidgetRoute], for routes that need no per-account store +/// or a different per-account store. class MaterialAccountWidgetRoute extends MaterialAccountPageRoute implements WidgetRoute { MaterialAccountWidgetRoute({ required super.context, @@ -69,6 +95,9 @@ class MaterialAccountWidgetRoute extends MaterialAccountPageRoute implemen final Widget page; } +/// A [PageRouteBuilder] that reuses the given context's per-account store. +/// +/// This is the [PageRouteBuilder] analogue of [MaterialAccountPageRoute]. class AccountPageRouteBuilder extends PageRouteBuilder with AccountPageRouteMixin { AccountPageRouteBuilder({ required BuildContext context, From 8fd3f36fdb080128814ce1b976361acd971d0d02 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 12:40:09 -0800 Subject: [PATCH 04/16] nav: Use page-styled loading placeholder for per-account data In particular this provides a back button so that the user can go try something else instead of waiting for the app to finish getting data from the server. (On Android the user could already do so, using the system back gesture or button, but there's no such thing on iOS.) --- integration_test/unreadmarker_test.dart | 2 ++ lib/notifications.dart | 1 + lib/widgets/app.dart | 1 + lib/widgets/page.dart | 13 +++++++++++++ 4 files changed, 17 insertions(+) diff --git a/integration_test/unreadmarker_test.dart b/integration_test/unreadmarker_test.dart index 2db4222dc2..1870bc2655 100644 --- a/integration_test/unreadmarker_test.dart +++ b/integration_test/unreadmarker_test.dart @@ -7,6 +7,7 @@ import 'package:zulip/api/model/model.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/message_list.dart'; +import 'package:zulip/widgets/page.dart'; import 'package:zulip/widgets/store.dart'; import '../test/api/fake_api.dart'; @@ -38,6 +39,7 @@ void main() { home: GlobalStoreWidget( child: PerAccountStoreWidget( accountId: eg.selfAccount.id, + placeholder: const LoadingPlaceholderPage(), child: const MessageListPage(narrow: AllMessagesNarrow()))))); await tester.pumpAndSettle(); return messages; diff --git a/lib/notifications.dart b/lib/notifications.dart index 9ed1e99f91..f9b5baede2 100644 --- a/lib/notifications.dart +++ b/lib/notifications.dart @@ -391,6 +391,7 @@ class NotificationDisplayManager { // TODO(nav): Better interact with existing nav stack on notif open navigator.push(MaterialWidgetRoute( page: PerAccountStoreWidget(accountId: account.id, + placeholder: const LoadingPlaceholderPage(), // TODO(#82): Open at specific message, not just conversation child: MessageListPage(narrow: narrow)))); return; diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index dc546707d6..3c935e3d80 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -213,6 +213,7 @@ class HomePage extends StatelessWidget { static Route buildRoute({required int accountId}) { return MaterialWidgetRoute( page: PerAccountStoreWidget(accountId: accountId, + placeholder: const LoadingPlaceholderPage(), child: const HomePage())); } diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart index 0857d750b8..a9eb9e05ce 100644 --- a/lib/widgets/page.dart +++ b/lib/widgets/page.dart @@ -39,6 +39,7 @@ mixin AccountPageRouteMixin on PageRoute { Widget buildPage(BuildContext context, Animation animation, Animation secondaryAnimation) { return PerAccountStoreWidget( accountId: accountId, + placeholder: const LoadingPlaceholderPage(), child: super.buildPage(context, animation, secondaryAnimation)); } } @@ -118,3 +119,15 @@ class AccountPageRouteBuilder extends PageRouteBuilder with AccountPageRou @override final int accountId; } + +class LoadingPlaceholderPage extends StatelessWidget { + const LoadingPlaceholderPage({super.key}); + + @override + Widget build(BuildContext context) { + return Scaffold( + appBar: AppBar(), + body: const LoadingPlaceholder(), + ); + } +} From 582bc446015854e5a96c5c94f0b1375be5bee9e9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 14:15:43 -0800 Subject: [PATCH 05/16] nav [nfc]: Accept an explicit account ID on account page routes --- lib/widgets/inbox.dart | 4 +- lib/widgets/lightbox.dart | 4 +- lib/widgets/message_list.dart | 5 +- lib/widgets/page.dart | 67 ++++++++++++++++++------ lib/widgets/profile.dart | 5 +- lib/widgets/recent_dm_conversations.dart | 4 +- lib/widgets/subscription_list.dart | 4 +- 7 files changed, 65 insertions(+), 28 deletions(-) diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index cbbde8061e..a1e706946f 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -15,8 +15,8 @@ import 'unread_count_badge.dart'; class InboxPage extends StatefulWidget { const InboxPage({super.key}); - static Route buildRoute({required BuildContext context}) { - return MaterialAccountWidgetRoute(context: context, + static Route buildRoute({int? accountId, BuildContext? context}) { + return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: const InboxPage()); } diff --git a/lib/widgets/lightbox.dart b/lib/widgets/lightbox.dart index 7c3b2a4146..7a3ca5230e 100644 --- a/lib/widgets/lightbox.dart +++ b/lib/widgets/lightbox.dart @@ -208,11 +208,13 @@ class _LightboxPageState extends State<_LightboxPage> { } Route getLightboxRoute({ - required BuildContext context, + int? accountId, + BuildContext? context, required Message message, required Uri src, }) { return AccountPageRouteBuilder( + accountId: accountId, context: context, fullscreenDialog: true, pageBuilder: ( diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 93c4e9c1f5..a957b29cfc 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -26,8 +26,9 @@ import 'text.dart'; class MessageListPage extends StatefulWidget { const MessageListPage({super.key, required this.narrow}); - static Route buildRoute({required BuildContext context, required Narrow narrow}) { - return MaterialAccountWidgetRoute(context: context, + static Route buildRoute({int? accountId, BuildContext? context, + required Narrow narrow}) { + return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: MessageListPage(narrow: narrow)); } diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart index a9eb9e05ce..f964b2a0f1 100644 --- a/lib/widgets/page.dart +++ b/lib/widgets/page.dart @@ -16,7 +16,7 @@ abstract class WidgetRoute extends PageRoute { /// This is useful for making the route more transparent for a test to inspect. /// /// See also: -/// * [MaterialAccountWidgetRoute], a subclass which automates reusing a +/// * [MaterialAccountWidgetRoute], a subclass which automates providing a /// per-account store on the new route. class MaterialWidgetRoute extends MaterialPageRoute implements WidgetRoute { MaterialWidgetRoute({ @@ -44,10 +44,7 @@ mixin AccountPageRouteMixin on PageRoute { } } -/// A [MaterialPageRoute] that reuses the given context's per-account store. -/// -/// This reuse is the desired behavior for any navigation that's meant to stay -/// within a given account. +/// A [MaterialPageRoute] providing a per-account store for a given account. /// /// See also: /// * [MaterialAccountWidgetRoute], a subclass which is more transparent @@ -55,36 +52,58 @@ mixin AccountPageRouteMixin on PageRoute { /// * [AccountPageRouteBuilder], for defining one-off page routes /// in terms of callbacks. class MaterialAccountPageRoute extends MaterialPageRoute with AccountPageRouteMixin { + /// Construct a [MaterialAccountPageRoute] using either the given account ID, + /// or the ambient one from the given context. + /// + /// The account ID used is [accountId] if specified, + /// else the ambient account ID from [context]. + /// One of those parameters must be specified, and not both. + /// + /// Generally most navigation in the app is within a given account, + /// and should use [context]. Using [accountId] is appropriate for + /// navigating across accounts, or navigating into an account from contexts + /// (like login or the choose-account page) that don't have an ambient account. MaterialAccountPageRoute({ - required BuildContext context, + int? accountId, + BuildContext? context, required super.builder, super.settings, super.maintainState, super.fullscreenDialog, super.allowSnapshotting, - }) : accountId = PerAccountStoreWidget.accountIdOf(context); + }) : assert((accountId != null) ^ (context != null), + "exactly one of accountId or context must be specified"), + accountId = accountId ?? PerAccountStoreWidget.accountIdOf(context!); @override final int accountId; } -/// A [MaterialPageRoute] that reuses the given context's per-account store +/// A [MaterialPageRoute] that provides a per-account store for a given account /// and always builds the same widget. /// /// This is the [PageRoute] subclass to use for most navigation in the app. /// -/// The reuse of the per-account store is the desired behavior for any -/// navigation that's meant to stay within a given account. -/// /// Always building the same widget is useful for making the route /// more transparent for a test to inspect. /// /// See also: -/// * [MaterialWidgetRoute], for routes that need no per-account store -/// or a different per-account store. +/// * [MaterialWidgetRoute], for routes that need no per-account store. class MaterialAccountWidgetRoute extends MaterialAccountPageRoute implements WidgetRoute { + /// Construct a [MaterialAccountWidgetRoute] using either the given account ID, + /// or the ambient one from the given context. + /// + /// The account ID used is [accountId] if specified, + /// else the ambient account ID from [context]. + /// One of those parameters must be specified, and not both. + /// + /// Generally most navigation in the app is within a given account, + /// and should use [context]. Using [accountId] is appropriate for + /// navigating across accounts, or navigating into an account from contexts + /// (like login or the choose-account page) that don't have an ambient account. MaterialAccountWidgetRoute({ - required super.context, + super.accountId, + super.context, required this.page, super.settings, super.maintainState, @@ -96,12 +115,24 @@ class MaterialAccountWidgetRoute extends MaterialAccountPageRoute implemen final Widget page; } -/// A [PageRouteBuilder] that reuses the given context's per-account store. +/// A [PageRouteBuilder] providing a per-account store for a given account. /// /// This is the [PageRouteBuilder] analogue of [MaterialAccountPageRoute]. class AccountPageRouteBuilder extends PageRouteBuilder with AccountPageRouteMixin { + /// Construct an [AccountPageRouteBuilder] using either the given account ID, + /// or the ambient one from the given context. + /// + /// The account ID used is [accountId] if specified, + /// else the ambient account ID from [context]. + /// One of those parameters must be specified, and not both. + /// + /// Generally most navigation in the app is within a given account, + /// and should use [context]. Using [accountId] is appropriate for + /// navigating across accounts, or navigating into an account from contexts + /// (like login or the choose-account page) that don't have an ambient account. AccountPageRouteBuilder({ - required BuildContext context, + int? accountId, + BuildContext? context, super.settings, required super.pageBuilder, super.transitionsBuilder, @@ -114,7 +145,9 @@ class AccountPageRouteBuilder extends PageRouteBuilder with AccountPageRou super.maintainState, super.fullscreenDialog, super.allowSnapshotting, - }) : accountId = PerAccountStoreWidget.accountIdOf(context); + }) : assert((accountId != null) ^ (context != null), + "exactly one of accountId or context must be specified"), + accountId = accountId ?? PerAccountStoreWidget.accountIdOf(context!); @override final int accountId; diff --git a/lib/widgets/profile.dart b/lib/widgets/profile.dart index 6d125fbff0..055772017e 100644 --- a/lib/widgets/profile.dart +++ b/lib/widgets/profile.dart @@ -22,8 +22,9 @@ class ProfilePage extends StatelessWidget { final int userId; - static Route buildRoute({required BuildContext context, required int userId}) { - return MaterialAccountWidgetRoute(context: context, + static Route buildRoute({int? accountId, BuildContext? context, + required int userId}) { + return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: ProfilePage(userId: userId)); } diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index 1dc95dc78d..dac1d638c5 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -14,8 +14,8 @@ import 'unread_count_badge.dart'; class RecentDmConversationsPage extends StatefulWidget { const RecentDmConversationsPage({super.key}); - static Route buildRoute({required BuildContext context}) { - return MaterialAccountWidgetRoute(context: context, + static Route buildRoute({int? accountId, BuildContext? context}) { + return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: const RecentDmConversationsPage()); } diff --git a/lib/widgets/subscription_list.dart b/lib/widgets/subscription_list.dart index 97c75f8eca..9769311543 100644 --- a/lib/widgets/subscription_list.dart +++ b/lib/widgets/subscription_list.dart @@ -15,8 +15,8 @@ import 'unread_count_badge.dart'; class SubscriptionListPage extends StatefulWidget { const SubscriptionListPage({super.key}); - static Route buildRoute({required BuildContext context}) { - return MaterialAccountWidgetRoute(context: context, + static Route buildRoute({int? accountId, BuildContext? context}) { + return MaterialAccountWidgetRoute(accountId: accountId, context: context, page: const SubscriptionListPage()); } From 6e8f1a1bcebe1d1f3691589942ac3fd469cf2af4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 14:33:59 -0800 Subject: [PATCH 06/16] nav [nfc]: Use MaterialAccountWidgetRoute more comprehensively Now PerAccountStoreWidget has just two constructor call sites in the app. There's one for the flight shuttle in LightboxHero, a rare example of a widget not belonging to any page; and then the one in AccountPageRouteMixin covers every page in the app that needs a per-account store. --- lib/notifications.dart | 8 +++----- lib/widgets/app.dart | 6 ++---- test/notifications_test.dart | 11 ++++------- test/widgets/page_checks.dart | 4 ++++ 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/lib/notifications.dart b/lib/notifications.dart index f9b5baede2..d7ee87f34a 100644 --- a/lib/notifications.dart +++ b/lib/notifications.dart @@ -389,11 +389,9 @@ class NotificationDisplayManager { assert(debugLog(' account: $account, narrow: $narrow')); // TODO(nav): Better interact with existing nav stack on notif open - navigator.push(MaterialWidgetRoute( - page: PerAccountStoreWidget(accountId: account.id, - placeholder: const LoadingPlaceholderPage(), - // TODO(#82): Open at specific message, not just conversation - child: MessageListPage(narrow: narrow)))); + navigator.push(MaterialAccountWidgetRoute(accountId: account.id, + // TODO(#82): Open at specific message, not just conversation + page: MessageListPage(narrow: narrow))); return; } } diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 3c935e3d80..378edab0bf 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -211,10 +211,8 @@ class HomePage extends StatelessWidget { const HomePage({super.key}); static Route buildRoute({required int accountId}) { - return MaterialWidgetRoute( - page: PerAccountStoreWidget(accountId: accountId, - placeholder: const LoadingPlaceholderPage(), - child: const HomePage())); + return MaterialAccountWidgetRoute(accountId: accountId, + page: const HomePage()); } @override diff --git a/test/notifications_test.dart b/test/notifications_test.dart index 38d2d7bd98..c51dee4d60 100644 --- a/test/notifications_test.dart +++ b/test/notifications_test.dart @@ -14,7 +14,6 @@ import 'package:zulip/notifications.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; -import 'package:zulip/widgets/store.dart'; import 'flutter_checks.dart'; import 'model/binding.dart'; @@ -22,7 +21,6 @@ import 'example_data.dart' as eg; import 'test_navigation.dart'; import 'widgets/message_list_checks.dart'; import 'widgets/page_checks.dart'; -import 'widgets/store_checks.dart'; FakeAndroidFlutterLocalNotificationsPlugin get notifAndroid => testBinding.notifications @@ -218,12 +216,11 @@ void main() { } void matchesNavigation(Subject route, Account account, Message message) { - route.isA().page - .isA() + route.isA() ..accountId.equals(account.id) - ..child.isA() - .narrow.equals(SendableNarrow.ofMessage(message, - selfUserId: account.userId)); + ..page.isA() + .narrow.equals(SendableNarrow.ofMessage(message, + selfUserId: account.userId)); } Future checkOpenNotification(Account account, Message message) async { diff --git a/test/widgets/page_checks.dart b/test/widgets/page_checks.dart index 26590df8a5..f6effa2606 100644 --- a/test/widgets/page_checks.dart +++ b/test/widgets/page_checks.dart @@ -5,3 +5,7 @@ import 'package:zulip/widgets/page.dart'; extension WidgetRouteChecks on Subject { Subject get page => has((x) => x.page, 'page'); } + +extension AccountPageRouteMixinChecks on Subject { + Subject get accountId => has((x) => x.accountId, 'accountId'); +} From ec48d251ee7151d01f15d98503a42fe811e849a0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 13:14:29 -0800 Subject: [PATCH 07/16] nav [nfc]: Switch to onGenerateRoute instead of home This has the same effect, but gives us a more general form of API to work with for more complex effects. Effectively `onGenerateRoute` is a fallback for when a route name isn't resolved by `home` or `routes`; and this version has the same effect as the `home` (and omitted `routes`) we'd been providing. I find the relevant area of the docs somewhat hard to follow, so to see this in the implementation: the main effects of both `home` and `onGenerateRoute` are implemented in [WidgetsApp._onGenerateRoute], and the one other effect is in [WidgetsApp._usesNavigator]. The [MaterialWidgetRoute] part corresponds to `pageRouteBuilder`, which is specified by the [MaterialApp] implementation. --- lib/widgets/app.dart | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 378edab0bf..33fc4d0f48 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -127,7 +127,12 @@ class ZulipApp extends StatelessWidget { GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); return child!; }, - home: const ChooseAccountPage())); + onGenerateRoute: (settings) { + return settings.name == Navigator.defaultRouteName + ? MaterialWidgetRoute( + settings: settings, page: const ChooseAccountPage()) + : null; + })); } } From cf7d3bd4828e106dfb2d75520e7315c56c0d8c27 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 15:40:06 -0800 Subject: [PATCH 08/16] nav test [nfc]: Complete TestNavigatorObserver The base class offers six methods, so we may as well provide access to all six of them in the same way. --- test/test_navigation.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/test_navigation.dart b/test/test_navigation.dart index 02d7b48f6e..b5065d684c 100644 --- a/test/test_navigation.dart +++ b/test/test_navigation.dart @@ -11,6 +11,7 @@ class TestNavigatorObserver extends NavigatorObserver { void Function(Route route, Route? previousRoute)? onRemoved; void Function(Route? route, Route? previousRoute)? onReplaced; void Function(Route route, Route? previousRoute)? onStartUserGesture; + void Function()? onStopUserGesture; @override void didPush(Route route, Route? previousRoute) { @@ -36,4 +37,9 @@ class TestNavigatorObserver extends NavigatorObserver { void didStartUserGesture(Route route, Route? previousRoute) { onStartUserGesture?.call(route, previousRoute); } + + @override + void didStopUserGesture() { + onStopUserGesture?.call(); + } } From 21d1215100998332f4d2db05257c78cb40cf8b87 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 15:59:56 -0800 Subject: [PATCH 09/16] nav test: Add a test that we start on the choose-account page --- test/widgets/app_test.dart | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 test/widgets/app_test.dart diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart new file mode 100644 index 0000000000..2f94d82a3d --- /dev/null +++ b/test/widgets/app_test.dart @@ -0,0 +1,33 @@ +import 'package:checks/checks.dart'; +import 'package:flutter/widgets.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/widgets/app.dart'; +import 'package:zulip/widgets/page.dart'; + +import '../model/binding.dart'; +import '../test_navigation.dart'; +import 'page_checks.dart'; + +void main() { + TestZulipBinding.ensureInitialized(); + + group('ZulipApp initial navigation', () { + late List> pushedRoutes = []; + + Future>> initialRoutes(WidgetTester tester) async { + pushedRoutes = []; + final testNavObserver = TestNavigatorObserver() + ..onPushed = (route, prevRoute) => pushedRoutes.add(route); + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); + return pushedRoutes; + } + + testWidgets('go to choose-account page', (tester) async { + addTearDown(testBinding.reset); + check(await initialRoutes(tester)).deepEquals([ + (Subject it) => it.isA().page.isA(), + ]); + }); + }); +} From 0f669849e23e3620d827d7fb4d756fb2efe07684 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 13:27:59 -0800 Subject: [PATCH 10/16] nav [nfc]: Generalize further, to using onGenerateInitialRoutes This mimics what the default implementation of onGenerateInitialRoutes was doing, given that we never specify initialRoute and given our implementation of onGenerateRoute. --- lib/widgets/app.dart | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 33fc4d0f48..3d19bb876e 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -117,6 +117,7 @@ class ZulipApp extends StatelessWidget { localizationsDelegates: ZulipLocalizations.localizationsDelegates, supportedLocales: ZulipLocalizations.supportedLocales, theme: theme, + navigatorKey: navigatorKey, navigatorObservers: navigatorObservers ?? const [], builder: (BuildContext context, Widget? child) { @@ -127,11 +128,22 @@ class ZulipApp extends StatelessWidget { GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); return child!; }, - onGenerateRoute: (settings) { - return settings.name == Navigator.defaultRouteName - ? MaterialWidgetRoute( - settings: settings, page: const ChooseAccountPage()) - : null; + + // 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: (_) { + return [ + MaterialWidgetRoute( + settings: const RouteSettings(name: Navigator.defaultRouteName), + page: const ChooseAccountPage()), + ]; })); } } From 7fbacc5ed28df00ecb7dbe1e3c1bb058ac52a8b4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 15:07:38 -0800 Subject: [PATCH 11/16] nav [nfc]: Drop a RouteSettings we don't need --- lib/widgets/app.dart | 4 +--- test/notifications_test.dart | 7 +++---- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 3d19bb876e..17dab9bad9 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -140,9 +140,7 @@ class ZulipApp extends StatelessWidget { onGenerateInitialRoutes: (_) { return [ - MaterialWidgetRoute( - settings: const RouteSettings(name: Navigator.defaultRouteName), - page: const ChooseAccountPage()), + MaterialWidgetRoute(page: const ChooseAccountPage()), ]; })); } diff --git a/test/notifications_test.dart b/test/notifications_test.dart index c51dee4d60..a4edccb92d 100644 --- a/test/notifications_test.dart +++ b/test/notifications_test.dart @@ -15,7 +15,6 @@ import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; -import 'flutter_checks.dart'; import 'model/binding.dart'; import 'example_data.dart' as eg; import 'test_navigation.dart'; @@ -290,9 +289,9 @@ void main() { // Now let the GlobalStore get loaded and the app's main UI get mounted. await tester.pump(); - // The navigator first pushes the home route… + // The navigator first pushes the starting route… check(pushedRoutes).length.equals(2); - check(pushedRoutes[0]).settings.name.equals("/"); + check(pushedRoutes[0]).isA().page.isA(); // … and then the one the notification leads to. matchesNavigation(check(pushedRoutes[1]), eg.selfAccount, message); }); @@ -315,7 +314,7 @@ void main() { // Once the app is ready, we navigate to the conversation. await tester.pump(); check(pushedRoutes).length.equals(2); - check(pushedRoutes[0]).settings.name.equals("/"); + check(pushedRoutes[0]).isA().page.isA(); matchesNavigation(check(pushedRoutes[1]), account, message); }); }); From 84d53a279d3b79fbaf463ec4eb350ac99dfc05e6 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 13:46:42 -0800 Subject: [PATCH 12/16] app [nfc]: Insert a Builder under GlobalStoreWidget This will let us use the global store in computing our arguments for MaterialApp. --- lib/widgets/app.dart | 57 +++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 17dab9bad9..5be7cc8b89 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -111,37 +111,40 @@ class ZulipApp extends StatelessWidget { // a finger or thumb than the area above. tooltipTheme: const TooltipThemeData(preferBelow: false), ); + return GlobalStoreWidget( - child: MaterialApp( - title: 'Zulip', - localizationsDelegates: ZulipLocalizations.localizationsDelegates, - supportedLocales: ZulipLocalizations.supportedLocales, - theme: theme, + child: Builder(builder: (context) { + return MaterialApp( + title: 'Zulip', + localizationsDelegates: ZulipLocalizations.localizationsDelegates, + supportedLocales: ZulipLocalizations.supportedLocales, + theme: theme, - navigatorKey: navigatorKey, - navigatorObservers: navigatorObservers ?? const [], - builder: (BuildContext context, Widget? child) { - if (!ready.value) { - SchedulerBinding.instance.addPostFrameCallback( - (_) => _declareReady()); - } - GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context); - return child!; - }, + navigatorKey: navigatorKey, + navigatorObservers: navigatorObservers ?? const [], + builder: (BuildContext context, Widget? child) { + if (!ready.value) { + SchedulerBinding.instance.addPostFrameCallback( + (_) => _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, + // 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: (_) { - return [ - MaterialWidgetRoute(page: const ChooseAccountPage()), - ]; + onGenerateInitialRoutes: (_) { + return [ + MaterialWidgetRoute(page: const ChooseAccountPage()), + ]; + }); })); } } From e1ff476eeeec28298331104e0ee7e2c9632d6601 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 17:51:43 -0800 Subject: [PATCH 13/16] notif test: Make tests more robust by providing more data These tests had been relying on the fact that there wouldn't be any PerAccountStoreWidget that reached the point of attempting to load data for these accounts. But that was true only because, after opening a notification, the test code waited only for microtasks (with `await null`), and not for a new frame (like `tester.pump()`). That makes them brittle, as they'd break on an otherwise innocent change to pump another frame. Fortunately it's easy to fix. --- test/notifications_test.dart | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/notifications_test.dart b/test/notifications_test.dart index a4edccb92d..5eb14671bb 100644 --- a/test/notifications_test.dart +++ b/test/notifications_test.dart @@ -229,13 +229,13 @@ void main() { } testWidgets('stream message', (tester) async { - testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await checkOpenNotification(eg.selfAccount, eg.streamMessage()); }); testWidgets('direct message', (tester) async { - testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await checkOpenNotification(eg.selfAccount, eg.dmMessage(from: eg.otherUser, to: [eg.selfUser])); @@ -248,7 +248,7 @@ void main() { }); testWidgets('mismatching account', (tester) async { - testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester); await openNotification(eg.otherAccount, eg.streamMessage()); check(pushedRoutes).isEmpty(); @@ -266,7 +266,7 @@ void main() { eg.account(id: 1004, realmUrl: realmUrlB, user: user2), ]; for (final account in accounts) { - testBinding.globalStore.insertAccount(account.toCompanion(false)); + testBinding.globalStore.add(account, eg.initialSnapshot()); } await prepare(tester); @@ -277,7 +277,7 @@ void main() { }); testWidgets('wait for app to become ready', (tester) async { - testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); await prepare(tester, early: true); final message = eg.streamMessage(); await openNotification(eg.selfAccount, message); @@ -307,7 +307,7 @@ void main() { NotificationAppLaunchDetails(true, notificationResponse: response); // Now start the app. - testBinding.globalStore.insertAccount(account.toCompanion(false)); + testBinding.globalStore.add(account, eg.initialSnapshot()); await prepare(tester, early: true); check(pushedRoutes).isEmpty(); // GlobalStore hasn't loaded yet From 9df204d3cd785787bf3b34fd2ddee77d9c1ab13b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 17:58:30 -0800 Subject: [PATCH 14/16] notif test [nfc]: Factor out details of app's initial routes --- test/notifications_test.dart | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/test/notifications_test.dart b/test/notifications_test.dart index 5eb14671bb..10cefa9a34 100644 --- a/test/notifications_test.dart +++ b/test/notifications_test.dart @@ -191,6 +191,14 @@ void main() { group('NotificationDisplayManager open', () { late List> pushedRoutes; + void takeStartingRoutes() { + final expected = [ + (Subject it) => it.isA().page.isA(), + ]; + check(pushedRoutes.take(expected.length)).deepEquals(expected); + pushedRoutes.removeRange(0, expected.length); + } + Future prepare(WidgetTester tester, {bool early = false}) async { await init(); pushedRoutes = []; @@ -202,8 +210,8 @@ void main() { return; } await tester.pump(); - check(pushedRoutes).length.equals(1); - pushedRoutes.clear(); + takeStartingRoutes(); + check(pushedRoutes).isEmpty(); } Future openNotification(Account account, Message message) async { @@ -289,11 +297,10 @@ void main() { // Now let the GlobalStore get loaded and the app's main UI get mounted. await tester.pump(); - // The navigator first pushes the starting route… - check(pushedRoutes).length.equals(2); - check(pushedRoutes[0]).isA().page.isA(); + // The navigator first pushes the starting routes… + takeStartingRoutes(); // … and then the one the notification leads to. - matchesNavigation(check(pushedRoutes[1]), eg.selfAccount, message); + matchesNavigation(check(pushedRoutes).single, eg.selfAccount, message); }); testWidgets('at app launch', (tester) async { @@ -313,9 +320,8 @@ void main() { // Once the app is ready, we navigate to the conversation. await tester.pump(); - check(pushedRoutes).length.equals(2); - check(pushedRoutes[0]).isA().page.isA(); - matchesNavigation(check(pushedRoutes[1]), account, message); + takeStartingRoutes(); + matchesNavigation(check(pushedRoutes).single, account, message); }); }); } From cbca1e268c6855d17b97dde6e1f531330bd9ef72 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 13:47:04 -0800 Subject: [PATCH 15/16] nav: Go straight to an account's home page on launch, when available --- lib/widgets/app.dart | 6 ++++++ test/notifications_test.dart | 14 ++++++++++---- test/widgets/app_test.dart | 19 ++++++++++++++++++- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 5be7cc8b89..5f4d053dd0 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -114,6 +114,9 @@ class ZulipApp extends StatelessWidget { 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, @@ -143,6 +146,9 @@ class ZulipApp extends StatelessWidget { onGenerateInitialRoutes: (_) { return [ MaterialWidgetRoute(page: const ChooseAccountPage()), + if (initialAccountId != null) ...[ + HomePage.buildRoute(accountId: initialAccountId), + ], ]; }); })); diff --git a/test/notifications_test.dart b/test/notifications_test.dart index 10cefa9a34..a49da88582 100644 --- a/test/notifications_test.dart +++ b/test/notifications_test.dart @@ -191,15 +191,21 @@ void main() { group('NotificationDisplayManager open', () { late List> pushedRoutes; - void takeStartingRoutes() { + void takeStartingRoutes({bool withAccount = true}) { final expected = [ (Subject it) => it.isA().page.isA(), + if (withAccount) ...[ + (Subject it) => it.isA() + ..accountId.equals(eg.selfAccount.id) + ..page.isA(), + ], ]; check(pushedRoutes.take(expected.length)).deepEquals(expected); pushedRoutes.removeRange(0, expected.length); } - Future prepare(WidgetTester tester, {bool early = false}) async { + Future prepare(WidgetTester tester, + {bool early = false, bool withAccount = true}) async { await init(); pushedRoutes = []; final testNavObserver = TestNavigatorObserver() @@ -210,7 +216,7 @@ void main() { return; } await tester.pump(); - takeStartingRoutes(); + takeStartingRoutes(withAccount: withAccount); check(pushedRoutes).isEmpty(); } @@ -250,7 +256,7 @@ void main() { }); testWidgets('no accounts', (tester) async { - await prepare(tester); + await prepare(tester, withAccount: false); await openNotification(eg.selfAccount, eg.streamMessage()); check(pushedRoutes).isEmpty(); }); diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index 2f94d82a3d..6cfe80dd06 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -4,6 +4,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/page.dart'; +import '../example_data.dart' as eg; import '../model/binding.dart'; import '../test_navigation.dart'; import 'page_checks.dart'; @@ -23,11 +24,27 @@ void main() { return pushedRoutes; } - testWidgets('go to choose-account page', (tester) async { + testWidgets('when no accounts, go to choose account', (tester) async { addTearDown(testBinding.reset); check(await initialRoutes(tester)).deepEquals([ (Subject it) => it.isA().page.isA(), ]); }); + + testWidgets('when have accounts, go to home page for first account', (tester) async { + addTearDown(testBinding.reset); + + // We'll need per-account data for the account that a page will be opened + // for, but not for the other account. + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await testBinding.globalStore.insertAccount(eg.otherAccount.toCompanion(false)); + + check(await initialRoutes(tester)).deepEquals([ + (Subject it) => it.isA().page.isA(), + (Subject it) => it.isA() + ..accountId.equals(eg.selfAccount.id) + ..page.isA(), + ]); + }); }); } From 256fa7e23723523a11e9ba3387dc9736eab8681c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 16 Feb 2024 13:47:48 -0800 Subject: [PATCH 16/16] nav: Go straight to inbox on launch, when an account available Fixes: #516 --- lib/widgets/app.dart | 1 + test/notifications_test.dart | 4 ++++ test/widgets/app_test.dart | 6 +++++- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 5f4d053dd0..a153125e49 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -148,6 +148,7 @@ class ZulipApp extends StatelessWidget { MaterialWidgetRoute(page: const ChooseAccountPage()), if (initialAccountId != null) ...[ HomePage.buildRoute(accountId: initialAccountId), + InboxPage.buildRoute(accountId: initialAccountId), ], ]; }); diff --git a/test/notifications_test.dart b/test/notifications_test.dart index a49da88582..b3f9bdccb1 100644 --- a/test/notifications_test.dart +++ b/test/notifications_test.dart @@ -12,6 +12,7 @@ import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/notifications.dart'; import 'package:zulip/widgets/app.dart'; +import 'package:zulip/widgets/inbox.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; @@ -198,6 +199,9 @@ void main() { (Subject it) => it.isA() ..accountId.equals(eg.selfAccount.id) ..page.isA(), + (Subject it) => it.isA() + ..accountId.equals(eg.selfAccount.id) + ..page.isA(), ], ]; check(pushedRoutes.take(expected.length)).deepEquals(expected); diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index 6cfe80dd06..020a95986f 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -2,6 +2,7 @@ import 'package:checks/checks.dart'; import 'package:flutter/widgets.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/app.dart'; +import 'package:zulip/widgets/inbox.dart'; import 'package:zulip/widgets/page.dart'; import '../example_data.dart' as eg; @@ -31,7 +32,7 @@ void main() { ]); }); - testWidgets('when have accounts, go to home page for first account', (tester) async { + testWidgets('when have accounts, go to inbox for first account', (tester) async { addTearDown(testBinding.reset); // We'll need per-account data for the account that a page will be opened @@ -44,6 +45,9 @@ void main() { (Subject it) => it.isA() ..accountId.equals(eg.selfAccount.id) ..page.isA(), + (Subject it) => it.isA() + ..accountId.equals(eg.selfAccount.id) + ..page.isA(), ]); }); });