search

zulip / zulip-flutter

Upcoming Zulip mobile apps for Android and iOS, using Flutter
Apache License 2.0
196 stars 189 forks source link

Clarify action sheet's "page context"; fix small bugs #1044

Closed gnprice closed 2 weeks ago

gnprice commented 2 weeks ago

This was prompted by my comment at https://github.com/zulip/zulip-flutter/pull/1041#discussion_r1828562960 (/cc @PIG208). But then as I got into this code I found some other small cleanups to make, and also spotted a couple of bugs to fix.

Selected commit messages

action_sheet [nfc]: Rename pageContext field on button widgets

Most of the ways we use this field (specifically, all the ways we use it other than in findMessageListPage) aren't related to it being the message list in particular.

When we add action sheets other than the message action sheet, they'll still need a page context. So prepare the way for those by giving this a name that can generalize.

action_sheet [nfc]: Avoid passing footgun context to onPressed methods

The usual Flutter idiom, which we use across the rest of the app, would call for using the parameter called context wherever a BuildContext is required: for getting the store or localizations, or passing to a method like showErrorDialog, and so on.

That idiom doesn't work here, because that context belongs to the action sheet itself, which gets dismissed at the start of the method. Worse, it will sometimes seem to work, nondeterministically: the context only gets unmounted after the animation to dismiss the action sheet is complete, which takes something like 200ms. So the temptation to use it has been a recurring source of small bugs.

Fix the problem by not passing the action sheet's context to these methods at all.

action_sheet: Mark as unread in current narrow, vs. from when action sheet opened

The narrow of a given MessageListPage can change over time, in particular if viewing a topic that gets moved.

If it does, the mark-unread action should follow along. Otherwise we'd have a frustrating race where mark-unread just didn't work if, for example, someone happened to resolve the topic while you were in the middle of trying to mark as unread.

Fixes: #1045

gnprice commented 2 weeks ago

Thanks @PIG208 for the detailed review! Pushed a revision, and replied to some subthreads above; PTAL.

PIG208 commented 2 weeks ago

LGTM. Thanks for the update!

gnprice commented 2 weeks ago

Thanks! Merging.