From 27deb97c5ce2855ef809b84d3c6ed9fbbba0dd13 Mon Sep 17 00:00:00 2001 From: Leonard Kim Date: Fri, 11 Aug 2017 17:27:30 -0700 Subject: [PATCH] ref(filmstrip): hook filmstrip to redux for 1-on-1 mode - Remove non-redux paths for hiding and showing remote videos. - Hook web filmstrip to redux to know when to hide remote videos. This works, even though VideoLayout is handling RemoteVideo appending, because react is only monitoring filmstrip's declared JSX which does not change except for attributes (css classes). --- conference.js | 6 ++- modules/UI/UI.js | 14 ------ modules/UI/videolayout/Filmstrip.js | 27 +---------- .../filmstrip/components/Filmstrip.web.js | 45 +++++++++++++++++-- react/features/filmstrip/reducer.js | 7 ++- .../components/VideoQualityLabel.web.js | 3 +- 6 files changed, 56 insertions(+), 46 deletions(-) diff --git a/conference.js b/conference.js index cf1b00388..0db7b7cdc 100644 --- a/conference.js +++ b/conference.js @@ -62,6 +62,7 @@ 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 @@ -2176,8 +2177,9 @@ export default { || remoteVideosCount > 1 || remoteParticipantsCount !== remoteVideosCount; - APP.UI.setRemoteThumbnailsVisibility( - Boolean(shouldShowRemoteThumbnails)); + APP.store.dispatch( + setFilmstripRemoteVideosVisibility( + Boolean(shouldShowRemoteThumbnails))); } }, /** diff --git a/modules/UI/UI.js b/modules/UI/UI.js index 70cdf022f..59ce0d02e 100644 --- a/modules/UI/UI.js +++ b/modules/UI/UI.js @@ -290,10 +290,6 @@ UI.start = function () { SideContainerToggler.init(eventEmitter); Filmstrip.init(eventEmitter); - // By default start with remote videos hidden and rely on other logic to - // make them visible. - UI.setRemoteThumbnailsVisibility(false); - VideoLayout.init(eventEmitter); if (!interfaceConfig.filmStripOnly) { VideoLayout.initLargeVideo(); @@ -1220,16 +1216,6 @@ UI.onUserFeaturesChanged = user => VideoLayout.onUserFeaturesChanged(user); */ UI.getRemoteVideosCount = () => VideoLayout.getRemoteVideosCount(); -/** - * Makes remote thumbnail videos visible or not visible. - * - * @param {boolean} shouldHide - True if remote thumbnails should be hidden, - * false f they should be visible. - * @returns {void} - */ -UI.setRemoteThumbnailsVisibility - = shouldHide => Filmstrip.setRemoteVideoVisibility(shouldHide); - /** * Sets the remote control active status for a remote participant. * diff --git a/modules/UI/videolayout/Filmstrip.js b/modules/UI/videolayout/Filmstrip.js index 0ff8345e2..05754cf2a 100644 --- a/modules/UI/videolayout/Filmstrip.js +++ b/modules/UI/videolayout/Filmstrip.js @@ -1,9 +1,6 @@ -/* global $, APP, config, JitsiMeetJS, interfaceConfig */ +/* global $, APP, JitsiMeetJS, interfaceConfig */ -import { - setFilmstripRemoteVideosVisibility, - setFilmstripVisibility -} from '../../../react/features/filmstrip'; +import { setFilmstripVisibility } from '../../../react/features/filmstrip'; import UIEvents from "../../../service/UI/UIEvents"; import UIUtil from "../util/UIUtil"; @@ -30,26 +27,6 @@ const Filmstrip = { } }, - /** - * Sets a class on the remote videos container for CSS to adjust visibility - * of the remote videos. Will no-op if config.debug is truthy, as should be - * the case with torture tests. - * - * @param {boolean} shouldHide - True if remote videos should be hidden, - * false if they should be visible. - * @returns {void} - */ - setRemoteVideoVisibility(shouldShow) { - // Allow disabling on 1-on-1 UI mode. Used by torture tests. - if (config.disable1On1Mode) { - return; - } - - APP.store.dispatch(setFilmstripRemoteVideosVisibility(shouldShow)); - $(`.${this.filmstripContainerClassName}`) - .toggleClass('hide-videos', !shouldShow); - }, - /** * Initializes the filmstrip toolbar. */ diff --git a/react/features/filmstrip/components/Filmstrip.web.js b/react/features/filmstrip/components/Filmstrip.web.js index 0fe94f4c1..9c345ae56 100644 --- a/react/features/filmstrip/components/Filmstrip.web.js +++ b/react/features/filmstrip/components/Filmstrip.web.js @@ -1,6 +1,7 @@ /* @flow */ import React, { Component } from 'react'; +import { connect } from 'react-redux'; import { Toolbox } from '../../toolbox'; @@ -10,8 +11,14 @@ import { Toolbox } from '../../toolbox'; * * @extends Component */ -export default class Filmstrip extends Component { +class Filmstrip extends Component { static propTypes = { + /** + * Whether or not the remote videos should be visible. Will toggle + * a class for hiding the videos. + */ + _remoteVideosVisible: React.PropTypes.bool, + /** * Whether or not the toolbox should be displayed within the filmstrip. */ @@ -25,8 +32,20 @@ export default class Filmstrip extends Component { * @returns {ReactElement} */ render() { + /** + * Note: Appending of {@code RemoteVideo} views is handled through + * VideoLayout. The views do not get blown away on render() because + * ReactDOMComponent is only aware of the given JSX and not new appended + * DOM. As such, when updateDOMProperties gets called, only attributes + * will get updated without replacing the DOM. If the known DOM gets + * modified, then the views will get blown away. + */ + + const filmstripClassNames = `filmstrip ${this.props._remoteVideosVisible + ? '' : 'hide-videos'}`; + return ( -
+
{ this.props.displayToolbox ? : null }
- {/* + {/** * This extra video container is needed for scrolling * thumbnails in Firefox; otherwise, the flex * thumbnails resize instead of causing overflow. @@ -75,3 +94,23 @@ export default class Filmstrip extends Component { ); } } + +/** + * Maps (parts of) the Redux state to the associated {@code Filmstrip}'s props. + * + * @param {Object} state - The Redux state. + * @private + * @returns {{ + * _remoteVideosVisible: boolean + * }} + */ +function _mapStateToProps(state) { + const { remoteVideosVisible } = state['features/filmstrip']; + const { disable1On1Mode } = state['features/base/config']; + + return { + _remoteVideosVisible: Boolean(remoteVideosVisible || disable1On1Mode) + }; +} + +export default connect(_mapStateToProps)(Filmstrip); diff --git a/react/features/filmstrip/reducer.js b/react/features/filmstrip/reducer.js index 4615680b6..a1e1ce8da 100644 --- a/react/features/filmstrip/reducer.js +++ b/react/features/filmstrip/reducer.js @@ -5,7 +5,12 @@ import { } from './actionTypes'; const DEFAULT_STATE = { - remoteVideosVisible: true, + /** + * 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 }; diff --git a/react/features/video-quality/components/VideoQualityLabel.web.js b/react/features/video-quality/components/VideoQualityLabel.web.js index adb80ead1..0490c4b1e 100644 --- a/react/features/video-quality/components/VideoQualityLabel.web.js +++ b/react/features/video-quality/components/VideoQualityLabel.web.js @@ -252,6 +252,7 @@ function _mapStateToProps(state) { audioOnly, conference } = state['features/base/conference']; + const { disable1On1Mode } = state['features/base/config']; const { remoteVideosVisible, visible @@ -264,7 +265,7 @@ function _mapStateToProps(state) { _audioOnly: audioOnly, _conferenceStarted: Boolean(conference), _filmstripVisible: visible, - _remoteVideosVisible: remoteVideosVisible, + _remoteVideosVisible: Boolean(remoteVideosVisible || disable1On1Mode), _resolution: resolution }; }