From b31ed4030958696460e4a6f95fef9d246f83eb06 Mon Sep 17 00:00:00 2001 From: Hristo Terezov Date: Thu, 26 Aug 2021 18:23:38 -0500 Subject: [PATCH] fix(mobile-pagination): address PR review comments --- .../filmstrip/components/native/Filmstrip.js | 2 +- .../filmstrip/components/native/Thumbnail.js | 88 +++++++++++-------- .../filmstrip/components/native/TileView.js | 17 ++-- .../filmstrip/components/native/styles.js | 18 ++-- react/features/filmstrip/middleware.native.js | 5 ++ 5 files changed, 78 insertions(+), 52 deletions(-) diff --git a/react/features/filmstrip/components/native/Filmstrip.js b/react/features/filmstrip/components/native/Filmstrip.js index 9635e9c0f..bc3fab6d6 100644 --- a/react/features/filmstrip/components/native/Filmstrip.js +++ b/react/features/filmstrip/components/native/Filmstrip.js @@ -237,7 +237,7 @@ class Filmstrip extends PureComponent { renderItem = { this._renderThumbnail } showsHorizontalScrollIndicator = { false } showsVerticalScrollIndicator = { false } - style = { styles.scrollView } + style = { styles.flatListStageView } viewabilityConfig = { this._viewabilityConfig } windowSize = { 2 } /> { diff --git a/react/features/filmstrip/components/native/Thumbnail.js b/react/features/filmstrip/components/native/Thumbnail.js index ce085c6a8..653c85bf9 100644 --- a/react/features/filmstrip/components/native/Thumbnail.js +++ b/react/features/filmstrip/components/native/Thumbnail.js @@ -52,7 +52,7 @@ type Props = { _isFakeParticipant: boolean, /** - * Indicates whether the participant is fake. + * Indicates whether the participant is screen sharing. */ _isScreenShare: boolean, @@ -200,12 +200,11 @@ class Thumbnail extends PureComponent { } /** - * Implements React's {@link Component#render()}. + * Renders the indicators for the thumbnail. * - * @inheritdoc * @returns {ReactElement} */ - render() { + _renderIndicators() { const { _audioMuted: audioMuted, _isScreenShare: isScreenShare, @@ -213,10 +212,56 @@ class Thumbnail extends PureComponent { _renderDominantSpeakerIndicator: renderDominantSpeakerIndicator, _renderModeratorIndicator: renderModeratorIndicator, _participantId: participantId, + _videoMuted: videoMuted + } = this.props; + const indicators = []; + + if (renderModeratorIndicator) { + indicators.push( + + ); + } + + if (!_isFakeParticipant) { + indicators.push( + + { renderDominantSpeakerIndicator && } + ); + indicators.push( + + ); + indicators.push( + { audioMuted && } + { videoMuted && } + { isScreenShare && } + ); + } + + return indicators; + } + + /** + * Implements React's {@link Component#render()}. + * + * @inheritdoc + * @returns {ReactElement} + */ + render() { + const { + _isScreenShare: isScreenShare, + _isFakeParticipant, + _participantId: participantId, _participantInLargeVideo: participantInLargeVideo, _pinned, _styles, - _videoMuted: videoMuted, disableTint, height, renderDisplayName, @@ -255,39 +300,8 @@ class Thumbnail extends PureComponent { } - { renderModeratorIndicator - && - - - } { - !_isFakeParticipant - && - - { renderDominantSpeakerIndicator && } - - } - { - !_isFakeParticipant - && - - - } - { - !_isFakeParticipant - && - { audioMuted && } - { videoMuted && } - { isScreenShare && } - + this._renderIndicators() } ); diff --git a/react/features/filmstrip/components/native/TileView.js b/react/features/filmstrip/components/native/TileView.js index ece276a63..fad17a13e 100644 --- a/react/features/filmstrip/components/native/TileView.js +++ b/react/features/filmstrip/components/native/TileView.js @@ -71,6 +71,12 @@ type Props = { onClick: Function }; +/** + * An empty array. The purpose of the constant is to use the same reference every time we need an empty array. + * This will prevent unnecessary re-renders. + */ +const EMPTY_ARRAY = []; + /** * Implements a React {@link PureComponent} which displays thumbnails in a two * dimensional grid. @@ -109,7 +115,7 @@ class TileView extends PureComponent { itemVisiblePercentThreshold: 30 }; this._flatListStyles = { - ...styles.flatList + ...styles.flatListTileView }; this._contentContainerStyles = { ...styles.contentContainer @@ -162,7 +168,7 @@ class TileView extends PureComponent { if (this._flatListStyles.minHeight !== _height || this._flatListStyles.minWidth !== _width) { this._flatListStyles = { - ...styles.flatList, + ...styles.flatListTileView, minHeight: _height, minWidth: _width }; @@ -207,11 +213,12 @@ class TileView extends PureComponent { */ _getSortedParticipants() { const { _localParticipant, _remoteParticipants } = this.props; - const participants = []; - _localParticipant && participants.push(_localParticipant.id); + if (!_localParticipant) { + return EMPTY_ARRAY; + } - return [ ...participants, ..._remoteParticipants ]; + return [ _localParticipant?.id, ..._remoteParticipants ]; } _renderThumbnail: Object => Object; diff --git a/react/features/filmstrip/components/native/styles.js b/react/features/filmstrip/components/native/styles.js index 36cc082fd..45f6c8564 100644 --- a/react/features/filmstrip/components/native/styles.js +++ b/react/features/filmstrip/components/native/styles.js @@ -71,9 +71,16 @@ export default { }, /** - * The styles for the FlatList. + * The styles for the FlatList component in stage view. */ - flatList: { + flatListStageView: { + flexGrow: 0 + }, + + /** + * The styles for the FlatList component in tile view. + */ + flatListTileView: { flex: 0 }, @@ -94,13 +101,6 @@ export default { right: 4 }, - /** - * The style of the scrollview containing the remote thumbnails. - */ - scrollView: { - flexGrow: 0 - }, - /** * The style of a participant's Thumbnail which renders either the video or * the avatar of the associated participant. diff --git a/react/features/filmstrip/middleware.native.js b/react/features/filmstrip/middleware.native.js index 7f450e9a4..ed41cdb6e 100644 --- a/react/features/filmstrip/middleware.native.js +++ b/react/features/filmstrip/middleware.native.js @@ -13,6 +13,11 @@ import './subscriber'; */ MiddlewareRegistry.register(store => next => action => { if (action.type === PARTICIPANT_LEFT) { + // This have to be executed before we remove the participant from features/base/participants state in order to + // remove the related thumbnail component before we need to re-render it. If we do this after next() + // we will be in sitation where the participant exists in the remoteParticipants array in features/filmstrip + // but doesn't exist in features/base/participants state which will lead to rendering a thumbnail for + // non-existing participant. updateRemoteParticipantsOnLeave(store, action.participant?.id); }