-
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. |
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]>
"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)
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've raised a concern about getting this addressed.
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.
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 see. Yes, I don't think we need this.
@mscbot resolve binds behaviour of future unspecified versions |
Not a blocking concern, but I'm a little worried that the |
Co-authored-by: Johannes Marbach <[email protected]>
@mscbot concern which order are the representations in? |
The wrapping `m.topic` content block is similar to `m.caption` for file | ||
uploads as defined in [MSC3551]. It avoids clients accidentally rendering |
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.
Non-blocking comment:
Honestly I think the reference to MSC3551 is confusing rather than helpful: I still see m.topic
as quite different from m.caption
, as the whole purpose of m.room.topic
is to set the topic, whereas m.caption
is a small part of the functionality.
(Also, MSC3551 is way off landing, and basing this MSC on another WIP MSC isn't helpful for clarity)
The wrapping `m.topic` content block is similar to `m.caption` for file | |
uploads as defined in [MSC3551]. It avoids clients accidentally rendering | |
The wrapping `m.topic` content block avoids clients accidentally rendering |
the topic as a room message. Note that while [MSC1767] had explicitly | ||
excluded state events from being treated as extensible, this is being | ||
changed with [MSC4252]. The extra content block, therefore, allows putting |
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.
Non-blocking comment:
I think we could do with clarifying why the m.topic
block avoids rendering as a room message: this was the main thing I was missing when I opened the other comment thread:
the topic as a room message. Note that while [MSC1767] had explicitly | |
excluded state events from being treated as extensible, this is being | |
changed with [MSC4252]. The extra content block, therefore, allows putting | |
the topic as a room message. ([MSC1767] specifies that unknown events with | |
an `m.text` content block should be rendered as a regular room message, and | |
while [MSC1767] had explicitly excluded state events from being treated as | |
extensible, this is being changed with [MSC4252].) The extra content block, therefore, allows putting |
excluded state events from being treated as extensible, this is being | ||
changed with [MSC4252]. The extra content block, therefore, allows putting | ||
a fallback representation that is actually designated for the timeline | ||
into a separate `content['m.text']` field. Lastly, the `m.topic` content |
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.
Non-blocking comment:
AFAICT, there are only two arguments here, so "Lastly" reads strangely.
into a separate `content['m.text']` field. Lastly, the `m.topic` content | |
into a separate `content['m.text']` field. In addition, the `m.topic` content |
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 counted avoiding rendering as a fallback and consciously supplying a fallback in a separate field as two. But "In addition" works fine either way.
I'm also fine with your other two suggestions above but will need somebody else to apply them again. I'm sorry for the mess this creates but I don't know what else to do about it. 😔
into a separate `content['m.text']` field. Lastly, the `m.topic` content | ||
block also serves as a good place for additional fields to be added by | ||
other MSCs in the future. |
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.
Non-blocking comment:
I don't entirely follow why we need an m.topic
block as a place to put extra fields (unless we think that we're going to end up with topics inside other event types?) but maybe that's my lack of familiarity with extensible events.
@ara4n as the person that proposed this (I think?) can you clarify?
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.
I see. Yes, I don't think we need this.
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