Add read receipt display

[alanpoon]

https://github.com/project-robius/robrix/issues/153

1. With tooltip indicating number of seen - mouse over

2. mouse out

• Currently only supports Avatar with username. I need some advice how to put in Avatar with image.
• Implemented max display of 4 avatars
• Currently tooltip seems to be not able customize width and position. @jmbejar

[alanpoon]

[WIP] Will try to change sequencer to follow GenUI's way of creating a list of widgets

[alanpoon]

1. Modified set_avatar_and_get_username function to use &mut Avatar instead of AvatarRef
2. Modified set_avatar_and_get_username function to take in an option of Avatar User profile. Read_receipts do not provide user profile. So the User Profile is gotten from the Sender cache.
3. Added Caching for Avatar that is loaded with Image.

[kevinaboos]

• Currently only supports Avatar with username. I need some advice how to put in Avatar with image.

Using the existing Avatar widget should work just fine. The reason you never get any proper Avatar profile info is because you always pass None into the set_avatar_and_get_username() function. If you're able to obtain the Avatar image therein (by grabbing it from the cache), then that function should automatically pick it up.

Alternatively, we can create a separate function (or further modify set_avatar_and_get_username()) to utilize the user profile and avatar caches if the passed-in avatar info is None or not-yet available.

[kevinaboos]

is this ready for another review? i noticed you made some changes but didn't change the waiting-on-* label nor request another review from me

[alanpoon]

is this ready for another review? i noticed you made some changes but didn't change the waiting-on-* label nor request another review from me

Not ready at the moment, sync_once is throwing error. 'extensions.to_device.since' is invalid (should look like an int) (type=value_error). Initially I thought by moving it after sync service.start would resolve the issue, but it didn't. The backend is expecting url parameter "Since" to be timestamp, but it is sending string. I am not finding the same issue with a separate matrix test repository.

[kevinaboos]

sync_once is throwing error. 'extensions.to_device.since' is invalid (should look like an int) (type=value_error). Initially I thought by moving it after sync service.start would resolve the issue, but it didn't. The backend is expecting url parameter "Since" to be timestamp, but it is sending string.

Yea that sounds like what I would expect, since we're not supposed to use that API because we're using sliding sync instead. I was surprised that it ever worked.

I am not finding the same issue with a separate matrix test repository.

probably because you're not saving and restoring a session that was created via sliding sync, i presume?

[kevinaboos]

I think you should reach out to the Matrix SDK developers on the matrix room for that, and ask them about the issue you mentioned here: https://github.com/project-robius/robrix/pull/162#discussion_r1894946412

There must be a way to get this to work correctly with sliding sync. At the very least, Element X probably has this feature working, so we could also reach out to them on the Element X matrix rooms.

[kevinaboos]

@alanpoon i see you marked this as "blocked". Are you considering this blocked on Matrix SDK features? or you're waiting to hear back from the SDK dev team? (I haven't seen any conversations from you on the Matrix Rust SDK room...)

If so, kindly take a look at Element X's source code (either Android or iOS, whichever one you prefer) to see what they do to make read receipts work.

[alanpoon]

@alanpoon i see you marked this as "blocked". Are you considering this blocked on Matrix SDK features? or you're waiting to hear back from the SDK dev team? (I haven't seen any conversations from you on the Matrix Rust SDK room...)

If so, kindly take a look at Element X's source code (either Android or iOS, whichever one you prefer) to see what they do to make read receipts work.

I asked in the matrix chat room. https://matrix.to/#/!IlysflVuQZDIcVLkcg:matrix.org/$3VJtkdI0yxVnlAMzh42q7lPl1MtrUzz6KxUmibpLUtM?via=matrix.org&via=element.io&via=envs.net

It seems like sync_once does not throw error, after deleting the Robius's local data. Hence I am requesting another review.

Element X Android's code is

        client.getRoom(notifiableEvent.roomId)?.use { room ->
            room.subscribeToSync()

            // If the app is in foreground, sync is already running, so just add the subscription.
            if (!appForegroundStateService.isInForeground.value) {
                val syncService = client.syncService()
                syncService.startSyncIfNeeded()
                if (isRingingCallEvent) {
                    room.waitsUntilUserIsInTheCall(timeout = 60.seconds)
                } else {
                    room.waitsUntilEventIsKnown(eventId = notifiableEvent.eventId, timeout = 10.seconds)
                }
                syncService.stopSyncIfNeeded()
            }
        }

Their android's SDK is syncService.startSync, while rust is just SyncService.start(). So it can be hard to compare.

https://docs.rs/matrix-sdk-ui/0.7.0/matrix_sdk_ui/sync_service/struct.SyncService.html

[kevinaboos]

I asked in the matrix chat room. https://matrix.to/#/!IlysflVuQZDIcVLkcg:matrix.org/$3VJtkdI0yxVnlAMzh42q7lPl1MtrUzz6KxUmibpLUtM?via=matrix.org&via=element.io&via=envs.net

oh ok, great. I did not know about that room.

You should also ask the question in the room dedicated to the Matrix Rust SDK itself, since this problem seems to be related to the SDK rather than sliding sync or the spec. Specifically, be sure to mention that we're using the new SyncService and that it seems strange/wrong that we'd need to completely bypass the sliding sync protocol in order to make a full sync_once() call.

It seems like sync_once does not throw error, after deleting the Robius's local data. Hence I am requesting another review.

Ah ok, that's interesting. It's probably due to the fact that the sessions that get saved to disk (and subsequently restored), so if you try to restore a pure SlidingSync-based session but treat it as a traditional-sync session, then it's not going to work well. The inverse of that is also true.

I'd still like to avoid having to call sync_once() simply because it is massively expensive and defeats the entire purpose of using sliding sync in the first place....

---

Element X Android's code is

        client.getRoom(notifiableEvent.roomId)?.use { room ->
            room.subscribeToSync()

            // If the app is in foreground, sync is already running, so just add the subscription.
            if (!appForegroundStateService.isInForeground.value) {
                val syncService = client.syncService()
                syncService.startSyncIfNeeded()
                if (isRingingCallEvent) {
                    room.waitsUntilUserIsInTheCall(timeout = 60.seconds)
                } else {
                    room.waitsUntilEventIsKnown(eventId = notifiableEvent.eventId, timeout = 10.seconds)
                }
                syncService.stopSyncIfNeeded()
            }
        }

Their android's SDK is syncService.startSync, while rust is just SyncService.start(). So it can be hard to compare.

If you dig a bit deeper, you'll see that they're just directly calling SyncService.start() via Rust SDK bindings: https://github.com/element-hq/element-x-android/blob/55a5d45d4fb446a84dc96d4305b559f84bc68ee4/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt#L38

So in summary, Element X appears to not need to call the expensive bloated sync_once() function, as I suspected. Please investigate this further to see if we truly must call sync_once(), because I definitely think it shouldn't be required (at least theoretically).

[kevinaboos]

A final thought below:

It seems like sync_once does not throw error, after deleting the Robius's local data. Hence I am requesting another review.

Ah ok, that's interesting. It's probably due to the fact that the sessions that get saved to disk (and subsequently restored), so if you try to restore a pure SlidingSync-based session but treat it as a traditional-sync session, then it's not going to work well. The inverse of that is also true.

Based on this, we actually cannot call sync_once() at all, as it will break Robrix's expectations of a pure-SlidingSync implementation. Thus, we need to find another way to forcibly synchronize the read receipt state of each room that gets opened (if that's the solution for this).

[kevinaboos]

I also just confirmed that we do enable track_read_marker_and_receipts() when building each room's Timeline instance, so that's thankfully not the problem.

[alanpoon]

A final thought below:

It seems like sync_once does not throw error, after deleting the Robius's local data. Hence I am requesting another review.

Ah ok, that's interesting. It's probably due to the fact that the sessions that get saved to disk (and subsequently restored), so if you try to restore a pure SlidingSync-based session but treat it as a traditional-sync session, then it's not going to work well. The inverse of that is also true.

Based on this, we actually cannot call sync_once() at all, as it will break Robrix's expectations of a pure-SlidingSync implementation. Thus, we need to find another way to forcibly synchronize the read receipt state of each room that gets opened (if that's the solution for this).

I've checked the element X android http request using emulator. It does also call /sync without pos parameter and with a timeout of 30 seconds. This suggests a sync_once api call. This request is missing in our current implementation. I don't think this will affect the performance as app handles the case of sync_once failing.

The possible error of implementing sync_once,'extensions.to_device.since' is invalid is likely a bug in the rust matrix-sdk-client since it is a parameter type error.

[kevinaboos]

I've checked the element X android http request using emulator. It does also call /sync without pos parameter and with a timeout of 30 seconds. This suggests a sync_once api call. This request is missing in our current implementation. I don't think this will affect the performance as app handles the case of sync_once failing.

Thanks for digging into it. I'm not disagreeing with you necessarily, but what your screenshot shows is a call to the SlidingSync sync endpoint (actually two calls, but yes). Note the simplified_msc3575 part, which refers to simplified (native) SlidingSync.

Is that the same as what sync_once() does, as in, does it also call the simplified_msc3575 sync endpoint? Please confirm this, because I thought it called the old original sync endpoint.

The possible error of implementing sync_once,'extensions.to_device.since' is invalid is likely a bug in the rust matrix-sdk-client since it is a parameter type error.

I've received this error before, I don't think it's a bug but rather an error that occurs when you try to restore an old session that was generated with a previous version of the SDK or different sync configuration that isn't compatible with the current one.

[alanpoon]

I've checked the element X android http request using emulator. It does also call /sync without pos parameter and with a timeout of 30 seconds. This suggests a sync_once api call. This request is missing in our current implementation. I don't think this will affect the performance as app handles the case of sync_once failing.

Thanks for digging into it. I'm not disagreeing with you necessarily, but what your screenshot shows is a call to the SlidingSync sync endpoint (actually two calls, but yes). Note the simplified_msc3575 part, which refers to simplified (native) SlidingSync.

Is that the same as what sync_once() does, as in, does it also call the simplified_msc3575 sync endpoint? Please confirm this, because I thought it called the old original sync endpoint.

The possible error of implementing sync_once,'extensions.to_device.since' is invalid is likely a bug in the rust matrix-sdk-client since it is a parameter type error.

I've received this error before, I don't think it's a bug but rather an error that occurs when you try to restore an old session that was generated with a previous version of the SDK or different sync configuration that isn't compatible with the current one.

You are right. Sync_once corresponds to client/v3/sync?timeout=30000 which is not found in element X android app. Let me check again.

[kevinaboos]

You are right. Sync_once corresponds to client/v3/sync?timeout=30000 which is not found in element X android app. Let me check again.

Thanks for confirming. Let me know if you find out any more info.

[alanpoon]

You are right. Sync_once corresponds to client/v3/sync?timeout=30000 which is not found in element X android app. Let me check again.

Thanks for confirming. Let me know if you find out any more info.

I have found the function to sync the room's read receipts. https://docs.rs/matrix-sdk-ui/0.9.0/matrix_sdk_ui/room_list_service/struct.RoomListService.html#method.subscribe_to_rooms.

The room_subscription was previously missing in the http request payload.

[kevinaboos]

I have found the function to sync the room's read receipts. docs.rs/matrix-sdk-ui/0.9.0/matrix_sdk_ui/room_list_service/struct.RoomListService.html#method.subscribe_to_rooms.

The room_subscription was previously missing in the http request payload.

Oh weird, I didn't know we had to call that function. Based on the docs, I specifically chose not to call that function since I didn't want to oversubscribe to lots of rooms that we don't necessarily care about. That's frustrating, I'm going to raise this problem with the SDK devs.

I wonder if subscribing to a room is also the reason that we don't properly receive tombstone events (see #178)

[alanpoon]

Merged in main

[kevinaboos]

Ok I found the problem to (1) above -- you just needed to move the room_screen_tooltip inside of the room_screen_wrapper view, not beneath it. Probably just an accident.

I've fixed it in my latest commit to this branch. You can apply the same change to the other PR #168.

[kevinaboos]

now that i've merged #168, you'll need to resolve conflicts here and combine some of the code shared between this PR and #168.

[alanpoon]

The code structure looks all good to me. There are just a few UI issues:

1. The RoomScreen is only filling half of the height of the window, similar to your other PR. Probably the same issue with the room screen tooltip?
2. It takes a really long time for the AvatarRow to actually show the profile pictures after the user profile request completes. I bet we're missing a UI Signal or something that would trigger a redraw of that AvatarRow, so please look into that.
3. The tooltip shows the correct total count of people that have seen the message, but then incorrectly lists only the first 5 names. We should either display the entire list of all user names, or if you want to show only 5, then you should say "Seen by 8 people: A, B, C, D, E, and 3 more". Otherwise the user will think there's an odd math mistake. Also, the word "people" is missing, i.e., it should be "Seen by 8 people: ...".

4. The right-side margin used for the read receipt's Avatar Row is causing a lot of horizontal empty space on the right side of the RoomScreen. We're now wasting like 80 or so pixels of space there, so please fix that. Here's a screenshot of what I'm referring to:

I am still troubleshooting Point 2. To see if the performance of loading the avatar

[alanpoon]

The code structure looks all good to me. There are just a few UI issues:

1. The RoomScreen is only filling half of the height of the window, similar to your other PR. Probably the same issue with the room screen tooltip?
2. It takes a really long time for the AvatarRow to actually show the profile pictures after the user profile request completes. I bet we're missing a UI Signal or something that would trigger a redraw of that AvatarRow, so please look into that.
3. The tooltip shows the correct total count of people that have seen the message, but then incorrectly lists only the first 5 names. We should either display the entire list of all user names, or if you want to show only 5, then you should say "Seen by 8 people: A, B, C, D, E, and 3 more". Otherwise the user will think there's an odd math mistake. Also, the word "people" is missing, i.e., it should be "Seen by 8 people: ...".

4. The right-side margin used for the read receipt's Avatar Row is causing a lot of horizontal empty space on the right side of the RoomScreen. We're now wasting like 80 or so pixels of space there, so please fix that. Here's a screenshot of what I'm referring to:

I am still troubleshooting Point 2. To see if the performance of loading the avatar

There is SignalToUi for enqueue_avatar_update already, so I don't think I can optimize it further.

[kevinaboos]

There is SignalToUi for enqueue_avatar_update already, so I don't think I can optimize it further.

Hmm, ok. Yeah, the only potential cause I could think of was a missing call to set the UI Signal, but if that's already there, then there might be a different cause for the delay. What I observed is a 10+ second delay between (1) a log statement that says the avatar/profile was fetched for a given user, and (2) the actual image being refreshed for the avatar.

Another cause of this might be how we cache whether a given event was drawn in the timeline. We might need another variable that tracks the draw status of the read receipts separately from that of the event's content and the event's user profile details.

In any case, we can make this improvement later and merge this in now, since the fundamental functionality appears to be working correctly.

[kevinaboos]

Oh, and also feel free to ignore the positioning of the Message action bar (the small Reply button that pops up when you hover over a message). I am in the process of removing that, so you don't need to reserve any space for it on the right side of the room screen.

[alanpoon]

Code looks good! I've tested everything locally and it works well.

However, the UI layout of the AvatarRow is a bit inconsistent for different message types. For example, the Avatars are not all vertically aligned, so it looks a bit weird. There's also still a lot of wasted space on the right side of the RoomScreen.

I think you could fix both of these layout issues by setting the x-alignment of the AvatarRow to the far right side of its parent view in the room screen, and then using a right-margin of 10 or 20.

The final issue would be that the AvatarRow should be displayed towards the bottom of each message, not towards the top of it. This is particularly noticeable on regular (non-Condensed) messages, like the ones from 8:59am, 10:02am, and 10:10am in this screenshot.

Sped up the avatar loading, and right align the read receipt