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

Track display names and include them in WHOIS and WHO #70

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ToxicFrog
Copy link
Contributor

I'm using this locally and it seems to work fine. If display names are ever used to derive nicks this will need more work (in particular, send NICK when a user's display name changes), but this suffices for now.

The existing TODOs said to use the "most common" display name, but since display names are per-user rather than per-channel or per-message, we can just pick an arbitrary member record and get the display name from there; the only time these will be inconsistent is if the user has just changed their name and the server is still in the process of sending m.room.member events to propagate it to all the channels.

Previous behaviour was to use whatever the display name was on first
connect and drop any updates, which resulted in skew in displayed
messages between M51 and other clients.
@progval
Copy link
Owner

progval commented Feb 15, 2024

but since display names are per-user rather than per-channel or per-message

Display names are per-room. The way they are transmitted is through each room's latest m.room.member state event for the given user.

@ToxicFrog
Copy link
Contributor Author

ToxicFrog commented Feb 15, 2024

Display names are per-room. The way they are transmitted is through each room's latest m.room.member state event for the given user.

According to the spec, they're per-user:

There's nothing physically preventing a homeserver from sending m.room.member events to different rooms offering different display names for the same user, but that's not in spec, and if a server does that I think it's entirely valid to just pick one of those arbitrarily, although probably the most correct way to handle that sort of misbehaviour is to treat the m.presence (which the server should also send) as canonical.

@progval
Copy link
Owner

progval commented Feb 15, 2024

I see, that's for fetching display names from the user's profile, which is global.

However, this PR uses room state rather than the profile to fetch display names, which may differ from each other and/or the profile.

@ToxicFrog
Copy link
Contributor Author

Looking into it more (especially matrix-org/matrix-spec#103 which has a lot of discussion on this topic), it looks like the current state of affairs is:

  • clients can set their profile information per-room by manually crafting an m.room.member state event and sending it to the server, which the spec discourages but does not outright forbid
  • some clients, including Element, expose this capability to the user
  • it's unclear if the server is actually required to retain this per-room information, although Synapse does
  • setting your profile information "normally" overwrites your per-room information in all rooms that you are in, and this behaviour is required by the spec (9.2.1 again)

Based on this I'm pretty comfortable saying that per-room display names are not well supported, and are more of a side effect of how most servers handle m.room.member events than an intentional feature. There have been various attempts over the years to make it intentional (e.g. matrix-org/matrix-spec#323, matrix-org/matrix-spec-proposals#1302, matrix-org/matrix-spec-proposals#3189) but they're all either stalled out or blocked on more fundamental changes to the protocol.

All that said, we have a few other options here, if we don't trust room state to be consistent; the easiest and most spirit-of-the-law is, I think, to simply call the /displayname endpoint and use the user's profile displayname in WHOIS, rather than risking skew between different room states, and I'm happy to rework the PR to do that if you'd rather take that approach.

@progval
Copy link
Owner

progval commented Feb 15, 2024

All that said, we have a few other options here, if we don't trust room state to be consistent; the easiest and most spirit-of-the-law is, I think, to simply call the /displayname endpoint and use the user's profile displayname in WHOIS, rather than risking skew between different room states, and I'm happy to rework the PR to do that if you'd rather take that approach.

Sounds good

@progval
Copy link
Owner

progval commented Feb 15, 2024

(but keep the per-room display-name in the @draft/display-name message tag)

@ToxicFrog
Copy link
Contributor Author

Will do, not planning to touch that code, just WHO/WHOIS. I'll also keep the commit that properly retains updates to per-channel display names even though it won't be load-bearing for the rest of the PR after this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants