-
-
Notifications
You must be signed in to change notification settings - Fork 642
[MatrixRTC] Sticky Events support (MSC4354) #5017
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
base: develop
Are you sure you want to change the base?
Conversation
4ff6fb9
to
dde4abb
Compare
f8e0385
to
7750916
Compare
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.
temp review
interface SessionMembershipsForRoomOpts { | ||
listenForStickyEvents: boolean; | ||
listenForMemberStateEvents: boolean; | ||
} |
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.
Wondering if we really want to make this configurable?
It would be great if we can just always make it listen to this?
Not sure its possible however...
Do you have an idea for a case where "always on" would break?
} | ||
|
||
this.client.on(ClientEvent.Room, this.onRoom); | ||
this.client.on(ClientEvent.Event, this.onEvent); |
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 in combination with:
this.roomSubset.on(RoomEvent.StickyEvents, this.onStickyEventUpdate);
will result in two calls to recalculateSessionMembers
once via refreshRoom
-> onRTCSessionMemberUpdate
-> recalculateSessionMembers
And a second time throughonStickyEventUpdate
->`recalculateSessionMembers.
Or is there any check so one of them gets ignored?
A possible solution would be to check if the associated room already has a session (in onRoom
) and if so, we do not call refresh room? So it only gets called on creating a new session?
This logic maybe should be applied to all of this file. refreshRoom
should be: refreshSessionStore/Map
and than we expect the session itself to listen to all the evnt updates. Listeners in the SessionManager only are responsible to find out if new sessions need to be created!
34be9c8
to
e3157e6
Compare
e36ff71
to
946c20a
Compare
I am relativly sure we are doing the right thing.
|
Signed-off-by: Timo K <[email protected]>
}); | ||
}); | ||
|
||
describe("StickyEventMembershipManager", () => { |
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 test suite needs much expanding really. This only touches the joins, we really need to test other behaviours.
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.
The only thing that this class does really is overwriting send and send delay + makeMyMembership. All this is covered in the join test.
There is also the error logging method... but that not being tested sounds fairly save.
All the rest of the behavior of the Memberhsip Manager stays as it was.
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.
Is there anything specific you have in mind?
Signed-off-by: Timo K <[email protected]>
Signed-off-by: Timo K <[email protected]>
src/matrixrtc/MembershipManager.ts
Outdated
}; | ||
|
||
protected actionUpdateFromErrors(e: unknown, t: MembershipActionType, m: string): ActionUpdate | undefined { | ||
const mappedMethod = new Map([ |
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'd just make this a static const, which gives you types for free and stops us creating a new Map each time we call this.
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.
yes that makes 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.
Otherwise LGTM
Signed-off-by: Timo K <[email protected]>
Depends on #5028
This PR adds support for sticky events in the MatrixRTCSession: