From 7a93845a270f873e92a0d5a270e2049f0816670a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Wed, 8 Jan 2025 09:57:59 +0800 Subject: [PATCH] rework nav wip Signed-off-by: Zixuan James Li --- lib/model/store.dart | 3 ++ lib/widgets/app.dart | 36 ++++++++++++++- lib/widgets/store.dart | 10 +--- test/widgets/store_test.dart | 90 ++++++++++++------------------------ 4 files changed, 69 insertions(+), 70 deletions(-) diff --git a/lib/model/store.dart b/lib/model/store.dart index 34d606ef5f..f602a9ddde 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -152,9 +152,12 @@ abstract class GlobalStore extends ChangeNotifier { assert(_accounts.containsKey(accountId)); final PerAccountStore store; try { + // await Future.delayed(const Duration(seconds: 2)); + // throw AccountNotFoundException(); store = await doLoadPerAccount(accountId); } catch (e) { switch (e) { + case AccountNotFoundException(): case ZulipApiException(code: 'INVALID_API_KEY'): // The API key is invalid and the store can never be loaded // unless the user retries manually. diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 643666693c..24bdc01267 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -187,6 +187,11 @@ class _ZulipAppState extends State with WidgetsBindingObserver { @override Widget build(BuildContext context) { final themeData = zulipThemeData(context); + final effectiveNavigatorObservers = [ + _EmptyStackNavigatorObserver(), + if (widget.navigatorObservers != null) + ...widget.navigatorObservers!, + ]; return GlobalStoreWidget( child: Builder(builder: (context) { final globalStore = GlobalStoreWidget.of(context); @@ -199,7 +204,7 @@ class _ZulipAppState extends State with WidgetsBindingObserver { theme: themeData, navigatorKey: ZulipApp.navigatorKey, - navigatorObservers: widget.navigatorObservers ?? const [], + navigatorObservers: effectiveNavigatorObservers, builder: (BuildContext context, Widget? child) { if (!ZulipApp.ready.value) { SchedulerBinding.instance.addPostFrameCallback( @@ -230,6 +235,35 @@ class _ZulipAppState extends State with WidgetsBindingObserver { } } +class _EmptyStackNavigatorObserver extends NavigatorObserver { + void _pushIfEmpty() async { + final navigator = await ZulipApp.navigator; + bool isEmptyStack = true; + navigator.popUntil((route) { + isEmptyStack = false; + return true; // never actually pops + }); + if (isEmptyStack) { + unawaited(navigator.push( + MaterialWidgetRoute(page: const ChooseAccountPage()))); + } + } + + @override + void didRemove(Route route, Route? previousRoute) async { + if (previousRoute == null) { + _pushIfEmpty(); + } + } + + @override + void didPop(Route route, Route? previousRoute) async { + if (previousRoute == null) { + _pushIfEmpty(); + } + } +} + class ChooseAccountPage extends StatelessWidget { const ChooseAccountPage({super.key}); diff --git a/lib/widgets/store.dart b/lib/widgets/store.dart index 46e277d6fe..9dd2b852ac 100644 --- a/lib/widgets/store.dart +++ b/lib/widgets/store.dart @@ -3,7 +3,6 @@ import 'package:flutter/scheduler.dart'; import '../model/binding.dart'; import '../model/store.dart'; -import 'app.dart'; import 'page.dart'; /// Provides access to the app's data. @@ -231,14 +230,7 @@ class _PerAccountStoreWidgetState extends State { SchedulerBinding.instance.addPostFrameCallback( (_) { final navigator = Navigator.of(context); - if (widget.routeToRemoveOnLogout!.isFirst) { - // This ensures that we never remove the root navigator level. - navigator.replace( - oldRoute: widget.routeToRemoveOnLogout!, - newRoute: MaterialWidgetRoute(page: const ChooseAccountPage())); - } else { - navigator.removeRoute(widget.routeToRemoveOnLogout!); - } + navigator.removeRoute(widget.routeToRemoveOnLogout!); }); } return; diff --git a/test/widgets/store_test.dart b/test/widgets/store_test.dart index cc1b25758d..786737f725 100644 --- a/test/widgets/store_test.dart +++ b/test/widgets/store_test.dart @@ -5,7 +5,6 @@ import 'package:zulip/api/exception.dart'; import 'package:zulip/model/store.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/home.dart'; -import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; import 'package:zulip/widgets/store.dart'; @@ -14,7 +13,6 @@ import '../model/binding.dart'; import '../example_data.dart' as eg; import '../model/store_checks.dart'; import '../model/test_store.dart'; -import '../notifications/display_test.dart'; import '../test_navigation.dart'; import 'dialog_checks.dart'; import 'page_checks.dart'; @@ -123,14 +121,10 @@ void main() { .contains('consider MaterialAccountWidgetRoute'); }); - testWidgets('PerAccountStoreWidget when log out, no duplicate choose-account page', (tester) async { - Route? newRoute, oldRoute; - final testNavObserver = TestNavigatorObserver() - ..onReplaced = (route, prevRoute) { - assert(newRoute == null && oldRoute == null); - newRoute = route!; - oldRoute = prevRoute!; - }; + testWidgets('PerAccountStoreWidget when log out, do not push route to non-empty navigator stack', (tester) async { + addTearDown(testBinding.reset); + + final testNavObserver = TestNavigatorObserver(); // A route to remove is a root level home route const loadPerAccountDuration = Duration(seconds: 30); assert(loadPerAccountDuration > kTryAnotherAccountWaitPeriod); @@ -155,77 +149,53 @@ void main() { await tester.pump(loadPerAccountDuration); // got the error await tester.pump(TestGlobalStore.removeAccountDuration); - checkErrorDialog(tester, - expectedTitle: 'Could not connect', - expectedMessage: - 'Your account at https://chat.example/ could not be authenticated.' - ' Please try logging in again or use another account.'); - // Ended up with two choose-account page routes on the stack, not good. - check(newRoute).isA().page.isA(); - check(oldRoute).isA().page.isA(); + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + check(removedRoutes).single.isA().page.isA(); check(testBinding.globalStore.takeDoRemoveAccountCalls()) .single.equals(eg.selfAccount.id); - }); - testWidgets('PerAccountStoreWidget when log out, replace root level route', (tester) async { - Route? newRoute, oldRoute; - final testNavObserver = TestNavigatorObserver() - ..onReplaced = (route, prevRoute) { - assert(newRoute == null && oldRoute == null); - newRoute = route!; - oldRoute = prevRoute!; - }; - // A route to remove is a root level home route - testBinding.globalStore.loadPerAccountException = ZulipApiException( - routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400, - data: {}, message: ''); - await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); - await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); - await tester.pump(); // start to load account - await tester.pump(); // got the error - await tester.pump(TestGlobalStore.removeAccountDuration); - - checkErrorDialog(tester, + await tester.tap(find.byWidget(checkErrorDialog(tester, expectedTitle: 'Could not connect', expectedMessage: 'Your account at https://chat.example/ could not be authenticated.' - ' Please try logging in again or use another account.'); - check(newRoute).isA().page.isA(); - check(oldRoute).isA().page.isA(); - check(testBinding.globalStore.takeDoRemoveAccountCalls()) - .single.equals(eg.selfAccount.id); + ' Please try logging in again or use another account.'))); + check(pushedRoutes).isEmpty(); }); - testWidgets('PerAccountStoreWidget when log out, remove non-root level route', (tester) async { - final testNavObserver = TestNavigatorObserver(); - await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); - await testBinding.globalStore.insertAccount(eg.otherAccount.toCompanion(false)); - await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); - await tester.pump(); // start to load selfAccount - await tester.pump(); // done loading + testWidgets('PerAccountStoreWidget when log out, push route when popping root level route', (tester) async { + addTearDown(testBinding.reset); + final testNavObserver = TestNavigatorObserver(); final pushedRoutes = >[]; final removedRoutes = >[]; testNavObserver.onPushed = (route, prevRoute) => pushedRoutes.add(route); testNavObserver.onRemoved = (route, prevRoute) => removedRoutes.add(route); + // A route to remove is a root level home route + testBinding.globalStore.loadPerAccountDuration = Duration.zero; testBinding.globalStore.loadPerAccountException = ZulipApiException( routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400, data: {}, message: ''); - await openNotification(tester, eg.otherAccount, eg.streamMessage()); - await tester.pump(); + await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); + await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pump(); // start to load account + check(pushedRoutes).single.isA().page.isA(); + pushedRoutes.clear(); + await tester.pump(); // got the error await tester.pump(TestGlobalStore.removeAccountDuration); - checkErrorDialog(tester, + check(pushedRoutes).single.isA>(); + pushedRoutes.clear(); + check(removedRoutes).single.isA().page.isA(); + check(testBinding.globalStore.takeDoRemoveAccountCalls()) + .single.equals(eg.selfAccount.id); + + await tester.tap(find.byWidget(checkErrorDialog(tester, expectedTitle: 'Could not connect', expectedMessage: 'Your account at https://chat.example/ could not be authenticated.' - ' Please try logging in again or use another account.'); - check(testBinding.globalStore.takeDoRemoveAccountCalls()) - .single.equals(eg.otherAccount.id); - check(pushedRoutes).deepEquals(>[ - (it) => it.isA().page.isA(), - (it) => it.isA>(), - ]); + ' Please try logging in again or use another account.'))); + check(pushedRoutes).single.isA().page.isA(); }); testWidgets('PerAccountStoreWidget immediate data after first loaded', (tester) async {