diff --git a/lib/notifications/open.dart b/lib/notifications/open.dart index 2813c79ce0..5dba2ecdf8 100644 --- a/lib/notifications/open.dart +++ b/lib/notifications/open.dart @@ -11,6 +11,7 @@ import '../host/notifications.dart'; import '../log.dart'; import '../model/binding.dart'; import '../model/narrow.dart'; +import '../model/store.dart' show Account; import '../widgets/app.dart'; import '../widgets/dialog.dart'; import '../widgets/home.dart'; @@ -90,13 +91,13 @@ class NotificationOpenService { return routeForNotification(context: context, data: notifNavData); } - /// Provides the route to open by parsing the notification payload. + /// Finds the account associated with the given notification. /// /// Returns null and shows an error dialog if the associated account is not /// found in the global store. /// /// The context argument should be a descendant of the app's main [Navigator]. - static AccountRoute? routeForNotification({ + static Account? _accountForNotification({ required BuildContext context, required NotificationOpenPayload data, }) { @@ -112,6 +113,21 @@ class NotificationOpenService { message: zulipLocalizations.errorNotificationOpenAccountNotFound); return null; } + return account; + } + + /// Provides the route to open by parsing the notification payload. + /// + /// Returns null and shows an error dialog if the associated account is not + /// found in the global store. + /// + /// The context argument should be a descendant of the app's main [Navigator]. + static AccountRoute? routeForNotification({ + required BuildContext context, + required NotificationOpenPayload data, + }) { + final account = _accountForNotification(context: context, data: data); + if (account == null) return null; return MessageListPage.buildRoute( accountId: account.id, @@ -119,9 +135,46 @@ class NotificationOpenService { narrow: data.narrow); } - /// Navigates to the [MessageListPage] of the specific conversation - /// for the provided payload that was attached while creating the - /// notification. + /// Navigate appropriately for opening the given notification. + static void _navigateForNotificationPayload( + NavigatorState navigator, NotificationOpenPayload data) { + assert(navigator.mounted); + final context = navigator.context; + final navStack = ZulipApp.navigationStack!; + + final account = _accountForNotification(context: context, data: data); + if (account == null) return; // TODO(log) + + final currentPageRoute = navStack.currentPageRoute; + if (currentPageRoute is MaterialAccountWidgetRoute + && currentPageRoute.accountId == account.id + && currentPageRoute.page is MessageListPage + && MessageListPage.currentNarrow(currentPageRoute) == data.narrow + ) { + // The current page is already a MessageListPage at the desired narrow. + // Instead of pushing another copy of it, stay there; see #1852. + + // Do dismiss any non-page routes, like dialogs and bottom sheets, though. + // That way we're presenting the page directly, like the user asked for + // by opening the notification. + navigator.popUntil((route) => route is PageRoute); + + // TODO(#1565): Scroll to the specific message if nearby; else jump there, + // or push a new page anchored there. + return; + } + + if (navStack.currentAccountId != account.id) { + HomePage.navigate(context, accountId: account.id); + } + unawaited(navigator.push(MessageListPage.buildRoute( + accountId: account.id, + // TODO(#1565): Open at specific message, not just conversation + narrow: data.narrow))); + } + + /// Navigate appropriately for opening the notification described by + /// the given [NotificationTapEvent]. static Future _navigateForNotification(NotificationTapEvent event) async { assert(defaultTargetPlatform == TargetPlatform.iOS); assert(debugLog('opened notif: ${jsonEncode(event.payload)}')); @@ -133,19 +186,15 @@ class NotificationOpenService { final notifNavData = _tryParseIosApnsPayload(context, event.payload); if (notifNavData == null) return; // TODO(log) - final route = routeForNotification(context: context, data: notifNavData); - if (route == null) return; // TODO(log) - - if (GlobalStoreWidget.of(context).lastVisitedAccount?.id != route.accountId) { - HomePage.navigate(context, accountId: route.accountId); - } - unawaited(navigator.push(route)); + _navigateForNotificationPayload(navigator, notifNavData); } - /// Navigates to the [MessageListPage] of the specific conversation - /// given the `zulip://notification/…` Android intent data URL, - /// generated with [NotificationOpenPayload.buildAndroidNotificationUrl] - /// while creating the notification. + /// Navigate appropriately for opening the notification described by + /// the given `zulip://notification/…` Android intent data URL. + /// + /// The URL should have been generated with + /// [NotificationOpenPayload.buildAndroidNotificationUrl] + /// when creating the notification. static Future navigateForAndroidNotificationUrl(Uri url) async { assert(defaultTargetPlatform == TargetPlatform.android); assert(debugLog('opened notif: url: $url')); @@ -158,13 +207,7 @@ class NotificationOpenService { assert(url.scheme == 'zulip' && url.host == 'notification'); final data = tryParseAndroidNotificationUrl(context: context, url: url); if (data == null) return; // TODO(log) - final route = routeForNotification(context: context, data: data); - if (route == null) return; // TODO(log) - - if (GlobalStoreWidget.of(context).lastVisitedAccount?.id != route.accountId) { - HomePage.navigate(context, accountId: route.accountId); - } - unawaited(navigator.push(route)); + _navigateForNotificationPayload(navigator, data); } static NotificationOpenPayload? _tryParseIosApnsPayload( diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 69dd62d7c9..9bb004dc31 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -78,6 +78,18 @@ class ZulipApp extends StatefulWidget { return ScaffoldMessenger.of(context); } + /// The app's stack of navigation routes. + /// + /// This is null when the navigator is not mounted, + /// i.e. until [ready] becomes true. + static NavigationStack? get navigationStack { + final navigatorState = navigatorKey.currentState; + if (navigatorState == null) return null; + final appState = navigatorState.context + .findAncestorStateOfType<_ZulipAppState>()!; + return appState._navStackTracker; + } + /// Reset the state of [ZulipApp] statics, for testing. /// /// TODO refactor this better, perhaps unify with ZulipBinding @@ -156,6 +168,8 @@ class ZulipApp extends StatefulWidget { } class _ZulipAppState extends State with WidgetsBindingObserver { + final _navStackTracker = _TrackNavigationStack(); + @override void initState() { super.initState(); @@ -255,6 +269,7 @@ class _ZulipAppState extends State with WidgetsBindingObserver { if (widget.navigatorObservers != null) ...widget.navigatorObservers!, _PreventEmptyStack(), + _navStackTracker, _UpdateLastVisitedAccount(GlobalStoreWidget.of(context)), ], builder: (BuildContext context, Widget? child) { @@ -280,6 +295,87 @@ class _ZulipAppState extends State with WidgetsBindingObserver { } } +/// A view of the app's navigation stack, plus convenient helpers to inspect it. +mixin NavigationStack { + List> get routes; + + /// The Zulip account ID being viewed topmost in the navigation, if any. + /// + /// Typically there should be only one account present in the nav stack + /// at a time, in which case this is that one account's ID. + int? get currentAccountId { + for (final route in routes.reversed) { + if (route case AccountPageRouteMixin(:final accountId)) { + return accountId; + } + } + return null; + } + + /// The topmost page route on the stack, if any. + /// + /// In particular this excludes dialogs and modal bottom sheets. + PageRoute? get currentPageRoute { + for (final route in routes.reversed) { + switch (route) { + case PageRoute(): + return route; + + case PopupRoute(): + // This case includes dialogs and modal bottom sheets. + continue; + + default: + // TODO(log) All known concrete Route subclasses are either of + // PageRoute or PopupRoute. If something else appears, we should + // decide how the callers of this method want to treat it. + continue; + } + } + return null; + } +} + +// TODO(upstream): why doesn't Navigator expose the list of routes itself? +class _TrackNavigationStack extends NavigatorObserver with NavigationStack { + @override + final List> routes = []; + + @override + void didPush(Route route, Route? previousRoute) { + assert(identical(routes.lastOrNull, previousRoute)); + routes.add(route); + } + + @override + void didPop(Route route, Route? previousRoute) { + assert(identical(routes.lastOrNull, route)); + routes.removeLast(); + assert(identical(routes.lastOrNull, previousRoute)); + } + + @override + void didRemove(Route route, Route? previousRoute) { + final index = routes.lastIndexOf(route); + assert(index >= 0); + routes.removeAt(index); + assert((previousRoute == null && index == 0) + || (previousRoute != null && routes[index - 1] == previousRoute)); + } + + @override + void didReplace({Route? newRoute, Route? oldRoute}) { + assert(newRoute != null); // TODO(upstream) why doesn't signature say this? + assert(oldRoute != null); // TODO(upstream) why doesn't signature say this? + final index = routes.lastIndexOf(oldRoute!); + assert(index >= 0); + routes[index] = newRoute!; + } + + // No didChangeTop; it summarizes changes that the observer was already + // notified of through the other methods above. +} + /// Pushes a route whenever the observed navigator stack becomes empty. class _PreventEmptyStack extends NavigatorObserver { void _pushRouteIfEmptyStack() async { diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 9678728987..1bc3bc2cc2 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -199,6 +199,41 @@ class MessageListPage extends StatefulWidget { initAnchorMessageId: initAnchorMessageId)); } + /// The [MessageListPageState] for the page at the given route. + /// + /// The route must be a [WidgetRoute] for [MessageListPage]. + /// + /// Null if the route is not mounted in the widget tree. + static MessageListPageState? stateOfRoute(Route route) { + if (!(route is WidgetRoute && route.page is MessageListPage)) { + assert(false, 'MessageListPage.stateOfRoute expects a MessageListPage route'); + return null; + } + final element = route.pageElement; + if (element == null) return null; + assert(element.widget == route.page); + + return (element as StatefulElement).state as MessageListPageState; + } + + /// The current narrow, as updated, for the given [MessageListPage] route. + /// + /// The route must be a [WidgetRoute] for [MessageListPage]. + /// + /// This uses [MessageListPageState.narrow] to take into account any updates + /// that have happened since the route was navigated to. + static Narrow currentNarrow(Route route) { + final state = stateOfRoute(route); + if (state == null) { + // The page is not yet mounted. Either the route has not yet been + // navigated to, or there hasn't yet been a new frame since it was. + // Either way, there's been no change to its narrow. + return ((route as WidgetRoute).page as MessageListPage).initNarrow; + } + // The page is mounted, and may have changed its narrow. + return state.narrow; + } + /// The "revealed" state of a message from a muted sender, /// if there is a [MessageListPage] ancestor, else null. /// diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart index af9e9e3a80..727844fe21 100644 --- a/lib/widgets/page.dart +++ b/lib/widgets/page.dart @@ -30,10 +30,32 @@ class PageRoot extends InheritedWidget { /// A page route that always builds the same widget. /// -/// This is useful for making the route more transparent for a test to inspect. -abstract class WidgetRoute extends PageRoute { +/// In addition to the [pageElement] getter, +/// this is useful for making the route more transparent for a test to inspect. +mixin WidgetRoute on PageRoute { /// The widget that this page route always builds. Widget get page; + + /// The element built from [page] for this route. + /// + /// Null if the route is not mounted in the widget tree. + Element? get pageElement { + final context = subtreeContext; + if (context == null) return null; + // Now subtreeContext is an element built by the ModalRoute implementation + // which tightly encloses the element built from [page]. + + Element? result; + void visitor(Element element) { + if (element.widget == page) { + result = element; + } else { + element.visitChildElements(visitor); + } + } + context.visitChildElements(visitor); + return result!; + } } /// A page route that specifies a particular Zulip account to use, by ID. @@ -49,7 +71,7 @@ abstract class AccountRoute extends PageRoute { /// See also: /// * [MaterialAccountWidgetRoute], a subclass which automates providing a /// per-account store on the new route. -class MaterialWidgetRoute extends MaterialPageRoute implements WidgetRoute { +class MaterialWidgetRoute extends MaterialPageRoute with WidgetRoute { MaterialWidgetRoute({ required this.page, super.settings, @@ -131,7 +153,7 @@ class MaterialAccountPageRoute extends MaterialPageRoute w /// /// See also: /// * [MaterialWidgetRoute], for routes that need no per-account store. -class MaterialAccountWidgetRoute extends MaterialAccountPageRoute implements WidgetRoute { +class MaterialAccountWidgetRoute extends MaterialAccountPageRoute with WidgetRoute { /// Construct a [MaterialAccountWidgetRoute] using either the given account ID, /// or the ambient one from the given context. /// diff --git a/test/notifications/open_test.dart b/test/notifications/open_test.dart index 509e692f21..1f7a6d0821 100644 --- a/test/notifications/open_test.dart +++ b/test/notifications/open_test.dart @@ -6,22 +6,27 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/notifications.dart'; +import 'package:zulip/api/route/channels.dart'; import 'package:zulip/host/notifications.dart'; -import 'package:zulip/model/database.dart'; import 'package:zulip/model/localizations.dart'; import 'package:zulip/model/narrow.dart'; import 'package:zulip/model/push_device.dart'; +import 'package:zulip/model/store.dart'; import 'package:zulip/notifications/open.dart'; import 'package:zulip/notifications/receive.dart'; import 'package:zulip/widgets/app.dart'; import 'package:zulip/widgets/home.dart'; +import 'package:zulip/widgets/icons.dart'; import 'package:zulip/widgets/message_list.dart'; import 'package:zulip/widgets/page.dart'; +import 'package:zulip/widgets/topic_list.dart'; +import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../model/binding.dart'; import '../model/narrow_checks.dart'; import '../model/store_checks.dart'; +import '../model/test_store.dart'; import '../stdlib_checks.dart'; import '../test_navigation.dart'; import '../widgets/checks.dart'; @@ -74,6 +79,7 @@ Map messageApnsPayload( void main() { TestZulipBinding.ensureInitialized(); + MessageListPage.debugEnableMarkReadOnScroll = false; final zulipLocalizations = GlobalLocalizations.zulipLocalizations; Future init() async { @@ -87,6 +93,7 @@ void main() { } group('NotificationOpenService', () { + late TransitionDurationObserver transitionDurationObserver; late List> pushedRoutes; Route? lastPoppedRoute; @@ -119,9 +126,11 @@ void main() { lastPoppedRoute = prevRoute; pushedRoutes.add(route!); }; + transitionDurationObserver = TransitionDurationObserver(); // This uses [ZulipApp] instead of [TestZulipApp] because notification // logic uses `await ZulipApp.navigator`. - await tester.pumpWidget(ZulipApp(navigatorObservers: [testNavObserver])); + await tester.pumpWidget(ZulipApp( + navigatorObservers: [transitionDurationObserver, testNavObserver])); if (early) { check(pushedRoutes).isEmpty(); return; @@ -245,10 +254,10 @@ void main() { await checkOpenNotification(tester, eg.selfAccount.copyWith(realmUrl: Uri.parse('http://chat.example/')), - eg.streamMessage()); + eg.streamMessage(topic: 'a')); await checkOpenNotification(tester, eg.selfAccount.copyWith(realmUrl: Uri.parse('http://chat.example')), - eg.streamMessage()); + eg.streamMessage(topic: 'b')); }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); testWidgets('no accounts', (tester) async { @@ -362,6 +371,102 @@ void main() { matchesNavigation(check(pushedRoutes).single, accountB, message); }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); + group('from MessageListPage', () { + late PerAccountStore store; + late FakeApiConnection connection; + late NavigatorState navigator; + + Future prepareMessageListPage(WidgetTester tester, { + required Narrow narrow, + required List messages, + }) async { + addTearDown(testBinding.reset); + await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); + await prepare(tester); + store = await testBinding.globalStore.perAccount(eg.selfAccount.id); + connection = store.connection as FakeApiConnection; + + navigator = await ZulipApp.navigator; + unawaited(navigator.push(MessageListPage.buildRoute( + accountId: eg.selfAccount.id, + narrow: narrow))); + pushedRoutes.clear(); + connection.prepare(json: eg.newestGetMessagesResult( + foundOldest: true, messages: messages).toJson()); + await tester.pump(); + await transitionDurationObserver.pumpPastTransition(tester); + } + + testWidgets('at same conversation, dedupes page', (tester) async { + final stream = eg.stream(); + final message1 = eg.streamMessage(stream: stream, topic: 'a'); + await prepareMessageListPage(tester, + narrow: TopicNarrow.ofMessage(message1), + messages: [message1]); + + final message2 = eg.streamMessage(stream: stream, topic: 'a'); + await openNotification(tester, eg.selfAccount, message2); + check(lastPoppedRoute).isNull(); + check(pushedRoutes).isEmpty(); + }); + + testWidgets('at different conversation, proceeds normally', (tester) async { + final stream = eg.stream(); + final message1 = eg.streamMessage(stream: stream, topic: 'a'); + await prepareMessageListPage(tester, + narrow: TopicNarrow.ofMessage(message1), + messages: [message1]); + + final message2 = eg.streamMessage(stream: stream, topic: 'b'); + await openNotification(tester, eg.selfAccount, message2); + check(lastPoppedRoute).isNull(); + matchesNavigation(check(pushedRoutes).single, eg.selfAccount, message2); + }); + + testWidgets('with a different page on top, proceeds normally', (tester) async { + final stream = eg.stream(); + final message1 = eg.streamMessage(stream: stream, topic: 'a'); + await prepareMessageListPage(tester, + narrow: TopicNarrow.ofMessage(message1), + messages: [message1]); + + connection.prepare(json: GetChannelTopicsResult(topics: []).toJson()); + await tester.tap(find.byIcon(ZulipIcons.topics)); + await tester.pump(); + check(pushedRoutes.single).isA().page.isA(); + pushedRoutes.clear(); + + final message2 = eg.streamMessage(stream: stream, topic: 'a'); + await openNotification(tester, eg.selfAccount, message2); + check(lastPoppedRoute).isNull(); + matchesNavigation(check(pushedRoutes).single, eg.selfAccount, message2); + }); + + testWidgets('with a dialog on top, dedupes but clears dialog', (tester) async { + final stream = eg.stream(); + final message1 = eg.streamMessage(stream: stream, topic: 'a'); + await prepareMessageListPage(tester, + narrow: TopicNarrow.ofMessage(message1), + messages: [message1]); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await tester.pump(); + + // Produce an error dialog by attempting to send (with an empty compose box). + await tester.tap(find.byIcon(ZulipIcons.send)); + await tester.pump(); + final pushed = pushedRoutes.single; + check(pushed).isA>(); + pushedRoutes.clear(); + + final message2 = eg.streamMessage(stream: stream, topic: 'a'); + await openNotification(tester, eg.selfAccount, message2); + // The dialog was popped (and nothing was pushed). + check(lastPoppedRoute).equals(pushed); + check(pushedRoutes).isEmpty(); + }); + }); + group('changes last visited account', () { testWidgets('app already opened, then notification is opened', (tester) async { addTearDown(testBinding.reset); @@ -414,10 +519,10 @@ void main() { await checkOpenNotification(tester, eg.otherAccount, eg.streamMessage(), expectHomePageReplaced: false); // Different account -> replace HomePage. - await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage(), + await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage(topic: 'a'), expectHomePageReplaced: true); // Remain on that different account -> keep the new nav stack in place. - await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage(), + await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage(topic: 'b'), expectHomePageReplaced: false); }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); });