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).
roy-loomy · Aug 17, 2017 · 27deb97 · 27deb97
1 parent 20379da
commit 27deb97
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 46 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 (
-            <div className = 'filmstrip'>
+            <div className = { filmstripClassNames }>
                 { this.props.displayToolbox ? <Toolbox /> : null }
                 <div
                     className = 'filmstrip__videos'
@@ -53,7 +72,7 @@ export default class Filmstrip extends Component {
                     <div
                         className = 'filmstrip__videos'
                         id = 'filmstripRemoteVideos'>
-                        {/*
+                        {/**
                           * 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
@@ -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
@@ -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
     };
 }