Merge pull request #2894 from robintown/stable-visibility

Determine which tiles are on screen in a more stable manner

Author: robintown

src/grid/Grid.tsx

@@ -24,7 +24,6 @@ import {
   createContext,
   forwardRef,
   memo,
-  useCallback,
   useContext,
   useEffect,
   useMemo,
@@ -54,7 +53,6 @@ interface Tile<Model> {
   id: string;
   model: Model;
   onDrag: DragCallback | undefined;
-  setVisible: (visible: boolean) => void;
 }
 
 type PlacedTile<Model> = Tile<Model> & Rect;
@@ -88,7 +86,6 @@ interface SlotProps<Model> extends Omit<ComponentProps<"div">, "onDrag"> {
   id: string;
   model: Model;
   onDrag?: DragCallback;
-  onVisibilityChange?: (visible: boolean) => void;
   style?: CSSProperties;
   className?: string;
 }
@@ -115,24 +112,47 @@ function offset(element: HTMLElement, relativeTo: Element): Offset {
   }
 }
 
+export type VisibleTilesCallback = (visibleTiles: number) => void;
+
 interface LayoutContext {
   setGeneration: Dispatch<SetStateAction<number | null>>;
+  setVisibleTilesCallback: Dispatch<
+    SetStateAction<VisibleTilesCallback | null>
+  >;
 }
 
 const LayoutContext = createContext<LayoutContext | null>(null);
 
+function useLayoutContext(): LayoutContext {
+  const context = useContext(LayoutContext);
+  if (context === null)
+    throw new Error("useUpdateLayout called outside a Grid layout context");
+  return context;
+}
+
 /**
  * Enables Grid to react to layout changes. You must call this in your Layout
  * component or else Grid will not be reactive.
  */
 export function useUpdateLayout(): void {
-  const context = useContext(LayoutContext);
-  if (context === null)
-    throw new Error("useUpdateLayout called outside a Grid layout context");
-
+  const { setGeneration } = useLayoutContext();
   // On every render, tell Grid that the layout may have changed
-  useEffect(() =>
-    context.setGeneration((prev) => (prev === null ? 0 : prev + 1)),
+  useEffect(() => setGeneration((prev) => (prev === null ? 0 : prev + 1)));
+}
+
+/**
+ * Asks Grid to call a callback whenever the number of visible tiles may have
+ * changed.
+ */
+export function useVisibleTiles(callback: VisibleTilesCallback): void {
+  const { setVisibleTilesCallback } = useLayoutContext();
+  useEffect(
+    () => setVisibleTilesCallback(() => callback),
+    [callback, setVisibleTilesCallback],
+  );
+  useEffect(
+    () => (): void => setVisibleTilesCallback(null),
+    [setVisibleTilesCallback],
   );
 }
 
@@ -245,39 +265,20 @@ export function Grid<
   const windowHeight = useObservableEagerState(windowHeightObservable);
   const [layoutRoot, setLayoutRoot] = useState<HTMLElement | null>(null);
   const [generation, setGeneration] = useState<number | null>(null);
+  const [visibleTilesCallback, setVisibleTilesCallback] =
+    useState<VisibleTilesCallback | null>(null);
   const tiles = useInitial(() => new Map<string, Tile<TileModel>>());
   const prefersReducedMotion = usePrefersReducedMotion();
 
   const Slot: FC<SlotProps<TileModel>> = useMemo(
     () =>
-      function Slot({
-        id,
-        model,
-        onDrag,
-        onVisibilityChange,
-        style,
-        className,
-        ...props
-      }) {
+      function Slot({ id, model, onDrag, style, className, ...props }) {
         const ref = useRef<HTMLDivElement | null>(null);
-        const prevVisible = useRef<boolean | null>(null);
-        const setVisible = useCallback(
-          (visible: boolean) => {
-            if (
-              onVisibilityChange !== undefined &&
-              visible !== prevVisible.current
-            ) {
-              onVisibilityChange(visible);
-              prevVisible.current = visible;
-            }
-          },
-          [onVisibilityChange],
-        );
 
         useEffect(() => {
-          tiles.set(id, { id, model, onDrag, setVisible });
+          tiles.set(id, { id, model, onDrag });
           return (): void => void tiles.delete(id);
-        }, [id, model, onDrag, setVisible]);
+        }, [id, model, onDrag]);
 
         return (
           <div
@@ -307,7 +308,10 @@ export function Grid<
     [],
   );
 
-  const context: LayoutContext = useMemo(() => ({ setGeneration }), []);
+  const context: LayoutContext = useMemo(
+    () => ({ setGeneration, setVisibleTilesCallback }),
+    [setVisibleTilesCallback],
+  );
 
   // Combine the tile definitions and slots together to create placed tiles
   const placedTiles = useMemo(() => {
@@ -342,9 +346,11 @@ export function Grid<
   );
 
   useEffect(() => {
-    for (const tile of placedTiles)
-      tile.setVisible(tile.y + tile.height <= visibleHeight);
-  }, [placedTiles, visibleHeight]);
+    visibleTilesCallback?.(
+      placedTiles.filter((tile) => tile.y + tile.height <= visibleHeight)
+        .length,
+    );
+  }, [placedTiles, visibleTilesCallback, visibleHeight]);
 
   // Drag state is stored in a ref rather than component state, because we use
   // react-spring's imperative API during gestures to improve responsiveness

src/grid/GridLayout.tsx

@@ -13,7 +13,7 @@ import { type GridLayout as GridLayoutModel } from "../state/CallViewModel";
 import styles from "./GridLayout.module.css";
 import { useInitial } from "../useInitial";
 import { type CallLayout, arrangeTiles } from "./CallLayout";
-import { type DragCallback, useUpdateLayout } from "./Grid";
+import { type DragCallback, useUpdateLayout, useVisibleTiles } from "./Grid";
 
 interface GridCSSProperties extends CSSProperties {
   "--gap": string;
@@ -73,6 +73,7 @@ export const makeGridLayout: CallLayout<GridLayoutModel> = ({
   // The scrolling part of the layout is where all the grid tiles live
   scrolling: forwardRef(function GridLayout({ model, Slot }, ref) {
     useUpdateLayout();
+    useVisibleTiles(model.setVisibleTiles);
     const { width, height: minHeight } = useObservableEagerState(minBounds);
     const { gap, tileWidth, tileHeight } = useMemo(
       () => arrangeTiles(width, minHeight, model.grid.length),
@@ -93,13 +94,7 @@ export const makeGridLayout: CallLayout<GridLayoutModel> = ({
         }
       >
         {model.grid.map((m) => (
-          <Slot
-            key={m.id}
-            className={styles.slot}
-            id={m.id}
-            model={m}
-            onVisibilityChange={m.setVisible}
-          />
+          <Slot key={m.id} className={styles.slot} id={m.id} model={m} />
         ))}
       </div>
     );

src/grid/OneOnOneLayout.tsx

@@ -52,7 +52,6 @@ export const makeOneOnOneLayout: CallLayout<OneOnOneLayoutModel> = ({
         <Slot
           id={model.remote.id}
           model={model.remote}
-          onVisibilityChange={model.remote.setVisible}
           className={styles.container}
           style={{ width: tileWidth, height: tileHeight }}
         >
@@ -61,7 +60,6 @@ export const makeOneOnOneLayout: CallLayout<OneOnOneLayoutModel> = ({
             id={model.local.id}
             model={model.local}
             onDrag={onDragLocalTile}
-            onVisibilityChange={model.local.setVisible}
             data-block-alignment={pipAlignmentValue.block}
             data-inline-alignment={pipAlignmentValue.inline}
           />

src/grid/SpotlightExpandedLayout.tsx

@@ -63,7 +63,6 @@ export const makeSpotlightExpandedLayout: CallLayout<
             id={model.pip.id}
             model={model.pip}
             onDrag={onDragPip}
-            onVisibilityChange={model.pip.setVisible}
             data-block-alignment={pipAlignmentValue.block}
             data-inline-alignment={pipAlignmentValue.inline}
           />

src/grid/SpotlightLandscapeLayout.tsx

@@ -12,7 +12,7 @@ import classNames from "classnames";
 import { type CallLayout } from "./CallLayout";
 import { type SpotlightLandscapeLayout as SpotlightLandscapeLayoutModel } from "../state/CallViewModel";
 import styles from "./SpotlightLandscapeLayout.module.css";
-import { useUpdateLayout } from "./Grid";
+import { useUpdateLayout, useVisibleTiles } from "./Grid";
 
 /**
  * An implementation of the "spotlight landscape" layout, in which the spotlight
@@ -50,6 +50,7 @@ export const makeSpotlightLandscapeLayout: CallLayout<
     ref,
   ) {
     useUpdateLayout();
+    useVisibleTiles(model.setVisibleTiles);
     useObservableEagerState(minBounds);
     const withIndicators =
       useObservableEagerState(model.spotlight.media).length > 1;
@@ -63,13 +64,7 @@ export const makeSpotlightLandscapeLayout: CallLayout<
         />
         <div className={styles.grid}>
           {model.grid.map((m) => (
-            <Slot
-              key={m.id}
-              className={styles.slot}
-              id={m.id}
-              model={m}
-              onVisibilityChange={m.setVisible}
-            />
+            <Slot key={m.id} className={styles.slot} id={m.id} model={m} />
           ))}
         </div>
       </div>

src/grid/SpotlightPortraitLayout.tsx

@@ -12,7 +12,7 @@ import classNames from "classnames";
 import { type CallLayout, arrangeTiles } from "./CallLayout";
 import { type SpotlightPortraitLayout as SpotlightPortraitLayoutModel } from "../state/CallViewModel";
 import styles from "./SpotlightPortraitLayout.module.css";
-import { useUpdateLayout } from "./Grid";
+import { useUpdateLayout, useVisibleTiles } from "./Grid";
 
 interface GridCSSProperties extends CSSProperties {
   "--grid-gap": string;
@@ -54,6 +54,7 @@ export const makeSpotlightPortraitLayout: CallLayout<
     ref,
   ) {
     useUpdateLayout();
+    useVisibleTiles(model.setVisibleTiles);
     const { width } = useObservableEagerState(minBounds);
     const { gap, tileWidth, tileHeight } = arrangeTiles(
       width,
@@ -84,13 +85,7 @@ export const makeSpotlightPortraitLayout: CallLayout<
         />
         <div className={styles.grid}>
           {model.grid.map((m) => (
-            <Slot
-              key={m.id}
-              className={styles.slot}
-              id={m.id}
-              model={m}
-              onVisibilityChange={m.setVisible}
-            />
+            <Slot key={m.id} className={styles.slot} id={m.id} model={m} />
           ))}
         </div>
       </div>

src/state/CallViewModel.test.ts

@@ -376,16 +376,16 @@ test("screen sharing activates spotlight layout", () => {
 
 test("participants stay in the same order unless to appear/disappear", () => {
   withTestScheduler(({ hot, schedule, expectObservable }) => {
-    const modeInputMarbles = "     a";
+    const visibilityInputMarbles = "a";
     // First Bob speaks, then Dave, then Alice
-    const aSpeakingInputMarbles = "n- 1998ms - 1999ms y";
-    const bSpeakingInputMarbles = "ny 1998ms n 1999ms -";
-    const dSpeakingInputMarbles = "n- 1998ms y 1999ms n";
+    const aSpeakingInputMarbles = " n- 1998ms - 1999ms y";
+    const bSpeakingInputMarbles = " ny 1998ms n 1999ms -";
+    const dSpeakingInputMarbles = " n- 1998ms y 1999ms n";
     // Nothing should change when Bob speaks, because Bob is already on screen.
     // When Dave speaks he should switch with Alice because she's the one who
     // hasn't spoken at all. Then when Alice speaks, she should return to her
     // place at the top.
-    const expectedLayoutMarbles = "a  1999ms b 1999ms a 57999ms c 1999ms a";
+    const expectedLayoutMarbles = " a  1999ms b 1999ms a 57999ms c 1999ms a";
 
     withCallViewModel(
       of([aliceParticipant, bobParticipant, daveParticipant]),
@@ -397,15 +397,12 @@ test("participants stay in the same order unless to appear/disappear", () => {
         [daveParticipant, hot(dSpeakingInputMarbles, { y: true, n: false })],
       ]),
       (vm) => {
-        schedule(modeInputMarbles, {
+        schedule(visibilityInputMarbles, {
           a: () => {
             // We imagine that only three tiles (the first three) will be visible
             // on screen at a time
             vm.layout.subscribe((layout) => {
-              if (layout.type === "grid") {
-                for (let i = 0; i < layout.grid.length; i++)
-                  layout.grid[i].setVisible(i < 3);
-              }
+              if (layout.type === "grid") layout.setVisibleTiles(3);
             });
           },
         });
@@ -435,6 +432,56 @@ test("participants stay in the same order unless to appear/disappear", () => {
   });
 });
 
+test("participants adjust order when space becomes constrained", () => {
+  withTestScheduler(({ hot, schedule, expectObservable }) => {
+    // Start with all tiles on screen then shrink to 3
+    const visibilityInputMarbles = "a-b";
+    // Bob and Dave speak
+    const bSpeakingInputMarbles = " ny";
+    const dSpeakingInputMarbles = " ny";
+    // Nothing should change when Bob or Dave initially speak, because they are
+    // on screen. When the screen becomes smaller Alice should move off screen
+    // to make way for the speakers (specifically, she should swap with Dave).
+    const expectedLayoutMarbles = " a-b";
+
+    withCallViewModel(
+      of([aliceParticipant, bobParticipant, daveParticipant]),
+      of([aliceRtcMember, bobRtcMember, daveRtcMember]),
+      of(ConnectionState.Connected),
+      new Map([
+        [bobParticipant, hot(bSpeakingInputMarbles, { y: true, n: false })],
+        [daveParticipant, hot(dSpeakingInputMarbles, { y: true, n: false })],
+      ]),
+      (vm) => {
+        let setVisibleTiles: ((value: number) => void) | null = null;
+        vm.layout.subscribe((layout) => {
+          if (layout.type === "grid") setVisibleTiles = layout.setVisibleTiles;
+        });
+        schedule(visibilityInputMarbles, {
+          a: () => setVisibleTiles!(Infinity),
+          b: () => setVisibleTiles!(3),
+        });
+
+        expectObservable(summarizeLayout(vm.layout)).toBe(
+          expectedLayoutMarbles,
+          {
+            a: {
+              type: "grid",
+              spotlight: undefined,
+              grid: ["local:0", `${aliceId}:0`, `${bobId}:0`, `${daveId}:0`],
+            },
+            b: {
+              type: "grid",
+              spotlight: undefined,
+              grid: ["local:0", `${daveId}:0`, `${bobId}:0`, `${aliceId}:0`],
+            },
+          },
+        );
+      },
+    );
+  });
+});
+
 test("spotlight speakers swap places", () => {
   withTestScheduler(({ hot, schedule, expectObservable }) => {
     // Go immediately into spotlight mode for the test