element-hq / element-meta

Shared/meta documentation and project artefacts for Element clients
65 stars 11 forks source link

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

Open manuroe opened 2 months ago

manuroe commented 2 months ago

Description

Second task:

image 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

Size estimate

None

Dependencies

Out of scope

Open questions

### Questions

Subtasks

### Android
- [ ] https://github.com/element-hq/element-x-android/issues/2722
### iOS
- [ ] https://github.com/element-hq/element-x-ios/issues/2706
### Bugs
- [ ] https://github.com/element-hq/element-x-ios/issues/2846
- [ ] https://github.com/element-hq/element-x-android/issues/2866
- [ ] "state_event_room_leave" = "%1$@ left the room" displays the user id on EXA and EXI

Sign-off

Android

iOS

amshakal commented 2 months ago

Here you go: Android - https://www.figma.com/file/Ni6Ii8YKtmXCKYNE90cC67/Timeline-(new)?type=design&node-id=917%3A80169&mode=design&t=fxqSw8Jgo2CmRXaR-1

iOS - https://www.figma.com/file/Ni6Ii8YKtmXCKYNE90cC67/Timeline-(new)?type=design&node-id=917%3A79842&mode=design&t=fxqSw8Jgo2CmRXaR-1

bmarty commented 2 months ago

@amshakal perfect, thanks!

bmarty commented 2 months ago

@stefanceriu I have added a status for each string in https://github.com/element-hq/element-x-android/issues/2722.

stefanceriu commented 2 months ago

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

Nice, thank you!

Velin92 commented 1 week ago

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 commented 1 week ago
{
  "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 commented 1 week ago

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