Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3266: Room summary API #3266
base: old_master
Are you sure you want to change the base?
MSC3266: Room summary API #3266
Changes from 27 commits
642f4e1
188b6e5
dc5b372
975ece5
d148acf
6776863
df376a3
43eecf0
66fee23
469b77b
04f807b
f1233c4
5fc2f5b
9e41b45
cab37e5
a93190f
8186b72
1a8ecff
82d8f3b
208a58c
33f3733
a5bc9ef
ac3d5da
dba6705
9719119
2ad832c
81fd904
57213f0
ed79007
284c181
e0d3a8b
938cbc0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why can't this be done today using the
/hierarchy
endpoint?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.
That is explained in the paragraph after this list. Some of these things can be achieved using the
/hierarchy
API. But this specific case for example works well if you want to show the whole space. It doesn't work if you want to show a single room in a space. The hierarchy API requires you to load possibly the whole hierarchy to show a specific room in the space. This falls under "heavier than necessary".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.
But if you're showing the space don't you already have the info to show the room in the room list? I just be misunderstanding something.
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.
What exactly does publicly accessible mean here?
join_rule: public
, presumably?(obligatory mention of matrix-org/matrix-spec#633)
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 means if the content can be accessed by anyone. This can be either
join_rule
ofpublic
, history visibility ofworld_readable
and also rooms you can knock on: matrix-org/matrix-spec#1184This might change in the future, but as you can currently preview such rooms via the
/hierarchy
API, this MSC doesn't change it. Whatever the conclusion ends up being for the hierarchy API long term should also apply to this MSC.An argument could be made, that world_readable should be required for unauthenticated users, but that doesn't match what is currently implemented. Another alternative could be to have a separate setting for if the room info is publicly readable instead of only having a toggle for messages and the room info. But in general this room info is already readable without joining the room, so it isn't clear why authentication would be required.
The wording of this MSC was changed multiple times already during the discussion on the issue I linked and in the end I reverted it to "publicly accessible" as I really don't want to split the discussion. I would prefer it someone wrote an MSC to clarify matrix-org/matrix-spec#1184, matrix-org/matrix-spec#633, etc, instead of me having to do that on the side on this MSC. I was also told to have the MSC match the current implementation and not do additional changes to 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.
To be explicit, I am concerned about this sentence:
I'm not sure it's true that this wording has changed multiple times, and I don't see anywhere it has been discussed. But anyway, it seems like you have a clear idea of what is meant; I'm just asking that we make it explicit.
It probably also needs more paragraph breaks, for the bullet list to make sense.
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.
This MSC doesn't use stripped state -- it uses a bunch of information derived from the room state.
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.
That sounds like a technicality to me? This MSC uses information, that is available event for rooms where only the events in the stripped state you linked to is available. Yes, this information is also available in the room state. But the stripped state of a room is also derived from the room state. The big difference between normal room state and the stripped state is:
This MSC specifically derives from this sentence of the spec:
The sentence you commented on points out that relationship: These previews also work for unjoined rooms, where the full room state might be unavailable.
So could you clarify what your comment is supposed to mean? Am I supposed to change something in the text? Is it a question? Or is it just about the technicality, that this MSC doesn't directly return stripped state events, even though it uses the definition of stripped state events to define what information is available and what rooms should be previewable, without anything for me to act on? I'm a bit at a loss here, how I am supposed to resolve or reply to comments like these.
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 found this sentence to imply that the stripped state was returned, which it is not. (Maybe it is a technicality, but this is a technical document -- details are important.) perhaps saying the information is derived from the stripped state would be enough. (I'm not even really sure the stripped aspect of it is needed 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.
Maybe that's clearer?