-
Notifications
You must be signed in to change notification settings - Fork 343
message: Consider unsubscribed channels in reconcileMessages #1912
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?
message: Consider unsubscribed channels in reconcileMessages #1912
Conversation
(Oops, I'll need to rethink this a bit following some discussion with Greg in the office.) |
3612e86
to
4fa0a1a
Compare
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! At #1798 (comment) you asked for an initial look at this logic, so comments on that below.
lib/model/message.dart
Outdated
// If the message is one we already know about (from a fetch), | ||
// clobber it with the one from the event system. | ||
// See [fetchedMessages] for reasoning. |
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 this existing comment meant to refer to reconcileMessages
? I think fetchedMessages
was a previous name for that.)
/// (We have seen a few such events, actually -- | ||
/// maybe because the channel was recently subscribed? -- | ||
/// but not consistently, and we're not supposed to rely on them.) |
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.
/// (We have seen a few such events, actually -- | |
/// maybe because the channel was recently subscribed? -- | |
/// but not consistently, and we're not supposed to rely on them.) | |
/// (We have seen a few such events, actually -- | |
/// maybe because the channel was recently unsubscribed? -- | |
/// but not consistently, and we're not supposed to rely on them.) |
?
lib/model/message.dart
Outdated
// [1] With one exception: if the version we have was in an unsubscribed | ||
// channel when we got it or sometime since, we take the fetched version |
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 like this formulation of which messages go in _maybeStaleChannelMessages
. It'd be good to put it in that field's doc.
lib/model/message.dart
Outdated
// Side effect: update our "potentially stale" knowledge based on | ||
// whether the channel is subscribed right now. | ||
&& (subscriptions[message.streamId] != null | ||
? _maybeStaleChannelMessages.remove(message.id) | ||
: !_maybeStaleChannelMessages.add(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.
It's a bit confusing that we need this side effect on the maybe-stale set here. I think this logic could benefit from some unpacking.
In particular, it seems like there are really four cases here:
- currently subscribed vs. not;
- for each of those, currently in the maybe-stale set or not.
Thanks to Greg for pointing this out: zulip#1912 (comment)
4fa0a1a
to
6ed2244
Compare
Thanks for taking a look! Revision pushed. |
e8f8054
to
a9b4906
Compare
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! I think that's helpful. A couple of comments below on the new version.
Message _stripMatchFields(Message message) { | ||
message.matchContent = null; | ||
message.matchTopic = null; | ||
return message; | ||
} |
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 factoring out of this step could happen in a prep commit. That would simplify the complexity of the main commit a bit — it'd be moving around where an expression like _stripMatchFields(incoming)
appears, and adding a case where something else gets used instead, but the expression itself would remain the same.
(Or maybe the main commit would change from "message" to "incoming", but anyway that's a rather smaller change.)
lib/model/message.dart
Outdated
Message _reconcileWhenPresent(Message current, Message incoming) { | ||
bool currentIsMaybeStale = false; | ||
if (incoming is StreamMessage) { | ||
if (subscriptions[incoming.streamId] != null) { | ||
// The incoming message won't grow stale; it's in a subscribed channel. | ||
// Remove it from _maybeStaleChannelMessages if it was there. | ||
currentIsMaybeStale = _maybeStaleChannelMessages.remove(incoming.id); | ||
} else { | ||
assert(_maybeStaleChannelMessages.contains(incoming.id)); | ||
currentIsMaybeStale = true; | ||
} | ||
} | ||
|
||
return currentIsMaybeStale | ||
? _stripMatchFields(incoming) | ||
: current; |
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 think this logic would benefit from having the explanation in the long comment above worked into it.
Probably that mostly means just moving that comment from reconcileMessages
into this method. After all, it starts with:
What to do when some of the just-fetched messages are already known?
so it's entirely about this "present" case.
Beyond that, I think once the comment is here in this method, the "normally" vs. "with one exception" cases can be worked a bit into the corresponding cases in this code's control flow.
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.
That makes a lot of sense, thanks!!
lib/model/message.dart
Outdated
} | ||
} | ||
|
||
Message _reconcileWhenPresent(Message current, Message incoming) { |
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 give these names that are a bit more semantic: like "reconcile known message" vs. "reconcile unknown message" (or "new" or "previously unknown"?)
lib/model/message.dart
Outdated
: current; | ||
} | ||
|
||
Message _reconcileWhenAbsent(Message incoming) { |
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 put the absent/unknown case first — it happens first chronologically, after all (except in the case of a message we learn about from an event), and it's also simpler, so I think conceptually prior
(can do the same at the call site too, thanks to Dart accepting named arguments before positional)
Thanks to Greg for pointing this out: zulip#1912 (comment)
It's way more common to be using the app with subscribed channels than unsubscribed channels, so we might as well test with subscribed channels except where we specifically want to check behavior for unsubscribed channels.
Like we did in the compose-box tests in the previous commit.
This fixes the "fourth buggy behavior" in zulip#1798: zulip#1798 (comment) Fixes-partly: zulip#1798
a9b4906
to
6f75b2b
Compare
Thanks for the review! Revision pushed. |
Thanks! I think that revision is helpful. Over to @rajveermalviya for normal maintainer-review. |
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, and tests great.
Moving over to Greg's review.
Thanks for the review! |
This fixes the "fourth buggy behavior" in #1798:
#1798 (comment)
Fixes-partly: #1798