-
Notifications
You must be signed in to change notification settings - Fork 318
unreads: Add/use locatorMap, to efficiently locate messages #1703
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
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 @chrisbobbe! LGTM, two small comments below.
lib/model/unreads.dart
Outdated
DmMessage() => DmNarrow.ofMessage(message, selfUserId: selfUserId), | ||
}; | ||
locatorMap[event.message.id] = narrow; | ||
|
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: maybe merge both switches? To avoid a copy of DmNarrow
and more specifically of allRecipientIds
list in DmNarrow.ofMessage
.
switch (message) {
case StreamMessage():
final narrow = TopicNarrow.ofMessage(message);
locatorMap[event.message.id] = narrow;
_addLastInStreamTopic(message.id, message.streamId, message.topic);
case DmMessage():
final narrow = DmNarrow.ofMessage(message, selfUserId: selfUserId);
locatorMap[event.message.id] = narrow;
_addLastInDm(message.id, narrow);
}
lib/model/unreads.dart
Outdated
if (messageIds.isEmpty) { | ||
newlyEmptyTopics.add(topic); | ||
} | ||
/// Remove any of [idsToRemove] that are in [streams]. |
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:
/// Remove any of [idsToRemove] that are in [streams]. | |
/// Remove any of [idsToRemove] that are in [streams]. |
Fixes zulip#332. Really the same optimization as described for zulip-mobile in zulip/zulip-mobile#4684 , and makes mark-as-read take 0ms (to the nearest ms) down from 3 or 4, in my testing, and so fixes zulip#332.
f3988b0
to
02f9779
Compare
Thanks for the review! Revision pushed. |
Fixes #332.
Really the same optimization as described for zulip-mobile in
zulip/zulip-mobile#4684
, and makes mark-as-read take 0ms (to the nearest ms) down from 3 or 4, in my testing, and so fixes #332.