Display read receipts next to each message as a row of Avatars for users who have seen that message #162

alanpoon · 2024-09-26T11:38:17Z

Fixes #153

With tooltip indicating number of seen - mouse over

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 · 2024-09-28T12:32:51Z

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

alanpoon · 2024-10-10T06:52:30Z

Modified set_avatar_and_get_username function to use &mut Avatar instead of AvatarRef
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.
Added Caching for Avatar that is loaded with Image.

kevinaboos

A great start, thanks!

I suggested some minor API changes that will make things clearer and less error-prone. After those changes are made, it should be ready to merge.

src/shared/avatar.rs

src/home/room_read_receipt.rs

src/home/room_screen.rs

src/profile/user_profile.rs

src/profile/user_profile_cache.rs

src/home/room_screen.rs

kevinaboos · 2024-10-31T19:06:19Z

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.

src/home/room_read_receipt.rs

Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>

…brix into read_receipt_display

kevinaboos

Thanks for refactoring the code; it does look nicer now.

I'm not sure how thoroughly you tested this, but I experience quite a few bugs or odd behaviors on my machine when testing it.

First, the on-hover tooltip is frequently cut off, so let's reposition that differently.

Second, the tooltip also doesn't disappear properly when you hover out of the avatar row:

Third, the log is absolutely filled with these messages:

src/sliding_sync.rs:433:21 - Sending get user profile request: ...

which indicates that you're not correctly using the user profile cache, as I see tons of duplicate requests. If you use the cache correctly (and do not re-draw the AvatarRow until a cache update is received), then you should see only one request max per unknown user ID.

Finally, and most importantly, the displayed read receipt info is actually incorrect and/or incomplete. If you scroll through any larger public room (e.g., Element Web/Desktop, Matrix HQ, etc) you'll see a total mismatch between the read receipts displayed on Robrix and the read receipts displayed by Element in the same room. Not sure what you're doing to cause this, but please look into the correctness/accuracy of the read receipts. Perhaps they're not getting updated?

Read receipts also seem to be completely missing for quite a few messages: I see them in Element but not in Robrix. Please investigate; this is probably related to the above issue too.

Finally, address all the compiler warnings that your PR has added (the main branch currently has 5 warnings, which you can ignore).

kevinaboos · 2024-11-07T19:23:43Z

src/shared/avatar.rs

+pub fn set_avatar_and_get_username(
+    cx: &mut Cx,
+    avatar: AvatarRef,


i think you missed my point that this should be a method implemented on the Avatar type, with a typical WidgetRef-style wrapper that is implemented for AvatarRef and just calls this inner function. That is the canonical Makepad way of doing things.

kevinaboos · 2024-11-07T19:24:48Z

src/shared/avatar.rs

+    /// Returns Hit event when mouse in
+    fn hit(&mut self, cx: &mut Cx, event: &Event, sweep_area: Area) -> Hit {


the docs and/or function name here need to be improved, because I can't understand what this function is doing or how I should be using it just by reading the docs alone (which is the goal).

kevinaboos · 2024-11-07T19:24:55Z

src/shared/avatar.rs

+    /// Returns an option of Hit event when mouse in
+    pub fn hit(&mut self, cx: &mut Cx, event: &Event, sweep_area: Area) -> Option<Hit> {


same comment here:
the docs and/or function name here need to be improved, because I can't understand what this function is doing or how I should be using it just by reading the docs alone (which is the goal).

kevinaboos · 2024-11-07T19:25:19Z

src/home/room_screen.rs

@@ -2577,10 +2593,10 @@ fn draw_replied_to_message(
                let (in_reply_to_username, is_avatar_fully_drawn) = set_avatar_and_get_username(
                    cx,
                    replied_to_message_view
-                        .avatar(id!(replied_to_message_content.reply_preview_avatar)),
+                    .avatar(id!(replied_to_message_content.reply_preview_avatar)),


the indentation was previously correct, please undo.

kevinaboos · 2024-11-07T19:26:57Z

src/home/room_read_receipt.rs

+    fn data_to_widget(&mut self, cx: &mut Cx, nodes: &[LiveNode], path: &[LiveId]) {}
+}
+impl AvatarRow {
+    fn set_range(&mut self, cx: &mut Cx, total_num_seen: usize) {


I still don't understand why we need this function ... see my other review comment within fn set_avatar_row().

Having this as a separate function invites someone to accidentally invoke it without understanding what other steps need to be simultaneously undertaken, i.e., it's easy to mis-use this.

All of this is just a fancy way of saying "use defensive programming techniques", in which you employ a design/implementation strategy that prevents a developer from accidentally making mistakes (yourself included). That is one of Rust's design goals as well.

kevinaboos · 2024-11-07T19:28:49Z

src/home/room_read_receipt.rs

+    pub fn set_avatar_row<'a, T>(&mut self, cx: &mut Cx, room_id: &RoomId, event_id: Option<&EventId>, receipts_len: usize, receipts_iter: T) 
+        where T:Iterator<Item = (&'a OwnedUserId, &'a Receipt)> {


this function can be simplified in two ways.

There's no need for generics here. Just directly accept the IndexMap of read receipts, which will also allow you do to number 2 below.

Remove the receipts_len parameter, since that is completely unnecessary and error-prone, as it's separate from (and redundant with) the IndexMap of read receipts.

kevinaboos · 2024-11-07T19:30:50Z

src/home/room_read_receipt.rs

+impl Widget for AvatarRow {
+    fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) {
+        let uid = self.widget_uid();
+        for button in self.buttons.iter_mut() {


isn't this a very expensive way to check for a hit? Surely we should just be checking the full area of the AvatarRow as a single "whole" entity, instead of checking each button.

Unless, that is, you're going to allow each mini avatar in the AvatarRow to be individually-clickable ... which is fine, but I don't think you're doing that.

kevinaboos · 2024-11-07T19:33:44Z

src/home/room_read_receipt.rs

+        for v in self.buttons.iter_mut() {
+            let _ = v.draw(cx, scope);
+        }
+        if self.total_num_seen > 5 {


extract this 5 into a const and place that const at the top of this file. You can name it MAX_VISIBLE_AVATARS_IN_READ_RECEIPT_ROW or something similar.

See the programming rule of no magic numbers.

kevinaboos · 2024-11-07T19:37:23Z

src/home/room_read_receipt.rs

+    pub fn set_avatar_row<'a, T>(&mut self, cx: &mut Cx, room_id: &RoomId, event_id: Option<&EventId>, receipts_len: usize, receipts_iter: T) 
+        where T:Iterator<Item = (&'a OwnedUserId, &'a Receipt)> {
+        if let Some(ref mut inner) = self.borrow_mut() {
+            inner.set_range(cx, receipts_len);


set_range should not be a separate function, as that overcomplicates the API. You should directly set the total_num_seen within this set_avatar_row() fn based on the len() of the read receipts IndexMap, as that guarantees that the value will always be correct/accurate.

kevinaboos · 2024-11-07T19:38:16Z

src/home/room_screen.rs

@@ -419,7 +420,7 @@ live_design! {

                message_annotations = <MessageAnnotations> {}
            }
-
+            avatar_row = <AvatarRow> {width: 40, height: 30, margin: { top: (12.0) }, hover_actions_enabled:true }


formatting fixes

Suggested change

avatar_row = <AvatarRow> {width: 40, height: 30, margin: { top: (12.0) }, hover_actions_enabled:true }

avatar_row = <AvatarRow> {

width: 40,

height: 30,

margin: { top: (12.0) },

hover_actions_enabled: true,

}

read_receipt_display

5c54512

alanpoon marked this pull request as ready for review September 27, 2024 04:20

Merge branch 'main' into read_receipt_display

4a78a93

alanpoon marked this pull request as draft September 28, 2024 12:31

alanpoon added 10 commits October 3, 2024 16:04

merge main

4ad555a

touchup whitespace

22638d9

Merge branch 'main' into read_receipt_display

0ccb874

added avatar img in read_receipts

f4de445

fixed tooltip position

e4e9b1d

code formating

961a00f

Merge branch 'main' into read_receipt_display

2ff7f7d

Merge branch 'main' into read_receipt_display

d4fd338

fix merge issue

c15e8da

change sender_id to generic avatar_id

3571b1e

alanpoon marked this pull request as ready for review October 10, 2024 06:52

alanpoon added 2 commits October 12, 2024 14:32

Merge branch 'main' into read_receipt_display

0c6dd14

fix conflicts

24daf6e

kevinaboos requested changes Oct 31, 2024

View reviewed changes

Merge branch 'main' into read_receipt_display

e5f6943

alanpoon commented Nov 4, 2024

View reviewed changes

src/home/room_read_receipt.rs Outdated Show resolved Hide resolved

alanpoon and others added 7 commits November 4, 2024 23:17

Update src/profile/user_profile.rs

687bb70

Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>

remove unneccessary portal_list

17d1bec

Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>

Change tooltip

fa5f166

Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com>

Merge branch 'main' into read_receipt_display

0e6ae80

changed to avatarref

a36049f

format fix

b3d75b9

Merge branch 'read_receipt_display' of https://github.com/alanpoon/ro…

7935ffb

…brix into read_receipt_display

moved set_avatar_and_get_username into avatar file

9a7af7a

kevinaboos added the waiting-on-author This issue is waiting on the original author for a response label Nov 7, 2024

alanpoon added 2 commits November 7, 2024 13:41

added plus label at the end of avatarrow

7fa6f39

clean up

b81e51a

alanpoon requested a review from kevinaboos November 7, 2024 06:12

kevinaboos added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 7, 2024

alanpoon self-assigned this Nov 7, 2024

kevinaboos requested changes Nov 7, 2024

View reviewed changes

kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 7, 2024

kevinaboos changed the title ~~Add read receipt display~~ Display read receipts next to each message as a row of Avatars for users who have seen that message Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display read receipts next to each message as a row of Avatars for users who have seen that message #162

Display read receipts next to each message as a row of Avatars for users who have seen that message #162

alanpoon commented Sep 26, 2024 •

edited

Loading

alanpoon commented Sep 28, 2024

alanpoon commented Oct 10, 2024

kevinaboos left a comment

kevinaboos commented Oct 31, 2024

kevinaboos left a comment

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

kevinaboos Nov 7, 2024

		/// Returns Hit event when mouse in
		fn hit(&mut self, cx: &mut Cx, event: &Event, sweep_area: Area) -> Hit {

		/// Returns an option of Hit event when mouse in
		pub fn hit(&mut self, cx: &mut Cx, event: &Event, sweep_area: Area) -> Option<Hit> {

		pub fn set_avatar_row<'a, T>(&mut self, cx: &mut Cx, room_id: &RoomId, event_id: Option<&EventId>, receipts_len: usize, receipts_iter: T)
		where T:Iterator<Item = (&'a OwnedUserId, &'a Receipt)> {

Display read receipts next to each message as a row of Avatars for users who have seen that message #162

Are you sure you want to change the base?

Display read receipts next to each message as a row of Avatars for users who have seen that message #162

Conversation

alanpoon commented Sep 26, 2024 • edited Loading

alanpoon commented Sep 28, 2024

alanpoon commented Oct 10, 2024

kevinaboos left a comment

Choose a reason for hiding this comment

kevinaboos commented Oct 31, 2024

kevinaboos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpoon commented Sep 26, 2024 •

edited

Loading