-
Notifications
You must be signed in to change notification settings - Fork 142
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
Update timeline items read receipts when the room members are loaded #2194
Update timeline items read receipts when the room members are loaded #2194
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2194 +/- ##
===========================================
+ Coverage 70.12% 70.16% +0.03%
===========================================
Files 1335 1335
Lines 32781 32790 +9
Branches 6811 6812 +1
===========================================
+ Hits 22988 23006 +18
+ Misses 6515 6504 -11
- Partials 3278 3280 +2 ☔ View full report in Codecov by Sentry. |
This seems to be a lot more reliable than before 👍. Avatars do all eventually load although it can still take a while (several seconds) in larger rooms as it seems to wait for all avatars to be available before showing them. What would be the performance/UX implications of displaying them one at a time as they're available? |
The issue that makes the images take so long to load is that, although we do have a cache for members in the Rust SDK, if I'm not mistaken (so take it with a grain of salt), the current Technically we could call So to improve this in the future I we have 3 options:
Also, loading times on Android are a lot higher than on iOS partly because we have to map those thousand of items to plain Kotlin objects to ensure memory safety given the current UniFFi memory model for Kotlin. This should be fixed in the near future, which should make the app more performant overall, but until then this is what we have. |
Thanks for taking the time to explain that! |
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.
LGTM, but maybe @ganfra can have a look on those changes too.
val updatedItem = if (cacheItem is TimelineItem.Event && roomMembers.isNotEmpty()) { | ||
eventItemFactory.update( | ||
timelineItem = cacheItem, | ||
receivedMatrixTimelineItem = timelineItems[index] as MatrixTimelineItem.Event, |
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.
Are you sure that timelineItems[index]
can always be cast to MatrixTimelineItem.Event
?
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 uses the unique id to identify cached items, so if the cached item is an event I expect the updated item with that id to also be an event. Maybe that's not always the case?
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.
Yes the cached value is obviously the same, otherwise there is a big issue :D
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.
Should we not fix this on the rust sdk instead?
So the Receipt
class should have the same logic than ProfileTimelineDetails
I believe we'd still have this caching issue, even if the SDK also re-emitted the current timeline with the updated read receipts: we'd still be asking the cache for an event with that id, and we'd use the cached one instead of the updated one containing the 'loaded' read receipts. |
It seems like we want to go with option 1 for now:
So it's probably better to wait until that is merged in the SDK side to also merge this PR. |
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.
Sorry for the delay to approve the PR, I think it's fine!
…e-timeline-items-read-receipts-and-sender-info
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Type of change
Content
When we fetch an item from the timeline cache, we now always update the read receipts with the latest data from the room members list.
Motivation and context
Fixes #2176 .
Tests
If after a few seconds they change and now contain the right images and user names, it's working.
Tested devices
Checklist