Keep topics data up-to-date by handling events #1508
Conversation
PIG208
force-pushed
the
pr-topics-2
branch
4 times, most recently
from
0670f6a to
6632f36
Compare
There was a problem hiding this comment.
Thanks @PIG208 for drafting this!
Generally this approach looks good. We'll have someone else pick it up from here.
I skimmed through the key points of the logic; some high-level comments below.
(For whoever picks this up next: do read through all the details yourself in addition to the points I've commented on, and follow the steps in Zulip's guide to continuing unfinished work.)
| bool handleMessageEvent(MessageEvent event) { | ||
| if (event.message is! StreamMessage) return false; | ||
| final StreamMessage(:streamId, :topic) = event.message as StreamMessage; | ||
|
|
||
| final latestMessageIdsByTopic = _latestMessageIdsByStreamTopic[streamId]; | ||
| if (latestMessageIdsByTopic == null) return false; | ||
| assert(!latestMessageIdsByTopic.containsKey(topic) | ||
| || latestMessageIdsByTopic[topic]! < event.message.id); | ||
| latestMessageIdsByTopic[topic] = event.message.id; |
There was a problem hiding this comment.
One thing we'll want to think through in how this part of the model should work is how to handle the fetch/event race which it introduces.
An example of that is that I think this assert isn't correct — we could do a fetch that learns about a topic with a new message, and only later get the MessageEvent about that message.
For an example of where we've reasoned through this sort of fetch/event race before, see the implementation of reconcileMessages in lib/model/message.dart .
| Future<void> fetchTopics(int streamId) async { | ||
| final result = await _apiGetStreamTopics(connection, streamId: streamId, | ||
| allowEmptyTopicName: true); |
There was a problem hiding this comment.
What if we've already successfully fetched the list for this channel? In that case I think we want to show the existing list (which, after all, this class will have been keeping up to date) rather than make a new fetch.
| final origLatestMessageIdsByTopics = _latestMessageIdsByStreamTopic[origStreamId]; | ||
| if (propagateMode == PropagateMode.changeAll | ||
| && origLatestMessageIdsByTopics != null) { | ||
| shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null; | ||
| } |
There was a problem hiding this comment.
I think this might be the right logic to have, but there are some edge cases it doesn't handle. So this code should have comments pointing out those cases and explaining why we're choosing to not handle them.
| // The message with `maxId` might not remain in `topic` since we last fetch | ||
| // the list of topics. Make sure we check for that before passing `maxId` | ||
| // to the topic action sheet. | ||
| // See also: [ChannelStore.getStreamTopics] | ||
| final message = store.messages[maxId]; | ||
| final isMaxIdInTopic = message is StreamMessage | ||
| && message.streamId == streamId | ||
| && message.topic.isSameAs(topic); |
There was a problem hiding this comment.
Hmm. This will mean that if we just don't happen to have any messages from that topic in MessageStore — for example, because it's not a topic that's included in the very last messages — then we won't give the user the option to resolve or unresolve the topic from this screen. That seems unfortunate.
What happens if we go ahead and use this message ID even though it might turn out to no longer be in the topic?
|
Hmm, infra failure in CI: That's the same symptom as at #1688 (comment) , when Flutter's And in fact there doesn't seem to be a commit in the upstream repo with the commit ID from that failure output: |
|
Discussion of that upstream issue: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flutter.20upgrade.20-.20downloding.20Dark.20SDK.20fails/near/2228299 Apparently fixed now. |
This is a follow-up to #1500.
Fixes: #1499