From 15a12b2d9c0eda53fd60a0be424b992965d6ba5d Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 15 Dec 2025 14:22:20 -0500 Subject: [PATCH 1/2] Make layout tests more concise --- src/state/CallViewModel/LayoutSwitch.test.ts | 267 ++++++------------- 1 file changed, 87 insertions(+), 180 deletions(-) diff --git a/src/state/CallViewModel/LayoutSwitch.test.ts b/src/state/CallViewModel/LayoutSwitch.test.ts index ae5a3896..a0d2d8c3 100644 --- a/src/state/CallViewModel/LayoutSwitch.test.ts +++ b/src/state/CallViewModel/LayoutSwitch.test.ts @@ -5,198 +5,105 @@ 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 { firstValueFrom, of } from "rxjs"; +import { describe, test } from "vitest"; import { createLayoutModeSwitch } from "./LayoutSwitch"; -import { ObservableScope } from "../ObservableScope"; -import { constant } from "../Behavior"; -import { withTestScheduler } from "../../utils/test"; +import { testScope, withTestScheduler } from "../../utils/test"; -let scope: ObservableScope; -beforeEach(() => { - scope = new ObservableScope(); -}); -afterEach(() => { - scope.end(); -}); - -describe("Default mode", () => { - test("Should be in grid layout by default", async () => { - const { gridMode$ } = createLayoutModeSwitch( - scope, - 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 => { +function testLayoutSwitch({ + windowMode = "n", + hasScreenShares = "n", + userSelection = "", + expectedGridMode, +}: { + windowMode?: string; + hasScreenShares?: string; + userSelection?: string; + expectedGridMode: string; +}): void { + withTestScheduler(({ behavior, schedule, expectObservable }) => { const { gridMode$, setGridMode } = createLayoutModeSwitch( - scope, - behavior("n", { n: "normal" }), - cold("f", { f: false, t: true }), + testScope(), + behavior(windowMode, { n: "normal", N: "narrow", f: "flat" }), + behavior(hasScreenShares, { y: true, n: false }), ); - - 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, { + schedule(userSelection, { g: () => setGridMode("grid"), s: () => setGridMode("spotlight"), }); - - expectObservable(gridMode$).toBe(expectation, { + expectObservable(gridMode$).toBe(expectedGridMode, { 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", () => { - withTestScheduler(({ cold, behavior, expectObservable }): void => { - const { gridMode$ } = createLayoutModeSwitch( - scope, - behavior("naf", { n: "normal", a: "narrow", f: "flat" }), - cold("f", { f: false, t: true }), - ); +test("allows switching modes manually", () => + testLayoutSwitch({ + userSelection: " --sgs", + expectedGridMode: "g-sgs", + })); - expectObservable(gridMode$).toBe("g-s-", { - g: "grid", - s: "spotlight", - }); - }); -}); +test("switches to spotlight mode when there is a remote screen share", () => + testLayoutSwitch({ + 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", + })); From 53cc79f7387d5cce44e5b138fbdf7b5668196410 Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 15 Dec 2025 14:33:00 -0500 Subject: [PATCH 2/2] Allow user to switch layouts while phone is in landscape This fixes a regression on the development branch: the layout switcher would not respond to input while the window mode is 'flat' (i.e. while a mobile phone is in landscape orientation). See https://github.com/element-hq/element-call/pull/3605#discussion_r2586226422 for more context. I was having a little trouble interpreting the emergent behavior of the layout switching code, so I refactored it in the process into a form that I think is a more direct description of the behavior we want (while not making it as terse as my original implementation). --- src/state/CallViewModel/CallViewModel.ts | 9 +- src/state/CallViewModel/LayoutSwitch.test.ts | 23 +++ src/state/CallViewModel/LayoutSwitch.ts | 143 +++++++------------ 3 files changed, 81 insertions(+), 94 deletions(-) diff --git a/src/state/CallViewModel/CallViewModel.ts b/src/state/CallViewModel/CallViewModel.ts index aac88a3b..e2e3924b 100644 --- a/src/state/CallViewModel/CallViewModel.ts +++ b/src/state/CallViewModel/CallViewModel.ts @@ -946,11 +946,12 @@ export function createCallViewModel$( ), ); - const hasRemoteScreenShares$: Observable = spotlight$.pipe( - map((spotlight) => - spotlight.some((vm) => !vm.local && vm instanceof ScreenShareViewModel), + const hasRemoteScreenShares$ = scope.behavior( + spotlight$.pipe( + map((spotlight) => + spotlight.some((vm) => !vm.local && vm instanceof ScreenShareViewModel), + ), ), - distinctUntilChanged(), ); const pipEnabled$ = scope.behavior(setPipEnabled$, false); diff --git a/src/state/CallViewModel/LayoutSwitch.test.ts b/src/state/CallViewModel/LayoutSwitch.test.ts index a0d2d8c3..0d184017 100644 --- a/src/state/CallViewModel/LayoutSwitch.test.ts +++ b/src/state/CallViewModel/LayoutSwitch.test.ts @@ -107,3 +107,26 @@ test("auto-switches to spotlight when in flat window mode", () => 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", + })); diff --git a/src/state/CallViewModel/LayoutSwitch.ts b/src/state/CallViewModel/LayoutSwitch.ts index 3ad93204..97a4ee6f 100644 --- a/src/state/CallViewModel/LayoutSwitch.ts +++ b/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, - scan, + Subject, + 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 - * or if the window mode is flat. + * The actual layout mode might switch automatically to spotlight if there is a + * 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 hasRemoteScreenShares$ - An observable indicating if there are remote screen shares active. + * @param windowMode$ - The current window mode. + * @param hasRemoteScreenShares$ - A behavior indicating if there are remote screen shares active. */ export function createLayoutModeSwitch( scope: ObservableScope, windowMode$: Behavior, - hasRemoteScreenShares$: Observable, + hasRemoteScreenShares$: Behavior, ): { gridMode$: Behavior; setGridMode: (value: GridMode) => void; } { - const gridModeUserSelection$ = new BehaviorSubject("grid"); - + const userSelection$ = new Subject(); // 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 => { - gridModeUserSelection$.next(value); - }; + const setGridMode = (value: GridMode): void => userSelection$.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( + 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$ = - // If the user hasn't selected spotlight and somebody starts screen sharing, - // automatically switch to spotlight mode and reset when screen sharing ends - scope.behavior( - combineLatest([ - gridModeUserSelection$, - hasRemoteScreenShares$, - windowMode$, - ]).pipe( - // Scan to keep track if we have auto-switched already or not. - // 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"; + const gridMode$ = scope.behavior( + // Whenever the user makes a selection, we enter a new mode of behavior: + userSelection$.pipe( + map((selection) => { + if (selection === "grid") + // The user has selected grid mode. Start by respecting their choice, + // but then follow the natural mode again as soon as it matches. + return naturalGridMode$.pipe( + skipWhile((naturalMode) => naturalMode !== selection), + startWith(selection), + ); - // Always force spotlight in flat mode, grid layout is not supported - // in that mode. - // TODO: strange that we do that for flat mode but not for other modes? - // TODO: Why is this not handled in layoutMedia$ like other window modes? - if (isFlatMode) { - logger.debug(`Forcing spotlight mode, windowMode=${windowMode}`); - return { - mode: "spotlight", - hasAutoSwitched: prev.hasAutoSwitched, - hasScreenShares, - }; - } - - // 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", - ); + // The user has selected spotlight mode. If this matches the natural + // mode, then follow the natural mode going forward. + return selection === naturalGridMode$.value + ? naturalGridMode$ + : constant(selection); + }), + // Initially the mode of behavior is to just follow the natural grid mode. + startWith(naturalGridMode$), + // Switch between each mode of behavior. + switchMap((mode$) => mode$), + ), + ); return { gridMode$,