From ef1b8fdb774ea130f7df50491fd4b1869a00667f Mon Sep 17 00:00:00 2001 From: virtuacoplenny Date: Tue, 29 Aug 2017 08:08:16 -0700 Subject: [PATCH] ref(1-on-1): move remote video visibility to a selector (#1922) * ref(1-on-1): move remote visibility to a selector Derive whether or not remote videos should display using a selector to look across different states. A selector was chosen over using memoized selectors (reselect) or subscribers as a first step approach, avoiding additional mutations caused by a subscriber updating the filmstrip state and avoiding additional api overhead introduced by reselect. * rename selector --- conference.js | 30 +--------------- react/features/filmstrip/actionTypes.js | 12 ------- react/features/filmstrip/actions.js | 18 ---------- .../filmstrip/components/Filmstrip.web.js | 7 ++-- react/features/filmstrip/functions.js | 34 +++++++++++++++++++ react/features/filmstrip/index.js | 1 + react/features/filmstrip/reducer.js | 12 ------- .../components/RecordingLabel.web.js | 5 +-- .../components/VideoQualityLabel.web.js | 19 ++++------- 9 files changed, 48 insertions(+), 90 deletions(-) create mode 100644 react/features/filmstrip/functions.js diff --git a/conference.js b/conference.js index d73fef644..d68592fe7 100644 --- a/conference.js +++ b/conference.js @@ -67,7 +67,6 @@ import { getLocationContextRoot } from './react/features/base/util'; import { statsEmitter } from './react/features/connection-indicator'; import { showDesktopPicker } from './react/features/desktop-picker'; import { maybeOpenFeedbackDialog } from './react/features/feedback'; -import { setFilmstripRemoteVideosVisibility } from './react/features/filmstrip'; import { mediaPermissionPromptVisibilityChanged, suspendDetected @@ -1626,17 +1625,14 @@ export default { // check the roles for the new user and reflect them APP.UI.updateUserRole(user); - - updateRemoteThumbnailsVisibility(); }); + room.on(ConferenceEvents.USER_LEFT, (id, user) => { APP.store.dispatch(participantLeft(id, user)); logger.log('USER %s LEFT', id, user); APP.API.notifyUserLeft(id); APP.UI.removeUser(id, user.getDisplayName()); APP.UI.onSharedVideoStop(id); - - updateRemoteThumbnailsVisibility(); }); room.on(ConferenceEvents.USER_STATUS_CHANGED, (id, status) => { @@ -1780,10 +1776,6 @@ export default { APP.UI.addListener( UIEvents.VIDEO_UNMUTING_WHILE_AUDIO_ONLY, () => this._displayAudioOnlyTooltip('videoMute')); - - APP.UI.addListener( - UIEvents.PINNED_ENDPOINT, - updateRemoteThumbnailsVisibility); } room.on(ConferenceEvents.CONNECTION_INTERRUPTED, () => { @@ -2147,8 +2139,6 @@ export default { } }); } - - updateRemoteThumbnailsVisibility(); }); room.addCommandListener( this.commands.defaults.SHARED_VIDEO, ({value, attributes}, id) => { @@ -2164,24 +2154,6 @@ export default { APP.UI.onSharedVideoUpdate(id, value, attributes); } }); - - function updateRemoteThumbnailsVisibility() { - const localUserId = APP.conference.getMyUserId(); - const remoteParticipantsCount = room.getParticipantCount() - 1; - - // Get the remote thumbnail count for cases where there are - // non-participants displaying video, such as with video sharing. - const remoteVideosCount = APP.UI.getRemoteVideosCount(); - - const shouldShowRemoteThumbnails = interfaceConfig.filmStripOnly - || (APP.UI.isPinned(localUserId) && remoteVideosCount) - || remoteVideosCount > 1 - || remoteParticipantsCount !== remoteVideosCount; - - APP.store.dispatch( - setFilmstripRemoteVideosVisibility( - Boolean(shouldShowRemoteThumbnails))); - } }, /** * Adds any room listener. diff --git a/react/features/filmstrip/actionTypes.js b/react/features/filmstrip/actionTypes.js index cc32cae7e..bd70e3e18 100644 --- a/react/features/filmstrip/actionTypes.js +++ b/react/features/filmstrip/actionTypes.js @@ -1,15 +1,3 @@ -/** - * The type of action which signals to change the visibility of remote videos in - * the filmstrip. - * - * { - * type: SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY, - * remoteVideosVisible: boolean - * } - */ -export const SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY - = Symbol('SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY'); - /** * The type of action sets the visibility of the entire filmstrip; * diff --git a/react/features/filmstrip/actions.js b/react/features/filmstrip/actions.js index 7fbb0cc86..9e1f297b4 100644 --- a/react/features/filmstrip/actions.js +++ b/react/features/filmstrip/actions.js @@ -1,25 +1,7 @@ import { - SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY, SET_FILMSTRIP_VISIBILITY } from './actionTypes'; -/** - * Sets the visibility of remote videos in the filmstrip. - * - * @param {boolean} remoteVideosVisible - Whether or not remote videos in the - * filmstrip should be visible. - * @returns {{ - * type: SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY, - * remoteVideosVisible: boolean - * }} - */ -export function setFilmstripRemoteVideosVisibility(remoteVideosVisible) { - return { - type: SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY, - remoteVideosVisible - }; -} - /** * Sets if the entire filmstrip should be visible. * diff --git a/react/features/filmstrip/components/Filmstrip.web.js b/react/features/filmstrip/components/Filmstrip.web.js index c0e1fe5d8..a7c54ce28 100644 --- a/react/features/filmstrip/components/Filmstrip.web.js +++ b/react/features/filmstrip/components/Filmstrip.web.js @@ -5,6 +5,8 @@ import { connect } from 'react-redux'; import { Toolbox } from '../../toolbox'; +import { shouldRemoteVideosBeVisible } from '../functions'; + /** * Implements a React {@link Component} which represents the filmstrip on * Web/React. @@ -89,11 +91,8 @@ class Filmstrip extends Component { * }} */ function _mapStateToProps(state) { - const { remoteVideosVisible } = state['features/filmstrip']; - const { disable1On1Mode } = state['features/base/config']; - return { - _remoteVideosVisible: Boolean(remoteVideosVisible || disable1On1Mode) + _remoteVideosVisible: shouldRemoteVideosBeVisible(state) }; } diff --git a/react/features/filmstrip/functions.js b/react/features/filmstrip/functions.js new file mode 100644 index 000000000..0040fbd3f --- /dev/null +++ b/react/features/filmstrip/functions.js @@ -0,0 +1,34 @@ +declare var interfaceConfig: Object; + +import { + getPinnedParticipant, + getLocalParticipant +} from '../base/participants'; + +/** + * A selector for determining whether or not remote video thumbnails should be + * displayed in the filmstrip. + * + * @param {Object} state - The full redux state. + * @returns {boolean} - True if remote video thumbnails should be displayed. + */ +export function shouldRemoteVideosBeVisible(state) { + const participants = state['features/base/participants']; + + const shouldShowVideos + = state['features/base/config'].disable1On1Mode + + || interfaceConfig.filmStripOnly + + // This is not a 1-on-1 call. + || participants.length > 2 + + // There is another participant and the local participant is pinned. + || (participants.length > 1 + && getLocalParticipant(state) === getPinnedParticipant(state)) + + // There is any non-person participant, like a shared video. + || participants.find(participant => participant.isBot); + + return Boolean(shouldShowVideos); +} diff --git a/react/features/filmstrip/index.js b/react/features/filmstrip/index.js index a29aa08e0..44000a1a7 100644 --- a/react/features/filmstrip/index.js +++ b/react/features/filmstrip/index.js @@ -1,6 +1,7 @@ export * from './actions'; export * from './actionTypes'; export * from './components'; +export * from './functions'; import './middleware'; import './reducer'; diff --git a/react/features/filmstrip/reducer.js b/react/features/filmstrip/reducer.js index a1e1ce8da..3b4c91043 100644 --- a/react/features/filmstrip/reducer.js +++ b/react/features/filmstrip/reducer.js @@ -1,16 +1,9 @@ import { ReducerRegistry } from '../base/redux'; import { - SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY, SET_FILMSTRIP_VISIBILITY } from './actionTypes'; const DEFAULT_STATE = { - /** - * By default start with remote videos hidden for 1-on-1 mode and rely on - * other logic to invoke an action to make them visible when needed. - */ - remoteVideosVisible: false, - visible: true }; @@ -18,11 +11,6 @@ ReducerRegistry.register( 'features/filmstrip', (state = DEFAULT_STATE, action) => { switch (action.type) { - case SET_FILMSTRIP_REMOTE_VIDEOS_VISIBLITY: - return { - ...state, - remoteVideosVisible: action.remoteVideosVisible - }; case SET_FILMSTRIP_VISIBILITY: return { ...state, diff --git a/react/features/recording/components/RecordingLabel.web.js b/react/features/recording/components/RecordingLabel.web.js index e3dd9d280..9b195d90c 100644 --- a/react/features/recording/components/RecordingLabel.web.js +++ b/react/features/recording/components/RecordingLabel.web.js @@ -2,6 +2,7 @@ import React, { Component } from 'react'; import { connect } from 'react-redux'; import { translate } from '../../base/i18n'; +import { shouldRemoteVideosBeVisible } from '../../filmstrip'; /** * Implements a React {@link Component} which displays the current state of @@ -140,7 +141,7 @@ class RecordingLabel extends Component { * }} */ function _mapStateToProps(state) { - const { remoteVideosVisible, visible } = state['features/filmstrip']; + const { visible } = state['features/filmstrip']; const { labelDisplayConfiguration } = state['features/recording']; return { @@ -164,7 +165,7 @@ function _mapStateToProps(state) { * * @type {boolean} */ - _remoteVideosVisible: remoteVideosVisible + _remoteVideosVisible: shouldRemoteVideosBeVisible(state) }; } diff --git a/react/features/video-quality/components/VideoQualityLabel.web.js b/react/features/video-quality/components/VideoQualityLabel.web.js index 784b4b096..3af3b4322 100644 --- a/react/features/video-quality/components/VideoQualityLabel.web.js +++ b/react/features/video-quality/components/VideoQualityLabel.web.js @@ -3,6 +3,8 @@ import { connect } from 'react-redux'; import { translate } from '../../base/i18n'; import { Popover } from '../../base/popover'; +import { shouldRemoteVideosBeVisible } from '../../filmstrip'; + import { VideoQualityDialog } from './'; import { @@ -209,24 +211,15 @@ export class VideoQualityLabel extends Component { * }} */ function _mapStateToProps(state) { - const { - audioOnly, - conference - } = state['features/base/conference']; - const { disable1On1Mode } = state['features/base/config']; - const { - remoteVideosVisible, - visible - } = state['features/filmstrip']; - const { - resolution - } = state['features/large-video']; + const { audioOnly, conference } = state['features/base/conference']; + const { visible } = state['features/filmstrip']; + const { resolution } = state['features/large-video']; return { _audioOnly: audioOnly, _conferenceStarted: Boolean(conference), _filmstripVisible: visible, - _remoteVideosVisible: Boolean(remoteVideosVisible || disable1On1Mode), + _remoteVideosVisible: shouldRemoteVideosBeVisible(state), _resolution: resolution }; }