-
Notifications
You must be signed in to change notification settings - Fork 309
Unconditionally add ApiNarrowIsUnread to markNarrowAsRead #427
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
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 @sirpengi! This all looks good modulo some revisions to the explanatory code comment, below.
I'll go ahead and merge with that change.
lib/widgets/message_list.dart
Outdated
// This process can take a long time in narrows with a lot | ||
// of history given the server ends up processing a large | ||
// number of already read messages. Unconditionally add | ||
// [ApiNarrowIsUnread] here to take advantage of a database | ||
// index on `is:unread`. | ||
// This optimization is also used server side for the | ||
// (deprecated) specialized endpoints for marking messages | ||
// as read. See `do_mark_stream_messages_as_read` in | ||
// `zulip:zerver/actions/message_flags.py`. |
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.
// This process can take a long time in narrows with a lot | |
// of history given the server ends up processing a large | |
// number of already read messages. Unconditionally add | |
// [ApiNarrowIsUnread] here to take advantage of a database | |
// index on `is:unread`. | |
// This optimization is also used server side for the | |
// (deprecated) specialized endpoints for marking messages | |
// as read. See `do_mark_stream_messages_as_read` in | |
// `zulip:zerver/actions/message_flags.py`. | |
// Include `is:unread` in the narrow. That has a database index, so | |
// this can be an important optimization in narrows with a lot of history. | |
// The server applies the same optimization within the (deprecated) | |
// specialized endpoints for marking messages as read; see | |
// `do_mark_stream_messages_as_read` in `zulip:zerver/actions/message_flags.py`. |
I think the main thing I wanted to adjust in this comment is that the other version, with "This process can take a long time" and "the server ends up processing", sounds like it's describing what does happen, whereas in fact it's describing what would happen if the code worked in a different way.
A direct fix for that could look like "If we used narrow.apiEncode()
directly, this process could …", or like "This process could … given the server would end up processing … messages. Avoid that by adding …".
(A related, smaller, point is that although "unconditionally" is a highly relevant term for the commit message, it's a bit out of place in the code comment. The reader of the code comment is trying to understand the code as it is, and not starting from how the code previously was, so they're left to wonder what the hypothetical other way of doing this was that would have been conditional.)
e27ae7e
to
0c4653d
Compare
(Oh also I tweaked the commit summary line: "Unconditionally add ApiNarrowIsUnread in markNarrowAsRead", rather than "to". To my mind it's being added to the ApiNarrow, or to the request the ApiNarrow is part of, but the function markNarrowAsRead is doing the adding.) |
Fixes: #377