Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Make video rooms compatible with matrix RTC #11829

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Nov 4, 2023

After a lot of trying I actually found a stepping store solution.
It is possible to get it working with very minimal changes when beeing careful.

Its all slighly more fragile than I would want It to be.
Especially there need to be desitions made on when we add/remove the widget.

For video rooms the widget now always lives and never gets destroyed.
For normal rooms the widget is the indicator that the usere created a call. So we create the widget on call button press but only connect the widget once the lobby ends.
If the call ends we destroy the widget because it is our indicator that the user created a call so the ui would show a call after the hangup as long as the widget lives.

        ElementCall.createOrGetCallWidget(room.roomId, room.client);
        WidgetStore.instance.emit(UPDATE_EVENT, null);

Sadly we need to emit here from the outside. It would be nicer if createVirtualWidget would call the WidgetStore update automatically. But then WidgetStore.instance.emit(UPDATE_EVENT, null); also would be called during the Call.get() fn. But this is causing iussues in the CallStore.

private updateRoom(room: Room): void {

   if (!this.calls.has(room.roomId)) {
              const call = Call.get(room); // HERE

              if (call) {
                  const onConnectionState = (state: ConnectionState): void => {

If Call.get() calls WidgetStore.instance.emit(UPDATE_EVENT, null); we create two widgets since the second call at HERE also would call the updateRoom again and we end up with two widgets.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@toger5 toger5 requested a review from a team as a code owner November 4, 2023 09:10
@toger5 toger5 added T-Task Refactoring, enabling or disabling functionality, other engineering tasks T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements and removed T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements labels Nov 6, 2023
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

This seems sensible to me. Agreed that emitting from outside the object isn't a good pattern - have I understood that the problem is that we always need an update event for calls whether it actually adds the widget or not, but for video rooms we don't?

@toger5
Copy link
Contributor Author

toger5 commented Nov 7, 2023

have I understood that the problem is that we always need an update event for calls whether it actually adds the widget or not, but for video rooms we don't?

yes whenever anything changed that might impact the return value of Call.get() we want to update. (so that the list of calls can be populated with the new value from Call.get(room) With this change we need to also update the Store when the virutal widget is added.

@toger5 toger5 added this pull request to the merge queue Nov 7, 2023
Merged via the queue into develop with commit 224b917 Nov 7, 2023
22 checks passed
@toger5 toger5 deleted the toger5/matrix-rtc-video-rooms branch November 7, 2023 12:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants