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

Friendlier room names when calculating from members #1602

Closed
wants to merge 2 commits into from

Conversation

tlaundal
Copy link

@tlaundal tlaundal commented Feb 12, 2021

This is my first PR, and an attempt to solve element-hq/element-meta#1389.
The implementation is driven by my own motivation for quickly being able to recognize my own rooms (bridged from Facebook Messenger). The style is inspired by Messenger and the issue mentioned above.

I know room naming is specified the client-server spec section 13.2.2.5 Calculating the display name for a room, and I don't believe this PR breaks that spec, although "calculate display names [...] and concatenating them" might be interpreted in several ways. I am of course open to changing my implementation should someone have better ideas, though.

In action

Here are some screenshots from Element.
Before
Skjermdump fra 2021-02-14 13-13-55
After
Skjermdump fra 2021-02-14 13-17-31

Possible caveats

  • Not all names are delimited by spaces, or are reasonable distinct in their first "word"
  • The first "word" of names might be long, so the total length of the room name is longer than ideal for an UI. This problem also exists in the existing implementation
  • The first name / word might not be a good representation of the relevant person in all cultures

Commit message

The following changes are made to the room name calculation from
list of members:

  1. Only show the first word of each member name (before first space)
  2. Show up to three members listed explicitly
  3. Show two names before appending "and n others"

The possible forms are now:

  • Empty room
  • Paul Brown
    • When there is only one member except you
  • Paul and John
    • When there are two members except you
  • Paul, John and Will
    • When there are three members except you
  • Paul, John and 3 others
    • When there are more than three members except you
    • Or, only two members have actually joined the room
  • Paul, John and 1 other
    • When the third member except you has not yet joined the room
  • Paul and n other(s)
    • When only one member except you has actually joined the room

Signed-off-by: Tobias Laundal [email protected]


Here's what your changelog entry will look like:

✨ Features

  • Friendlier room names when calculating from members (#1602). Contributed by @tlaundal.

The following changes are made to the room name calculation from
list of members:
 1. Only show the first word of each member name (before first space)
 3. Show up to three members listed explicitly
 2. Show two names before appending "and n others"

The possible forms are now:
 * Empty room
 * Paul Brown
   - When there is only one member except you
 * Paul and John
   - When there are two members except you
 * Paul, John and Will
   - When there are three members except you
 * Paul, John and 3 others
   - When there are more than three members except you
   - Or, only two members have actually joined the room
 * Paul, John and 1 other
   - When the third member except you has not yet joined the room
 * Paul and n other(s)
   - When only one member except you has actually joined the room

Signed-off-by: Tobias Laundal <[email protected]>
Comment on lines -1869 to -1870
// -1 because these numbers include the syncing user
const inviteJoinCount = joinedMemberCount + invitedMemberCount - 1;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually a bug it seems. inviteJoinCount is only used in the changed if statement below and for passing to memberNamesToRoomName, but memberNamesToRoomName expects the count to be including the current user.

@t3chguy t3chguy requested a review from a team February 24, 2021 10:23
@tlaundal tlaundal closed this Aug 2, 2021
@SimonBrandner
Copy link
Contributor

SimonBrandner commented Aug 2, 2021

@tlaundal, may I ask why have you closed this?

@ShadowJonathan
Copy link
Contributor

@SimonBrandner bitrot + no time?

@SimonBrandner
Copy link
Contributor

@SimonBrandner bitrot + no time?

Maybe, but I can only guess...

@tlaundal
Copy link
Author

tlaundal commented Aug 3, 2021

@tlaundal, may I ask why have you closed this?

Sure. After six months of no feedback I thought the likelihood of getting a merge was pretty low.

If there is interest for this PR and it actually has a chance to be merged, I am happy to reopen and fix the conflicts.

@SimonBrandner
Copy link
Contributor

thought the likelihood of getting a merge was pretty low.

The PR was stuck on product review which often takes a lot of time, the team is super busy, so this is mostly about waiting... I think it would make sense to leave the PR open and wait for the product review. If they (eventually) approve, you can either resolve the conflicts or someone from the team will do it instead (if they are resolvable). But it's of course up to you

Sorry for the trouble

@tlaundal
Copy link
Author

tlaundal commented Aug 3, 2021

I'll reopen this and await product review, then :)

Perhaps some info about this product review and expected wait time could go in CONTRIBUTING.md, or maybe one of those "Thanks for your PR"-bots could help with setting expectations for contributors?

@tlaundal tlaundal reopened this Aug 3, 2021
@pioneer
Copy link

pioneer commented Aug 3, 2021

For European languages, I personally think that's ok, but would like to hear feedback from someone who speaks other languages. Also, in post-USSR countries, there's sometimes a tradition of writing the surname prior to the name for some reason, this could break the implemented approach as well. Is the first word always the best representation of the full name?

@tlaundal
Copy link
Author

tlaundal commented Aug 5, 2021

Is the first word always the best representation of the full name?

I truly don't know, but I think it's the best compromise between simplicity and usability. It's fine for the few languages I know, but it would be very interesting to get some feedback with other perspectives.

@MightyCreak
Copy link

Is the first word always the best representation of the full name?

I truly don't know, but I think it's the best compromise between simplicity and usability. It's fine for the few languages I know, but it would be very interesting to get some feedback with other perspectives.

I, at least, have one concrete example of a nickname that won't pass that: "The Chose" (I redacted the nickname, but the idea is here). We would have "The, Paul and 4 others", that won't fly IMHO.

Isn't it possible to give the complete list and let the client do the job?

@MadLittleMods MadLittleMods added Z-Community-PR Issue is solved by a community member's PR T-Enhancement labels Jun 2, 2022
@MadLittleMods
Copy link
Contributor

Related to matrix-org/synapse#12835

@daniellekirkwood
Copy link

Thanks so much for all the feedback here (and the hard work to create the PR). We are going to close this for now as using a space to determine what's first name vs last name is not something we're comfortable doing. As a global product this might not scale well to all regions and we cannot make the assumption that people have used the Display Name field in the way we expect.

Having said that, we have used the attached issue to raise this with our design team so over the coming weeks we expect to have some ideas for how we might handle this issue. Please 'watch' the meta issue if you'd like to give further feedback on how to resolve this problem.

Kind Regards,
D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants