msglist: Make mark-as-read button listen to the Unreads model #2020
Conversation
Instead of just listening to PerAccountStore and (via an ancestor widget) a MessageListView. Issue zulip#370 is relevant here -- "Maintain total unread counts efficiently". The button causes linear scans through Unreads each time it rebuilds, and ideally we'd avoid that, especially since now it'll rebuild in more cases than it was before. I think the additional cases aren't very many, though: I've thought of just one kind of Zulip event that would cause Unreads but not PerAccountStore and MessageListView to notify listeners, and I added a test exercising that.
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! LGTM, moving over to Greg's review.
rajveermalviya
added
integration review
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks! Small comments below.
The new test exercises a scenario where the old code didn't behave right, so that's good to fix. But there was something else that prompted taking a look at this logic, right? ISTR a conversation a couple of weeks ago (before my vacation) where we were looking at notifyListeners calls in the context of a different change you were making. Remind me what the context was there?
| // omit `message`; if present, MessageListView would notify listeners | ||
| messages: List.generate(300, (i) => | ||
| eg.streamMessage(id: 950 + i, sender: eg.selfUser, | ||
| flags: [MessageFlag.read, MessageFlag.mentioned])), |
There was a problem hiding this comment.
Ah, I see. IIUC, the point is that the message had been in the narrow, and an event removes it from the narrow; but it hadn't been one of the messages fetched, because it was too old. Is that right? I think that high-level strategy would be good to make explicit in the test's main comment above.
It looks like the same thing would happen with a channel or topic narrow if the message got moved out of the channel/topic, too.
| // but not PerAccountStore and MessageListView to notify listeners, | ||
| // and check that the button responds. | ||
|
|
||
| final message = eg.streamMessage(flags: [MessageFlag.mentioned]); |
There was a problem hiding this comment.
Since the premise of this test scenario is that this message is older than all the messages that get fetched, it'd be good to give it a smaller message ID too in order to be consistent with that.
That or leave all the message IDs out — they're really only needed when the messages get constructed in the test out of order from when they were supposed to be sent.
I wonder if you're thinking about #1997 (comment) ? The fix in this PR is something I hadn't noticed at the time. IIRC I spotted it (and #2026) when reviewing #2006. |
No, what I recall is looking at a proposed change that added Anyway 🤷 I guess that was a coincidence. Not too surprising if there were just two unrelated reasons to be looking at this general area of logic in the span of a few weeks. |
3 participants
Instead of just listening to PerAccountStore and (via an ancestor widget) a MessageListView.
Issue #370 is relevant here -- "Maintain total unread counts efficiently". The button causes linear scans through Unreads each time it rebuilds, and ideally we'd avoid that, especially since now it'll rebuild in more cases than it was before. I think the additional cases aren't very many, though: I've thought of just one kind of Zulip event that would cause Unreads but not PerAccountStore and MessageListView to notify listeners, and I added a test exercising that.