Skip to content

Commit 3150797

Browse files
committed
action_sheet: Add and use PageRoot
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.)
1 parent 350edcc commit 3150797

File tree

4 files changed

+55
-20
lines changed

4 files changed

+55
-20
lines changed

lib/widgets/action_sheet.dart

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import 'emoji_reaction.dart';
2424
import 'icons.dart';
2525
import 'inset_shadow.dart';
2626
import 'message_list.dart';
27+
import 'page.dart';
2728
import 'store.dart';
2829
import 'text.dart';
2930
import 'theme.dart';
@@ -163,11 +164,15 @@ class ActionSheetCancelButton extends StatelessWidget {
163164
}
164165

165166
/// Show a sheet of actions you can take on a topic.
167+
///
168+
/// Needs a [PageRoot] ancestor.
166169
void showTopicActionSheet(BuildContext context, {
167170
required int channelId,
168171
required TopicName topic,
169172
}) {
170-
final store = PerAccountStoreWidget.of(context);
173+
final pageRootContext = PageRoot.contextOf(context);
174+
175+
final store = PerAccountStoreWidget.of(pageRootContext);
171176
final subscription = store.subscriptions[channelId];
172177

173178
final optionButtons = <ActionSheetMenuItemButton>[];
@@ -237,7 +242,7 @@ void showTopicActionSheet(BuildContext context, {
237242
currentVisibilityPolicy: visibilityPolicy,
238243
newVisibilityPolicy: to,
239244
narrow: TopicNarrow(channelId, topic),
240-
pageContext: context);
245+
pageContext: pageRootContext);
241246
}));
242247

243248
if (optionButtons.isEmpty) {
@@ -250,7 +255,7 @@ void showTopicActionSheet(BuildContext context, {
250255
return;
251256
}
252257

253-
_showActionSheet(context, optionButtons: optionButtons);
258+
_showActionSheet(pageRootContext, optionButtons: optionButtons);
254259
}
255260

256261
class UserTopicUpdateButton extends ActionSheetMenuItemButton {
@@ -374,35 +379,36 @@ class UserTopicUpdateButton extends ActionSheetMenuItemButton {
374379

375380
/// Show a sheet of actions you can take on a message in the message list.
376381
///
377-
/// Must have a [MessageListPage] ancestor.
382+
/// Must have a [MessageListPage] ancestor and a [PageRoot] ancestor.
378383
void showMessageActionSheet({required BuildContext context, required Message message}) {
379-
final store = PerAccountStoreWidget.of(context);
384+
final pageRootContext = PageRoot.contextOf(context);
385+
final store = PerAccountStoreWidget.of(pageRootContext);
380386

381387
// The UI that's conditioned on this won't live-update during this appearance
382388
// of the action sheet (we avoid calling composeBoxControllerOf in a build
383389
// method; see its doc).
384390
// So we rely on the fact that isComposeBoxOffered for any given message list
385391
// will be constant through the page's life.
386-
final messageListPage = MessageListPage.ancestorOf(context);
392+
final messageListPage = MessageListPage.ancestorOf(pageRootContext);
387393
final isComposeBoxOffered = messageListPage.composeBoxController != null;
388394

389395
final isMessageRead = message.flags.contains(MessageFlag.read);
390396
final markAsUnreadSupported = store.connection.zulipFeatureLevel! >= 155; // TODO(server-6)
391397
final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead;
392398

393399
final optionButtons = [
394-
ReactionButtons(message: message, pageContext: context),
395-
StarButton(message: message, pageContext: context),
400+
ReactionButtons(message: message, pageContext: pageRootContext),
401+
StarButton(message: message, pageContext: pageRootContext),
396402
if (isComposeBoxOffered)
397-
QuoteAndReplyButton(message: message, pageContext: context),
403+
QuoteAndReplyButton(message: message, pageContext: pageRootContext),
398404
if (showMarkAsUnreadButton)
399-
MarkAsUnreadButton(message: message, pageContext: context),
400-
CopyMessageTextButton(message: message, pageContext: context),
401-
CopyMessageLinkButton(message: message, pageContext: context),
402-
ShareButton(message: message, pageContext: context),
405+
MarkAsUnreadButton(message: message, pageContext: pageRootContext),
406+
CopyMessageTextButton(message: message, pageContext: pageRootContext),
407+
CopyMessageLinkButton(message: message, pageContext: pageRootContext),
408+
ShareButton(message: message, pageContext: pageRootContext),
403409
];
404410

405-
_showActionSheet(context, optionButtons: optionButtons);
411+
_showActionSheet(pageRootContext, optionButtons: optionButtons);
406412
}
407413

408414
abstract class MessageActionSheetMenuItemButton extends ActionSheetMenuItemButton {

lib/widgets/message_list.dart

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
277277
narrow: ChannelNarrow(streamId)))));
278278
}
279279

280-
return Scaffold(
280+
return PageRoot(child: Scaffold(
281281
appBar: ZulipAppBar(
282282
buildTitle: (willCenterTitle) =>
283283
MessageListAppBarTitle(narrow: narrow, willCenterTitle: willCenterTitle),
@@ -318,7 +318,7 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis
318318
))),
319319
if (ComposeBox.hasComposeBox(narrow))
320320
ComposeBox(key: _composeBoxKey, narrow: narrow)
321-
])));
321+
]))));
322322
}
323323
}
324324

lib/widgets/page.dart

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,30 @@ import 'package:flutter/material.dart';
33

44
import 'store.dart';
55

6+
/// An [InheritedWidget] for near the root of a page's widget subtree,
7+
/// providing its [BuildContext].
8+
///
9+
/// Useful when needing a context that persists through the page's lifespan,
10+
/// e.g. for a show-action-sheet function
11+
/// whose buttons use a context to close the sheet
12+
/// or show an error dialog / snackbar asynchronously.
13+
///
14+
/// (In this scenario, it would be buggy to use the context of the element
15+
/// that was long-pressed,
16+
/// if the element can unmount as part of handling a Zulip event.)
17+
class PageRoot extends InheritedWidget {
18+
const PageRoot({super.key, required super.child});
19+
20+
@override
21+
bool updateShouldNotify(covariant PageRoot oldWidget) => false;
22+
23+
static BuildContext contextOf(BuildContext context) {
24+
final element = context.getElementForInheritedWidgetOfExactType<PageRoot>();
25+
assert(element != null, 'No PageRoot ancestor');
26+
return element!;
27+
}
28+
}
29+
630
/// A page route that always builds the same widget.
731
///
832
/// This is useful for making the route more transparent for a test to inspect.
@@ -42,7 +66,8 @@ mixin AccountPageRouteMixin<T extends Object?> on PageRoute<T> {
4266
accountId: accountId,
4367
placeholder: loadingPlaceholderPage ?? const LoadingPlaceholderPage(),
4468
routeToRemoveOnLogout: this,
45-
child: super.buildPage(context, animation, secondaryAnimation));
69+
child: PageRoot(
70+
child: super.buildPage(context, animation, secondaryAnimation)));
4671
}
4772
}
4873

test/widgets/action_sheet_test.dart

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,11 @@ import 'package:zulip/model/narrow.dart';
2121
import 'package:zulip/model/store.dart';
2222
import 'package:zulip/model/typing_status.dart';
2323
import 'package:zulip/widgets/action_sheet.dart';
24+
import 'package:zulip/widgets/app.dart';
2425
import 'package:zulip/widgets/app_bar.dart';
2526
import 'package:zulip/widgets/compose_box.dart';
2627
import 'package:zulip/widgets/content.dart';
2728
import 'package:zulip/widgets/emoji.dart';
28-
import 'package:zulip/widgets/home.dart';
2929
import 'package:zulip/widgets/icons.dart';
3030
import 'package:zulip/widgets/inbox.dart';
3131
import 'package:zulip/widgets/message_list.dart';
@@ -160,9 +160,13 @@ void main() {
160160
]);
161161
}
162162

163-
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
164-
child: const HomePage()));
163+
// ZulipApp instead of TestZulipApp so that we can push a route to the nav,
164+
// exercising HomePage.buildRoute which (via AccountPageRouteMixin)
165+
// makes a PageRoot, which the action sheet needs.
166+
await tester.pumpWidget(ZulipApp());
165167
await tester.pump();
168+
169+
// If this fails, adjust setup so the inbox page is visible.
166170
check(find.byType(InboxPageBody)).findsOne();
167171

168172
await tester.longPress(find.text(topic));

0 commit comments

Comments
 (0)