Merge pull request #3637 from robintown/flat-layout-switch

Allow user to switch layouts while phone is in landscape

src/state/CallViewModel/CallViewModel.ts

@@ -952,11 +952,12 @@ export function createCallViewModel$(
     ),
   );
 
-  const hasRemoteScreenShares$: Observable<boolean> = spotlight$.pipe(
+  const hasRemoteScreenShares$ = scope.behavior<boolean>(
-    map((spotlight) =>
+    spotlight$.pipe(
-      spotlight.some((vm) => !vm.local && vm instanceof ScreenShareViewModel),
+      map((spotlight) =>
+        spotlight.some((vm) => !vm.local && vm instanceof ScreenShareViewModel),
+      ),
     ),
-    distinctUntilChanged(),
   );
 
   const pipEnabled$ = scope.behavior(setPipEnabled$, false);

src/state/CallViewModel/LayoutSwitch.test.ts

@@ -5,198 +5,128 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
 Please see LICENSE in the repository root for full details.
 */
 
-import { afterEach, beforeEach, describe, expect, test } from "vitest";
+import { describe, test } from "vitest";
-import { firstValueFrom, of } from "rxjs";
 
 import { createLayoutModeSwitch } from "./LayoutSwitch";
-import { ObservableScope } from "../ObservableScope";
+import { testScope, withTestScheduler } from "../../utils/test";
-import { constant } from "../Behavior";
-import { withTestScheduler } from "../../utils/test";
 
-let scope: ObservableScope;
+function testLayoutSwitch({
-beforeEach(() => {
+  windowMode = "n",
-  scope = new ObservableScope();
+  hasScreenShares = "n",
-});
+  userSelection = "",
-afterEach(() => {
+  expectedGridMode,
-  scope.end();
+}: {
-});
+  windowMode?: string;
-
+  hasScreenShares?: string;
-describe("Default mode", () => {
+  userSelection?: string;
-  test("Should be in grid layout by default", async () => {
+  expectedGridMode: string;
-    const { gridMode$ } = createLayoutModeSwitch(
+}): void {
-      scope,
+  withTestScheduler(({ behavior, schedule, expectObservable }) => {
-      constant("normal"),
-      of(false),
-    );
-
-    const mode = await firstValueFrom(gridMode$);
-    expect(mode).toBe("grid");
-  });
-
-  test("Should switch to spotlight mode when window mode is flat", async () => {
-    const { gridMode$ } = createLayoutModeSwitch(
-      scope,
-      constant("flat"),
-      of(false),
-    );
-
-    const mode = await firstValueFrom(gridMode$);
-    expect(mode).toBe("spotlight");
-  });
-});
-
-test("Should allow switching modes manually", () => {
-  withTestScheduler(({ cold, behavior, expectObservable, schedule }): void => {
     const { gridMode$, setGridMode } = createLayoutModeSwitch(
-      scope,
+      testScope(),
-      behavior("n", { n: "normal" }),
+      behavior(windowMode, { n: "normal", N: "narrow", f: "flat" }),
-      cold("f", { f: false, t: true }),
+      behavior(hasScreenShares, { y: true, n: false }),
     );
-
+    schedule(userSelection, {
-    schedule("--sgs", {
-      s: () => setGridMode("spotlight"),
-      g: () => setGridMode("grid"),
-    });
-
-    expectObservable(gridMode$).toBe("g-sgs", {
-      g: "grid",
-      s: "spotlight",
-    });
-  });
-});
-
-test("Should switch to spotlight mode when there is a remote screen share", () => {
-  withTestScheduler(({ cold, behavior, expectObservable }): void => {
-    const shareMarble = "f--t";
-    const gridsMarble = "g--s";
-    const { gridMode$ } = createLayoutModeSwitch(
-      scope,
-      behavior("n", { n: "normal" }),
-      cold(shareMarble, { f: false, t: true }),
-    );
-
-    expectObservable(gridMode$).toBe(gridsMarble, {
-      g: "grid",
-      s: "spotlight",
-    });
-  });
-});
-
-test("Can manually force grid when there is a screenshare", () => {
-  withTestScheduler(({ cold, behavior, expectObservable, schedule }): void => {
-    const { gridMode$, setGridMode } = createLayoutModeSwitch(
-      scope,
-      behavior("n", { n: "normal" }),
-      cold("-ft", { f: false, t: true }),
-    );
-
-    schedule("---g", {
-      g: () => setGridMode("grid"),
-    });
-
-    expectObservable(gridMode$).toBe("ggsg", {
-      g: "grid",
-      s: "spotlight",
-    });
-  });
-});
-
-test("Should auto-switch after manually selected grid", () => {
-  withTestScheduler(({ cold, behavior, expectObservable, schedule }): void => {
-    const { gridMode$, setGridMode } = createLayoutModeSwitch(
-      scope,
-      behavior("n", { n: "normal" }),
-      // Two screenshares will happen in sequence
-      cold("-ft-ft", { f: false, t: true }),
-    );
-
-    // There was a screen-share that forced spotlight, then
-    // the user manually switch back to grid
-    schedule("---g", {
-      g: () => setGridMode("grid"),
-    });
-
-    // If we did want to respect manual selection, the expectation would be:
-    // const expectation = "ggsg";
-    const expectation = "ggsg-s";
-
-    expectObservable(gridMode$).toBe(expectation, {
-      g: "grid",
-      s: "spotlight",
-    });
-  });
-});
-
-test("Should switch back to grid mode when the remote screen share ends", () => {
-  withTestScheduler(({ cold, behavior, expectObservable }): void => {
-    const shareMarble = "f--t--f-";
-    const gridsMarble = "g--s--g-";
-    const { gridMode$ } = createLayoutModeSwitch(
-      scope,
-      behavior("n", { n: "normal" }),
-      cold(shareMarble, { f: false, t: true }),
-    );
-
-    expectObservable(gridMode$).toBe(gridsMarble, {
-      g: "grid",
-      s: "spotlight",
-    });
-  });
-});
-
-test("can auto-switch to spotlight again after first screen share ends", () => {
-  withTestScheduler(({ cold, behavior, expectObservable }): void => {
-    const shareMarble = "ftft";
-    const gridsMarble = "gsgs";
-    const { gridMode$ } = createLayoutModeSwitch(
-      scope,
-      behavior("n", { n: "normal" }),
-      cold(shareMarble, { f: false, t: true }),
-    );
-
-    expectObservable(gridMode$).toBe(gridsMarble, {
-      g: "grid",
-      s: "spotlight",
-    });
-  });
-});
-
-test("can switch manually to grid after screen share while manually in spotlight", () => {
-  withTestScheduler(({ cold, behavior, schedule, expectObservable }): void => {
-    // Initially, no one is sharing. Then the user manually switches to
-    // spotlight. After a screen share starts, the user manually switches to
-    // grid.
-    const shareMarbles = "  f-t-";
-    const setModeMarbles = "-s-g";
-    const expectation = "   gs-g";
-    const { gridMode$, setGridMode } = createLayoutModeSwitch(
-      scope,
-      behavior("n", { n: "normal" }),
-      cold(shareMarbles, { f: false, t: true }),
-    );
-    schedule(setModeMarbles, {
       g: () => setGridMode("grid"),
       s: () => setGridMode("spotlight"),
     });
-
+    expectObservable(gridMode$).toBe(expectedGridMode, {
-    expectObservable(gridMode$).toBe(expectation, {
       g: "grid",
       s: "spotlight",
     });
   });
+}
+
+describe("default mode", () => {
+  test("uses grid layout by default", () =>
+    testLayoutSwitch({
+      expectedGridMode: "g",
+    }));
+
+  test("uses spotlight mode when window mode is flat", () =>
+    testLayoutSwitch({
+      windowMode: "      f",
+      expectedGridMode: "s",
+    }));
 });
 
-test("Should auto-switch to spotlight when in flat window mode", () => {
+test("allows switching modes manually", () =>
-  withTestScheduler(({ cold, behavior, expectObservable }): void => {
+  testLayoutSwitch({
-    const { gridMode$ } = createLayoutModeSwitch(
+    userSelection: "   --sgs",
-      scope,
+    expectedGridMode: "g-sgs",
-      behavior("naf", { n: "normal", a: "narrow", f: "flat" }),
+  }));
-      cold("f", { f: false, t: true }),
-    );
 
-    expectObservable(gridMode$).toBe("g-s-", {
+test("switches to spotlight mode when there is a remote screen share", () =>
-      g: "grid",
+  testLayoutSwitch({
-      s: "spotlight",
+    hasScreenShares: " n--y",
-    });
+    expectedGridMode: "g--s",
-  });
+  }));
-});
+
+test("can manually switch to grid when there is a screenshare", () =>
+  testLayoutSwitch({
+    hasScreenShares: " n-y",
+    userSelection: "   ---g",
+    expectedGridMode: "g-sg",
+  }));
+
+test("auto-switches after manually selecting grid", () =>
+  testLayoutSwitch({
+    // Two screenshares will happen in sequence. There is a screen share that
+    // forces spotlight, then the user manually switches back to grid.
+    hasScreenShares: " n-y-ny",
+    userSelection: "   ---g",
+    expectedGridMode: "g-sg-s",
+    // If we did want to respect manual selection, the expectation would be: g-sg
+  }));
+
+test("switches back to grid mode when the remote screen share ends", () =>
+  testLayoutSwitch({
+    hasScreenShares: " n--y--n",
+    expectedGridMode: "g--s--g",
+  }));
+
+test("auto-switches to spotlight again after first screen share ends", () =>
+  testLayoutSwitch({
+    hasScreenShares: " nyny",
+    expectedGridMode: "gsgs",
+  }));
+
+test("switches manually to grid after screen share while manually in spotlight", () =>
+  testLayoutSwitch({
+    // Initially, no one is sharing. Then the user manually switches to spotlight.
+    // After a screen share starts, the user manually switches to grid.
+    hasScreenShares: " n-y",
+    userSelection: "   -s-g",
+    expectedGridMode: "gs-g",
+  }));
+
+test("auto-switches to spotlight when in flat window mode", () =>
+  testLayoutSwitch({
+    // First normal, then narrow, then flat.
+    windowMode: "      nNf",
+    expectedGridMode: "g-s",
+  }));
+
+test("allows switching modes manually when in flat window mode", () =>
+  testLayoutSwitch({
+    // Window becomes flat, then user switches to grid and back.
+    // Finally the window returns to a normal shape.
+    windowMode: "      nf--n",
+    userSelection: "   --gs",
+    expectedGridMode: "gsgsg",
+  }));
+
+test("stays in spotlight while there are screen shares even when window mode changes", () =>
+  testLayoutSwitch({
+    windowMode: "      nfn",
+    hasScreenShares: " y",
+    expectedGridMode: "s",
+  }));
+
+test("ignores end of screen share until window mode returns to normal", () =>
+  testLayoutSwitch({
+    windowMode: "      nf-n",
+    hasScreenShares: " y-n",
+    expectedGridMode: "s--g",
+  }));

src/state/CallViewModel/LayoutSwitch.ts

@@ -6,122 +6,85 @@ Please see LICENSE in the repository root for full details.
 */
 
 import {
-  BehaviorSubject,
   combineLatest,
   map,
-  type Observable,
+  Subject,
-  scan,
+  startWith,
+  skipWhile,
+  switchMap,
 } from "rxjs";
-import { logger } from "matrix-js-sdk/lib/logger";
 
 import { type GridMode, type WindowMode } from "./CallViewModel.ts";
-import { type Behavior } from "../Behavior.ts";
+import { constant, type Behavior } from "../Behavior.ts";
 import { type ObservableScope } from "../ObservableScope.ts";
 
 /**
  * Creates a layout mode switch that allows switching between grid and spotlight modes.
- * The actual layout mode can be overridden to spotlight mode if there is a remote screen share active
+ * The actual layout mode might switch automatically to spotlight if there is a
- * or if the window mode is flat.
+ * remote screen share active or if the window mode is flat.
  *
  * @param scope - The observable scope to manage subscriptions.
- * @param windowMode$ - The current window mode observable.
+ * @param windowMode$ - The current window mode.
- * @param hasRemoteScreenShares$ - An observable indicating if there are remote screen shares active.
+ * @param hasRemoteScreenShares$ - A behavior indicating if there are remote screen shares active.
  */
 export function createLayoutModeSwitch(
   scope: ObservableScope,
   windowMode$: Behavior<WindowMode>,
-  hasRemoteScreenShares$: Observable<boolean>,
+  hasRemoteScreenShares$: Behavior<boolean>,
 ): {
   gridMode$: Behavior<GridMode>;
   setGridMode: (value: GridMode) => void;
 } {
-  const gridModeUserSelection$ = new BehaviorSubject<GridMode>("grid");
+  const userSelection$ = new Subject<GridMode>();
-
   // Callback to set the grid mode desired by the user.
   // Notice that this is only a preference, the actual grid mode can be overridden
   // if there is a remote screen share active.
-  const setGridMode = (value: GridMode): void => {
+  const setGridMode = (value: GridMode): void => userSelection$.next(value);
-    gridModeUserSelection$.next(value);
+
-  };
+  /**
+   * The natural grid mode - the mode that the grid would prefer to be in,
+   * not accounting for the user's manual selections.
+   */
+  const naturalGridMode$ = scope.behavior<GridMode>(
+    combineLatest(
+      [hasRemoteScreenShares$, windowMode$],
+      (hasRemoteScreenShares, windowMode) =>
+        // When there are screen shares or the window is flat (as with a phone
+        // in landscape orientation), spotlight is a better experience.
+        // We want screen shares to be big and readable, and we want flipping
+        // your phone into landscape to be a quick way of maximising the
+        // spotlight tile.
+        hasRemoteScreenShares || windowMode === "flat" ? "spotlight" : "grid",
+    ),
+  );
+
   /**
    * The layout mode of the media tile grid.
    */
-  const gridMode$ =
+  const gridMode$ = scope.behavior<GridMode>(
-    // If the user hasn't selected spotlight and somebody starts screen sharing,
+    // Whenever the user makes a selection, we enter a new mode of behavior:
-    // automatically switch to spotlight mode and reset when screen sharing ends
+    userSelection$.pipe(
-    scope.behavior<GridMode>(
+      map((selection) => {
-      combineLatest([
+        if (selection === "grid")
-        gridModeUserSelection$,
+          // The user has selected grid mode. Start by respecting their choice,
-        hasRemoteScreenShares$,
+          // but then follow the natural mode again as soon as it matches.
-        windowMode$,
+          return naturalGridMode$.pipe(
-      ]).pipe(
+            skipWhile((naturalMode) => naturalMode !== selection),
-        // Scan to keep track if we have auto-switched already or not.
+            startWith(selection),
-        // To allow the user to override the auto-switch by selecting grid mode again.
+          );
-        scan<
-          [GridMode, boolean, WindowMode],
-          {
-            mode: GridMode;
-            /** Remember if the change was user driven or not */
-            hasAutoSwitched: boolean;
-            /** To know if it is new screen share or an already handled */
-            hasScreenShares: boolean;
-          }
-        >(
-          (prev, [userSelection, hasScreenShares, windowMode]) => {
-            const isFlatMode = windowMode === "flat";
 
-            // Always force spotlight in flat mode, grid layout is not supported
+        // The user has selected spotlight mode. If this matches the natural
-            // in that mode.
+        // mode, then follow the natural mode going forward.
-            // TODO: strange that we do that for flat mode but not for other modes?
+        return selection === naturalGridMode$.value
-            // TODO: Why is this not handled in layoutMedia$ like other window modes?
+          ? naturalGridMode$
-            if (isFlatMode) {
+          : constant(selection);
-              logger.debug(`Forcing spotlight mode, windowMode=${windowMode}`);
+      }),
-              return {
+      // Initially the mode of behavior is to just follow the natural grid mode.
-                mode: "spotlight",
+      startWith(naturalGridMode$),
-                hasAutoSwitched: prev.hasAutoSwitched,
+      // Switch between each mode of behavior.
-                hasScreenShares,
+      switchMap((mode$) => mode$),
-              };
+    ),
-            }
+  );
-
-            // User explicitly chose spotlight.
-            // Respect that choice.
-            if (userSelection === "spotlight") {
-              return {
-                mode: "spotlight",
-                hasAutoSwitched: prev.hasAutoSwitched,
-                hasScreenShares,
-              };
-            }
-
-            // User has chosen grid mode. If a screen share starts, we will
-            // auto-switch to spotlight mode for better experience.
-            // But we only do it once, if the user switches back to grid mode,
-            // we respect that choice until they explicitly change it again.
-            const isNewShare = hasScreenShares && !prev.hasScreenShares;
-            if (isNewShare && !prev.hasAutoSwitched) {
-              return {
-                mode: "spotlight",
-                hasAutoSwitched: true,
-                hasScreenShares: true,
-              };
-            }
-
-            // Respect user's grid choice
-            // XXX If we want to forbid switching automatically again after we can
-            // return hasAutoSwitched: acc.hasAutoSwitched here instead of setting to false.
-            return {
-              mode: "grid",
-              hasAutoSwitched: false,
-              hasScreenShares,
-            };
-          },
-          // initial value
-          { mode: "grid", hasAutoSwitched: false, hasScreenShares: false },
-        ),
-        map(({ mode }) => mode),
-      ),
-      "grid",
-    );
 
   return {
     gridMode$,