outbox: Show outbox messages when 'caughtUp.newer' is false.

[agrawal-d]

Currently, we return a NULL_ARRAY in outboxMessagesForNarrow when
'caughtUp.newer' is false - but there is no reason to do this. We
should always show outbox messages regardless of the caughtUp state
because there can always be unsent messages for various reasons.

So, remove the conditional that causes this.

Fixes: #3800.

[chrisbobbe]

Interesting, thanks for this PR, and the debugging that led to it, @agrawal-d!

Over at #3800 (comment), you said:

I don't get the logic behind the if condition I mentioned above - what I understand from it is: 'If we haven't fetched the newest messages, don't show unsent messages'.

That does not make much sense to me. In fact think there is no need for this condition at all, because whatever may be our perceived knowledge about the fetched messages, the app can still have unsent messages, and they should always be shown.

I traced the origin of this bit of logic to b23628a, which links to #1262. Neither the commit nor the PR offers any explanation at all, which is unfortunate—not only because it doesn't tell us the motivation for this particular change, but because, looking at the commit, it seems plausible that there are numerous other changes associated with this change, and those are also unexplained.

These days, we have much stricter standards about what gets committed, and it pays off even if it feels a bit laborious to get things right. 😄

Do the changes in b23628a suggest to you that there might be more work to be done in this PR, if the goal is to undo the work of the commit message, "Do not show outbox if not caught up"?

[chrisbobbe]

For the reason for b23628a, I have one guess.

You said:

whatever may be our perceived knowledge about the fetched messages, the app can still have unsent messages

Very true.

Then:

and they should always be shown.

I mostly agree, but I suppose there's an argument for not showing them directly following some messages that we know might be old. While we're loading new messages, we might mistakenly think that the outbox messages are going to be sent directly following some messages that may be old in any of several ways, e.g.:

• more messages arrived after them
• they'd been edited or deleted
• they're just...old. Like a message saying "Shall we meet in 10 minutes?" that was actually sent a day ago.

That being said, if we're still retrying sending some messages, then our latest knowledge of the narrow should be...reasonably up-to-date, I think? The latest knowledge will likely line up with the time you first tried to send the messages. And we shouldn't be constantly retrying for an unreasonable amount of time (that's #3829, I think).

With this in mind, I think it's probably okay to proceed as we do in this PR. But does that sound reasonable to you?

[agrawal-d]

Thanks for the link to the original change. Yikes, the PR gives no explanation at all on why the change was made!

That being said, if we're still retrying sending some messages, then our latest knowledge of the narrow should be...reasonably up-to-date, I think? The latest knowledge will likely line up with the time you first tried to send the messages. And we shouldn't be constantly retrying for an unreasonable amount of time (that's #3829, I think).

Yeah, I agree, if I responded to message X with a message Y, and Y is still in the outbox, it is very unlikely that there are un-fetched messages between X and Y. An if there is a UX penalty for showing Y, the UX penalty for not showing Y is much higher, I think.

And if I am replying offline, then I will be reasonably aware that the messages shown are not latest. What would be very bad UX is that I sent a message when offline, and it disappeared.

I think both logics have drawbacks, but I feel the current logic has more drawbacks than my proposed one.

[chrisbobbe]

Linking to discussion.

[agrawal-d]

Closing as the solution discussed in the link above is very different to the one proposed in this PR.

[gnprice]

Thanks @agrawal-d for driving this forward!

If you'd like to tackle giving an error on send in this condition as discussed at that chat link, that would be excellent 🙂

[agrawal-d]

@gnprice sure! Working on it.

[gnprice]

(And that's #4208.)