-
Notifications
You must be signed in to change notification settings - Fork 128
Add sticky event support #3513
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 sticky event support #3513
Conversation
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.
So this is added a new lab option?
It is just for Netlify? so why a review?
} | ||
|
||
// There was call membership events at some point in the timeline. | ||
return timeline.getEvents().some( |
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.
There is no API to get the sticky events? Having the sticky key doesn't make the event sticky.
I am a bit lost with what this method is expecting to do
} | ||
|
||
// There was call membership events at some point in the timeline. | ||
return timeline.getEvents().some( |
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 is this method exactly? Try some heuristic to see if a room is a call room? like type": "org.matrix.msc3417.call"
?
Or if there might be an active call?
And now it is checking for possible past sticky events (but potentially not anymore sticky)?
I don't get what this doing and what for
Sorry failed to change the title 🤦 |
- use new js-sdk - use custom synapse - don't filter rooms by existing call state events Signed-off-by: Timo K <[email protected]>
Signed-off-by: Timo K <[email protected]>
eb586a5
to
6580555
Compare
… toger5/sticky-events-version
Signed-off-by: Timo K <[email protected]>
Signed-off-by: Timo K <[email protected]>
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 think the following might make sense:
-
since sticky events imply the new event format it also implies multi-sfu (new event format does not have a oldest member transport)
-
rename the dev option to: use sticky m.rtc.member` events
-
Always use multi sfu when stikcy evnets are activated (gray out the multi sfu option)
Currently turning multi-sfu off and sticky on results in not sending any transport at all
Signed-off-by: Timo K <[email protected]>
I think this might be ready to merge @robintown |
src/rtcSessionHelpers.ts
Outdated
membershipEventExpiryMs: | ||
matrixRtcSessionConfig?.membership_event_expiry_ms, | ||
useExperimentalToDeviceTransport, | ||
unstableSendStickyEvents: preferStickyEvents.getValue(), |
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.
Could be nice to have preferStickyEvents
be an explicit parameter of enterRtcSession
, like multiSfu
is, so we can easily turn on sticky events in the middle of a call when dogfooding
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.
we would still need to rejoin and setup a new membership manager. I am not sure its worth the effort?
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.
Sure but with the scope.reconcile
method already used for starting and stopping the membership managers as needed, this ends up being pretty trivial.
Incidentally I see it as necessary to make the CallViewModel also aware of the relationship between multiSfu
and preferStickyEvents
(the fact that one force-enables the other), otherwise it would pass the wrong transport to enterRtcSession
. So I just committed e313cf0 which handles both these things. Does it look reasonable to you?
src/home/useGroupCallRooms.ts
Outdated
|
||
// Check for *active* calls using sticky events. | ||
for (const sticky of room._unstable_getStickyEvents()) { | ||
if (sticky.getType() === EventType.GroupCallMemberPrefix) { |
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.
Presumably we should refer to RTCMembership
here rather than GroupCallMemberPrefix
(though TBH we could probably replace this entire function body with return true;
and it would be fine… we're definitely not aiming to support using the SPA in cases where the homeserver is also used by messenger apps… well, maybe we are with the guest access use case, but I don't think we'll care about giving guest access users a home page at all.)
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.
100%.
Lets keep the function in for now. There might be some use for it in the guest case and it is very detangled from the rest of element-call -> not complexifying the codebase. (just makes it larger)
Signed-off-by: Timo K <[email protected]>
Signed-off-by: Timo K <[email protected]>
CallViewModel would pass the wrong transport to enterRtcSession when the user enabled sticky events but didn't manually enable multi-SFU mode as well. This likely would've added some confusion to our attempts to test these modes.
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 think this looks pretty much ready now. But just one thing I want to get clarity on (for my future reference as we eventually roll the Matrix 2.0 changes out to the public):
Largely this just adds a new setting to enable it when starting a call. Sticky events will always work if someone else on the call enables it.
From what I can tell by looking at the matrix-js-sdk diff, it's not the case that it switches membership manager implementations when another participant sending sticky events is detected. Do you mean that the setting enables sending sticky events, but receiving sticky events will always work?
src/rtcSessionHelpers.ts
Outdated
membershipEventExpiryMs: | ||
matrixRtcSessionConfig?.membership_event_expiry_ms, | ||
useExperimentalToDeviceTransport, | ||
unstableSendStickyEvents: preferStickyEvents.getValue(), |
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.
Sure but with the scope.reconcile
method already used for starting and stopping the membership managers as needed, this ends up being pretty trivial.
Incidentally I see it as necessary to make the CallViewModel also aware of the relationship between multiSfu
and preferStickyEvents
(the fact that one force-enables the other), otherwise it would pass the wrong transport to enterRtcSession
. So I just committed e313cf0 which handles both these things. Does it look reasonable to you?
Signed-off-by: Timo K <[email protected]>
I added another todo comment so we can dont forget discussion the I will merge this into the main multi sfu branch as it is now however so we have less branches to handle. |
This uses the newly merged sticky event work. Largely this just adds a new setting to enable it when starting a call. Sticky events will always work if someone else on the call enables it.
Testing options
Netilify
homeserver
url parameter to link your custom homeserver (example:https://localhost:3000?homeserver=https%3A%2F%2Ftest.com
encoding is of course needed)Locally
pull/download this PR/branch
start the docker dev env via
yarn backend
(synapse, livekit sfu, nginx, jwt authorizationService...)(in a new terminal) start EC
yarn dev
Now it should "just work" to do calls on https://localhost:3000