Skip to content
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

[Story] Use display names instead of matrix ids for state events in EX #2395

Closed
5 tasks done
manuroe opened this issue Apr 17, 2024 · 10 comments
Closed
5 tasks done

[Story] Use display names instead of matrix ids for state events in EX #2395

manuroe opened this issue Apr 17, 2024 · 10 comments
Labels
App: ElementX Android App: ElementX iOS T-Epic Issue is at Epic level T-User Story Team: Element X Platform X-Needs-Signoff Stories and Epics which are ready for review by product, design and QA

Comments

@manuroe
Copy link
Member

manuroe commented Apr 17, 2024

Description

  • As a user
  • I want to see users display names instead of matrix ids in the timeline
  • So that I am not focused by technical noise

Second task:

  • the display name of sender for timeline items must be rendered with userId if there is an ambiguation. We need a design, we cannot just append the userId between parenthesis, since this can be impersonated too, (for instance bob can change their display name to `Alice (@alice:sever.org)'. FWIW, Web is doing this:
image
  • Also applicable to reply preview:
image

@amshakal can you provide a design for sender display name disambiguation on mobile? (or point me to where the design does exist?) Thanks!

Acceptance criteria

Display names must be disambiguated with the matrix id when necessary.

Following strings are based on the EXI code in an attempt to be as exhaustive as possible. Thanks to common strings, it should apply to Android too.

The app must display user name for %1$@ in:

"state_event_avatar_url_changed" = "%1$@ changed their avatar";
"state_event_demoted_to_member" = "%1$@ was demoted to member";
"state_event_demoted_to_moderator" = "%1$@ was demoted to moderator";
"state_event_promoted_to_administrator" = "%1$@ was promoted to admin";
"state_event_promoted_to_moderator" = "%1$@ was promoted to moderator";
"state_event_room_avatar_changed" = "%1$@ changed the room avatar";
"state_event_room_avatar_removed" = "%1$@ removed the room avatar";
"state_event_room_created" = "%1$@ created the room";
"state_event_room_invite_accepted" = "%1$@ accepted the invite"
"state_event_room_invite_by_you" = "You invited %1$@";
"state_event_room_invite_you" = "%1$@ invited you";
"state_event_room_join" = "%1$@ joined the room";
"state_event_room_knock" = "%1$@ requested to join";
"state_event_room_knock_accepted_by_you" = "%1$@ allowed you to join";
"state_event_room_knock_denied_by_you" = "You rejected %1$@'s request to join";
"state_event_room_knock_denied_you" = "%1$@ rejected your request to join";
"state_event_room_knock_retracted" = "%1$@ is no longer interested in joining";
"state_event_room_leave" = "%1$@ left the room";
"state_event_room_name_changed" = "%1$@ changed the room name to: %2$@";
"state_event_room_name_removed" = "%1$@ removed the room name";
"state_event_room_none" = "%1$@ made no changes";
"state_event_room_reject" = "%1$@ rejected the invitation";
"state_event_room_topic_changed" = "%1$@ changed the topic to: %2$@";
"state_event_room_topic_removed" = "%1$@ removed the room topic";
"state_event_room_unknown_membership_change" = "%1$@ made an unknown change to their membership";
"state_event_room_third_party_invite_by_you" = "You sent an invitation to %1$@ to join the room";
"state_event_room_third_party_revoked_invite_by_you" = "You revoked the invitation for %1$@ to join the room";

%1$@ and %2$@ must be user names in:

"state_event_room_invite" = "%1$@ invited %2$@";
"state_event_room_knock_accepted" = "%1$@ allowed %2$@ to join";
"state_event_room_knock_denied" = "%1$@ rejected %2$@'s request to join";
"state_event_room_third_party_invite" = "%1$@ sent an invitation to %2$@ to join the room";
"state_event_room_third_party_revoked_invite" = "%1$@ revoked the invitation for %2$@ to join the room";

%1$@ must be a matrix id in:

"state_event_display_name_changed_from" = "%1$@ changed their display name from %2$@ to %3$@";
"state_event_display_name_removed" = "%1$@ removed their display name (it was %2$@)";
"state_event_display_name_set" = "%1$@ set their display name to %2$@";
"state_event_room_ban_by_you" = "You banned %1$@";
"state_event_room_remove_by_you" = "You removed %1$@";
"state_event_room_unban_by_you" = "You unbanned %1$@";

%1$@ must be a user name and %2$@ must be a matrix id in:

"state_event_room_ban" = "%1$@ banned %2$@";
"state_event_room_remove" = "%1$@ removed %2$@";
"state_event_room_unban" = "%1$@ unbanned %2$@";

Other cases we do not need to check or change as part of this stoyr:

"state_event_avatar_changed_too" = "(avatar was changed too)";
"state_event_avatar_url_changed_by_you" = "You changed your avatar";
"state_event_display_name_changed_from_by_you" = "You changed your display name from %1$@ to %2$@";
"state_event_display_name_removed_by_you" = "You removed your display name (it was %1$@)";
"state_event_display_name_set_by_you" = "You set your display name to %1$@";
"state_event_room_avatar_changed_by_you" = "You changed the room avatar";
"state_event_room_avatar_removed_by_you" = "You removed the room avatar";
"state_event_room_created_by_you" = "You created the room";
"state_event_room_invite_accepted_by_you" = "You accepted the invite";
"state_event_room_join_by_you" = "You joined the room";
"state_event_room_knock_by_you" = "You requested to join";
"state_event_room_knock_retracted_by_you" = "You cancelled your request to join";
"state_event_room_leave_by_you" = "You left the room";
"state_event_room_name_changed_by_you" = "You changed the room name to: %1$@";
"state_event_room_name_removed_by_you" = "You removed the room name";
"state_event_room_none_by_you" = "You made no changes";
"state_event_room_reject_by_you" = "You rejected the invitation";
"state_event_room_topic_changed_by_you" = "You changed the topic to: %1$@";
"state_event_room_topic_removed_by_you" = "You removed the room topic";

Leads

  • Tech:
  • Design:

Size estimate

None

Dependencies

  • None

Out of scope

  • Nothing

Open questions

Questions

Preview Give feedback
No tasks being tracked yet.

Subtasks

Android

Preview Give feedback
  1. T-Task
    bmarty

iOS

Preview Give feedback
  1. T-Task

Bugs

Preview Give feedback
  1. T-Defect
    Velin92
  2. T-Defect
    jmartinesp

Sign-off

Android

  • Design sign-off on completion
  • QA sign-off on completion
  • Product sign-off on completion

iOS

  • Design sign-off on completion
  • QA sign-off on completion
  • Product sign-off on completion
@manuroe manuroe changed the title [Story] Use display names instead of matrix ids for membership changes in EX [Story] Use display names instead of matrix ids for state events in EX Apr 17, 2024
@bmarty
Copy link
Member

bmarty commented Apr 22, 2024

@amshakal perfect, thanks!

@bmarty
Copy link
Member

bmarty commented Apr 23, 2024

@stefanceriu I have added a status for each string in element-hq/element-x-android#2722.

@stefanceriu
Copy link
Member

@stefanceriu I have added a status for each string in element-hq/element-x-android#2722.

Nice, thank you!

@manuroe manuroe added the X-Needs-Signoff Stories and Epics which are ready for review by product, design and QA label Apr 25, 2024
@Velin92
Copy link
Member

Velin92 commented Jun 20, 2024

Some roomMembershipChange events coming from the SDK do not return a displayName (it returns as nil) even if the JSON content of the event contains it.
For example:
left and invitationRevoked

@Velin92
Copy link
Member

Velin92 commented Jun 20, 2024

{
  "content": {
    "membership": "leave",
    "avatar_url": "mxc://matrix.org/kSmRgrCcHHWtCKGogsSvgInx",
    "displayname": "Maurotest"
  },
  "origin_server_ts": 1718008452870,
  "sender": "@mauro.romito:element.io",
  "state_key": "@mauro.devtest:matrix.org",
  "type": "m.room.member",
  "unsigned": {
    "replaces_state": "$LzTHluued3LYQ4lywtkEyuwHtu_BwtMxcsaoX_v172g",
    "prev_content": {
      "avatar_url": "mxc://matrix.org/kSmRgrCcHHWtCKGogsSvgInx",
      "displayname": "Maurotest",
      "membership": "invite"
    },
    "prev_sender": "@mauro.romito:element.io",
    "io.element.msc4115.membership": "join",
    "age": 788561190
  },
  "event_id": "$Odx3v2ptnnXwjQsv4cmCLWGRX5C8YgsUWHSkcwq8iE0",
  "room_id": "!oQYgXvyPkZTyuOUzIl:element.io"
}
{
  "content": {
    "membership": "leave",
    "avatar_url": "mxc://matrix.org/kSmRgrCcHHWtCKGogsSvgInx",
    "displayname": "Maurotest"
  },
  "origin_server_ts": 1718797062145,
  "sender": "@mauro.devtest:matrix.org",
  "state_key": "@mauro.devtest:matrix.org",
  "type": "m.room.member",
  "unsigned": {
    "replaces_state": "$Iiwx4HkzLthOJv0MDH-TYGSSwZmeGL-A5If5Y6uxInU",
    "prev_content": {
      "avatar_url": "mxc://matrix.org/kSmRgrCcHHWtCKGogsSvgInx",
      "displayname": "Maurotest",
      "membership": "join"
    },
    "prev_sender": "@mauro.devtest:matrix.org",
    "io.element.msc4115.membership": "join"
  },
  "event_id": "$NdzYrgemVv3tbCswdNru7-x4iO3s6w6TrtIDGMOWGig",
  "room_id": "!oQYgXvyPkZTyuOUzIl:element.io"
}

The issue is that these two events are technically replacements of existing events, and we for now only fetch display name from the original content. However I see the original content is also present for these two so is unclear to why the original content is skipped i'll do some more digging

@Velin92
Copy link
Member

Velin92 commented Jun 20, 2024

Okay by debugging the SDK, and adding a some info! traces at this point in the code: https://github.com/matrix-org/matrix-rust-sdk/blob/a6c962b9b0c6a713028522b92f3a95d0cad97dbd/bindings/matrix-sdk-ffi/src/timeline/content.rs#L53-L64

I extracted the following logs for the models coming from the SDK.

matrix_sdk_ffi::timeline::content: membership: RoomMembershipChange { user_id: "@mauro.devtest:matrix.org", content: Original { content: RoomMemberEventContent { avatar_url: None, displayname: None, is_direct: None, membership: "leave", third_party_invite: None, blurhash: None, reason: None, join_authorized_via_users_server: None }, prev_content: Some(RoomMemberEventContent { avatar_url: Some("mxc://matrix.org/kSmRgrCcHHWtCKGogsSvgInx"), displayname: Some("Maurotest"), is_direct: None, membership: "join", third_party_invite: None, blurhash: None, reason: None, join_authorized_via_users_server: None }) }, change: Some(Left) }, displayname: None |

So for some reason the content is not containing the avatar and the display name even if its present in the JSON. This is the log of the leave event.

This is the invitation revoked one:

matrix_sdk_ffi::timeline::content: membership: RoomMembershipChange { user_id: "@mauro.devtest:matrix.org", content: Original { content: RoomMemberEventContent { avatar_url: None, displayname: None, is_direct: None, membership: "leave", third_party_invite: None, blurhash: None, reason: None, join_authorized_via_users_server: None }, prev_content: Some(RoomMemberEventContent { avatar_url: Some("mxc://matrix.org/kSmRgrCcHHWtCKGogsSvgInx"), displayname: Some("Maurotest"), is_direct: None, membership: "invite", third_party_invite: None, blurhash: None, reason: None, join_authorized_via_users_server: None }) }, change: Some(InvitationRevoked) }, displayname: None |

I see that the displayname is not nil in the previous content, which makes sense, but the original content given the JSON also contains the event so its unclear why is not getting set in the model for the SDK

I wonder if this is a Ruma bug, because the model RoomMemberEventContent is coming directly from Ruma

@bnjbvr
Copy link
Member

bnjbvr commented Jul 3, 2024

On my side, when looking at the room membership event in the SDK code, I see this:

{"content":{"membership":"leave"},"origin_server_ts":1720016416590,"sender":"@benjib:element.io","state_key":"@benjib:element.io","type":"m.room.member","unsigned":{"replaces_state":"$K4beJH8VcCC_jJU2a9aiMsNrJXSNkDjrxZDGe3uU8B8","prev_content":{"avatar_url":"mxc://element.io/xyz","displayname":"bnjbvr","membership":"join"},"prev_sender":"@benjib:element.io","membership":"join"},"event_id":"$xyz"}

So the content only includes the membership change.

Now, looking at what Element Web receives from sync v2, it's the same:

{
    "content": {
        "membership": "leave"
    },
    "origin_server_ts": 1720017199881,
    "sender": "@benjib:element.io",
    "state_key": "@benjib:element.io",
    "type": "m.room.member",
    "unsigned": {
        "replaces_state": "$xyz",
        "prev_content": {
            "avatar_url": "mxc://element.io/xyz",
            "displayname": "bnjbvr",
            "membership": "join"
        },
        "prev_sender": "@benjib:element.io",
        "membership": "join"
    },
    "event_id": "$xyz2"
}

And it's been confirmed by folks that Synapse only returned the membership change, so it's not the sliding sync proxy messing up with the event's content. We can't do better than this, at this point.

One solution would be to use the previous content's avatar/displayname, for leave/invitationRevoked membership changes. I'll play with that.

@bnjbvr
Copy link
Member

bnjbvr commented Jul 3, 2024

One solution would be to use the previous content's avatar/displayname, for leave/invitationRevoked membership changes. I'll play with that.

matrix-org/matrix-rust-sdk#3648
It would be super nice to have someone testing this and confirming it resolves the remaining issue 🙏

@manuroe
Copy link
Member Author

manuroe commented Jul 10, 2024

Just tested. The remaining (leave) use case works:

image

Thanks all!

@manuroe manuroe closed this as completed Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: ElementX Android App: ElementX iOS T-Epic Issue is at Epic level T-User Story Team: Element X Platform X-Needs-Signoff Stories and Epics which are ready for review by product, design and QA
Projects
None yet
Development

No branches or pull requests

6 participants