fix(filmstrip) Excludes partially visible tiles from visible participants calculation#16909
fix(filmstrip) Excludes partially visible tiles from visible participants calculation#16909jallamsetty1 wants to merge 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates filmstrip “visible participants” computation so partially visible thumbnails at the viewport edges are excluded, improving accuracy of the visible range used by the filmstrip feature.
Changes:
- Web: adds
_excludePartiallyVisibleTilesand applies it in_onListItemsRenderedbefore dispatchingsetVisibleRemoteParticipants. - Native: adjusts
_onViewableItemsChangedto shrink the computed visible index range based on container/item dimensions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
react/features/filmstrip/components/web/Filmstrip.tsx |
Adds helper to adjust visible indices and uses it before dispatching visible remote participants. |
react/features/filmstrip/components/native/Filmstrip.tsx |
Shrinks viewable index range to exclude partial tiles based on computed filmstrip dimensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Calculate how many items can fit fully in the container | ||
| const fullyVisibleCount = Math.floor(containerSize / itemSize); | ||
|
|
||
| // Calculate how many items are currently in the visible range (inclusive) | ||
| const currentVisibleCount = stopIndex - startIndex + 1; | ||
|
|
||
| // If we have more items visible than can fit fully, we have a partially visible tile at the end | ||
| if (currentVisibleCount > fullyVisibleCount) { | ||
| // Reduce stopIndex to exclude the partially visible tile | ||
| return { | ||
| startIndex, | ||
| stopIndex: startIndex + fullyVisibleCount - 1 | ||
| }; | ||
| } |
There was a problem hiding this comment.
The calculation here can produce fullyVisibleCount === 0 when containerSize < itemSize, which makes the returned stopIndex become startIndex - 1. That can later propagate into setVisibleRemoteParticipants(startIndex, stopIndex) and lead to an invalid/negative range. Consider clamping fullyVisibleCount to at least 1 (or short-circuiting to an empty visible range with a well-defined sentinel) so downstream code never receives endIndex < startIndex unexpectedly.
| dispatch, | ||
| _currentLayout, | ||
| _filmstripWidth, | ||
| _filmstripHeight, | ||
| _thumbnailWidth, | ||
| _thumbnailHeight, | ||
| _isVerticalFilmstrip | ||
| } = this.props; | ||
|
|
||
| let adjustedStartIndex = visibleStartIndex; | ||
| let adjustedStopIndex = visibleStopIndex; | ||
|
|
||
| // Exclude partially visible tiles | ||
| if (_currentLayout === LAYOUTS.HORIZONTAL_FILMSTRIP_VIEW) { | ||
| const itemSize = _thumbnailWidth + TILE_HORIZONTAL_MARGIN; | ||
|
|
||
| ({ startIndex: adjustedStartIndex, stopIndex: adjustedStopIndex } = this._excludePartiallyVisibleTiles( | ||
| visibleStartIndex, | ||
| visibleStopIndex, | ||
| _filmstripWidth, | ||
| itemSize | ||
| )); | ||
| } else if (_isVerticalFilmstrip) { | ||
| const itemSize = _thumbnailHeight + TILE_VERTICAL_MARGIN; | ||
|
|
||
| ({ startIndex: adjustedStartIndex, stopIndex: adjustedStopIndex } = this._excludePartiallyVisibleTiles( | ||
| visibleStartIndex, | ||
| visibleStopIndex, | ||
| _filmstripHeight, | ||
| itemSize | ||
| )); | ||
| } | ||
|
|
||
| const { startIndex, stopIndex } = this._calculateIndices(adjustedStartIndex, adjustedStopIndex); | ||
|
|
There was a problem hiding this comment.
This approach only uses visibleStartIndex/visibleStopIndex plus a count-based heuristic, so it can’t reliably exclude partially visible tiles at the start of the viewport (e.g., when scrolled mid-item) and can still include a partially visible first tile when both ends are partial. To truly exclude partials, you likely need the current scroll offset (e.g., from FixedSizeList onScroll) and adjust startIndex/stopIndex based on scrollOffset % itemSize and (scrollOffset + containerSize) % itemSize.
| dispatch, | |
| _currentLayout, | |
| _filmstripWidth, | |
| _filmstripHeight, | |
| _thumbnailWidth, | |
| _thumbnailHeight, | |
| _isVerticalFilmstrip | |
| } = this.props; | |
| let adjustedStartIndex = visibleStartIndex; | |
| let adjustedStopIndex = visibleStopIndex; | |
| // Exclude partially visible tiles | |
| if (_currentLayout === LAYOUTS.HORIZONTAL_FILMSTRIP_VIEW) { | |
| const itemSize = _thumbnailWidth + TILE_HORIZONTAL_MARGIN; | |
| ({ startIndex: adjustedStartIndex, stopIndex: adjustedStopIndex } = this._excludePartiallyVisibleTiles( | |
| visibleStartIndex, | |
| visibleStopIndex, | |
| _filmstripWidth, | |
| itemSize | |
| )); | |
| } else if (_isVerticalFilmstrip) { | |
| const itemSize = _thumbnailHeight + TILE_VERTICAL_MARGIN; | |
| ({ startIndex: adjustedStartIndex, stopIndex: adjustedStopIndex } = this._excludePartiallyVisibleTiles( | |
| visibleStartIndex, | |
| visibleStopIndex, | |
| _filmstripHeight, | |
| itemSize | |
| )); | |
| } | |
| const { startIndex, stopIndex } = this._calculateIndices(adjustedStartIndex, adjustedStopIndex); | |
| dispatch | |
| } = this.props; | |
| const { startIndex, stopIndex } = this._calculateIndices(visibleStartIndex, visibleStopIndex); |
| */ | ||
| _onViewableItemsChanged({ viewableItems = [] }: { viewableItems: ViewToken[]; }) { | ||
| const { _disableSelfView } = this.props; | ||
| const { _aspectRatio, _clientWidth, _clientHeight, _disableSelfView } = this.props; |
There was a problem hiding this comment.
_clientWidth and _clientHeight are destructured here but never used in the method. With typical TS/ESLint no-unused-vars settings this will fail lint/typecheck; please remove them or use them in the calculation (e.g., via _getDimensions() if that was the intent).
| const { _aspectRatio, _clientWidth, _clientHeight, _disableSelfView } = this.props; | |
| const { _aspectRatio, _disableSelfView } = this.props; |
jallamsetty1
force-pushed
the
fix-filmstrip
branch
from
8a1458d to
ef98686
Compare
|
This approach does not work since the client will not request videos for these partially visible endpoints and they will appear frozen. |
1 participant
No description provided.