Conversation
Signed-off-by: Tulir Asokan <tulir@beeper.com>
tulir
added
proposal
This comment was marked as resolved.
This comment was marked as resolved.
tulir
removed
the
needs-implementation
| ### New state events | ||
| Per-message profiles could be transmitted more compactly by defining the profile | ||
| in a new state event and only referencing the state key in the message event. | ||
| However, that approach wouldn't enable encrypting per-message profiles without |
There was a problem hiding this comment.
Per-room profile data (in m.room.member) is not currently encrypted. Is there a reason that per-message profile data is more sensitive than per-room profile data, so that using state events for per-message profiles should be rejected while using them for per-room profiles is okay?
There was a problem hiding this comment.
As covered in the use cases section, bridges and other things that want to protect profile metadata can opt to use only per-message profiles and not store any of that in room state.
|
|
||
| ## Alternatives | ||
| ### New state events | ||
| Per-message profiles could be transmitted more compactly by defining the profile |
There was a problem hiding this comment.
This design has a second, more significant advantage, beyond just efficiency. It allows retroactively updating the profile information in old messages sent with a given profile id in a straightforward way. With the proposed design where profile fields are embedded in the messages, changing profile information in old messages requires sending an update to every message, generating a significant number of m.replacement events. This also requires the client searching through the full backscroll for the room, which may not even always be possible.
There was a problem hiding this comment.
I've expanded the description of that alternative in d4161fd (this MSC won't be pursuing that option as the benefits of just having it in the message still outweigh the negatives)
| tag with a `data-mx-profile-fallback` attribute, or replace matches of the | ||
| following regex with an empty string: `<strong\s+data-mx-profile-fallback(?:="")?\s*>([^<]+): </strong\s*>` | ||
|
|
||
| #### Other details |
There was a problem hiding this comment.
My understanding is that, with the current spec text, m.replacement events are able to edit per-message profile fields, and clients should display the post-edit profile information in the UI. This is a bit non-obvious, and at least one current client implementation (sable) does not implement this behavior, so I think it's worth calling out explicitly if it is intended.
(if it is not intended, then there is a gap in this design for changing the profile information associated with existing messages)
There was a problem hiding this comment.
Probably makes sense to allow edits as there's no particular reason to disallow them, have specified that explicitly in a399fda
This comment was marked as resolved.
This comment was marked as resolved.
|
|
||
| To remove the HTML fallback, either use a HTML parser to drop the entire `strong` | ||
| tag with a `data-mx-profile-fallback` attribute, or replace matches of the | ||
| following regex with an empty string: `<strong\s+data-mx-profile-fallback(?:="")?\s*>([^<]+): </strong\s*>` |
There was a problem hiding this comment.
@Stealthii said outside of a thread:
To address one of the main criticisms I have with this MSC: the suggestion of matching specifically this tag:
<strong\s+data-mx-profile-fallback(?:="")?\s*>([^<]+): </strong\s*>If we are recommending regular expressions, I would suggest the following, which is safer, handling multi-line input, other tags, matching the same closing tag, etc.
input = input.replace(/<(\w+)[^>]*\bdata-mx-profile-fallback\b[^>]*>[\s\S]*?<\/\1>/g, "")This however is not 100% safe, no regular expression is. Personally I would like to argue for non-fallback content to be explicitly stated in that MSC in a separate property field, because regex should be used for matching, not content manipulation in matrix client or bridge implementations.
Similarly, suggesting trimming the plaintext component by "PMP name length + 2" also assumes that clients implement the spec accurately as it stands today, and do not deviate. This also cannot be guaranteed.
There was a problem hiding this comment.
Personally I would like to argue for non-fallback content to be explicitly stated in that MSC in a separate property field
Duplicating the entire content is a really bad idea. Each fallback doubles the content, so if there are 3 different fallbacks (edits, formatting, per-message profiles), that'd result in 2*2*2 = 8 duplicates of the content. It might be "only" 6 duplicates if the non-fallback per-message profile is only inside m.new_content, but if anyone decided to invent a new thing needing a fallback copy, it'd jump to 12 or more
which is safer, handling multi-line input, other tags, matching the same closing tag, etc.
The regex is intentionally fairly strict, other tags are not meant to be allowed. It could maybe slightly be relaxed in terms of the trailing space inside the fallback to allow \n there (<br> is obviously not allowed), perhaps :\s* instead of :
Alternatively, I guess the fallback itself could be put in another field, such that clients can simply do formatted_body.replace(fallback, "") rather than using a regex 🤔
Similarly, suggesting trimming the plaintext component by "PMP name length + 2" also assumes that clients implement the spec accurately as it stands today, and do not deviate. This also cannot be guaranteed.
The only consequence of deviating from the spec is that the message will render incorrectly. If clients don't implement the spec correctly and their messages render differently than they intended, that's their problem, not the spec's.
11 participants
Rendered
Implementations:
Signed-off-by: Tulir Asokan tulir@beeper.com