Clarify action sheet's "page context"; fix small bugs #1044
Conversation
gnprice
force-pushed
the
pr-action-sheet
branch
from
adc5923 to
ef09133
Compare
| @@ -315,8 +324,7 @@ class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { | |||
| // This will be null only if the compose box disappeared after the | |||
| // message action sheet opened, and before "Quote and reply" was pressed. | |||
| // Currently a compose box can't ever disappear, so this is impossible. | |||
| ComposeBoxController composeBoxController = | |||
| MessageListPage.ancestorOf(messageListContext).composeBoxController!; | |||
| var composeBoxController = findMessageListPage().composeBoxController!; | |||
There was a problem hiding this comment.
Is var used for fitting this in one line?
There was a problem hiding this comment.
More that specifying the type feels redundant. It'd look like this:
Well, the alternative is:
ComposeBoxController composeBoxController =
findMessageListPage().composeBoxController!;(or the same with findMessageListPage() moved before the newline).
Our usual practice is to say final and let the type be inferred. Here it's not final because it gets updated below; so var is the choice that corresponds to that.
lib/widgets/action_sheet.dart
Outdated
| /// don't call this in a build method. | ||
| MessageListPageState findMessageListPage() { | ||
| assert(messageListContext.mounted, | ||
| "findMessageListPage should be called only when messageListContext is known to still be mounted"); |
There was a problem hiding this comment.
nit: let's use '' for consistency
test/widgets/action_sheet_test.dart
Outdated
| anchor: message.id, | ||
| foundNewest: true, | ||
| foundOldest: true, | ||
| foundAnchor: true, |
There was a problem hiding this comment.
Does the test rely on foundAnchor: true or anchor: message.id? If not, eg.newestResult suffices.
There was a problem hiding this comment.
Yeah, newestResult would be an improvement. (Though it doesn't currently live in eg; maybe it should, with a longer name in that case.)
Adding a separate prep commit to switch to that.
| ..method.equals('POST') | ||
| ..url.path.equals('/api/v1/messages/flags/narrow') | ||
| ..bodyFields['narrow'].equals( | ||
| jsonEncode(TopicNarrow(newStream.streamId, newTopic).apiEncode())); |
There was a problem hiding this comment.
This looks right to me. Do you mean indenting it like this?
..bodyFields['narrow'].equals(
jsonEncode(TopicNarrow(newStream.streamId, newTopic).apiEncode()));That'd make bodyFields and jsonEncode appear to be at the same level, which isn't desired.
FWIW dartfmt agrees with my indentation (though it moves jsonEncode( to before the newline). I checked with a quick Ctrl+Alt+L after selecting the surrounding few lines.
test/widgets/action_sheet_test.dart
Outdated
| const topic = 'old topic'; | ||
| final message = eg.streamMessage(flags: [MessageFlag.read], | ||
| stream: stream, topic: topic); | ||
| await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); |
There was a problem hiding this comment.
nit: it might be good to wrap this line because narrow is particularly interesting to this test
| stream: stream, topic: topic); | ||
| await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); | ||
| // Get the action sheet fully deployed while the old narrow applies. | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
Hmm, the test passes and would fail as expected without the fix, if I remove this. I see that setupToMessageActionSheet calls tester.pump with a 250ms delay. Maybe that's sufficient?
There was a problem hiding this comment.
I'll expand the comment. Does this version explain why this call is here?
// Get the action sheet fully deployed while the old narrow applies.
// (This way we maximize the range of potential bugs this test can catch,
// by giving the code maximum opportunity to latch onto the old topic.)
await tester.pumpAndSettle();
test/widgets/action_sheet_test.dart
Outdated
| // it doesn't matter anyway: [MessageStoreImpl.reconcileMessages] will | ||
| // keep the version updated by the event. If that somehow changes in | ||
| // some future refactor, it'll cause this test to fail. | ||
| connection.prepare(json: _getMessagesResult(message).toJson()); |
There was a problem hiding this comment.
For tests where we need to prepare the new messages, I typically used eg.updateMessageEventMoveTo with the message that has the new channel and topic, but I can see that we are in favor of eg.updateMessageEventMoveFrom here because in this context — moving away from the current narrow — the "move from" variant feels a bit more natural to use.
gnprice
force-pushed
the
pr-action-sheet
branch
from
ef09133 to
7145647
Compare
|
Thanks @PIG208 for the detailed review! Pushed a revision, and replied to some subthreads above; PTAL. |
|
LGTM. Thanks for the update! |
|
Thanks! Merging. |
gnprice
force-pushed
the
pr-action-sheet
branch
from
7145647 to
887fafc
Compare
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.
And add a bit of docs clarifying the expected practice.
…sheet We closed issue zulip#24 because it was apparently resolved by an OS update. There's still a milder issue; mention that instead.
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.
…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: zulip#1045
This is a nice small bonus from renaming the parameter that was `messageListContext` to the shorter name `pageContext`. Some of these don't quite fit in 80 columns. But the details of the parameter list are boring (because they're all the same), so it's OK that some of them go a little over.
These tests are all already relying on the detail that the setupToMessageActionSheet helper uses `eg.selfAccount` for the account. Better to encapsulate that detail, and have them rely on it giving the test cases a reference to the store directly.
Their callers always give them the shared store prepared by the setup helper setupToMessageActionSheet. Have these helpers just use that directly. If in the future we end up having a use case for customizing this behavior, it'll probably be most convenient to have them accept a connection instead of a store, anyway.
gnprice
force-pushed
the
pr-action-sheet
branch
from
887fafc to
cdce570
Compare
This was prompted by my comment at #1041 (comment) (/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
contextwherever aBuildContext 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