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

feat: add listusers command #854

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Sep 20, 2022

Adds the listusers command to the Discord bot, which will list the users on the Matrix side. Context and motivation is written up on the related issue at the bottom.

On the side, I also:

  • Capitalized "matrix" on some lines as Matrix is a noun.
  • Added line breaks between test cases.
  • Make the #Process command return the response as I believe this makes sense, and makes testing command output a lot easier.

Screenshots

image

Output when there are no Matrix users in the channel. This originally said "There are no users on the Matrix side.", but later decided 0 was better, it's less code so why not. Feedback welcome!

image

Output when there is 1 Matrix user in the channel with bridge.disablePresence: false.

image

Tail of the output when there are too many Matrix users, so the output is over 2,000+ characters with bridge.disablePresence: true.

Referenced how the UI in Element presents it, except I format numbers, i.e. 1,610, not 1610:
image

Draft

Open Questions

  • Can anything be moved into general helpers or utilities to reduce the size of the command? The command came out a bit fat, exploring the project to see where I can split this up, but advice is welcome.
  • Is it fine to reuse bridge.disablePresence for whether the output should include user presence in the response, or should we introduce a new setting?
  • Can we think of a good structure to include other info like last activity (last active 2 hours ago) or status messages? I'd like to, but am wary that the output will get too cluttered.
  • Should we have caching of presence here? (I'm thinking maybe that belongs in the SDK?)

Decisions

Prefixed Message

Prefixed a message at the top to make sure Discord users understand that the list can change between channels in the same server. They are accustomed to guild and channels being coupled together. This reminds them a Matrix user can choose which chat rooms to join, so being in one doesn't mean they're in others.

Handling 2,000+ Characters

A proposal in the issue was made that we could upload a file instead. [source]
I've read the Discord docs, file previews are up to 100 lines when expanded, and up to 50 kb (presumably around 50,000~ characters) when previewed. [source]

On Discord, guild and user settings won't let disable file previews, so that doesn't hinder this either, but I only checked on desktop, not mobile.

I didn't implement this since I thought the file approach looked clunky. Numerous users have expressed that the important thing is knowing how many people are on Matrix. While for small communities names are nice, for larger communities the names are just noise, they just want to know how many people are on the other side.

This makes me think coming up with a good and predictable output has more value than changing it based on the number of users or the online state of users. For example, users going from online (6 chars) to offline (7 chars), which makes the output over 2,000 characters and so sends a file instead. It's annoying if the output format changes unpredictably.

Presence Config

The output can display presence, since this seems valuable for smaller communities or hosts, but might not scale to larger ones with thousands of users in a single chat room.

By adding the option, people self-hosting for their community can enable it safely, but public hosts like t2bot may want to disable it.

Presence UI

Original plan was to replace the bullet points with 🟢, 🟠, and ⚫ for "online", "unavailable", and "offline" respectively, however that looked ugly and was visually inaccessible depending on the client color-scheme. Then I experimented with ⊕, ⊘, and ⊖, as this uses the font color, but text-to-speech (screen readers) won't read it (tested with eSpeak NG), and it's unclear what any of the icons mean.

In the end, we decided the best approach was to just append a hyphen followed by the state to the end of the line if the presence was available. This is visually accessible, screen reader friendly, and doesn't require a legend to understand.

When a user's presence is "unavailable", we display Idle as this term will be more familiar to Discord users.

Sorting Results

  • Online Presence (Online, Idle, Offline, Unknown)
  • Display Name, case-sensitive (If not set, end of list sorted by MXID.)
  • MXID

Related

@SethFalco SethFalco force-pushed the add-listusers-command branch 3 times, most recently from 62e0946 to 2214348 Compare September 20, 2022 19:06
@SethFalco SethFalco force-pushed the add-listusers-command branch 3 times, most recently from 163efd9 to 1c7914a Compare September 20, 2022 19:35
@SethFalco SethFalco force-pushed the add-listusers-command branch 6 times, most recently from d362978 to 8738ebd Compare September 27, 2022 16:03
@SethFalco SethFalco force-pushed the add-listusers-command branch 4 times, most recently from 9d08408 to 6b974f1 Compare October 8, 2022 14:56
@SethFalco SethFalco marked this pull request as ready for review October 8, 2022 15:04
@SethFalco SethFalco requested a review from a team as a code owner October 8, 2022 15:04
@SethFalco SethFalco force-pushed the add-listusers-command branch from 6b974f1 to 36e02e3 Compare October 8, 2022 15:14
@Miepee
Copy link
Contributor

Miepee commented Oct 8, 2022

Numerous users have expressed that the important thing is knowing how many people are on Matrix. While for small communities names are nice, for larger communities the names are just noise, they just want to know how many people are on the other side.

May be a little late with thoughts, but maybe take out the whole listing users then, and only show how many are in the other room.
Then you could also make that feature for the Matrix users as well, so they can see how many people are in the discord guild (/have access to the bridged channel).

@SethFalco
Copy link
Contributor Author

SethFalco commented Oct 8, 2022

May be a little late with thoughts

Never too late, no worries!

but maybe take out the whole listing users then,

I don't think this is a fair conclusion from my statement. The reason that the number of users is more notable is because when a chat rooms has hundreds or thousands of users, no one really cares about or recognizes the names in the list.

However, for small communities this is still desirable. At least in the communities I'm participating in and implementing this feature for, seeing a list of usernames was the whole point of this feature request! 🥲

The goal in my statement was that by the time the message hits the 2,000+ limit, it's already too many users to keep track of, so it might not be worth finding a way to display more results. (If anything, perhaps it's worth truncating results to 32~ or so.)

If the consensus from maintainers is to remove the list entirely, while I'm happy to oblige, I just want to say that I think it'd really inhibit how practical this feature is for smaller communities.

Then you could also make that feature for the Matrix users as well

Happy to look into this, but do you have a use-case for it?

The reason I implemented this to be Discord only is because on the Matrix side we can already see a list of Discord users. There is a lot of transparency and a pretty good UI in clients already, so it seemed redundant to make a similar command on the Matrix side.

Whereas on Discord, there is no transparency.

@Miepee
Copy link
Contributor

Miepee commented Oct 8, 2022

we can already see a list of Discord users

we can only see users who actively participated afaik. Users who are just lurking will not show up.

and a pretty good UI in clients already

May be me, whenever i tried to find who's from discord, i needed to painstakingly filter out the discord users, which gets a bit painful if the room is big.

@SethFalco
Copy link
Contributor Author

SethFalco commented Oct 8, 2022

Users who are just lurking will not show up.

Ahh alright, good call!
Though it could be argued maybe, there should be a setting/command in the bridge to create puppets upfront for users who haven't interacted yet then. Thoughts on that? 🤔

Could be worth opening a separate issue to discuss a way to see lurkers from Discord in Matrix? Once a conclusion is determined, either of us can implement it. I think making a way to force syncing Discord users is better than a command to list the number of users.

Funnily enough, someone from Discord literally just commented about this while I was typing this response. ^-^'


Meanwhile, I've been getting further feedback on this from users.

So far, pretty much everyone agrees that an actual list of users is desirable.
However, the consensus is that is they'd prefer to see pagination over truncating results. 🥲

I think if we extend this to paginate users, it'd probably be best as a 2nd iteration, and we can try to get the command merged without it for now. It could be either with the list truncated, or omitted entirely. I've noted my preference and why, but happy to have more discussion here first.

There is a lot to consider with extending this to paginate users, or establishing a mechanism to query users in another way like adding a search argument to list users that contain a certain string.

For example, pagination with reactions requires permissions in the guild, and users are able to disable reactions on their end. If we added pages via arguments, it'd be very spammy to navigate through. Adding a search argument could make sense to check the status on certain users, that could be reasonable. 🤔

I'll write up an issue with the idea and possible implementations when this PR is concluded.

@Miepee
Copy link
Contributor

Miepee commented Oct 8, 2022

However, the consensus is that is they'd prefer to see pagination over truncating results.

Do wanna quickly mention that uploading a file is theoretically still an option, but that also requires another permission, and would fail in case the Matrix room has so many users that a textual representation exceeds 8MB. (From a quick test I did, that'd be ~8670000 characters). Also quick note from what you said above, is that file previews AFAIK do not show up on mobile yet.
Having a file for the end user would also make it easier to search for specific users and have the data "at once" instead of needing to paginate through everything.

And am still wondering, if your users really need all matrix users, why can't they just do a POST query at the HS?

@SethFalco
Copy link
Contributor Author

SethFalco commented Oct 8, 2022

if your users really need all matrix users

They don't need it, just noting a preference. I only participate in smaller communities, so I had to get out of my way to demonstrate the "edge-case". That's why I'm happy to discuss it further here since everyone has their own expectations of what's best.

why can't they just do a POST query at the HS

And I don't think doing a POST is the most user-friendly. ^-^'
Most of them probably don't know how, and it'd be pretty inconvenient anyway for the ones that do know.

The whole point of putting the command here is to offer ease-of-use for users, expecting them to check through the API defeats the point a bit imo.

Having a file for the end user would also make it easier to search for specific users and have the data

My reservations here is that I'm partial to normalizing downloading files from Discord bots. If we decide that that's the way to go, then by all means we can do that, I just didn't think it offered enough value to justify asking users to download files.

@SethFalco SethFalco force-pushed the add-listusers-command branch 3 times, most recently from bd15968 to 6d5e5f0 Compare October 9, 2022 15:55
@SethFalco SethFalco force-pushed the add-listusers-command branch 2 times, most recently from caed825 to 728ba8c Compare October 9, 2022 16:03
@SethFalco SethFalco force-pushed the add-listusers-command branch from 728ba8c to f2d51a7 Compare November 17, 2022 09:35
@SethFalco SethFalco force-pushed the add-listusers-command branch from f2d51a7 to 0baf443 Compare July 5, 2023 21:05
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.

Command on Discord to list users on Matrix
2 participants