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

MSC1943: Set default room as v3 #1943

Closed
wants to merge 10 commits into from
Closed

Conversation

neilisfragile
Copy link
Contributor

@neilisfragile neilisfragile commented Mar 27, 2019

@turt2live turt2live added proposal-in-review proposal A matrix spec change proposal labels Mar 27, 2019

The default room should be updated to be room v3 from v1.

Until we have greater confidence in the room upgrade UX v1 and v2 will continue
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) Given that v2 is probably not widely used publicly, and would have been created by people who hopefully should know what they're doing, I'd be tempted to mark it as unstable right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

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 corrections

# Set v3 to be the default room version

Homeservers support multiple room versions, though one version is the default.
All new rooms created by a server are of the default version.
Copy link
Member

Choose a reason for hiding this comment

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

the default is a recommendation; we can't change the behaviour of old servers.

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 don't see a contradiction here, whatever the default the server has, the description is accurate. I've made a change further down to clarify that the default is only a recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

technically speaking /createRoom allows for specifying a room version which is not of the default, making the statement here untrue. However, I don't think this is the thing we need to get stuck on.

Copy link
Member

Choose a reason for hiding this comment

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

ok I think I misread this. All new rooms are inherently created by a server, so I just read it as "All new rooms are of the default version."

I think what you mean is "By default, new rooms created by a server are of the default version for that server", which is somewhat tautologous. I guess I'm just not sure this sentence adds clarity rather than confusion. Perhaps:

Homeservers support multiple room versions. Each server has a default version which is used when creating new rooms.

proposals/1943-v3-default-room-version.md Outdated Show resolved Hide resolved
proposals/1943-v3-default-room-version.md Outdated Show resolved Hide resolved
proposals/1943-v3-default-room-version.md Outdated Show resolved Hide resolved
proposals/1943-v3-default-room-version.md Outdated Show resolved Hide resolved
proposals/1943-v3-default-room-version.md Outdated Show resolved Hide resolved
proposals/1943-v3-default-room-version.md Outdated Show resolved Hide resolved
@turt2live turt2live self-requested a review March 27, 2019 15:08
richvdh and others added 6 commits March 27, 2019 15:22
Co-Authored-By: neilisfragile <neil@matrix.org>
Co-Authored-By: neilisfragile <neil@matrix.org>
Co-Authored-By: neilisfragile <neil@matrix.org>
Co-Authored-By: neilisfragile <neil@matrix.org>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

While servers are ready, I would like to see how clients hold up to v3 before going forward with the proposal. Riot does a good enough job (in future releases, not in 1.1.0rc1) that I'm okay with marking it as safe, however there's potentially problems with Nheko-reborn, Quaternion, etc which make this concerning to land.

As mentioned elsewhere, landing MSC1884 with appropriate adoption from servers would also be sufficient.

Edit: also as mentioned elsewhere, client authors are encouraged to comment on this MSC

proposals/1943-v3-default-room-version.md Show resolved Hide resolved
proposals/1943-v3-default-room-version.md Show resolved Hide resolved
@richvdh richvdh self-requested a review March 28, 2019 18:51
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

# Set v3 to be the default room version

Homeservers support multiple room versions, though one version is the default.
All new rooms created by a server are of the default version.
Copy link
Member

Choose a reason for hiding this comment

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

ok I think I misread this. All new rooms are inherently created by a server, so I just read it as "All new rooms are of the default version."

I think what you mean is "By default, new rooms created by a server are of the default version for that server", which is somewhat tautologous. I guess I'm just not sure this sentence adds clarity rather than confusion. Perhaps:

Homeservers support multiple room versions. Each server has a default version which is used when creating new rooms.

@@ -0,0 +1,58 @@
# Set v3 to be the default room version
Copy link
Member

Choose a reason for hiding this comment

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

If #1884 merges, then we should probably be setting default room to v4 instead.

Copy link
Member

Choose a reason for hiding this comment

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

unconvinced: it will take a while for v4 (whatever it ends up being) to roll out across the federation, whereas v3 has already been out for several weeks.

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've changed my mind here fwiw. If we go to the trouble of implementing #1884 then I can't see sense in shipping v3 as default. Though agree with @richvdh comment that it pushes back the date that we can change default from v1.

@turt2live turt2live added the c2s r0.5.0 Part of the r0.5.0 goal (and related releases) label Apr 1, 2019
@ara4n ara4n changed the title set default room as v3 MSC1943: Set default room as v3 Apr 1, 2019
@turt2live turt2live dismissed their stale review April 5, 2019 21:04

dismissing review: Client authors haven't shown up in hoards to complain about this, so I am assuming it is probably fine

@turt2live
Copy link
Member

I think it's safe to get this going, even though v4 and friends are on the horizon. In the interest of getting the federation to a more stable place, I propose we merge this now.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Apr 5, 2019

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Apr 5, 2019
@ara4n
Copy link
Member

ara4n commented May 20, 2019

@mscbot fcp cancel

as per #2002 we’re skipping to v4.

@mscbot
Copy link
Collaborator

mscbot commented May 20, 2019

@ara4n proposal cancelled.

@mscbot mscbot removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels May 20, 2019
@ara4n
Copy link
Member

ara4n commented May 20, 2019

shall we just mark this as obsolete and close?

@turt2live
Copy link
Member

Considering we're fast-tracking #2002, yes. Closing as obsolete.

@turt2live turt2live closed this May 20, 2019
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review labels May 20, 2019
@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c2s r0.5.0 Part of the r0.5.0 goal (and related releases) kind:maintenance MSC which clarifies/updates existing spec obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants