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

Don't update the participant list when only you joined #6777

Merged

Conversation

nickvergessen
Copy link
Member

Signed-off-by: Joas Schilling coding@schilljs.com

@nickvergessen
Copy link
Member Author

/backport to stable23

src/utils/signaling.js Outdated Show resolved Hide resolved
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/dont-update-participant-list-for-self-join branch from 329c18b to 7945581 Compare January 13, 2022 18:40
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

When you receive the join event from the external signaling server after you joined a room I would still emit the participantListChanged, as the participant list indeed changed (participant list as the concept, not the Vue component), but with an extra parameter to know if it is a "self-join" (which could be just the list of joined users) so it can be ignored if not needed. That is, I would move the deciding logic from the signaling to the getParticipants mixin, as it is the one that fetches the participants when joining a conversation and thus the one that knows that it is not needed to fetch them again.

Nevertheless, the approach in this pull request (or the extra parameter) only prevents an extra request to the server after joining if no other user is in the room. If you join a room in which there are other users the external signaling server will send a join event about your own participant to all the participants in the room (including you) and another join event sent just to you with all the other participants (that is, excluding you). Although the events are published in that order they arrive in reverse order (the first event goes through the NAT and the second is sent directly, I guess it is what causes that), although I am not sure if that would always happen.

Due to this a different approach could be to pass the list of joined sessions in the participantListChanged* (with a parameter to know that they were added and not changed or removed) and check in the getParticipants mixin if all the joined sessions are already in the participants data to fetch it again or not. Although this might require to queue the checks if the participant data is being fetched to prevent deciding based on stalled data... (for example, if a participant leaves and quickly joins again with different parameters, like the status). So maybe it is better to improve all this at a later point and for now just remove the request for your own join 🤷

*Or listen to usersJoined, usersChanged and usersLeft rather than participantListChanged when the external signaling server is used.

@nickvergessen
Copy link
Member Author

So maybe it is better to improve all this at a later point and for now just remove the request for your own join shrug

Which is exactly what I was trying to do. Simplicity and still removing a refresh.

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Which is exactly what I was trying to do. Simplicity and still removing a refresh.

Fine by me 👍 Nevertheless, my comment about moving the logic from the signaling object to the getParticipant mixin still stands :-)

@nickvergessen
Copy link
Member Author

my comment about moving the logic from the signaling object to the getParticipant mixin still stands :-)

Well the participant list didn't change, we updated it already with the response of our own join. But sure, let's continue emitting a false event and modify all listeners to ignore it instead of not emitting it

@nickvergessen
Copy link
Member Author

Because the other listeners also reload the room information I don't dare to touch them just yet. So ready for review

@danxuliu danxuliu force-pushed the bugfix/noid/dont-update-participant-list-for-self-join branch from a73a771 to 7945581 Compare January 19, 2022 22:45
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

I thought that the joinedUsers array provided the Nextcloud session id, so what I meant was just calling this._trigger('participantListChanged', { joined: joinedUsers }) from signaling.js and then ignoring the update in getParticipants.js if participants?.joined?.find(joinedParticipant => joinedParticipant.nextcloudSessionId === this.participant.sessionId. But joinedUsers only provides the signaling session ID, not the Nextcloud session ID, so the current participant can not be matched 🤷

Due to this I agree to use just the original commit, so I dropped the second commit that emitted whether the participant list was dirty or not, as the event logic is still coupled to the getParticipants mixin (and others) and moving the check does not provide a benefit; the other listeners of participantListChanged event (FilesSidebarTabApp, PublicShareAuthSidebar and PublicShareSidebar) do not need to listen to the event when self-joined either.

Tested and works 👍

@fancycode
Copy link
Member

But joinedUsers only provides the signaling session ID, not the Nextcloud session ID, so the current participant can not be matched

FYI, this will change with the next release of the signaling server: strukturag/nextcloud-spreed-signaling#178

@nickvergessen nickvergessen merged commit 1f85b4c into master Jan 20, 2022
@nickvergessen nickvergessen deleted the bugfix/noid/dont-update-participant-list-for-self-join branch January 20, 2022 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants