From 88f660e43f154509f2e5a9de9ed55b0c1f16d3e0 Mon Sep 17 00:00:00 2001 From: Timo K Date: Fri, 15 May 2026 18:37:25 +0200 Subject: [PATCH] review --- locales/en/app.json | 1 + src/components/CallFooter.module.css | 4 + src/components/CallFooter.stories.tsx | 21 +++- src/components/CallFooter.tsx | 139 ++++++++++++--------- src/components/CallFooterViewModel.test.ts | 9 +- src/components/CallFooterViewModel.tsx | 42 +++++-- src/room/InCallView.test.tsx | 21 ++-- src/room/InCallView.tsx | 28 ++--- src/settings/DeviceSelection.tsx | 2 +- src/state/CallViewModel/CallViewModel.ts | 14 +++ src/state/ViewModel.ts | 28 +++-- vitest.config.ts | 1 + 12 files changed, 192 insertions(+), 118 deletions(-) diff --git a/locales/en/app.json b/locales/en/app.json index 1d5eaa19..b51c6ed9 100644 --- a/locales/en/app.json +++ b/locales/en/app.json @@ -202,6 +202,7 @@ "camera_numbered": "Camera {{n}}", "change_device_button": "Change audio device", "default": "Default", + "default_named": "Default <2>({{name}})", "handset": "Handset", "loudspeaker": "Loudspeaker", "microphone": "Microphone", diff --git a/src/components/CallFooter.module.css b/src/components/CallFooter.module.css index 228d7654..adff99d5 100644 --- a/src/components/CallFooter.module.css +++ b/src/components/CallFooter.module.css @@ -26,6 +26,10 @@ Please see LICENSE in the repository root for full details. ); } +.footer.hidden { + display: none; +} + .footer.overlay { /* Note that the footer is still position: sticky in this case so that certain tiles can move up out of the way of the footer when visible. */ diff --git a/src/components/CallFooter.stories.tsx b/src/components/CallFooter.stories.tsx index 88910bfd..14f82506 100644 --- a/src/components/CallFooter.stories.tsx +++ b/src/components/CallFooter.stories.tsx @@ -13,7 +13,7 @@ import { Link } from "@vector-im/compound-web"; import type { Meta, StoryObj } from "@storybook/react-vite"; import { CallFooter, type FooterSnapshot } from "./CallFooter"; import inCallViewStyles from "../room/InCallView.module.css"; -import { createStaticViewModel } from "../state/ViewModel"; +import { useStaticViewModel } from "../state/ViewModel"; import { ReactionsSenderContext } from "../reactions/useReactionsSender"; import { type ReactionOption } from "../reactions"; import { type GridMode } from "../state/CallViewModel/CallViewModel"; @@ -39,7 +39,7 @@ function CallFooterStoryWrapper({ }: FooterSnapshot & { children?: false | JSX.Element | JSX.Element[] | undefined; }): ReactNode { - const vm = createStaticViewModel(vmSnapshot); + const vm = useStaticViewModel(vmSnapshot); return (
void; } -export interface FooterSnapshot { - audioEnabled: boolean; +/** + * The Snapshot combines all fields required to populate the view. + * + * It is a combination of Actions and State. + * All Actions and State will be wrappen in behaviors. + * This has the advantage, that actions can mutate. + * (example: a device gets disconnected, the swicht action is not possible anymore, the actions becomes undefined) + * With it being reactive we can use the existance of the action to update the rendering without + * requiring additional state. + * + * Comment: It might not make sense to seperate the two interfaces. Hence the seperation + * just happens on the syntax level with the `type = ... & ...` notation. + */ +export type FooterSnapshot = FooterActions & FooterState; +export interface FooterActions { /** Also controls if the audioMute button is disabled */ toggleAudio: (() => void) | undefined; - - videoEnabled: boolean; /** Also controls if the videoMute button is disabled */ toggleVideo: (() => void) | undefined; + /** Also controls if the layout button is visible */ + setLayoutMode: ((mode: GridMode) => void) | undefined; + toggleScreenSharing: (() => void) | undefined; + /** Also controls if the settings button is visible */ + openSettings: (() => void) | undefined; + /** Also controls if the hangup button is visible */ + hangup: (() => void) | undefined; +} +// we do not use any ? optional properties so that the vm type is including all fields. +export interface FooterState { + audioEnabled: boolean; + videoEnabled: boolean; + showFooter: boolean; /* This is needed for WindowMode = "flat" */ - hideControls?: boolean; + hideControls: boolean; /** The footer should be used as an overlay. * (Over the Call Grid) This saves spaces on small screens. */ - asOverlay?: boolean; + asOverlay: boolean; buttonSize: "md" | "lg"; - showSettingsButton?: boolean; - showLayoutSwitcher?: boolean; - showLogo?: boolean; + showSettingsButton: boolean; + showLayoutSwitcher: boolean; + showLogo: boolean; - layoutMode?: GridMode; - /** Also controls if the layout button is visible */ - setLayoutMode?: (mode: GridMode) => void; + layoutMode: GridMode | undefined; - sharingScreen?: boolean; - toggleScreenSharing?: () => void; + sharingScreen: boolean; /** Also controls if the audio output button is visible */ - audioOutputSwitcher?: AudioOutputSwitcher; - /** Also controls if the settings button is visible */ - openSettings?: () => void; - /** Also controls if the hangup button is visible */ - hangup?: () => void; + audioOutputSwitcher: AudioOutputSwitcher | undefined; - reactionIdentifier?: string; - reactionData?: ReactionData; + reactionIdentifier: string | undefined; + reactionData: ReactionData | undefined; // debug stuff - debugTileLayout?: boolean; - tileStoreGeneration?: number; + debugTileLayout: boolean; + tileStoreGeneration: number | undefined; /** Providing no options `[]` or `undefined` will imply that we dont have a audio fast switcher */ - audioOptions?: MenuOptions[]; + audioOptions: MenuOptions[]; /** Providing no options `[]` or `undefined` will imply that we dont have a audio fast switcher */ - videoOptions?: MenuOptions[]; - selectedAudio?: string; - selectedVideo?: string; - selectAudioButtonOption?: (deviceId: string) => void; - selectVideoButtonOption?: (option: string) => void; - videoToggles?: ToggleOption[]; + videoOptions: MenuOptions[]; + selectedAudio: string | undefined; + selectedVideo: string | undefined; + selectAudioButtonOption: ((deviceId: string) => void) | undefined; + selectVideoButtonOption: ((option: string) => void) | undefined; + videoToggles: ToggleOption[]; } export interface FooterProps { @@ -99,35 +117,34 @@ export interface FooterProps { vm: ViewModel; } export const CallFooter: FC = ({ ref, children, vm }) => { - const { - asOverlay, - hideControls, - layoutMode, - setLayoutMode, - openSettings, - audioEnabled, - videoEnabled, - toggleAudio, - toggleVideo, - sharingScreen, - toggleScreenSharing, - reactionIdentifier, - reactionData, - audioOutputSwitcher, - hangup, - debugTileLayout, - tileStoreGeneration, - videoOptions, - selectedVideo, - audioOptions, - selectedAudio, - selectAudioButtonOption, - selectVideoButtonOption, - videoToggles, - buttonSize, - showSettingsButton, - showLogo, - } = useViewModel(vm); + const asOverlay = useBehavior(vm.asOverlay$); + const showFooter = useBehavior(vm.showFooter$); + const hideControls = useBehavior(vm.hideControls$); + const layoutMode = useBehavior(vm.layoutMode$); + const setLayoutMode = useBehavior(vm.setLayoutMode$); + const openSettings = useBehavior(vm.openSettings$); + const audioEnabled = useBehavior(vm.audioEnabled$); + const videoEnabled = useBehavior(vm.videoEnabled$); + const toggleAudio = useBehavior(vm.toggleAudio$); + const toggleVideo = useBehavior(vm.toggleVideo$); + const sharingScreen = useBehavior(vm.sharingScreen$); + const toggleScreenSharing = useBehavior(vm.toggleScreenSharing$); + const reactionIdentifier = useBehavior(vm.reactionIdentifier$); + const reactionData = useBehavior(vm.reactionData$); + const audioOutputSwitcher = useBehavior(vm.audioOutputSwitcher$); + const hangup = useBehavior(vm.hangup$); + const debugTileLayout = useBehavior(vm.debugTileLayout$); + const tileStoreGeneration = useBehavior(vm.tileStoreGeneration$); + const videoOptions = useBehavior(vm.videoOptions$); + const selectedVideo = useBehavior(vm.selectedVideo$); + const audioOptions = useBehavior(vm.audioOptions$); + const selectedAudio = useBehavior(vm.selectedAudio$); + const selectAudioButtonOption = useBehavior(vm.selectAudioButtonOption$); + const selectVideoButtonOption = useBehavior(vm.selectVideoButtonOption$); + const videoToggles = useBehavior(vm.videoToggles$); + const buttonSize = useBehavior(vm.buttonSize$); + const showSettingsButton = useBehavior(vm.showSettingsButton$); + const showLogo = useBehavior(vm.showLogo$); const buttons: JSX.Element[] = []; @@ -267,8 +284,10 @@ export const CallFooter: FC = ({ ref, children, vm }) => { return (
diff --git a/src/components/CallFooterViewModel.test.ts b/src/components/CallFooterViewModel.test.ts index d07f9245..99015162 100644 --- a/src/components/CallFooterViewModel.test.ts +++ b/src/components/CallFooterViewModel.test.ts @@ -47,6 +47,9 @@ function buildMinimalCallViewModel(layout: Layout): CallViewModel { handsRaised$: constant({}), reactions$: constant({}), tileStoreGeneration$: constant(0), + showFooter$: constant(true), + settingsOpen$: constant(false), + setSettingsOpen$: constant(() => {}), } as unknown as CallViewModel; } @@ -95,12 +98,11 @@ describe("createCallFooterViewModel", () => { buildMinimalCallViewModel(layout), mockMuteStates(), twoMicsAndOneCamMediaDevices, - /* openSettings */ undefined, /* reactionIdentifier */ undefined, ); - expect(vm.audioOptions$?.value).toEqual([]); - expect(vm.videoOptions$?.value).toEqual([]); + expect(vm.audioOptions$.value).toEqual([]); + expect(vm.videoOptions$.value).toEqual([]); } it("are empty when both the platform is iOS", () => { checkEmptyFor("ios", gridLayout); @@ -117,7 +119,6 @@ describe("createCallFooterViewModel", () => { buildMinimalCallViewModel(gridLayout), mockMuteStates(), twoMicsAndOneCamMediaDevices, - /* openSettings */ undefined, /* reactionIdentifier */ undefined, ); diff --git a/src/components/CallFooterViewModel.tsx b/src/components/CallFooterViewModel.tsx index 6d1d7acf..c2cacd8c 100644 --- a/src/components/CallFooterViewModel.tsx +++ b/src/components/CallFooterViewModel.tsx @@ -159,8 +159,6 @@ function buildDeviceBehaviors( * @param callModel - The root CallViewModel; provides layout, grid mode, reactions, etc. * @param muteStates - Audio and video mute state + toggles. * @param mediaDevices - Available and selected input devices. - * @param openSettings - Callback to open the settings modal, or undefined if the - * settings button should be hidden (e.g. when it is already shown in an app bar). * @param reactionIdentifier - The local user's reaction identifier string, or * undefined when reactions are not supported (hides the reaction button). */ @@ -169,7 +167,6 @@ export function createCallFooterViewModel( callModel: CallViewModel, muteStates: MuteStates, mediaDevices: MediaDevices, - openSettings: (() => void) | undefined, reactionIdentifier: string | undefined, ): ViewModel { const { showControls, header: headerStyle } = getUrlParams(); @@ -184,7 +181,8 @@ export function createCallFooterViewModel( return { ...buildMuteBehaviors(scope, muteStates), ...buildDeviceBehaviors(scope, mediaDevices, disableDeviceSwitcher$), - + // candidat to move into the FooterViewModel + showFooter$: callModel.showFooter$, hideControls$: constant(!showControls), asOverlay$: scope.behavior( callModel.windowMode$.pipe(map((mode) => mode === "flat")), @@ -193,10 +191,14 @@ export function createCallFooterViewModel( isPip$.pipe(map((pip) => (pip ? "md" : "lg") as "md" | "lg")), ), showSettingsButton$: scope.behavior( - combineLatest([isPip$, callModel.showHeader$]).pipe( + combineLatest([ + isPip$, + callModel.showHeader$, + callModel.settingsOpen$, + ]).pipe( map( - ([isPip, showHeader]) => - openSettings !== undefined && + ([isPip, showHeader, settingsOpen]) => + settingsOpen !== undefined && !isPip && showControls && !(headerStyle === HeaderStyle.AppBar && showHeader), @@ -221,11 +223,11 @@ export function createCallFooterViewModel( ), openSettings$: scope.behavior( - callModel.showHeader$.pipe( - map((showHeader) => + combineLatest([callModel.showHeader$, callModel.setSettingsOpen$]).pipe( + map(([showHeader, setSettingsOpen]) => headerStyle === HeaderStyle.AppBar && showHeader ? undefined - : openSettings, + : (): void => setSettingsOpen(true), ), ), ), @@ -281,6 +283,26 @@ export function createLobbyFooterViewModel( hangup, debugTileLayout: false, showSettingsButton: openSettings !== undefined, + showFooter: true, + toggleAudio: undefined, + toggleVideo: undefined, + setLayoutMode: undefined, + toggleScreenSharing: undefined, + audioEnabled: undefined, + videoEnabled: undefined, + layoutMode: undefined, + sharingScreen: false, + audioOutputSwitcher: undefined, + reactionIdentifier: undefined, + reactionData: undefined, + tileStoreGeneration: undefined, + audioOptions: undefined, + videoOptions: undefined, + selectedAudio: undefined, + selectedVideo: undefined, + selectAudioButtonOption: undefined, + selectVideoButtonOption: undefined, + videoToggles: undefined, }), ...buildMuteBehaviors(scope, muteStates), ...buildDeviceBehaviors(scope, mediaDevices, constant(false)), diff --git a/src/room/InCallView.test.tsx b/src/room/InCallView.test.tsx index cf4c2d92..0b770715 100644 --- a/src/room/InCallView.test.tsx +++ b/src/room/InCallView.test.tsx @@ -200,7 +200,7 @@ describe("InCallView", () => { it("mobile landscape, is accessible when showHeader is false", () => { // windowSize with height <= 600 results in "flat" windowMode, // which means showHeader$ emits false. - const { getAllByRole, queryAllByRole, vm } = createInCallView({ + const { getAllByRole, getByRole, getByTestId, vm } = createInCallView({ withAppBar: true, callViewModelOptions: { // Set windowMode$ to "flat" (height <= 600) @@ -210,7 +210,12 @@ describe("InCallView", () => { // In flat (landscape) mode the footer starts hidden until the user // taps the screen, so no settings button should be accessible yet. - expect(queryAllByRole("button", { name: "Settings" })).toHaveLength(0); + + expect(getByTestId("footer-container")).not.toBeVisible(); + const buttons = getAllByRole("button", { name: "Settings" }); + for (const b of buttons) { + expect(b).not.toBeVisible(); + } // Simulate a touch tap on the call view to reveal the footer. // (PointerEvent is not available in JSDOM, so we call tapScreen() directly, @@ -219,17 +224,15 @@ describe("InCallView", () => { // When showHeader is false, hideSettingsButton is false, // so the settings button is visible in the footer. - const settingsBtn = getAllByRole("button", { name: "Settings" }); - // here we check for two settings buttons because there are two buttons in the bottom bar. One for the + const settingsBtn = getByRole("button", { name: "Settings" }); + // There are two buttons in the bottom bar. One for the // the narrow layout and another one for the wide layout. - // Their visibility uses @media css queries, which cannot be tested in JSDOM, - // but we can at least check that both buttons are rendered and have the correct classes. - expect(settingsBtn.length).toBe(2); - expect(settingsBtn[0]).toHaveAttribute( + // Their visibility uses @media css queries, which we can test JSDOM (see `test.css.include` vitest config). + expect(settingsBtn).toHaveAttribute( "data-testid", "settings-bottom-left", ); - expect(settingsBtn[0]).toBeVisible(); + expect(settingsBtn).toBeVisible(); }); it("mobile portrait, is accessible when showHeader is true", () => { diff --git a/src/room/InCallView.tsx b/src/room/InCallView.tsx index 932bb40d..74c369af 100644 --- a/src/room/InCallView.tsx +++ b/src/room/InCallView.tsx @@ -237,7 +237,8 @@ export const InCallView: FC = ({ const windowMode = useBehavior(vm.windowMode$); const layout = useBehavior(vm.layout$); const showHeader = useBehavior(vm.showHeader$); - const showFooter = useBehavior(vm.showFooter$); + const settingsOpen = useBehavior(vm.settingsOpen$); + const setSettingsOpen = useBehavior(vm.setSettingsOpen$); const earpieceMode = useBehavior(vm.earpieceMode$); const audioOutputSwitcher = useBehavior(vm.audioOutputSwitcher$); @@ -284,28 +285,18 @@ export const InCallView: FC = ({ ); const onPointerOut = useCallback(() => vm.unhoverScreen(), [vm]); - const [settingsModalOpen, setSettingsModalOpen] = useState(false); const [settingsTab, setSettingsTab] = useState(defaultSettingsTab); - const openSettings = useCallback( - () => setSettingsModalOpen(true), - [setSettingsModalOpen], - ); - const closeSettings = useCallback( - () => setSettingsModalOpen(false), - [setSettingsModalOpen], - ); - const openProfile = useMemo( () => // Profile settings are unavailable in widget mode widget === null ? (): void => { setSettingsTab("profile"); - setSettingsModalOpen(true); + setSettingsOpen(true); } : null, - [setSettingsTab, setSettingsModalOpen], + [setSettingsTab, setSettingsOpen], ); const [headerRef, headerBounds] = useMeasure(); @@ -555,7 +546,6 @@ export const InCallView: FC = ({ vm, muteStates, mediaDevices, - openSettings, supportsReactions ? `${client.getUserId()}:${client.getDeviceId()}` : undefined, @@ -564,19 +554,19 @@ export const InCallView: FC = ({ return (): void => { footerScope.end(); }; - }, [client, mediaDevices, muteStates, openSettings, supportsReactions, vm]); + }, [client, mediaDevices, muteStates, supportsReactions, vm]); useAppBarSecondaryButton( setSettingsOpen(true)} data-testid="settings-app-bar" />, ); // Only hide the settings button if we have an AppBar header and we are showing the header const footer = footerVm !== null && ( - <>{showFooter && } + ); const allConnections = useBehavior(vm.allConnections$); @@ -614,8 +604,8 @@ export const InCallView: FC = ({ setSettingsOpen(false)} tab={settingsTab} onTabChange={setSettingsTab} livekitRooms={allConnections diff --git a/src/settings/DeviceSelection.tsx b/src/settings/DeviceSelection.tsx index f189348b..f5cb8584 100644 --- a/src/settings/DeviceSelection.tsx +++ b/src/settings/DeviceSelection.tsx @@ -46,7 +46,7 @@ export function mediaDeviceLabelToString( labelText = label.name === null ? t("settings.devices.default") - : t("settings.devices.default") + " (" + label.name + ")"; + : t("settings.devices.default_named", { name: label.name }); break; case "speaker": labelText = t("settings.devices.loudspeaker"); diff --git a/src/state/CallViewModel/CallViewModel.ts b/src/state/CallViewModel/CallViewModel.ts index e298bcfd..bd1ed5f3 100644 --- a/src/state/CallViewModel/CallViewModel.ts +++ b/src/state/CallViewModel/CallViewModel.ts @@ -15,6 +15,7 @@ import { } from "livekit-client"; import { type Room as MatrixRoom } from "matrix-js-sdk"; import { + BehaviorSubject, catchError, combineLatest, distinctUntilChanged, @@ -352,6 +353,9 @@ export interface CallViewModel { showHeader$: Behavior; showFooter$: Behavior; + settingsOpen$: Behavior; + setSettingsOpen$: Behavior<(open: boolean) => void>; + // audio routing /** * Whether audio is currently being output through the earpiece. @@ -1332,6 +1336,7 @@ export function createCallViewModel$( const showFooterUrlParams = !( urlParams.header === HeaderStyle.None && urlParams.showControls === false ); + // candidat to move into the FooterViewModel const showFooterLayout$ = scope.behavior( windowMode$.pipe( switchMap((mode) => { @@ -1386,11 +1391,18 @@ export function createCallViewModel$( }), ), ); + // candidat to move into the FooterViewModel const showFooter$ = scope.behavior( showFooterLayout$.pipe( map((showFooter) => showFooter && showFooterUrlParams), ), ); + + const settingsOpen$ = new BehaviorSubject(false); + const setSettingsOpen$ = constant((open: boolean) => { + settingsOpen$.next(open); + }); + /** * Whether audio is currently being output through the earpiece. */ @@ -1622,6 +1634,8 @@ export function createCallViewModel$( showSpeakingIndicators$: showSpeakingIndicators$, showHeader$: showHeader$, showFooter$: showFooter$, + settingsOpen$: settingsOpen$, + setSettingsOpen$: setSettingsOpen$, earpieceMode$: earpieceMode$, audioOutputSwitcher$: audioOutputSwitcher$, reconnecting$: localMembership.reconnecting$, diff --git a/src/state/ViewModel.ts b/src/state/ViewModel.ts index 2436d520..9a0a5d2e 100644 --- a/src/state/ViewModel.ts +++ b/src/state/ViewModel.ts @@ -6,26 +6,14 @@ Please see LICENSE in the repository root for full details. */ import { BehaviorSubject } from "rxjs"; +import { useState, useEffect } from "react"; -import { useBehavior } from "../useBehavior"; import { type Behavior } from "./Behavior"; export type ViewModel = { [K in keyof Snapshot as `${string & K}$`]: Behavior; }; -export function useViewModel(vm: ViewModel): Snapshot { - const snapshot = {} as Snapshot; - for (const key in vm) { - const value$ = (vm as Record>)[key]; - const snapshotKey = key.slice(0, -1) as keyof Snapshot; - // we allow using hooks in a loop here because we know the shape of the vm is static and won't change between renders, so the order of hooks calls will always be the same. - // eslint-disable-next-line react-hooks/rules-of-hooks - snapshot[snapshotKey] = useBehavior(value$) as Snapshot[keyof Snapshot]; - } - return snapshot; -} - /** * This allows to build a view model (or Partial view model) * with BehaviorSubjects. @@ -45,3 +33,17 @@ export function createStaticViewModel( } return vm; } + +export function useStaticViewModel( + snapshot: Snapshot, +): ViewModel { + const [vm] = useState(createStaticViewModel(snapshot)); + useEffect(() => { + for (const key in snapshot) { + (vm as unknown as Record>)[ + `${key}$` + ].next(snapshot[key]); + } + }, [snapshot, vm]); + return vm; +} diff --git a/vitest.config.ts b/vitest.config.ts index 22b5e88a..074a57f4 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -19,6 +19,7 @@ export default defineConfig((configEnv) => test: { name: "unit", css: { + include: /.+/, modules: { classNameStrategy: "non-scoped", },