Dedupe message-list pages on opening a notification

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<void>? routeForNotification({
+  static Account? _accountForNotification({
     required BuildContext context,
     required NotificationOpenPayload data,
   }) {
@@ -112,16 +113,68 @@ 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<void>? 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,
       // TODO(#1565): Open at specific message, not just conversation
       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<void> _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<void> 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(

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<ZulipApp> with WidgetsBindingObserver {
+  final _navStackTracker = _TrackNavigationStack();
+
   @override
   void initState() {
     super.initState();
@@ -255,6 +269,7 @@ class _ZulipAppState extends State<ZulipApp> 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<ZulipApp> with WidgetsBindingObserver {
   }
 }
 
+/// A view of the app's navigation stack, plus convenient helpers to inspect it.
+mixin NavigationStack {
+  List<Route<dynamic>> 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<dynamic>? 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<Route<dynamic>> routes = [];
+
+  @override
+  void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) {
+    assert(identical(routes.lastOrNull, previousRoute));
+    routes.add(route);
+  }
+
+  @override
+  void didPop(Route<dynamic> route, Route<dynamic>? previousRoute) {
+    assert(identical(routes.lastOrNull, route));
+    routes.removeLast();
+    assert(identical(routes.lastOrNull, previousRoute));
+  }
+
+  @override
+  void didRemove(Route<dynamic> route, Route<dynamic>? 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<dynamic>? newRoute, Route<dynamic>? 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 {

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<void> 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<void> 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.
   ///

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<T extends Object?> extends PageRoute<T> {
+/// In addition to the [pageElement] getter,
+/// this is useful for making the route more transparent for a test to inspect.
+mixin WidgetRoute<T extends Object?> on PageRoute<T> {
   /// 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<T extends Object?> extends PageRoute<T> {
 /// See also:
 ///  * [MaterialAccountWidgetRoute], a subclass which automates providing a
 ///    per-account store on the new route.
-class MaterialWidgetRoute<T extends Object?> extends MaterialPageRoute<T> implements WidgetRoute<T> {
+class MaterialWidgetRoute<T extends Object?> extends MaterialPageRoute<T> with WidgetRoute<T> {
   MaterialWidgetRoute({
     required this.page,
     super.settings,
@@ -131,7 +153,7 @@ class MaterialAccountPageRoute<T extends Object?> extends MaterialPageRoute<T> w
 ///
 /// See also:
 ///  * [MaterialWidgetRoute], for routes that need no per-account store.
-class MaterialAccountWidgetRoute<T extends Object?> extends MaterialAccountPageRoute<T> implements WidgetRoute<T> {
+class MaterialAccountWidgetRoute<T extends Object?> extends MaterialAccountPageRoute<T> with WidgetRoute<T> {
   /// Construct a [MaterialAccountWidgetRoute] using either the given account ID,
   /// or the ambient one from the given context.
   ///