feat(ui): consider unthreaded read receipts when computing the user's latest on thread aware timeline instances

[stefanceriu]

This change modifies the existing latest_user_read_receipt method to retrieve and use unthreaded read receipts and chosing the most recent one.

We do this in order improve compatibility with non-threaded clients and correctly show what the other party has read on flattened out timelines.

Fixes #5440

[bnjbvr]

Stealing review.

[codecov]

Codecov Report

Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@ada68e1). Learn more about missing BASE report.
Report is 8 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ix-sdk-ui/src/timeline/controller/read_receipts.rs	95.23%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5442   +/-   ##
=======================================
  Coverage        ?   88.84%           
=======================================
  Files           ?      333           
  Lines           ?    91586           
  Branches        ?    91586           
=======================================
  Hits            ?    81372           
  Misses          ?     6388           
  Partials        ?     3826           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bnjbvr]

Thanks for trying it out, but I don't think this is the right fix, for multiple reasons:

• on the one hand, read receipts attached to items aren't computed with this method, so this likely wouldn't fix the problem observed in the linked issue. The state of read receipts in the timeline is the messiest part of the timeline's code, so that's not on you.
• on the other hand, a threaded timeline should likely not take into account the unthreaded receipt at all. I agree that a main thread timeline should use unthreaded|main, though, like a non-threaded timeline uses both.

[bnjbvr]

I think this is wrong, because the bound receipt_thread variable can be Unthreaded as well, in which case we're computing the same thing twice. Also, a thread-focused timeline should not take into account the unthreaded receipt at all, as it's not even certain that the event pointed to by the unthreaded receipt belongs to the thread or not. (So there are more chances it can't be ordered relative to the correct thread receipt at all, since the unthreaded receipt more likely points to another event of another thread.)

[andybalaam]

If the meaning of an unthreaded receipt is "I have read everything before this" then I argue an unthreaded receipt should be considered when deciding the unread state of a thread. Otherwise, if I switch from an unthreaded client to a threaded one, all threads from all time will be unread.

On the other hand, in practice, if you flip back to an unthreaded client, read the latest messages and then go back to a threaded one, all your threads will be read, so this might need us to be more pragmatic and follow what you are suggesting Benji - I don't know.

[bnjbvr]

Unfortunately, the read receipts in the timeline are computed locally, using only the current timeline's events. To know if an event is read, we compare the position of the event timeline item pointed to by the receipt, and the positions of event timeline items. It's more likely that the unthreaded receipt points to an event that's outside a threaded timeline, so it won't be found by the timeline code. As a result, it's not possible, in the current form of the code, to figure that a thread is fully read or not read, based on the threaded timeline. All the information is around, though, so we could compute this information (on a user basis), with some extra effort.

[stefanceriu]

Thanks for comments, let's try this again:

• 3d9d5c6 uses untreaded read receipts within latest_user_read_receipt only for Main timelines
• 227f68c then goes and fixes the real issue reported initially (tested against EX)
• and finally, on @bnjbvr's suggestion, 2ca4d6c rewrites latest_user_read_receipt to make use of the timeline, going through timeline items in reverse order until finding a read receipt, to have one single source of truth. This means changing the tests to have them inject timeline items and work around not having access to one's own receipts.

[bnjbvr]

Sorry about the long review time, but I needed a bit of time to think about this.

I think the actual fix is the 4th commit, and is untested; any chances you could add a test for it?

Other than that: I think that, on the one hand, all the changes in the other commits could be isolated in a separate PR. But, on the other hand, it doesn't look like the current implementation really needs a timeline; it could even go inside the event cache (and that would be a good move towards moving the read receipt code into the event cache). But this is completely outside the scope of this bugfix, sooooo…

1. We could either get rid of all the commits that aren't fix to the actual problem at hand (provided we'd have at least one regression test for the initial problem).
2. Or we could keep those commits, and put them in a different PR. The reason I'm suggesting that, is that the latest_user_read_receipt() API is not used in the SDK, so we'd need to clarify what the expectations are for this public API (i.e. how it's supposed to behave).
3. Or, with enough effort :) we could move the latest_user_read_receipt() code inside the EventCache, likely in a RoomReadReceipts API object (that includes a Arc<RoomEventCacheInner>).

I'm tempted to suggest (1) or (2) for the time being, and I could look into (3) myself later. How does that sound?