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

MSC3309: Simple counters #3309

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

MSC3309: Simple counters #3309

wants to merge 1 commit into from

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Aug 3, 2021

Rendered

Specs the simple counter that is implemented in Element Web (behind a labs flag). Implementation PR: matrix-org/matrix-react-sdk#2388

Example:

50401767-f3dffc00-0788-11e9-98af-b8542ef839b4

@erikjohnston erikjohnston added proposal-in-review kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal labels Aug 3, 2021
@turt2live turt2live added the client-server Client-Server API label Aug 3, 2021
|------|----------|------|-------------|
| `title` | `false` | string | The text to render when displaying the counter. If empty the counter should be hidden entirely. |
| `link` | `false` | string | A link to direct the user to when the counter is clicked |
| `value` | `false` | integer | A numeric value associated with the counter. If omitted then no value should be rendered |
Copy link

Choose a reason for hiding this comment

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

Why limit this to integers? Why not allow arbitrary strings? For example you may have "latest version" in addition to "open bugs" or even "time remaining" where you may want to put "1d 3h". The client doesn't seem to do anything "number-like" with this anyways so it is technically a very easy change. I realize that this is no longer a "counter" but a "badge" or "pill" but it seems like these two use cases are very similar.

Choose a reason for hiding this comment

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

Not that my opinion matters here, but, yes, if you're going to add something like this, just let it be strings, so it could be version numbers, "Passing/Failing", etc. And yes, some kind of a generic "badge" would be more appropriate.

But I'd go further and suggest that this isn't generally useful enough to be in the spec. Clients are complex enough to implement already. These little counters/badges can sit in room topics already, and that's generally good enough.

Instead of having a separate `value` field it could be pulled into the `title`
field, simplifying the event format. Having the `value` separate allows the
value to be more easily tracked over time.

Copy link
Member

Choose a reason for hiding this comment

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

"Putting the counter in the room topic" has been suggested several times as an alternative, so I think it would be worthwhile to include a discussion about that here. From my perspective, there are problems with using the room topic:

  • the room topic currently doesn't support formatting, so we clients wouldn't be able to display different severities differently. This could be partially fixed by adding formatting to topics, but this would force the counters to be formatted in certain ways (e.g. a client on a black-and-white screen could not make a distinction between "warning" and "alert" if the topic uses the formatting suggested in this MSC)
  • it's nice to have things in machine-readable format, so that it can be used easily by other things
  • updating the counter would involve parsing the topic, which may be error-prone
  • updating the counter is not atomic, so there could be races e.g. counter 1 and counter 2 want to update at the same time, so they both read the topic simultaneously, then update their own values and set the topic, which means that only one counter gets updated. (For extra fun, add in a person who wants to update the actual topic.)

Choose a reason for hiding this comment

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

* the room topic currently doesn't support formatting, so we clients wouldn't be able to display different severities differently.  

It seems like having a formatted_topic in the spec would be a sensible counterpart to messages' formatted_body. It could include rich text, badge images, etc., and simpler clients could stick to the fallback, unformatted topic.

* it's nice to have things in machine-readable format, so that it can be used easily by other things

ISTM that Matrix already has this covered: custom event types can be used in rooms to communicate certain kinds of data according to the users' needs. That can be done without adding to the spec that all clients are expected to support (or if they don't, they are "incomplete" compared to Element).

* updating the counter would involve parsing the topic, which may be error-prone

Using custom events would avoid that problem, since they would be JSON, which is already parsable by any client. But it's not hard to parse a very simple format with regexps, e.g. using | as a separator between fields. That wouldn't require adding to the spec.

* updating the counter is not atomic, so there could be races  e.g. counter 1 and counter 2 want to update at the same time, so they both read the topic simultaneously, then update their own values and set the topic, which means that only one counter gets updated.  (For extra fun, add in a person who wants to update the actual topic.)

I would imagine a room having a bot dedicated to maintaining metadata in the topic, and users in the room could use bot commands to tell it to update parts of the topic.

ISTM that all of these issues have solutions already well within reach, without requiring additions to the spec that would require clients to add support for. This seems like a feature that would add a lot of complexity for little gain. And for every feature like this that gets added to the spec, the bar of full interoperability with Element, the flagship client, becomes that much higher for alternative clients to reach. IMO that's not good for Matrix as a whole, but what do I know... :)

Copy link
Member

Choose a reason for hiding this comment

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

It seems like having a formatted_topic in the spec would be a sensible counterpart to messages' formatted_body. It could include rich text, badge images, etc., and simpler clients could stick to the fallback, unformatted topic.

Yes, I already said that was a possible solution, but I already pointed out an issue with that. Another issue is that clients that want to allow users to edit the topic now need to allow them to edit HTML. Past suggestions of adding formatting to the topic have also been met with complaints about it adding complexity to clients.

ISTM that Matrix already has this covered: custom event types can be used in rooms to communicate certain kinds of data according to the users' needs. That can be done without adding to the spec that all clients are expected to support (or if they don't, they are "incomplete" compared to Element).

Yes, that's basically how this started. It was implemented as a custom event, and Element Web can display those custom events when that feature is enabled. So the question of this MSC is basically whether this should be standardized in the spec so that other clients can implement it and do the same thing, or if it should essentially remain an Element-only feature (or if other clients choose to implement it, they can do so too, but it remains undocumented).

To me, it seems like a bit of a niche feature, so it isn't something that I'd expect every client to implement.

Using custom events would avoid that problem, since they would be JSON, which is already parsable by any client. But it's not hard to parse a very simple format with regexps, e.g. using | as a separator between fields. That wouldn't require adding to the spec.

How does using custom events solve the problem of updating the topic?

Using a | as a separator between fields is brittle, as is any sort of parsing. You can't change the order of fields, and if the actual topic gains (or loses) a |, then things may break.

I would imagine a room having a bot dedicated to maintaining metadata in the topic, and users in the room could use bot commands to tell it to update parts of the topic.

So, users would have to use different methods to change the topic in different rooms? Also, having all counters being handled by a single bot may not always be possible. You may, for example, want to have different bots running on different machines (e.g. if they're monitoring some local statistics).

ISTM that all of these issues have solutions already well within reach ...

You call them "solutions", I call them "workarounds" or "hacks". Yes, it's possible to use the topic for this, but things harder, IMHO.


Another issue with using the topic is that it doesn't allow the client to reorder the counters. Say one person wants the counters sorted by severity (they want to make sure that the high-severity items are at the front), while another person wants counters to always be in the same place (because they can glance at the screen and know that red in a certain location means that some specific thing needs attention, without needing to read the actual counter). If the counters are stuffed into the topic, it isn't easy for clients to reorder them.


In summary, using topics instead of counters may be possible, but come with (IMHO) some disadvantages. Part of the MSC process is determining whether a feature is worth adding to the spec, and I think you've made it clear that your opinion is that it would create extra work for client authors that isn't worth the effort, and that you think that putting that information into the topic is a better way to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

FTR I think its a fair concern to say that we should be weary of adding too many random events to the spec that clients need to implement. I wasn't really envisaging this as something that all clients would necessarily implement, certainly not a first, but as an extension I think something along these lines will be very useful for quite a few use cases.

I'm not sure if we have a good way atm of speccing random event types like this that are optional?

@@ -0,0 +1,76 @@
# MSC3309: Room Counters
Copy link
Contributor

@MTRNord MTRNord Aug 6, 2021

Choose a reason for hiding this comment

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

My only concern with this and the existing element web implementation is the amount of spam this can cause.

This goes on for a few pages in that room. However I have no solution as this is I guess the nature of state in matrix.

(Also note element-hq/element-web#17214 as thats the directly related ele web issue on my concern)

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, we've been using this feature internally in the Synapse team for a while now (to count PRs pending review and such) and it's quite OK spam-wise imo

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW a new event should only be sent when the counter changes (servers should deduplicate setting state to the same value). This would also be the case if you put the counters in the topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW a new event should only be sent when the counter changes (servers should deduplicate setting state to the same value). This would also be the case if you put the counters in the topic.

well those are actually deduplicated state events and only changes. Thats realtime serverstats data happening over at https://matrix.to/#/#server_stats:nordgedanken.dev Used for the (non broken by redaction) "Rooms found" counter

Copy link
Contributor

Choose a reason for hiding this comment

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

Also just to clarify I batch changes already in ~5m intervals

@turt2live turt2live changed the title MSC3309 - Simple counters MSC3309: Simple counters Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants