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

Fix "silent" parameter not sent again when reconnecting #10688

Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Oct 12, 2023

When a call was joined the value of the silent parameter was not tracked by the signaling object, so if the call was joined again (for example, in a forced reconnection, or if joining the call was deferred because the room was not joined yet by the signaling) silent was undefined, which was treated as false by the API.

The current set of scenarios that could trigger the issue is unlikely to happen in the real world, although it is possible nevertheless. Other slightly more common cases, like the participant starting without audio and video and then selecting the devices while already in the call would not trigger a reconnection, just an update of flags (at least if no one else joined yet, but if someone else joined then the reconnection will not cause the call to be started again, it would be just a participant leaving and then joining again).

I was surprised to find that the second scenario below is also fixed by this pull request. But it turns out that when another participant joins the call it always uses silent = true, no matter how the call was originally started (I assumed that, as the call was already started and no notification would be sent anyway, silent would be false, which would have caused a notification to be sent if the rest of participants left and a participant that joined once the call was already started then had a reconnection, as that would cause the call to be started again). But it is fixed, so 🥳

How to test (scenario 1)

  • Enable the notifications app
  • Create a conversation and add user A and user B
  • Set default permissions of the conversation to allow starting a call, but do not allow publishing audio and video
  • In a private window, log in as user A
  • Open the conversation
  • Start a silent call
  • In another browser, log in as user B
  • In the original browser window, grant all permissions to user A

Result with this pull request

The user B does not receive any notification about a started call, even if the call was started again due to the reconnection of user A

Result without this pull request

The user B receives a notification about a started call

How to test (scenario 2)

  • Enable the notifications app
  • Create a conversation and add user A and user B
  • Set default permissions of the conversation to allow starting a call, but do not allow publishing audio and video
  • Start a silent call
  • In a private window, log in as user A
  • Open the conversation
  • Join the call
  • In another browser, log in as user B
  • In the original browser window, leave the call
  • Grant all permissions to user A

Result with this pull request

The user B does not receive any notification about a started call, even if the call was started again due to the reconnection of user A

Result without this pull request

The user B receives a notification about a started call

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When a call was joined the value of the "silent" parameter was not
tracked by the signaling object, so if the call was joined again (for
example, in a forced reconnection, or if joining the call was deferred
because the room was not joined yet by the signaling) "silent" was
undefined, which was treated as "false" by the API.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Copy link
Member Author

/backport to stable27

@danxuliu
Copy link
Member Author

/backport to stable26

@danxuliu
Copy link
Member Author

/backport to stable25

@nickvergessen
Copy link
Member

I was surprised to find that the second scenario below is also fixed by this pull request. But it turns out that when another participant joins the call it always uses silent = true, no matter how the call was originally started

This was done, so that people that click "Join call" while the call is being ended by a moderator do not recall everyone. Had this too often with the company call after "End for everyone" was added

@Antreesy
Copy link
Contributor

Got this when updating audio/video permissions, while participant is in call:

  • Screen 'Waiting for others to join the call …' was replaced with infinite "Connecting ..." spinner.

Might be a visual bug only, but trace look like this:

  • EmptyCallView
  • this.isConnecting === true
  • participantsStore.state.connecting is not falsy
  • commit('finishedConnecting') is not called
  • EventBus.$once('signaling-users-in-room') is triggered only once (same for setTimeout)
    async joinCall({ commit, getters }, { token, participantIdentifier, flags, silent }) {

@nickvergessen
Copy link
Member

Screen 'Waiting for others to join the call …' was replaced with infinite "Connecting ..." spinner.

Didn't happen for me 🤔

@danxuliu
Copy link
Member Author

I can reproduce with the HPB. It is unrelated to this pull request, so I will merge it :-)

During a forced reconnection with the HPB connecting is set again due to calling setInCall to disconnect the previous session and then connect the new one. However, connecting is reset only once the first time that the signaling users are got after joining the call, or 10 seconds after joining the call, whichever comes first. As joinCall is not called again in the store during a forced reconnection connecting = true is kept until the user leaves the call.

@danxuliu danxuliu merged commit 1c85075 into master Oct 13, 2023
@danxuliu danxuliu deleted the fix-silent-parameter-not-sent-again-when-reconnecting branch October 13, 2023 12:22
@nickvergessen
Copy link
Member

/backport to stable27

@nickvergessen
Copy link
Member

/backport to stable26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants