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

MSC3765: Rich text in room topics #3765

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Apr 3, 2022

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

Signed-off-by: Johannes Marbach <[email protected]>
@Johennes Johennes changed the title MSC3677: Rich text in room topics MSC3765: Rich text in room topics Apr 3, 2022
@turt2live turt2live added proposal-in-review proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff labels Apr 3, 2022
@alphapapa

This comment was marked as duplicate.

@Johennes

This comment was marked as duplicate.

@emorrp1

This comment was marked as duplicate.

@@ -0,0 +1,96 @@
# MSC3765: Rich text in room topics
Copy link
Member

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.)

Copy link
Member

Choose a reason for hiding this comment

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

@Johennes replies:

Interesting idea. Pinned events seem to already exist. However, in their current form, these are not fit to be used for what you describe because, depending on room settings, users joining the room after the events were sent could be unable to see them.

@turt2live
Copy link
Member

@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.

Copy link
Member

@KitsuneRal KitsuneRal left a 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.

proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
@tulir tulir mentioned this pull request Oct 26, 2024
Co-authored-by: Johannes Marbach <[email protected]>
Co-authored-by: Johannes Marbach <[email protected]>
richvdh
richvdh previously requested changes Nov 5, 2024
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Nov 6, 2024

@mscbot concern binds behaviour of future unspecified versions

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Nov 6, 2024
Co-authored-by: Johannes Marbach <[email protected]>
@turt2live turt2live requested a review from richvdh December 12, 2024 21:11
proposals/3765-rich-room-topics.md Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
Comment on lines +28 to +33
"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>"
}]
Copy link
Member

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.

Copy link
Member

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)

Copy link
Member

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.

proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
proposals/3765-rich-room-topics.md Outdated Show resolved Hide resolved
Comment on lines +112 to +113
as `org.matrix.msc3765.topic`. Note that extensible events and content
blocks might have their own prefixing requirements.
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is stolen from MSC3381. It was originally intended to make m.text into org.matrix.msc1767.text. However, I suppose we don't need this anymore because MSC1767 has since been accepted?

Copy link
Member

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.

@richvdh
Copy link
Member

richvdh commented Dec 17, 2024

@mscbot resolve binds behaviour of future unspecified versions

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label Dec 17, 2024
@richvdh richvdh dismissed their stale review December 17, 2024 17:47

changes made

@ara4n
Copy link
Member

ara4n commented Jan 14, 2025

Not a blocking concern, but I'm a little worried that the m.topic wrapper on the content block is unnecessary and just makes things more verbose and clunky - see #3765 (comment) for context. Thoughts welcome on whether this should be a blocking concern or not. (i'm happy with everything else and have ticked anyway to avoid blocking)

@richvdh
Copy link
Member

richvdh commented Jan 21, 2025

@mscbot concern which order are the representations in?

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Jan 21, 2025
Comment on lines 44 to 45
The wrapping `m.topic` content block is similar to `m.caption` for file
uploads as defined in [MSC3551]. It avoids clients accidentally rendering
Copy link
Member

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)

Suggested change
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

Comment on lines +46 to +48
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
Copy link
Member

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:

Suggested change
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
Copy link
Member

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.

Suggested change
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

Copy link
Contributor Author

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. 😔

Comment on lines +50 to +52
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.
Copy link
Member

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?

Comment on lines +112 to +113
as `org.matrix.msc3765.topic`. Note that extensible events and content
blocks might have their own prefixing requirements.
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API disposition-merge kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.