msglist: Fix the first and third buggy behaviors in #1798! #2034
Conversation
This fixes half of the "first buggy behavior" described in zulip#1798, specifically the send-message case. We'll fix it for the edit-message case too, coming up. Fixes-partly: zulip#1798
This fixes half of the "third buggy behavior" described in zulip#1798: the send-message case. We'll fix it for the edit-message case too, coming up. Fixes-partly: zulip#1798
This fixes the rest of the "first buggy behavior" described in zulip#1798: it fixes it for the edit-message case. Fixes-partly: zulip#1798
This fixes the rest of the "third buggy behavior" described in zulip#1798: it fixes it for the edit-message case. Fixes-partly: zulip#1798
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! LGTM, and tests great! Moving over to Greg's review.
rajveermalviya
added
integration review
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks. Comments below on the first two commits:
7d73858 msglist: In unsubscribed channel, refresh on message-send success
2e82af2 msglist: In unsubscribed channel, clear outbox message on send success
It looks like the remaining two are fairly parallel to those:
522df03 msglist: In unsubscribed channel, refresh on message-edit success
2be8ead msglist: In unsubscribed channel, clear edit progress on request success
so I'll read them in more detail after we're settled on specifics of the first two.
| } | ||
|
|
||
| final store = PerAccountStoreWidget.of(context); | ||
| PerAccountStore store = PerAccountStoreWidget.of(context); |
There was a problem hiding this comment.
Ah I see, the point is we'll mutate this variable after the async gap, looking up the new value it should have.
nit: Let's move this line to just before the one place this version is used. (And it can go back to final.) That way it's clearer that this should only be used before the async gap, and harder to accidentally use it after.
| } | ||
|
|
||
| store = PerAccountStoreWidget.of(context); | ||
| final destination = widget.getDestination(); |
There was a problem hiding this comment.
The point is this is the destination we just sent to, right? Perhaps save that earlier in a variable, vs. recomputing here, to make that clearer.
| ) { | ||
| // We don't get new-message events for unsubscribed channels, | ||
| // but we can refresh the view when a send-message request succeeds, |
There was a problem hiding this comment.
I think this could use a line setting the context a bit more broadly:
| ) { | |
| // We don't get new-message events for unsubscribed channels, | |
| // but we can refresh the view when a send-message request succeeds, | |
| ) { | |
| // The message was sent to an unsubscribed channel. | |
| // We don't get new-message events for unsubscribed channels, | |
| // but we can refresh the view when a send-message request succeeds, |
| /// Event received, or [sendMessage] | ||
| /// request succeeds and we're sending to | ||
| /// an unsubscribed channel. | ||
| /// (any state) ───────────────────────────────────────► (delete) |
There was a problem hiding this comment.
nit: In general with these changes, I worry somewhat about making it harder to understand the main case. This can help a bit here:
| /// Event received, or [sendMessage] | |
| /// request succeeds and we're sending to | |
| /// an unsubscribed channel. | |
| /// (any state) ───────────────────────────────────────► (delete) | |
| /// Event received. Or [sendMessage] | |
| /// request succeeds and we're sending to | |
| /// an unsubscribed channel. | |
| /// (any state) ───────────────────────────────────────► (delete) |
In particular that makes it clear up front that "event received" is a complete description of one way this transition can happen, and there aren't further caveats about it coming later.
| if (destination is StreamDestination && subscriptions[destination.streamId] == null) { | ||
| // We don't expect an event (we're sending to an unsubscribed channel); | ||
| // clear the loading spinner. | ||
| _outboxMessages.remove(localMessageId); |
There was a problem hiding this comment.
Hmm. This means we'll remove not only the loading spinner, but also the local-echo version of the message itself, right? That seems potentially unhelpful.
Though I guess this might be simultaneous with reloading the affected message lists from scratch? In that case the user wouldn't see a state where the message appears to have vanished.
3 participants
This PR gets us much closer to #1798, i.e. handling that we don't expect events about messages in unsubscribed channels. 🙂
When the "first" and "third" are fixed (as here), we can finish the rest of the "second" (reasoning is explained in #1873). 🙂
Fixes-partly: #1798