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 using signaling session ID instead of Nextcloud session ID #12961

Conversation

danxuliu
Copy link
Member

CallParticipantModels provide the signaling session ID and the Nextcloud session ID. When the internal signaling server is used both IDs are the same, but not with the external signaling server. The session ID in the participant and peer data provided by the Nextcloud server is the Nextcloud session ID, so no participant or peer data was found in the CallView components when the external signaling server was used, as the data was looked for using the signaling session ID.

This does not seem to have been a problem, though, as generally the CallParticipantModels provide the needed data from the signaling messages, and the components only fall back to the participant or peer data when the data is not found in the models. This seems to be the case only with SIP participants, as the fall back to the peer data was initially introduced when SIP support was added, and then the fall back to the participant data was introduced in a later fix. However... given that even when it was introduced the peer data was already using the signaling session ID but indexed using the Nextcloud session ID the fallbacks may not have even worked 🤔 Anyway, I could not test with SIP participants, so this pull request just fixes the usage of the session ID, independently of whether it is actually needed or not 🤷

Besides that, note that the guest name store can not be used by guest themselves, as the guest display name is set from the participant data, which is not available to guest users. But again the display name is expected to be provided by the model, and got from other places only as a fallback. The VideoVue component falls back to both the guest store and the peer data, but the Screen and ReactionToaster only fall back to the guest store, so it would not work when the current participant is a guest and the UI shows the screen or reaction of other guests. However, if the fallback is needed only for SIP participants then it should not be a problem, as they can not share the screen nor send a reaction.

Independently of all that, guestName in LocalVideo is unused since NcAvatar was replaced by AvatarWrapper, so the computed property and all the related code was removed.

How to test

  • Remove the code to get the participant name, actor type and user id from the model in VideoVue, Screen and ReactionToaster so only the fallbacks are used (and for a thorough testing, leave a single fallback when there are several in the same method; be careful, as the peer data itself falls back to the participant data when a user ID is provided, so you may get data even if you were not expecting it, which can be a bit puzzling)
  • Create a public conversation
  • Start a call
  • In a private window, join the conversation as a guest and set a name
  • Join the call
  • Share the screen and send reactions

Result with this pull request

The name and avatar of the guest user is properly shown in the original window for the video, screen and reactions (note that the name and avatar of the registered user may not be shown in the guest window if the data is not got from the model)

Result without this pull request

The name and avatar of the guest user is not set or is a generic one

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Makes sense in general, tested with guest participants

src/components/CallView/shared/LocalVideo.vue Show resolved Hide resolved
src/components/CallView/shared/VideoVue.vue Show resolved Hide resolved
@danxuliu danxuliu force-pushed the fix-using-signaling-session-id-instead-of-nextcloud-session-id branch from 011efc2 to 903f212 Compare August 14, 2024 12:40
@Antreesy Antreesy requested a review from DorraJaouad August 14, 2024 13:02
@nickvergessen
Copy link
Member

/backport to stable30

@nickvergessen
Copy link
Member

conflicts?

CallParticipantModels provide the signaling session ID and the Nextcloud
session ID. When the internal signaling server is used both IDs are the
same, but not with the external signaling server. The session ID in the
participant and peer data provided by the Nextcloud server is the
Nextcloud session ID, so no participant or peer data was found in the
CallView components when the external signaling server was used, as the
data was looked for using the signaling session ID.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
"guestName" in "LocalVideo" is unused since "NcAvatar" was replaced by
"AvatarWrapper" in 973a735.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@nickvergessen nickvergessen force-pushed the fix-using-signaling-session-id-instead-of-nextcloud-session-id branch from 903f212 to 38c4427 Compare August 15, 2024 07:26
@nickvergessen nickvergessen merged commit 48536eb into main Aug 15, 2024
47 checks passed
@nickvergessen nickvergessen deleted the fix-using-signaling-session-id-instead-of-nextcloud-session-id branch August 15, 2024 07:31
@danxuliu
Copy link
Member Author

Backport to 29 and 28?

@Antreesy
Copy link
Contributor

Antreesy commented Aug 19, 2024

Should be possible to backport, and fine by me, but I don't think it's necessary (unless other PR based on this is also need to be backported)

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