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

Add Spaces to the spec #3610

Merged
merged 7 commits into from
Jan 17, 2022
Merged

Add Spaces to the spec #3610

merged 7 commits into from
Jan 17, 2022

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Dec 31, 2021

Requires #3606 (already landed)

Please see pull/3606/head...pull/3610/head for the true diff. Because of squash merges and how this branch was built it's not easy/recommended to remove 3606 from this PR for now - it should still merge cleanly.

MSCs:

Note that this makes modifications to the underlying MSCs as well. These are intended to be minor edits to aid clarity/accuracy of the MSCs, as per the proposal process. Functionally, clients and servers might need to change their behaviour slightly as is expected of implementing this stuff early. Synapse has these changes (alongside backwards compatibility) here: matrix-org/synapse#11667

Preview: https://pr3610--matrix-org-previews.netlify.app

@turt2live turt2live changed the title Travis/spec/spaces 1 1772 3288 2946 Add Spaces to the spec Dec 31, 2021
@turt2live turt2live force-pushed the travis/spec/spaces-1-1772-3288-2946 branch 2 times, most recently from d5003cc to e68dc2f Compare December 31, 2021 00:20
@turt2live turt2live marked this pull request as ready for review December 31, 2021 00:22
@turt2live turt2live requested a review from a team December 31, 2021 00:23
data/event-schemas/schema/m.space.child.yaml Outdated Show resolved Hide resolved
content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
Comment on lines +18 to +23
aliases:
type: array
description: Aliases of the room. May be empty.
items:
type: string
example: ["#general:example.org"]
Copy link
Member

Choose a reason for hiding this comment

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

This is being discussed on matrix-org/synapse#11667 (comment), but to cross-link. It seems that Synapse no longer returns aliases for /publicRooms, although MSC2432 never specified that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in #3624 because it's a bit more clear that way.

data/api/client-server/space_hierarchy.yaml Show resolved Hide resolved
data/api/server-server/space_hierarchy.yaml Outdated Show resolved Hide resolved
data/api/server-server/space_hierarchy.yaml Outdated Show resolved Hide resolved
proposals/2946-spaces-summary.md Outdated Show resolved Hide resolved
turt2live added a commit that referenced this pull request Jan 5, 2022
Through attempting to land [Synapse#11667](matrix-org/synapse#11667) it was found that Synapse doesn't return the `aliases` property on `/publicRooms` as it was [removed](matrix-org/synapse#6970) by a [tracking issue](matrix-org/synapse#6898) referencing [MSC2432](#2432) as its base. Though MSC2432 does not make mention of this, the [document](https://docs.google.com/document/d/1NNDkobiFLeUkJtyj0H6qvKIedgvIkZnFKo78-03cGEk/edit) the MSC is based upon makes deliberate effort to mention the endpoint and the removal of `aliases`. Thus, it is determined as a likely accidental omission from the formal MSC and therefore the formal spec.

This has been corrected here by amending the MSC (per the process) and removing the field, basing itself off of the [spec PR for spaces](#3610) for diff clarity.
@turt2live turt2live force-pushed the travis/spec/spaces-1-1772-3288-2946 branch from 51bbc2e to e68dc2f Compare January 5, 2022 20:02
turt2live added a commit that referenced this pull request Jan 5, 2022
Through attempting to land [Synapse#11667](matrix-org/synapse#11667) it was found that Synapse doesn't return the `aliases` property on `/publicRooms` as it was [removed](matrix-org/synapse#6970) by a [tracking issue](matrix-org/synapse#6898) referencing [MSC2432](#2432) as its base. Though MSC2432 does not make mention of this, the [document](https://docs.google.com/document/d/1NNDkobiFLeUkJtyj0H6qvKIedgvIkZnFKo78-03cGEk/edit) the MSC is based upon makes deliberate effort to mention the endpoint and the removal of `aliases`. Thus, it is determined as a likely accidental omission from the formal MSC and therefore the formal spec.

This has been corrected here by amending the MSC (per the process) and removing the field, basing itself off of the [spec PR for spaces](#3610) for diff clarity.
@turt2live
Copy link
Member Author

Apologies for the force push - the merge from main->here didn't fix the diff like how I thought it would because of our squash merging (was hoping it'd cancel out the stripped state commit and become a clean PR, but that only works with merge commits rather than squashes)

MSCs:
* #3288
* #2946
* #1772

Note that this makes modifications to the underlying MSCs as well. These are intended to be minor edits to aid clarity/accuracy of the MSCs, as per the proposal process. Functionally, clients and servers might need to change their behaviour slightly as is expected of implementing this stuff early. Synapse has these changes (alongside backwards compatibility) here: matrix-org/synapse#11667
@richvdh richvdh force-pushed the travis/spec/spaces-1-1772-3288-2946 branch from 6616a5e to 493f730 Compare January 11, 2022 19:30
@richvdh
Copy link
Member

richvdh commented Jan 11, 2022

I've rebased this, because it was hard to comment on otherwise. Apologies for any review comments it has outdated.

richvdh pushed a commit that referenced this pull request Jan 11, 2022
Through attempting to land [Synapse#11667](matrix-org/synapse#11667) it was found that Synapse doesn't return the `aliases` property on `/publicRooms` as it was [removed](matrix-org/synapse#6970) by a [tracking issue](matrix-org/synapse#6898) referencing [MSC2432](#2432) as its base. Though MSC2432 does not make mention of this, the [document](https://docs.google.com/document/d/1NNDkobiFLeUkJtyj0H6qvKIedgvIkZnFKo78-03cGEk/edit) the MSC is based upon makes deliberate effort to mention the endpoint and the removal of `aliases`. Thus, it is determined as a likely accidental omission from the formal MSC and therefore the formal spec.

This has been corrected here by amending the MSC (per the process) and removing the field, basing itself off of the [spec PR for spaces](#3610) for diff clarity.
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

a few minor things, but lgtm otherwise!

changelogs/server_server/newsfragments/3610.new Outdated Show resolved Hide resolved
data/event-schemas/schema/m.room.create.yaml Show resolved Hide resolved
content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
Comment on lines +60 to +61
When using this approach, the state events get sent into the space-room which is the
parent to the room. The `state_key` for the event is the child room's ID.
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be clearer/more intuitive to say a state event is set on than sent into ?

Suggested change
When using this approach, the state events get sent into the space-room which is the
parent to the room. The `state_key` for the event is the child room's ID.
When using this approach, `m.state.child` state events are set on the space-room which is the
parent to the room. The `state_key` for the event is the child room's ID.

ymmv, just a suggestion. Probably needs updates elsewhere for consistency if you want to adopt this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't really talk about setting things "onto" a room in the spec, so am hesitant to introduce the concept here.

content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
content/client-server-api/modules/spaces.md Show resolved Hide resolved
content/client-server-api/modules/spaces.md Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

@turt2live turt2live merged commit 9af83df into main Jan 17, 2022
@turt2live turt2live deleted the travis/spec/spaces-1-1772-3288-2946 branch January 17, 2022 17:06
turt2live added a commit that referenced this pull request Jan 17, 2022
Through attempting to land [Synapse#11667](matrix-org/synapse#11667) it was found that Synapse doesn't return the `aliases` property on `/publicRooms` as it was [removed](matrix-org/synapse#6970) by a [tracking issue](matrix-org/synapse#6898) referencing [MSC2432](#2432) as its base. Though MSC2432 does not make mention of this, the [document](https://docs.google.com/document/d/1NNDkobiFLeUkJtyj0H6qvKIedgvIkZnFKo78-03cGEk/edit) the MSC is based upon makes deliberate effort to mention the endpoint and the removal of `aliases`. Thus, it is determined as a likely accidental omission from the formal MSC and therefore the formal spec.

This has been corrected here by amending the MSC (per the process) and removing the field, basing itself off of the [spec PR for spaces](#3610) for diff clarity.
turt2live added a commit that referenced this pull request Jan 18, 2022
Through attempting to land [Synapse#11667](matrix-org/synapse#11667) it was found that Synapse doesn't return the `aliases` property on `/publicRooms` as it was [removed](matrix-org/synapse#6970) by a [tracking issue](matrix-org/synapse#6898) referencing [MSC2432](#2432) as its base. Though MSC2432 does not make mention of this, the [document](https://docs.google.com/document/d/1NNDkobiFLeUkJtyj0H6qvKIedgvIkZnFKo78-03cGEk/edit) the MSC is based upon makes deliberate effort to mention the endpoint and the removal of `aliases`. Thus, it is determined as a likely accidental omission from the formal MSC and therefore the formal spec.

This has been corrected here by amending the MSC (per the process) and removing the field, basing itself off of the [spec PR for spaces](#3610) for diff clarity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants