action_sheet: Add and use PageRoot

chrisbobbe · chrisbobbe · commit 6ffba757d86b · 2025-01-28T17:35:03.000-08:00
Without this, the upcoming resolve/unresolve feature would have a
bug where certain error dialogs would fail to appear. (Specifically
the ones that explain to the user that a topic has changed between
opening the action sheet and pressing the resolve/unresolve button.)
diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart
@@ -24,6 +24,7 @@ import 'emoji_reaction.dart';
 import 'icons.dart';
 import 'inset_shadow.dart';
 import 'message_list.dart';
+import 'page.dart';
 import 'store.dart';
 import 'text.dart';
 import 'theme.dart';
@@ -163,11 +164,15 @@ class ActionSheetCancelButton extends StatelessWidget {
 }
 
 /// Show a sheet of actions you can take on a topic.
+///
+/// Needs a [PageRoot] ancestor.
 void showTopicActionSheet(BuildContext context, {
   required int channelId,
   required TopicName topic,
 }) {
-  final store = PerAccountStoreWidget.of(context);
+  final pageRootContext = PageRoot.contextOf(context);
+
+  final store = PerAccountStoreWidget.of(pageRootContext);
   final subscription = store.subscriptions[channelId];
 
   final optionButtons = <ActionSheetMenuItemButton>[];
@@ -237,7 +242,7 @@ void showTopicActionSheet(BuildContext context, {
       currentVisibilityPolicy: visibilityPolicy,
       newVisibilityPolicy: to,
       narrow: TopicNarrow(channelId, topic),
-      pageContext: context);
+      pageContext: pageRootContext);
   }));
 
   if (optionButtons.isEmpty) {
@@ -250,7 +255,7 @@ void showTopicActionSheet(BuildContext context, {
     return;
   }
 
-  _showActionSheet(context, optionButtons: optionButtons);
+  _showActionSheet(pageRootContext, optionButtons: optionButtons);
 }
 
 class UserTopicUpdateButton extends ActionSheetMenuItemButton {
@@ -374,35 +379,36 @@ class UserTopicUpdateButton extends ActionSheetMenuItemButton {
 
 /// Show a sheet of actions you can take on a message in the message list.
 ///
-/// Must have a [MessageListPage] ancestor.
+/// Must have a [MessageListPage] ancestor and a [PageRoot] ancestor.
 void showMessageActionSheet({required BuildContext context, required Message message}) {
-  final store = PerAccountStoreWidget.of(context);
+  final pageRootContext = PageRoot.contextOf(context);
+  final store = PerAccountStoreWidget.of(pageRootContext);
 
   // The UI that's conditioned on this won't live-update during this appearance
   // of the action sheet (we avoid calling composeBoxControllerOf in a build
   // method; see its doc).
   // So we rely on the fact that isComposeBoxOffered for any given message list
   // will be constant through the page's life.
-  final messageListPage = MessageListPage.ancestorOf(context);
+  final messageListPage = MessageListPage.ancestorOf(pageRootContext);
   final isComposeBoxOffered = messageListPage.composeBoxController != null;
 
   final isMessageRead = message.flags.contains(MessageFlag.read);
   final markAsUnreadSupported = store.connection.zulipFeatureLevel! >= 155; // TODO(server-6)
   final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead;
 
   final optionButtons = [
-    ReactionButtons(message: message, pageContext: context),
-    StarButton(message: message, pageContext: context),
+    ReactionButtons(message: message, pageContext: pageRootContext),
+    StarButton(message: message, pageContext: pageRootContext),
     if (isComposeBoxOffered)
-      QuoteAndReplyButton(message: message, pageContext: context),
+      QuoteAndReplyButton(message: message, pageContext: pageRootContext),
     if (showMarkAsUnreadButton)
-      MarkAsUnreadButton(message: message, pageContext: context),
-    CopyMessageTextButton(message: message, pageContext: context),
-    CopyMessageLinkButton(message: message, pageContext: context),
-    ShareButton(message: message, pageContext: context),
+      MarkAsUnreadButton(message: message, pageContext: pageRootContext),
+    CopyMessageTextButton(message: message, pageContext: pageRootContext),
+    CopyMessageLinkButton(message: message, pageContext: pageRootContext),
+    ShareButton(message: message, pageContext: pageRootContext),
   ];
 
-  _showActionSheet(context, optionButtons: optionButtons);
+  _showActionSheet(pageRootContext, optionButtons: optionButtons);
 }
 
 abstract class MessageActionSheetMenuItemButton extends ActionSheetMenuItemButton {
diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
@@ -277,48 +277,51 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
             narrow: ChannelNarrow(streamId)))));
     }
 
-    return Scaffold(
-      appBar: ZulipAppBar(
-        buildTitle: (willCenterTitle) =>
-          MessageListAppBarTitle(narrow: narrow, willCenterTitle: willCenterTitle),
-        actions: actions,
-        backgroundColor: appBarBackgroundColor,
-        shape: removeAppBarBottomBorder
-          ? const Border()
-          : null, // i.e., inherit
-      ),
-      // TODO question for Vlad: for a stream view, should we set the Scaffold's
-      //   [backgroundColor] based on stream color, as in this frame:
-      //     https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9684&mode=dev
-      //   That's not obviously preferred over the default background that
-      //   we matched to the Figma in 21dbae120. See another frame, which uses that:
-      //     https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=147%3A9088&mode=dev
-      body: Builder(
-        builder: (BuildContext context) => Column(
-          // Children are expected to take the full horizontal space
-          // and handle the horizontal device insets.
-          // The bottom inset should be handled by the last child only.
-          children: [
-            MediaQuery.removePadding(
-              // Scaffold knows about the app bar, and so has run this
-              // BuildContext, which is under `body`, through
-              // MediaQuery.removePadding with `removeTop: true`.
-              context: context,
-
-              // The compose box, when present, pads the bottom inset.
-              // TODO(#311) If we have a bottom nav, it will pad the bottom
-              //   inset, and this should always be true.
-              removeBottom: ComposeBox.hasComposeBox(narrow),
-
-              child: Expanded(
-                child: MessageList(
-                  key: _messageListKey,
-                  narrow: narrow,
-                  onNarrowChanged: _narrowChanged,
-                ))),
-            if (ComposeBox.hasComposeBox(narrow))
-              ComposeBox(key: _composeBoxKey, narrow: narrow)
-          ])));
+    // The Builder enables MessageListPage.ancestorOf
+    // on the context provided to the message action sheet by PageRoot.
+    return Builder(builder: (context) => PageRoot(context: context,
+      child: Scaffold(
+        appBar: ZulipAppBar(
+          buildTitle: (willCenterTitle) =>
+            MessageListAppBarTitle(narrow: narrow, willCenterTitle: willCenterTitle),
+          actions: actions,
+          backgroundColor: appBarBackgroundColor,
+          shape: removeAppBarBottomBorder
+            ? const Border()
+            : null, // i.e., inherit
+        ),
+        // TODO question for Vlad: for a stream view, should we set the Scaffold's
+        //   [backgroundColor] based on stream color, as in this frame:
+        //     https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9684&mode=dev
+        //   That's not obviously preferred over the default background that
+        //   we matched to the Figma in 21dbae120. See another frame, which uses that:
+        //     https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=147%3A9088&mode=dev
+        body: Builder(
+          builder: (BuildContext context) => Column(
+            // Children are expected to take the full horizontal space
+            // and handle the horizontal device insets.
+            // The bottom inset should be handled by the last child only.
+            children: [
+              MediaQuery.removePadding(
+                // Scaffold knows about the app bar, and so has run this
+                // BuildContext, which is under `body`, through
+                // MediaQuery.removePadding with `removeTop: true`.
+                context: context,
+
+                // The compose box, when present, pads the bottom inset.
+                // TODO(#311) If we have a bottom nav, it will pad the bottom
+                //   inset, and this should always be true.
+                removeBottom: ComposeBox.hasComposeBox(narrow),
+
+                child: Expanded(
+                  child: MessageList(
+                    key: _messageListKey,
+                    narrow: narrow,
+                    onNarrowChanged: _narrowChanged,
+                  ))),
+              if (ComposeBox.hasComposeBox(narrow))
+                ComposeBox(key: _composeBoxKey, narrow: narrow)
+            ])))));
   }
 }
 
diff --git a/lib/widgets/page.dart b/lib/widgets/page.dart
@@ -3,6 +3,33 @@ import 'package:flutter/material.dart';
 
 import 'store.dart';
 
+/// An [InheritedWidget] for near the root of a page's widget subtree,
+/// providing the given [context].
+///
+/// Useful when needing a context that persists through the page's lifespan,
+/// e.g. for a show-action-sheet function
+/// whose buttons use a context to close the sheet
+/// or show an error dialog / snackbar asynchronously.
+///
+/// (In this scenario, it would be buggy to use the context of the element
+/// that was long-pressed if Zulip events can cause the element to unmount,
+/// e.g., a delete-message event.)
+class PageRoot extends InheritedWidget {
+  const PageRoot({super.key, required this.context, required super.child});
+
+  final BuildContext context;
+
+  @override
+  bool updateShouldNotify(covariant PageRoot oldWidget) =>
+    !identical(oldWidget.context, context);
+
+  static BuildContext contextOf(BuildContext context) {
+    final widget = context.dependOnInheritedWidgetOfExactType<PageRoot>();
+    assert(widget != null, 'No PageRoot ancestor');
+    return widget!.context;
+  }
+}
+
 /// A page route that always builds the same widget.
 ///
 /// This is useful for making the route more transparent for a test to inspect.
@@ -42,7 +69,10 @@ mixin AccountPageRouteMixin<T extends Object?> on PageRoute<T> {
       accountId: accountId,
       placeholder: loadingPlaceholderPage ?? const LoadingPlaceholderPage(),
       routeToRemoveOnLogout: this,
-      child: super.buildPage(context, animation, secondaryAnimation));
+      // The Builder enables PerAccountStoreWidget.of
+      // on the context provided by PageRoot (e.g. to action-sheet buttons).
+      child: Builder(builder: (context) => PageRoot(context: context,
+        child: super.buildPage(context, animation, secondaryAnimation))));
   }
 }
 
diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart
@@ -21,11 +21,11 @@ import 'package:zulip/model/narrow.dart';
 import 'package:zulip/model/store.dart';
 import 'package:zulip/model/typing_status.dart';
 import 'package:zulip/widgets/action_sheet.dart';
+import 'package:zulip/widgets/app.dart';
 import 'package:zulip/widgets/app_bar.dart';
 import 'package:zulip/widgets/compose_box.dart';
 import 'package:zulip/widgets/content.dart';
 import 'package:zulip/widgets/emoji.dart';
-import 'package:zulip/widgets/home.dart';
 import 'package:zulip/widgets/icons.dart';
 import 'package:zulip/widgets/inbox.dart';
 import 'package:zulip/widgets/message_list.dart';
@@ -160,9 +160,13 @@ void main() {
         ]);
       }
 
-      await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
-        child: const HomePage()));
+      // ZulipApp instead of TestZulipApp so that we can push a route to the nav,
+      // exercising HomePage.buildRoute which (via AccountPageRouteMixin)
+      // makes a PageRoot, which the action sheet needs.
+      await tester.pumpWidget(ZulipApp());
       await tester.pump();
+
+      // If this fails, adjust setup so the inbox page is visible.
       check(find.byType(InboxPageBody)).findsOne();
 
       await tester.longPress(find.text(topic));
