-
-
Notifications
You must be signed in to change notification settings - Fork 648
Support for creator power level #4937
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
Conversation
Adds support for infinite power level specified by [MSC4289](matrix-org/matrix-spec-proposals#4289).
as room versions strings aren't ordered
|
@dbkr did you mean to request review? it is still in draft |
Stop using powerLevelNorm and reading PL events manually. To support matrix-org/matrix-js-sdk#4937
|
well, I was still doing the element web PR but figured it could go through review at least |
* Update for compatibility with v12 rooms Stop using powerLevelNorm and reading PL events manually. To support matrix-org/matrix-js-sdk#4937 * Add test for leave space dialog * Don't compute stuff if we don't need it * Use room.client * Use getSafeUserId * Remove client arg * Use getJoinedMembers and add doc * Fix tests * Fix more tests * Fix other test * Clarify comment Co-authored-by: Michael Telatynski <[email protected]> --------- Co-authored-by: Michael Telatynski <[email protected]>
* Update for compatibility with v12 rooms Stop using powerLevelNorm and reading PL events manually. To support matrix-org/matrix-js-sdk#4937 * Add test for leave space dialog * Don't compute stuff if we don't need it * Use room.client * Use getSafeUserId * Remove client arg * Use getJoinedMembers and add doc * Fix tests * Fix more tests * Fix other test * Clarify comment Co-authored-by: Michael Telatynski <[email protected]> --------- Co-authored-by: Michael Telatynski <[email protected]>
* Update for compatibility with v12 rooms Stop using powerLevelNorm and reading PL events manually. To support matrix-org/matrix-js-sdk#4937 * Add test for leave space dialog * Don't compute stuff if we don't need it * Use room.client * Use getSafeUserId * Remove client arg * Use getJoinedMembers and add doc * Fix tests * Fix more tests * Fix other test * Clarify comment Co-authored-by: Michael Telatynski <[email protected]> --------- Co-authored-by: Michael Telatynski <[email protected]>
MidhunSureshR
left a comment
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.
Looks great, just two minor things!
t3chguy
left a comment
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 will need deprecating first, this PR should be rebased to vNext
src/models/room-member.ts
Outdated
| * Fires {@link RoomMemberEvent.PowerLevel} | ||
| */ | ||
| public setPowerLevelEvent(powerLevelEvent: MatrixEvent): void { | ||
| public setPowerLevelEvent(powerLevelEvent: MatrixEvent, roomCreateEvent: MatrixEvent, roomVersion: string): void { |
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.
Having to pass the roomVersion & roomCreateEvent feels like a really odd signature, IMO RoomMember should know its room, it should probably be a c'tor param it holds onto then it can get these itself.
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 will be a pretty significant change which will also need RoomState to know about the room it's for as it has to construct RoomMembers. The thinking here was that you pass all the inputs into the function rather than it taking some from the state which hopefully makes it easier to reason about and less magic, plus just wanting to keep the class as pure as possible and not knowing anything about the context it exists in, only what it represents itself. Admittedly it does make the function signature a little odd: I was going to rename it to something that reflected it taking all the information that affected power levels but I didn't feel like it helped a great deal.
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 feel like its a massive footgun like setPowerLevel used to be on MatrixClient back when you needed to pass in the existing power level event. We had a bunch of issues about people holding it wrong. I think the RoomState holding the Room object is very sane.
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.
Alternatively the logic could be hoisted into RoomState where it lives in a private method and RoomMember just blindly trusts the powerLevel you give it, rather than needing the whole event. Either way its a breaking change.
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.
Well, this is all breaking change unfortunately: hydra sort of necessitates it. I'm not sure people holding it wrong is really a concern here? This isn't really public facing API.
I'm very reluctant to block our hydra changes on a significant refactor of a part of js-sdk's data model: i think this would add significant risk to our hydra deadline. I can look at moving it RoomState.
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 appears as public in the API docs https://matrix-org.github.io/matrix-js-sdk/classes/matrix.RoomMember.html#setpowerlevelevent
Assume unknown room versions do use hydra
Co-authored-by: Richard van der Hoff <[email protected]>
* Support for creator power level Adds support for infinite power level specified by [MSC4289](matrix-org/matrix-spec-proposals#4289). * Update unit test * Hardcode versions as room versions strings aren't ordered * Add test for v12 rooms * Use more compact syntax Co-authored-by: R Midhun Suresh <[email protected]> * Fix doc Co-authored-by: R Midhun Suresh <[email protected]> * Fix additionalCreators from PR edit * Split out hydra room version check * Move power level logic into room state Which already has knowledge of the room create event * Add docs * Fix unused bits * Fix docs * Fix lying docstring * Reverse logic for hydra semantics Assume unknown room versions do use hydra * Use backticks Co-authored-by: Richard van der Hoff <[email protected]> * Switch back to hardcoding just the two hydra versions --------- Co-authored-by: R Midhun Suresh <[email protected]> Co-authored-by: Richard van der Hoff <[email protected]> (cherry picked from commit e119bf9)
Follows on from #4937
* Support for creator power level Adds support for infinite power level specified by [MSC4289](matrix-org/matrix-spec-proposals#4289). * Update unit test * Hardcode versions as room versions strings aren't ordered * Add test for v12 rooms * Use more compact syntax * Fix doc * Fix additionalCreators from PR edit * Split out hydra room version check * Move power level logic into room state Which already has knowledge of the room create event * Add docs * Fix unused bits * Fix docs * Fix lying docstring * Reverse logic for hydra semantics Assume unknown room versions do use hydra * Use backticks * Switch back to hardcoding just the two hydra versions --------- (cherry picked from commit e119bf9) Co-authored-by: David Baker <[email protected]> Co-authored-by: R Midhun Suresh <[email protected]> Co-authored-by: Richard van der Hoff <[email protected]>
Follows on from #4937 (cherry picked from commit dea184e) Co-authored-by: David Baker <[email protected]>
This inverts the check for whether to use hydra semantics to only NOT use it for known, old room versions and use hydra for everything else, so rooms with versions we don't know about will use hydra semantics. This will cause any rooms using old/experiental versions unknown to the js-sdk to break, but will mean that wehn the next room version comes out, we'll use hydra for it which is, of course, not a given, but is way more likely than going back to the old semantics. The mobile Element clients currently hardcode hydra versions (ie. as it is without this change, but we expect them to make this same change soon after the hydra release. We do NOT expect this to land with the hydra release, but target it for the release after. Reverts 1e5054a from #4937 See element-hq/element-meta#2921 for public discussion.
This inverts the check for whether to use hydra semantics to only NOT use it for known, old room versions and use hydra for everything else, so rooms with versions we don't know about will use hydra semantics. This will cause any rooms using old/experiental versions unknown to the js-sdk to break, but will mean that wehn the next room version comes out, we'll use hydra for it which is, of course, not a given, but is way more likely than going back to the old semantics. The mobile Element clients currently hardcode hydra versions (ie. as it is without this change, but we expect them to make this same change soon after the hydra release. We do NOT expect this to land with the hydra release, but target it for the release after. Reverts 1e5054a from #4937 See element-hq/element-meta#2921 for public discussion.
* Update for compatibility with v12 rooms Stop using powerLevelNorm and reading PL events manually. To support matrix-org/matrix-js-sdk#4937 * Add test for leave space dialog * Don't compute stuff if we don't need it * Use room.client * Use getSafeUserId * Remove client arg * Use getJoinedMembers and add doc * Fix tests * Fix more tests * Fix other test * Clarify comment Co-authored-by: Michael Telatynski <[email protected]> --------- Co-authored-by: Michael Telatynski <[email protected]>
Adds support for infinite power level specified by MSC4289.
This version removes powerLevelNorm which we only use in one place in Element Web (where we probably should not) and I'm not sure is generally that sensible, so I've removed it since it's a difficult concept to keep and would at best be a slight lie when one of the PLs is always infinity (eg. we'd have to say creator is also 100, probably). This makes this a BREAKING CHANGE.
I'll leave this as draft until I have a matching PR for element.
Checklist
public/exportedsymbols have accurate TSDoc documentation.