From 42c8677be1047c63bee31ba7e8c6796a518d19ae Mon Sep 17 00:00:00 2001
From: Robin <robin@robin.town>
Date: Mon, 20 Nov 2023 00:29:15 -0500
Subject: [PATCH] Fix jumpy speaker tiles in spotlight mode

reorderTiles was programmed to only place a tile in the speaker section if that tile's previous position was off-screen. But for speakers that started off-screen, this would cause them to oscillate in and out of the speaker section on each render, because the speaker section is, of course, on-screen. The solution I've gone with here is to avoid referencing the previous position, and instead go by the computed natural ordering, which ought to be more stable.
---
 src/video-grid/VideoGrid.tsx      | 27 ++++++++----
 test/video-grid/VideoGrid-test.ts | 72 +++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 8 deletions(-)
 create mode 100644 test/video-grid/VideoGrid-test.ts

diff --git a/src/video-grid/VideoGrid.tsx b/src/video-grid/VideoGrid.tsx
index 59cf78a7..3363a8b2 100644
--- a/src/video-grid/VideoGrid.tsx
+++ b/src/video-grid/VideoGrid.tsx
@@ -54,7 +54,7 @@ interface TilePosition {
   zIndex: number;
 }
 
-interface Tile<T> {
+export interface Tile<T> {
   key: string;
   order: number;
   item: TileDescriptor<T>;
@@ -728,7 +728,7 @@ function displayedTileCount(
 
 // Sets the 'order' property on tiles based on the layout param and
 // other properties of the tiles, eg. 'focused' and 'presenter'
-function reorderTiles<T>(
+export function reorderTiles<T>(
   tiles: Tile<T>[],
   layout: Layout,
   displayedTile = -1,
@@ -750,7 +750,6 @@ function reorderTiles<T>(
   } else {
     const focusedTiles: Tile<T>[] = [];
     const presenterTiles: Tile<T>[] = [];
-    const speakerTiles: Tile<T>[] = [];
     const onlyVideoTiles: Tile<T>[] = [];
     const otherTiles: Tile<T>[] = [];
 
@@ -763,8 +762,6 @@ function reorderTiles<T>(
         focusedTiles.push(tile);
       } else if (tile.isPresenter) {
         presenterTiles.push(tile);
-      } else if (tile.isSpeaker && displayedTile < tile.order) {
-        speakerTiles.push(tile);
       } else if (tile.hasVideo) {
         if (tile.order === 0 && tile.item.local) {
           firstLocalTile = tile;
@@ -788,13 +785,27 @@ function reorderTiles<T>(
       }
     }
 
-    [
+    const reorderedTiles = [
       ...focusedTiles,
       ...presenterTiles,
-      ...speakerTiles,
       ...onlyVideoTiles,
       ...otherTiles,
-    ].forEach((tile, i) => (tile.order = i));
+    ];
+    let nextSpeakerTileIndex = focusedTiles.length + presenterTiles.length;
+
+    reorderedTiles.forEach((tile, i) => {
+      // If a speaker's natural ordering would place it outside the default
+      // visible area, promote them to the section dedicated to speakers
+      if (tile.isSpeaker && displayedTile <= i && nextSpeakerTileIndex < i) {
+        // Remove the tile from its current section
+        reorderedTiles.splice(i, 1);
+        // Insert it into the speaker section
+        reorderedTiles.splice(nextSpeakerTileIndex, 0, tile);
+        nextSpeakerTileIndex++;
+      }
+    });
+
+    reorderedTiles.forEach((tile, i) => (tile.order = i));
   }
 }
 
diff --git a/test/video-grid/VideoGrid-test.ts b/test/video-grid/VideoGrid-test.ts
new file mode 100644
index 00000000..46134a13
--- /dev/null
+++ b/test/video-grid/VideoGrid-test.ts
@@ -0,0 +1,72 @@
+/*
+Copyright 2023 New Vector Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import {
+  Tile,
+  TileDescriptor,
+  reorderTiles,
+} from "../../src/video-grid/VideoGrid";
+
+const alice: Tile<unknown> = {
+  key: "alice",
+  order: 0,
+  item: { local: false } as unknown as TileDescriptor<unknown>,
+  remove: false,
+  focused: false,
+  isPresenter: false,
+  isSpeaker: false,
+  hasVideo: true,
+};
+const bob: Tile<unknown> = {
+  key: "bob",
+  order: 1,
+  item: { local: false } as unknown as TileDescriptor<unknown>,
+  remove: false,
+  focused: false,
+  isPresenter: false,
+  isSpeaker: false,
+  hasVideo: false,
+};
+
+test("reorderTiles does not promote a non-speaker", () => {
+  const tiles = [{ ...alice }, { ...bob }];
+  reorderTiles(tiles, "spotlight", 1);
+  expect(tiles).toEqual([
+    expect.objectContaining({ key: "alice", order: 0 }),
+    expect.objectContaining({ key: "bob", order: 1 }),
+  ]);
+});
+
+test("reorderTiles promotes a speaker into the visible area", () => {
+  const tiles = [{ ...alice }, { ...bob, isSpeaker: true }];
+  reorderTiles(tiles, "spotlight", 1);
+  expect(tiles).toEqual([
+    expect.objectContaining({ key: "alice", order: 1 }),
+    expect.objectContaining({ key: "bob", order: 0 }),
+  ]);
+});
+
+test("reorderTiles keeps a promoted speaker in the visible area", () => {
+  const tiles = [
+    { ...alice, order: 1 },
+    { ...bob, isSpeaker: true, order: 0 },
+  ];
+  reorderTiles(tiles, "spotlight", 1);
+  expect(tiles).toEqual([
+    expect.objectContaining({ key: "alice", order: 1 }),
+    expect.objectContaining({ key: "bob", order: 0 }),
+  ]);
+});