-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378
msglist: Set unreads.oldUnreadsMissing to false on batched mark-all-as-read #378
Conversation
lib/widgets/message_list.dart
Outdated
// success themselves; the event from the last batch is unmarked. | ||
// Discussion: | ||
// <https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680275> | ||
PerAccountStoreWidget.of(context).unreads.oldUnreadsMissing = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifyListeners
isn't being called after mutating the unreads
model here. I think there's an argument for not doing that here (as we aren't using this flag in the UI) but leaving a note to that effect might be good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, thanks for catching that! I think it'd be good to call notifyListeners
. We might in the future use this flag in the UI; here's from the doc on old_unreads_missing
in the /register
response:
[…] When true, we recommend that clients display a warning, as they are likely to produce erroneous results until reloaded with the user having fewer than MAX_UNREAD_MESSAGES unread messages.
I'll send a revision with one way of making that happen (edit: done).
ed57148
to
4e62e7a
Compare
Thanks for the review! Revision pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to you both! Sorry for the delayed review. This looks good, with a few nits below.
test/widgets/message_list_test.dart
Outdated
@@ -608,6 +626,19 @@ void main() { | |||
narrow: narrow, messages: [message], unreadMsgs: unreadMsgs); | |||
check(isMarkAsReadButtonVisible(tester)).isTrue(); | |||
|
|||
int unreadsNotifiedCount = 0; | |||
final unreadsModel = store.unreads; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just inline this, it's more transparent and not materially longer
test/widgets/message_list_test.dart
Outdated
int unreadsNotifiedCount = 0; | ||
final unreadsModel = store.unreads; | ||
unreadsModel.addListener(() { | ||
unreadsNotifiedCount++; | ||
}); | ||
// Might as well test with oldUnreadsMissing: true. | ||
// | ||
// We didn't fill the model with 50k unreads, so this is questionably | ||
// realistic… but the 50k cap isn't actually API-guaranteed, and this is | ||
// plausibly realistic for a hypothetical server that decides based on | ||
// message age rather than the 50k cap. | ||
unreadsModel.oldUnreadsMissing = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this "might as well" reasoning works so long as this wrinkle isn't adding so much to read (in code and comments) that it comes to take over the test case.
One simplification is that I think this test is just as effective without the unreadsNotifiedCount
part, just checking that oldUnreadsMissing
is still true. So:
int unreadsNotifiedCount = 0; | |
final unreadsModel = store.unreads; | |
unreadsModel.addListener(() { | |
unreadsNotifiedCount++; | |
}); | |
// Might as well test with oldUnreadsMissing: true. | |
// | |
// We didn't fill the model with 50k unreads, so this is questionably | |
// realistic… but the 50k cap isn't actually API-guaranteed, and this is | |
// plausibly realistic for a hypothetical server that decides based on | |
// message age rather than the 50k cap. | |
unreadsModel.oldUnreadsMissing = true; | |
// Might as well test with oldUnreadsMissing: true. | |
store.unreads.oldUnreadsMissing = true; |
test/widgets/message_list_test.dart
Outdated
// Didn't redundantly call [Unreads.handleAllMessagesReadSuccess], which | ||
// is meant only for the modern protocol and would be redundant in the | ||
// legacy protocol. (In the legacy protocol, we set oldUnreadsMissing to | ||
// false when an event comes, not when the mark-as-read request | ||
// succeeds.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same vein:
// Didn't redundantly call [Unreads.handleAllMessagesReadSuccess], which | |
// is meant only for the modern protocol and would be redundant in the | |
// legacy protocol. (In the legacy protocol, we set oldUnreadsMissing to | |
// false when an event comes, not when the mark-as-read request | |
// succeeds.) | |
// Check that [Unreads.handleAllMessagesReadSuccess] wasn't called; | |
// in the legacy protocol, that'd be redundant with the mark-read event. |
test/widgets/message_list_test.dart
Outdated
check(isMarkAsReadButtonVisible(tester)).isTrue(); | ||
store.unreads.oldUnreadsMissing = true; | ||
|
||
final connection = store.connection as FakeApiConnection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there's an outer connection
that this is equivalent to, in the same way as store
:
final connection = store.connection as FakeApiConnection; |
(I see there's a bunch of existing cases of similar redundancy in this file. I'll send a quick cleanup for those. → #506)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
4e62e7a
to
36f344d
Compare
Thanks for the review! Revision pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! One nit still open, though.
test/widgets/message_list_test.dart
Outdated
check(isMarkAsReadButtonVisible(tester)).isTrue(); | ||
store.unreads.oldUnreadsMissing = true; | ||
|
||
final connection = store.connection as FakeApiConnection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
36f344d
to
c7e894f
Compare
Ah indeed; thanks for catching that. Revision pushed! |
We'll reuse this to handle the TODO in this _handlePress method.
c7e894f
to
08a0827
Compare
Thanks! Looks good; merging. |
No description provided.