MSC4482: Ability to bookmark messages#4482
Conversation
Init Bookmark MSC
…y-to-bookmark-messages.md
|
Signed-off-by: xeni xeni@koeln.ccc.de |
turt2live
left a comment
There was a problem hiding this comment.
This MSC appears to be a work in progress, so I haven't done the normal "implementation requirements" comment yet. When ready, feel free to request such a comment in the SCT Office.
|
I completed the proposal part. Tell me if I'm too verbose @Gernomaly |
|
Love the additions! I am going to elaborate on the alternatives parts later today. Including the comparison that travis suggested 👍 |
| ### New room type | ||
|
|
||
| This MSC introduces a new room type called `m.bookmarks`. | ||
| This kind of room SHOULD only hold some `m.bookmark` events defined below. |
There was a problem hiding this comment.
@Gernomaly I'm not sure anymore about this one.
As said in private, I want to adapt my Refs client so it supports this MSC. However, when I need to create a personal bookmark ex-nihilo (not from an existing Matrix event), I need to send my custom Ref event somewhere, and then reference it with our new m.bookmark "pointer event".
Currently I'm using my own custom room type to store my custom events, so using just this m.bookmarks instead would be nice.
So my proposition is that instead of SHOULD only we put MAY ? Does that sound good ?
There was a problem hiding this comment.
i think should is fine, its not a must and may is too soft imo
There was a problem hiding this comment.
id just stay with should only tbh, its not like the should only prevents you of not adding anything else imo, and from my point of view your events are a kind of bookmark as well :P . To me it its important to at least for the moment keep the scope together a bit and adapt it if needed in the future if the protocol develops into that direction (referencing msc guide)
There was a problem hiding this comment.
Ok I will put back the only then. You're right we should keep it scoped to the generic usecase for now
| } | ||
| ], | ||
| "m.pointer": { | ||
| "roomId": "<roomId>", |
There was a problem hiding this comment.
Since room IDs alone are not routable in Matrix, would you also need a via property possibly? I assume you'd normally already be in the room so maybe not? 🤔
There was a problem hiding this comment.
Our approach of pointers is kinda simple, we are ok if some pointers are just "dead". It should be up to the client to eventually clean up dead pointers (bookmarks pointing to rooms you're no longer in for instance).
Also, I guess if you don't clean a bookmark to a public room you're no longer part of, having the via parameter could help you joining this room back, so I have mixed opinions on this.
There was a problem hiding this comment.
Since room IDs alone are not routable in Matrix, would you also need a
viaproperty possibly? I assume you'd normally already be in the room so maybe not? 🤔
you should be in the room, or at the very least join via the server of the user who created the bookmark event. if their homeserver isn't participating in that room anymore, any via's you'd have set would also likely be useless.
if you really want to, we could use the most popular server by users that's in that room, hence having a decent chance of still being part of that room?
I don't particularly care either way, for my use case it's not necessary but I'm open to adding it.
| ## General | ||
|
|
||
| If the client support the `m.bookmarks` room type, it should'nt be displayed as a normal room in the regular room list. Instead the client could display bookmarks in dedicated sections of the client. | ||
| The client should ensure that only one `m.bookmarks` room is existing at once. |
There was a problem hiding this comment.
Could an account data event help to let clients store the room ID of the one bookmarks room?
There was a problem hiding this comment.
We imagined that but it would probably require extra work if we need to upgrade the m.bookmarks room at some point.
|
|
||
| ## Room creation | ||
|
|
||
| The room should be created upon first bookmark creation. |
There was a problem hiding this comment.
This feels like it could be an implementation detail. Would there be any issues with letting clients create the room prematurely?
There was a problem hiding this comment.
When we discussed it IRL I remember two arguments :
- waste of ressources
- zero click creation of a room that contains nothing yet, so it could be weird for a user that also uses a client that does not support
m.bookmarksrooms, and thus display the fallback room containing nothing.
We're aware that the first bookmark creation should be a little longer, but that is probably up to the client to make it smooth.
There was a problem hiding this comment.
@Johennes is that a sufficient answer to that question or do you still think we should change it?
| } | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Suggestion: Add the fact that each message of a room should be bookmarked at most once (maximum one m.bookmark event pointing to an event). This ensures that we have a binary state for each message: either bookmarked or not.
There was a problem hiding this comment.
agreed tho i dont really know how we should implement this
There was a problem hiding this comment.
I already started implementing it on the rust-sdk if you're curious. That's why I'm coming with all these questions and precisions.
Implementation is not that obvious tbh.
| "event_id": "$EVENT_ID", | ||
| "via": ["$SERVER_NAME"] | ||
|
|
||
| } |
There was a problem hiding this comment.
Suggestion: add a m.relates_to block on this event so we can handle editing the event with m.replace relations. This isn't very useful as is because editing the pointer or the fallback text isn't very useful, but since it is an extensible event, one may want to add extra blocks to this bookmark. We want to have a future proof event.
|
I'm sorry I didn't notice this sooner but you should have a look at MSC2437: Store tagged events in Room Account Data too:
|
This is a different approach. We describe why we think it is a better way from line 108, but here are the main points:
Let us know if you think this different approach is valuable enough ! @stefanceriu |
|
I mostly wanted to link the two together. We prototyped message bookmarks ourselves in one of our hackatons (based on bnjbvr/bookmarks) in which we decided to go against #2437 and store everything in the account data, as it made building the UI we wanted easier to build. Your solution on the other hand would take care of both paths but I do wonder if it is a specialisation of message forwarding i.e. something like #2723 in which |
|
I missed that (great) demo, thank you for the link. I was aware of this other MSC implementing things through account data, but I never saw it landing in Element X (I don't have access to the branch you mentioned), so my guess was that no effort was made in the rust sdk to implement this already. About the |
It might actually be an idea to think about as a protocol if we should resuse "m.pointer" for "m.forwarded" in the future |
rendered