From 771d60f95416a37ebde63dcafc685b086335cac2 Mon Sep 17 00:00:00 2001 From: Lyubo Marinov Date: Tue, 22 May 2018 16:47:43 -0500 Subject: [PATCH] Associate remote participant w/ JitsiConference (_UPDATED) The commit message of "Associate remote participant w/ JitsiConference (_JOINED)" explains the motivation for this commit. Practically, _JOINED and _LEFT combined with "Remove remote participants who are no longer of interest" should alleviate the problem with multiplying remote participants to an acceptable level of annoyance. Technically though, a remote participant cannot be identified by an ID only. The ID is (somewhat) "unique" in the context of a single JitsiConference instance. So in order to not have to scratch our heads over an obscure corner, racing case, it's better to always identify remote participants by the pair id-conference. Unfortunately, that's a bit of a high order given the existing source code. So I've implemented the cases which are the easiest so that new source code written with participantUpdated is more likely to identify a remote participant with the pair id-conference. Additionally, the commit "Reduce direct read access to the features/base/participants redux state" brings more control back to the functions of the feature base/participants so that one day we can (if we choose to) do something like, for example: If getParticipants is called with a conference, it returns the participants from features/base/participants who are associated with the specified conference. If no conference is specified in the function call, then default to the conference which is the primary focus of the app at the time of the function call. Added to the above, this should allow us to further reduce the cases in which we're identifying remote participants by id only and get us even closer to a more "predictable" behavior in corner, racing cases. --- conference.js | 29 +++++++++++++++++-- react/features/base/conference/actions.js | 6 +++- react/features/base/jwt/middleware.js | 10 +++++-- react/features/base/participants/actions.js | 8 ++++- react/features/base/participants/functions.js | 4 +-- .../features/base/participants/middleware.js | 13 ++++----- react/features/base/participants/reducer.js | 20 ++++++++++--- .../toolbox/components/web/Toolbox.js | 6 ++++ 8 files changed, 75 insertions(+), 21 deletions(-) diff --git a/conference.js b/conference.js index f30cf0ff5..d5ac0a340 100644 --- a/conference.js +++ b/conference.js @@ -1804,9 +1804,9 @@ export default { APP.UI.participantConnectionStatusChanged(id); }); - room.on(JitsiConferenceEvents.DOMINANT_SPEAKER_CHANGED, id => { - APP.store.dispatch(dominantSpeakerChanged(id)); - }); + room.on( + JitsiConferenceEvents.DOMINANT_SPEAKER_CHANGED, + id => APP.store.dispatch(dominantSpeakerChanged(id, room))); if (!interfaceConfig.filmStripOnly) { room.on(JitsiConferenceEvents.CONNECTION_INTERRUPTED, () => { @@ -1883,6 +1883,7 @@ export default { = displayName.substr(0, MAX_DISPLAY_NAME_LENGTH); APP.store.dispatch(participantUpdated({ + conference: room, id, name: formattedDisplayName })); @@ -1916,6 +1917,7 @@ export default { switch (name) { case 'raisedHand': APP.store.dispatch(participantUpdated({ + conference: room, id: participant.getId(), raisedHand: newValue === 'true' })); @@ -2015,6 +2017,7 @@ export default { APP.UI.addListener(UIEvents.EMAIL_CHANGED, this.changeLocalEmail); room.addCommandListener(this.commands.defaults.EMAIL, (data, from) => { APP.store.dispatch(participantUpdated({ + conference: room, id: from, email: data.value })); @@ -2026,6 +2029,7 @@ export default { (data, from) => { APP.store.dispatch( participantUpdated({ + conference: room, id: from, avatarURL: data.value })); @@ -2035,6 +2039,7 @@ export default { (data, from) => { APP.store.dispatch( participantUpdated({ + conference: room, id: from, avatarID: data.value })); @@ -2579,6 +2584,12 @@ export default { const localId = localParticipant.id; APP.store.dispatch(participantUpdated({ + // XXX Only the local participant is allowed to update without + // stating the JitsiConference instance (i.e. participant property + // `conference` for a remote participant) because the local + // participant is uniquely identified by the very fact that there is + // only one local participant. + id: localId, local: true, email: formattedEmail @@ -2606,6 +2617,12 @@ export default { } APP.store.dispatch(participantUpdated({ + // XXX Only the local participant is allowed to update without + // stating the JitsiConference instance (i.e. participant property + // `conference` for a remote participant) because the local + // participant is uniquely identified by the very fact that there is + // only one local participant. + id, local: true, avatarURL: formattedUrl @@ -2662,6 +2679,12 @@ export default { } APP.store.dispatch(participantUpdated({ + // XXX Only the local participant is allowed to update without + // stating the JitsiConference instance (i.e. participant property + // `conference` for a remote participant) because the local + // participant is uniquely identified by the very fact that there is + // only one local participant. + id, local: true, name: formattedNickname diff --git a/react/features/base/conference/actions.js b/react/features/base/conference/actions.js index b07714fac..8c18b7f1d 100644 --- a/react/features/base/conference/actions.js +++ b/react/features/base/conference/actions.js @@ -125,13 +125,14 @@ function _addConferenceListeners(conference, dispatch) { conference.on( JitsiConferenceEvents.DISPLAY_NAME_CHANGED, (id, displayName) => dispatch(participantUpdated({ + conference, id, name: displayName.substr(0, MAX_DISPLAY_NAME_LENGTH) }))); conference.on( JitsiConferenceEvents.DOMINANT_SPEAKER_CHANGED, - (...args) => dispatch(dominantSpeakerChanged(...args))); + id => dispatch(dominantSpeakerChanged(id, conference))); conference.on( JitsiConferenceEvents.PARTICIPANT_CONN_STATUS_CHANGED, @@ -155,18 +156,21 @@ function _addConferenceListeners(conference, dispatch) { conference.addCommandListener( AVATAR_ID_COMMAND, (data, id) => dispatch(participantUpdated({ + conference, id, avatarID: data.value }))); conference.addCommandListener( AVATAR_URL_COMMAND, (data, id) => dispatch(participantUpdated({ + conference, id, avatarURL: data.value }))); conference.addCommandListener( EMAIL_COMMAND, (data, id) => dispatch(participantUpdated({ + conference, id, email: data.value }))); diff --git a/react/features/base/jwt/middleware.js b/react/features/base/jwt/middleware.js index 75664a74d..ee1067741 100644 --- a/react/features/base/jwt/middleware.js +++ b/react/features/base/jwt/middleware.js @@ -143,7 +143,10 @@ function _overwriteLocalParticipant( if ((avatarURL || email || name) && (localParticipant = getLocalParticipant(getState))) { - const newProperties: Object = { id: localParticipant.id }; + const newProperties: Object = { + id: localParticipant.id, + local: true + }; if (avatarURL) { newProperties.avatarURL = avatarURL; @@ -264,7 +267,10 @@ function _undoOverwriteLocalParticipant( if ((avatarURL || name || email) && (localParticipant = getLocalParticipant(getState))) { - const newProperties: Object = { id: localParticipant.id }; + const newProperties: Object = { + id: localParticipant.id, + local: true + }; if (avatarURL === localParticipant.avatarURL) { newProperties.avatarURL = undefined; diff --git a/react/features/base/participants/actions.js b/react/features/base/participants/actions.js index b08cafeb9..59575e306 100644 --- a/react/features/base/participants/actions.js +++ b/react/features/base/participants/actions.js @@ -22,17 +22,23 @@ import { getLocalParticipant } from './functions'; * Create an action for when dominant speaker changes. * * @param {string} id - Participant's ID. + * @param {JitsiConference} conference - The {@code JitsiConference} associated + * with the participant identified by the specified {@code id}. Only the local + * participant is allowed to not specify an associated {@code JitsiConference} + * instance. * @returns {{ * type: DOMINANT_SPEAKER_CHANGED, * participant: { + * conference: JitsiConference, * id: string * } * }} */ -export function dominantSpeakerChanged(id) { +export function dominantSpeakerChanged(id, conference) { return { type: DOMINANT_SPEAKER_CHANGED, participant: { + conference, id } }; diff --git a/react/features/base/participants/functions.js b/react/features/base/participants/functions.js index 64957fca3..33fdda0c4 100644 --- a/react/features/base/participants/functions.js +++ b/react/features/base/participants/functions.js @@ -127,9 +127,7 @@ export function getLocalParticipant(stateful: Object | Function) { * @private * @returns {(Participant|undefined)} */ -export function getParticipantById( - stateful: Object | Function, - id: string) { +export function getParticipantById(stateful: Object | Function, id: string) { const participants = _getAllParticipants(stateful); return participants.find(p => p.id === id); diff --git a/react/features/base/participants/middleware.js b/react/features/base/participants/middleware.js index 61735812d..a4c99faf0 100644 --- a/react/features/base/participants/middleware.js +++ b/react/features/base/participants/middleware.js @@ -63,20 +63,19 @@ MiddlewareRegistry.register(store => next => action => { case DOMINANT_SPEAKER_CHANGED: { // Ensure the raised hand state is cleared for the dominant speaker. + + const { conference, id } = action.participant; const participant = getLocalParticipant(store.getState()); - if (participant) { - const { id } = action.participant; - - store.dispatch(participantUpdated({ + participant + && store.dispatch(participantUpdated({ + conference, id, local: participant.id === id, raisedHand: false })); - } - typeof APP === 'object' - && APP.UI.markDominantSpeaker(action.participant.id); + typeof APP === 'object' && APP.UI.markDominantSpeaker(id); break; } diff --git a/react/features/base/participants/reducer.js b/react/features/base/participants/reducer.js index 260432e52..20d86aadf 100644 --- a/react/features/base/participants/reducer.js +++ b/react/features/base/participants/reducer.js @@ -32,12 +32,24 @@ import { LOCAL_PARTICIPANT_DEFAULT_ID, PARTICIPANT_ROLE } from './constants'; declare var APP: Object; /** - * These properties should not be bulk assigned when updating a particular - * @see Participant. + * The participant properties which cannot be updated through + * {@link PARTICIPANT_UPDATED}. They either identify the participant or can only + * be modified through property-dedicated actions. + * * @type {string[]} */ -const PARTICIPANT_PROPS_TO_OMIT_WHEN_UPDATE - = [ 'dominantSpeaker', 'id', 'local', 'pinned' ]; +const PARTICIPANT_PROPS_TO_OMIT_WHEN_UPDATE = [ + + // The following properties identify the participant: + 'conference', + 'id', + 'local', + + // The following properties can only be modified through property-dedicated + // actions: + 'dominantSpeaker', + 'pinned' +]; /** * Listen for actions which add, remove, or update the set of participants in diff --git a/react/features/toolbox/components/web/Toolbox.js b/react/features/toolbox/components/web/Toolbox.js index d418b3076..d04cbe749 100644 --- a/react/features/toolbox/components/web/Toolbox.js +++ b/react/features/toolbox/components/web/Toolbox.js @@ -505,6 +505,12 @@ class Toolbox extends Component { const { _localParticipantID, _raisedHand } = this.props; this.props.dispatch(participantUpdated({ + // XXX Only the local participant is allowed to update without + // stating the JitsiConference instance (i.e. participant property + // `conference` for a remote participant) because the local + // participant is uniquely identified by the very fact that there is + // only one local participant. + id: _localParticipantID, local: true, raisedHand: !_raisedHand