-
Notifications
You must be signed in to change notification settings - Fork 363
Keep track of the known topics in store #1951
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
base: main
Are you sure you want to change the base?
Conversation
1019746 to
2e0fcfd
Compare
chrisbobbe
left a comment
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! Glad to be maintaining a data structure for this. 🙂
Comments below, from reading the implementation in the first commit (I haven't yet read the tests):
d6b2242 channel: Keep track of channel topics, and keep up-to-date with events
lib/model/channel.dart
Outdated
| import 'store.dart'; | ||
| import 'user.dart'; | ||
|
|
||
| final _apiGetChannelTopics = getStreamTopics; // similar to _apiSendMessage in lib/model/message.dart |
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: too-long line; put comment on line above
lib/model/channel.dart
Outdated
| /// can be retrieved with [getChannelTopics]. | ||
| Future<void> fetchTopics(int channelId); | ||
|
|
||
| /// Pairs of the known topics and its latest message ID, in the given channel. |
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.
| /// Pairs of the known topics and its latest message ID, in the given channel. | |
| /// The topics in the given channel, along with their latest message ID. |
lib/model/channel.dart
Outdated
|
|
||
| /// Pairs of the known topics and its latest message ID, in the given channel. | ||
| /// | ||
| /// Returns null if the data has never been fetched yet. |
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:
| /// Returns null if the data has never been fetched yet. | |
| /// Returns null if the data has not been fetched yet. |
lib/model/channel.dart
Outdated
| /// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId] | ||
| /// descending, and the topics are guaranteed to be distinct. |
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, omit needless words:
| /// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId] | |
| /// descending, and the topics are guaranteed to be distinct. | |
| /// The result is sorted by [GetStreamTopicsEntry.maxId] descending, | |
| /// and the topics are distinct. |
lib/model/channel.dart
Outdated
| /// In some cases, the same maxId affected by message moves can be present in | ||
| /// multiple [GetStreamTopicsEntry] entries. For this reason, the caller | ||
| /// should not rely on [getChannelTopics] to determine which topic the message | ||
| /// is in. Instead, refer to [PerAccountStore.messages]. | ||
| /// See [handleUpdateMessageEvent] on how this could happen. |
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.
It seems worth highlighting in general that maxId might be…incorrect, basically. (Not just that two topics might have the same maxId.) Maybe something like:
(Also, isn't message deletion another reason maxId could be wrong?)
| /// In some cases, the same maxId affected by message moves can be present in | |
| /// multiple [GetStreamTopicsEntry] entries. For this reason, the caller | |
| /// should not rely on [getChannelTopics] to determine which topic the message | |
| /// is in. Instead, refer to [PerAccountStore.messages]. | |
| /// See [handleUpdateMessageEvent] on how this could happen. | |
| /// Occasionally, [GetStreamTopicsEntry.maxId] will refer to a message | |
| /// that doesn't exist or is no longer in the topic. | |
| /// This happens when a topic's latest message is moved or deleted | |
| /// and we don't have enough information | |
| /// to replace [GetStreamTopicsEntry.maxId] accurately. | |
| /// (We don't keep a snapshot of all messages.) | |
| /// Use [PerAccountStore.messages] to check a message's topic accurately. |
lib/model/channel.dart
Outdated
| /// Handle a [MessageEvent], returning whether listeners should be notified. | ||
| bool handleMessageEvent(MessageEvent event) { | ||
| if (event.message is! StreamMessage) return false; | ||
| final StreamMessage(:streamId, :topic) = event.message as StreamMessage; |
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:
| final StreamMessage(:streamId, :topic) = event.message as StreamMessage; | |
| final StreamMessage(streamId: channelId, :topic) = event.message as StreamMessage; |
lib/model/channel.dart
Outdated
| // If this message is already the latest message in the topic because it was | ||
| // received through fetch in fetch/event race, or it is a message sent even | ||
| // before the latest message of the fetch, we don't do the update. |
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'm getting confused trying to parse this sentence. Instead, how about, inside the if just below this, say something like:
// The event raced with a message fetch.
lib/model/channel.dart
Outdated
| // If we don't already know about the list of topics of the channel this | ||
| // message belongs to, we don't want to proceed and put one entry about the | ||
| // topic of this message, otherwise [fetchTopics] and the callers of | ||
| // [getChannelTopics] would assume that the channel only has this one topic | ||
| // and would never fetch the complete list of topics for that matter. | ||
| if (latestMessageIdsByTopic == null) return 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.
How about:
| // If we don't already know about the list of topics of the channel this | |
| // message belongs to, we don't want to proceed and put one entry about the | |
| // topic of this message, otherwise [fetchTopics] and the callers of | |
| // [getChannelTopics] would assume that the channel only has this one topic | |
| // and would never fetch the complete list of topics for that matter. | |
| if (latestMessageIdsByTopic == null) return false; | |
| if (latestMessageIdsByTopic == null) { | |
| // We're not tracking this channel's topics yet. | |
| // We start doing that when [fetchTopics] is called, | |
| // and we fill in all the topics at that time. | |
| return false; | |
| } |
lib/model/channel.dart
Outdated
| final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId]; | ||
| // We only handle the case where all the messages of [origTopic] are | ||
| // moved to [newTopic]; in that case we can remove [origTopic] safely. | ||
| // But if only one messsage is moved (`PropagateMode.changeOne`) or a few | ||
| // messages are moved (`PropagateMode.changeLater`), we cannot do anything | ||
| // about [origTopic] here as we cannot determine the new `maxId` for it. | ||
| // (This is the case where there could be multiple channel-topic keys with | ||
| // the same `maxId`) | ||
| if (propagateMode == PropagateMode.changeAll | ||
| && origLatestMessageIdsByTopics != null) { | ||
| shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null; | ||
| } |
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.
With a switch/case on propagateMode, I think we can be less verbose in the comment, e.g.:
| final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId]; | |
| // We only handle the case where all the messages of [origTopic] are | |
| // moved to [newTopic]; in that case we can remove [origTopic] safely. | |
| // But if only one messsage is moved (`PropagateMode.changeOne`) or a few | |
| // messages are moved (`PropagateMode.changeLater`), we cannot do anything | |
| // about [origTopic] here as we cannot determine the new `maxId` for it. | |
| // (This is the case where there could be multiple channel-topic keys with | |
| // the same `maxId`) | |
| if (propagateMode == PropagateMode.changeAll | |
| && origLatestMessageIdsByTopics != null) { | |
| shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null; | |
| } | |
| final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId]; | |
| switch (propagateMode) { | |
| case PropagateMode.changeOne: | |
| case PropagateMode.changeLater: | |
| // We can't know the new `maxId` for the original topic. | |
| // Shrug; leave it unchanged. (See dartdoc of [getChannelTopics], | |
| // where we call out this possibility that `maxId` is incorrect. | |
| break; | |
| case PropagateMode.changeAll: | |
| if (origLatestMessageIdsByTopics != null) { | |
| origLatestMessageIdsByTopics.remove(origTopic); | |
| shouldNotify = true; | |
| } | |
| } |
lib/model/channel.dart
Outdated
| final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId]; | ||
| if (newLatestMessageIdsByTopics != null) { | ||
| final movedMaxId = event.messageIds.max; | ||
| if (!newLatestMessageIdsByTopics.containsKey(newTopic) | ||
| || newLatestMessageIdsByTopics[newTopic]! < movedMaxId) { | ||
| newLatestMessageIdsByTopics[newTopic] = movedMaxId; | ||
| shouldNotify = 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.
A bit easier to read, I think:
| final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId]; | |
| if (newLatestMessageIdsByTopics != null) { | |
| final movedMaxId = event.messageIds.max; | |
| if (!newLatestMessageIdsByTopics.containsKey(newTopic) | |
| || newLatestMessageIdsByTopics[newTopic]! < movedMaxId) { | |
| newLatestMessageIdsByTopics[newTopic] = movedMaxId; | |
| shouldNotify = true; | |
| } | |
| } | |
| final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId]; | |
| if (newLatestMessageIdsByTopics != null) { | |
| final movedMaxId = event.messageIds.max; | |
| final currentMaxId = newLatestMessageIdsByTopics[newTopic]; | |
| if (currentMaxId == null || currentMaxId < movedMaxId) { | |
| newLatestMessageIdsByTopics[newTopic] = movedMaxId; | |
| shouldNotify = true; | |
| } | |
| } |
2e0fcfd to
c37de07
Compare
|
Thanks @chrisbobbe for the review. Pushed new revision. |
02ed253 to
7fca9bb
Compare
chrisbobbe
left a comment
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, and sorry for the delay! Comments below, this time from a full review.
test/widgets/topic_list_test.dart
Outdated
| }); | ||
|
|
||
| testWidgets('fetch again when navigating away and back', (tester) async { | ||
| testWidgets("dont't fetch again when navigating away and back", (tester) async { |
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: spelling of "don't"
test/api/route/route_checks.dart
Outdated
| Subject<Uri> get realmUrl => has((e) => e.realmUrl, 'realmUrl'); | ||
| } | ||
|
|
||
| extension GetStreamTopicEntryChecks on Subject<GetStreamTopicsEntry> { |
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.
"GetStreamTopicsEntryChecks"
test/model/store_checks.dart
Outdated
| Subject<Map<int, ZulipStream>> get streams => has((x) => x.streams, 'streams'); | ||
| Subject<Map<String, ZulipStream>> get streamsByName => has((x) => x.streamsByName, 'streamsByName'); | ||
| Subject<Map<int, Subscription>> get subscriptions => has((x) => x.subscriptions, 'subscriptions'); | ||
| Subject<List<GetStreamTopicsEntry>?> getStreamTopics(int streamId) => has((x) => x.getChannelTopics(streamId), 'getStreamTopics'); |
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.
getChannelTopics and 'getChannelTopics', to match PerAccountStore.getChannelTopics
lib/model/channel.dart
Outdated
| import 'user.dart'; | ||
|
|
||
| // similar to _apiSendMessage in lib/model/message.dart | ||
| final _apiGetChannelTopics = getStreamTopics; |
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.
Could you add a prep commit that renames the existing binding getStreamTopics to getChannelTopics, then consistently use "channel topics" naming instead of "stream topics" naming?
lib/model/channel.dart
Outdated
| /// can be retrieved with [getChannelTopics]. | ||
| Future<void> fetchTopics(int channelId); | ||
|
|
||
| /// The topics in the given channel, along with their latest message ID. |
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.
| /// The topics in the given channel, along with their latest message ID. | |
| /// The topics the user can access, along with their latest message ID, | |
| /// reflecting updates from events that arrived since the data was fetched. |
test/widgets/topic_list_test.dart
Outdated
| connection.prepare(json: GetStreamTopicsResult( | ||
| topics: [eg.getStreamTopicsEntry(name: 'topic B')]).toJson()); |
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.
If we don't expect to fetch again, we shouldn't prepare a response, right?
test/widgets/topic_list_test.dart
Outdated
| ]); | ||
| testWidgets('show topic action sheet before and after moves', (tester) async { | ||
| final channel = eg.stream(); | ||
| final message = eg.streamMessage(id: 123, stream: channel, topic: 'foo'); |
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.
Is 123 chosen because it equals eg.defaultStreamMessageStreamId? We should use eg.defaultStreamMessageStreamId instead, if so, or else pass 123 as streamId to the eg.stream() call.
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 this case 123 is just an arbitrary message ID, compared to eg.defaultStreamMessageStreamId which is stream/channel ID. Anyways, changed it to 100 to differentiate from the stream ID.
test/widgets/topic_list_test.dart
Outdated
| // After the move, the message with maxId moved away from "foo". The topic | ||
| // actions that require `someMessageIdInTopic` is no longer available. |
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: "are no longer available"
test/widgets/topic_list_test.dart
Outdated
| await tester.tap(find.text('Cancel')); | ||
| await tester.pump(); | ||
|
|
||
| // Topic actions that require `someMessageIdInTopic` is available |
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: "are available"
test/widgets/topic_list_test.dart
Outdated
| check(find.descendant(of: topicItemFinder, | ||
| matching: find.text('foo'))).findsOne(); |
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.
How about:
check(findInTopicItemAt(0, find.text('foo'))).findsOne();?
7fca9bb to
62ed7ea
Compare
62ed7ea to
b008d16
Compare
|
Thanks @chrisbobbe for the review. New changes pushed, PTAL. |
b008d16 to
ec8f84d
Compare
gnprice
left a comment
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!
I see this is still in maintainer review, and want that to continue until it's done. Since I'll be on vacation next week, though (and with the Thanksgiving holiday here starting Thursday), here's some high-level comments from an initial skim.
| bool containsMessage(MessageBase message) { | ||
| final conversation = message.conversation; | ||
| return conversation is StreamConversation | ||
| && conversation.streamId == streamId && conversation.topic == topic; | ||
| && conversation.streamId == streamId && conversation.topic.isSameAs(topic); |
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.
Hmm, good catch.
narrow: Use TopicName.isSameAs in TopicNarrow.containsMessage
TopicNarrow.containsMessage used to use raw equality operator (==)
for comparing topics, which was not ideal for how we treat two
topics the same.
Did this have a user-visible symptom? What were the symptoms?
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.
Yeah, there is one: when looking at a topic narrow for a topic named "t", and then there's a new message event in the topic named "T", it will not be shown in the current view.
Will mention this in the commit message.
lib/model/channel.dart
Outdated
| Future<void> fetchChannelTopics(int channelId) async { | ||
| if (_latestMessageIdsByChannelTopic[channelId] != null) return; | ||
|
|
||
| final result = await _apiGetChannelTopics(connection, channelId: channelId, | ||
| allowEmptyTopicName: true); | ||
| (_latestMessageIdsByChannelTopic[channelId] = makeTopicKeyedMap()) | ||
| .addEntries(result.topics.map((entry) => MapEntry(entry.name, entry.maxId))); |
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.
If this method gets called while another call is already in the middle of a request, it'll make a fresh request. Then whichever request finishes later will clobber the results of the one that finished first.
Is that a situation that's likely to happen in practice? What would the consequences be?
If we want to handle that situation smoothly, see the logic in GlobalStore.perAccount around _perAccountStoresLoading.
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.
Yeah, that can happen in cases like when a topic-list page is opened, but then before the request is finished, the page is closed and then opened again, resulting in a fresh request. It may result in stale data when the data of the latest request is replaced by an earlier request that took longer.
Added the logic you mentioned to handle this situation.
lib/model/store.dart
Outdated
| unreads.handleMessageEvent(event); | ||
| recentDmConversationsView.handleMessageEvent(event); | ||
| recentSenders.handleMessage(event.message); // TODO(#824) | ||
| if (_channels.handleMessageEvent(event)) notifyListeners(); |
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.
Hmmm — this means that once the user has been either looking at a topic list, or choosing a topic in a channel-narrow compose box, any new-message events in that channel will cause the overall PerAccountStore to notify listeners.
There are a small number of event types that happen frequently: new message, mark as read, typing status, and presence. Because those happen much more often than other events, those are the ones where we've spent effort on having separate ChangeNotifiers so that the bulk of the app's UI doesn't need to rebuild when those events happen.
It'd therefore be good to maintain that property here. Probably cleanest is to have this data structure go on a new substore, maybe TopicListStore. The whole substore could be the ChangeNotifier (like Unreads and friends), or there could be one ChangeNotifier per channel; whichever seems easier to implement is fine.
| // `maxId` might be incorrect (see [ChannelStore.getChannelTopics]). | ||
| // Check if it refers to a message that's currently in the topic; | ||
| // if not, we just won't have `someMessageIdInTopic` for the action sheet. |
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.
Hmm interesting. This was already something that could happen, right? So it seems like this change could just as well happen in a separate earlier commit, and that'd be clearer.
What was the consequence of using maxId here without this check?
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.
What was the consequence of using
maxIdhere without this check?
Quoting from #1508 (comment):
If the message ID is not in the topic anymore, then resolve/unresolve will not be successful and will display a dialog with the following message:
You only have permission to move the 1/7 most recent messages in this topic.
(the topic where the ID now actually belongs to has 7 messages in total)
That's the case when the message is moved.
And when the message is deleted, it will show the dialog with this message: Invalid message(s).
13b44e6 to
e6866a2
Compare
TopicNarrow.containsMessage used to use raw equality operator (==) for comparing topics, which was not ideal for how we treat two topics the same. This would result in not displaying a new message of topic "T" when looking at a topic narrow of topic "t" (treated message topics case-sensitively in topic narrow).
To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.
To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.
To make things consistent for renaming the binding getStreamTopics to getChannelTopics in the following commits.
Co-authored-by: Zixuan James Li <[email protected]>
This will be cleaner when we add more tests in future commits.
…n topic Co-authored-by: Zixuan James Li <[email protected]>
Fixes: zulip#1499 Co-authored-by: Zixuan James Li <[email protected]>
e6866a2 to
7f1e560
Compare
|
Thanks @gnprice for the review. New changes pushed. @chrisbobbe This is now ready for a new round of maintainer review. |
#1508 but rebased on top of main with the review of @gnprice addressed.
(Thanks @PIG208 for all of your previous work in #1508)
Fixes: #1499