-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC3765: Rich text in room topics #3765
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Johannes Marbach <[email protected]>
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
@@ -0,0 +1,96 @@ | |||
# MSC3765: Rich text in room topics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alphapapa says:
On one hand, I can see some elegance in repurposing room topics for general-purpose, long-term room reference information. OTOH, it seems like overloading the purpose of topics with what, in other systems, would go in "pinned" topics or messages, or a wiki, etc.
So IMHO I would consider implementing support for pinned messages/events before overloading topics like this. It would seem relatively straightforward for a room's state to have a list of pinned events, which could be sent in initial sync by the server or be retrieved manually by clients. Clients could then display these pinned events in a room's timeline view, optionally hiding them, compressing them, etc. And the pinned events could be edited by room moderators using existing event-editing tools. (Forgive me if there's already a proposal for something like that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alphapapa and others: please use threads on the diff to have your comments considered. This can be done by adding a line comment. If there's no obvious line for where to put a comment, please use the line containing the title. |
@mscbot fcp merge |
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocking concern but it would be great to polish the wording a little.
Co-authored-by: Johannes Marbach <[email protected]>
Co-authored-by: Johannes Marbach <[email protected]>
@mscbot concern binds behaviour of future unspecified versions |
Co-authored-by: Johannes Marbach <[email protected]>
Details of how `m.text` works may be found in [MSC1767] and are not | ||
repeated here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't really expected m.room.topic
events to be the way that the m.text
stuff makes it into the spec, but I guess that's ok.
uploads as defined in [MSC3551]. It avoids clients accidentally rendering | ||
the topic state event as a room message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It avoids clients accidentally rendering the topic state event as a room message.
Is this actually necessary?
It seems to me that the reason for m.caption
in MSC3551 is quite different to the reason for it here: in that case, m.text
at the top level has a very different meaning to m.text
within m.caption
, whereas here there is no ambiguity.
This might be a good thing to do anyway, for consistency for example, but I'm unconvinced this paragraph captures a good reason for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra wrapping block was based off of @turt2live's comment from an earlier review. I don't feel strongly either way but am curious what Travis thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tripped over this too when reviewing it just now. My initial reaction was the same as richvdh's: state events don't get rendered as fallbacks, plus clients know to special-case topics already - plus the contents of the topic is semantically a top-level 'm.text' (unlike m.caption, which is effectively describing a nested event). If topics actually required some more custom topic-specific datatype (e.g. show_at_all_times: true
or something) then perhaps one might wrap it... but in practice, I can't think of any plausible customisation which couldn't/shouldn't be pulled in as another mix-in.
The only argument I can see for it is consistency with m.caption and other places where you might want to explicitly say "don't fall back to displaying m.text if you don't recognise the event type. even if it's a state event". Or possibly futureproofing for additional topic-specific keys we just haven't thought of yet.
If it were me, I'd probably go ahead without the m.topic
wrapper and just have it as m.text
. But my strength of feeling here is about 6/10.
"m.text": [{ | ||
"body": "All about **pizza** | [Recipes](https://recipes.pizza.net)" | ||
}, { | ||
"mimetype": "text/html", | ||
"body": "All about <b>pizza</b> | <a href=\"https://recipes.pizza.net\">Recipes</a>" | ||
}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these the wrong way round? According to MSC1767, the first supported representation should be used, so nobody will ever use the html representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(clients can also optimize and pick a preferred represenation, but indeed the preference should be to order them. I remain unconvinced that MSC1767 picked the right order here though, because everyone (including me) keeps putting plaintext first)
On the server side, any logic that currently operates on the `topic` field is | ||
updated to use the `m.topic` content block instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right in understanding that the next four paragraphs are examples of this change? If so, it would be good to make that a little clearer:
On the server side, any logic that currently operates on the `topic` field is | |
updated to use the `m.topic` content block instead. | |
On the server side, any logic that currently operates on the `topic` field is | |
updated to use the `m.topic` content block instead. For example: |
... and make the following paragraphs bullet points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually meant to be a conclusive list. I hope I didn't miss anything. Maybe ending with a colon (without "For example") and making the next paragraphs bullets would be clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have captured it into https://github.com/matrix-org/matrix-spec-proposals/pull/3765/files#r1890019768 which I'll need you to push the button on once more. Sorry for the hassle.
as `org.matrix.msc3765.topic`. Note that extensible events and content | ||
blocks might have their own prefixing requirements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that extensible events and content blocks might have their own prefixing requirements.
I don't really know what this means, as it pertains to this MSC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscbot resolve binds behaviour of future unspecified versions |
On the server side, any logic that currently operates on the `topic` field is | ||
updated to use the `m.topic` content block instead. | ||
|
||
In [`/_matrix/client/v3/createRoom`], the `topic` parameter should cause `m.room.topic` | ||
to be written with a `text/plain` mimetype in `m.topic`. If at the same time an | ||
`m.room.topic` event is supplied in `initial_state`, it is overwritten entirely. | ||
A future MSC may generalize the `topic` parameter to allow specifying other mime | ||
types without `initial_state`. | ||
|
||
In [`GET /_matrix/client/v3/publicRooms`], [`GET /_matrix/federation/v1/publicRooms`] | ||
and their `POST` siblings, the `topic` response field should be read from the | ||
`text/plain` mimetype of `m.topic` if it exists or omitted otherwise. | ||
A plain text topic is sufficient here because this data is commonly | ||
only displayed to users that are *not* a member of the room yet. These | ||
users don't commonly have the same need for rich room topics as users | ||
who already reside in the room. A future MSC may update these endpoints | ||
to support rich text topics. | ||
|
||
The same logic is applied to [`/_matrix/client/v1/rooms/{roomId}/hierarchy`] | ||
and [`/_matrix/federation/v1/hierarchy/{roomId}`]. | ||
|
||
In [server side search], the `room_events` category is expanded to search | ||
over the `m.text` content block of `m.room.topic` events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the server side, any logic that currently operates on the `topic` field is | |
updated to use the `m.topic` content block instead. | |
In [`/_matrix/client/v3/createRoom`], the `topic` parameter should cause `m.room.topic` | |
to be written with a `text/plain` mimetype in `m.topic`. If at the same time an | |
`m.room.topic` event is supplied in `initial_state`, it is overwritten entirely. | |
A future MSC may generalize the `topic` parameter to allow specifying other mime | |
types without `initial_state`. | |
In [`GET /_matrix/client/v3/publicRooms`], [`GET /_matrix/federation/v1/publicRooms`] | |
and their `POST` siblings, the `topic` response field should be read from the | |
`text/plain` mimetype of `m.topic` if it exists or omitted otherwise. | |
A plain text topic is sufficient here because this data is commonly | |
only displayed to users that are *not* a member of the room yet. These | |
users don't commonly have the same need for rich room topics as users | |
who already reside in the room. A future MSC may update these endpoints | |
to support rich text topics. | |
The same logic is applied to [`/_matrix/client/v1/rooms/{roomId}/hierarchy`] | |
and [`/_matrix/federation/v1/hierarchy/{roomId}`]. | |
In [server side search], the `room_events` category is expanded to search | |
over the `m.text` content block of `m.room.topic` events. | |
On the server side, any logic that currently operates on the `topic` field is | |
updated to use the `m.topic` content block instead: | |
- In [`/_matrix/client/v3/createRoom`], the `topic` parameter should cause `m.room.topic` | |
to be written with a `text/plain` mimetype in `m.topic`. If at the same time an | |
`m.room.topic` event is supplied in `initial_state`, it is overwritten entirely. | |
A future MSC may generalize the `topic` parameter to allow specifying other mime | |
types without `initial_state`. | |
- In [`GET /_matrix/client/v3/publicRooms`], [`GET /_matrix/federation/v1/publicRooms`] | |
and their `POST` siblings, the `topic` response field should be read from the | |
`text/plain` mimetype of `m.topic` if it exists or omitted otherwise. | |
A plain text topic is sufficient here because this data is commonly | |
only displayed to users that are *not* a member of the room yet. These | |
users don't commonly have the same need for rich room topics as users | |
who already reside in the room. A future MSC may update these endpoints | |
to support rich text topics. | |
- The same logic is applied to [`/_matrix/client/v1/rooms/{roomId}/hierarchy`] | |
and [`/_matrix/federation/v1/hierarchy/{roomId}`]. | |
- In [server side search], the `room_events` category is expanded to search | |
over the `m.text` content block of `m.room.topic` events. |
Not a blocking concern, but I'm a little worried that the |
Rendered
Implementations:
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am a Systems Architect at gematik, Software Engineer at Unomed, Matrix community member and former Element employee. This proposal was written and published with my community member hat on.
FCP tickyboxes