From 0c6e53cda43ef727a2ca0dc5ac4ce63242e6f112 Mon Sep 17 00:00:00 2001 From: Robin Date: Fri, 18 Oct 2024 17:51:37 -0400 Subject: [PATCH 1/2] Make the volume slider less silly Previously, dragging it all the way to the left would *not* mute the participant but rather bottom out at 10% volume, and people have found this unintuitive. Let's make it less silly by giving the slider a range of 0% to 100%, and making the mute toggle button have the same effect as dragging the slider to zero. When unmuting, it will reset to the last non-zero "committed" volume, similar to how the volume sliders in desktop environments work. --- src/Slider.tsx | 7 ++++ src/state/MediaViewModel.test.ts | 48 ++++++++++++----------- src/state/MediaViewModel.ts | 66 ++++++++++++++++++++++++-------- src/tile/GridTile.tsx | 5 ++- 4 files changed, 85 insertions(+), 41 deletions(-) diff --git a/src/Slider.tsx b/src/Slider.tsx index ba4fbeb5..750836f6 100644 --- a/src/Slider.tsx +++ b/src/Slider.tsx @@ -16,6 +16,7 @@ interface Props { label: string; value: number; onValueChange: (value: number) => void; + onValueCommit?: (value: number) => void; min: number; max: number; step: number; @@ -30,6 +31,7 @@ export const Slider: FC = ({ label, value, onValueChange: onValueChangeProp, + onValueCommit: onValueCommitProp, min, max, step, @@ -39,12 +41,17 @@ export const Slider: FC = ({ ([v]: number[]) => onValueChangeProp(v), [onValueChangeProp], ); + const onValueCommit = useCallback( + ([v]: number[]) => onValueCommitProp?.(v), + [onValueCommitProp], + ); return ( { +test("control a participant's volume", async () => { const setVolumeSpy = vi.fn(); await withRemoteMedia({}, { setVolume: setVolumeSpy }, (vm) => withTestScheduler(({ expectObservable, schedule }) => { - schedule("-a|", { - a() { - vm.setLocalVolume(0.8); - expect(setVolumeSpy).toHaveBeenLastCalledWith(0.8); - }, - }); - expectObservable(vm.localVolume).toBe("ab", { a: 1, b: 0.8 }); - }), - ); -}); - -test("mute and unmute a participant", async () => { - const setVolumeSpy = vi.fn(); - await withRemoteMedia({}, { setVolume: setVolumeSpy }, (vm) => - withTestScheduler(({ expectObservable, schedule }) => { - schedule("-abc|", { + schedule("-ab---c---d|", { a() { + // Try muting by toggling vm.toggleLocallyMuted(); expect(setVolumeSpy).toHaveBeenLastCalledWith(0); }, b() { + // Try unmuting by dragging the slider back up + vm.setLocalVolume(0.6); vm.setLocalVolume(0.8); - expect(setVolumeSpy).toHaveBeenLastCalledWith(0); + vm.commitLocalVolume(); + expect(setVolumeSpy).toHaveBeenCalledWith(0.6); + expect(setVolumeSpy).toHaveBeenLastCalledWith(0.8); }, c() { + // Try muting by dragging the slider back down + vm.setLocalVolume(0.2); + vm.setLocalVolume(0); + vm.commitLocalVolume(); + expect(setVolumeSpy).toHaveBeenCalledWith(0.2); + expect(setVolumeSpy).toHaveBeenLastCalledWith(0); + }, + d() { + // Try unmuting by toggling vm.toggleLocallyMuted(); + // The volume should return to the last non-zero committed volume expect(setVolumeSpy).toHaveBeenLastCalledWith(0.8); }, }); - expectObservable(vm.locallyMuted).toBe("ab-c", { - a: false, - b: true, - c: false, + expectObservable(vm.localVolume).toBe("ab(cd)(ef)g", { + a: 1, + b: 0, + c: 0.6, + d: 0.8, + e: 0.2, + f: 0, + g: 0.8, }); }), ); diff --git a/src/state/MediaViewModel.ts b/src/state/MediaViewModel.ts index 47344b3a..60e2a061 100644 --- a/src/state/MediaViewModel.ts +++ b/src/state/MediaViewModel.ts @@ -26,10 +26,12 @@ import { RoomMember, RoomMemberEvent } from "matrix-js-sdk/src/matrix"; import { BehaviorSubject, Observable, + Subject, combineLatest, distinctUntilKeyChanged, fromEvent, map, + merge, of, startWith, switchMap, @@ -39,6 +41,7 @@ import { useEffect } from "react"; import { ViewModel } from "./ViewModel"; import { useReactiveState } from "../useReactiveState"; import { alwaysShowSelf } from "../settings/settings"; +import { accumulate } from "../utils/observable"; // TODO: Move this naming logic into the view model export function useDisplayName(vm: MediaViewModel): string { @@ -232,18 +235,45 @@ export class LocalUserMediaViewModel extends BaseUserMediaViewModel { * A remote participant's user media. */ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { - private readonly _locallyMuted = new BehaviorSubject(false); - /** - * Whether we've disabled this participant's audio. - */ - public readonly locallyMuted: Observable = this._locallyMuted; + private readonly locallyMutedToggle = new Subject(); + private readonly localVolumeAdjustment = new Subject(); + private readonly localVolumeCommit = new Subject(); - private readonly _localVolume = new BehaviorSubject(1); /** - * The volume to which we've set this participant's audio, as a scalar + * The volume to which this participant's audio is set, as a scalar * multiplier. */ - public readonly localVolume: Observable = this._localVolume; + public readonly localVolume: Observable = merge( + this.locallyMutedToggle.pipe(map(() => "toggle mute" as const)), + this.localVolumeAdjustment, + this.localVolumeCommit.pipe(map(() => "commit" as const)), + ).pipe( + accumulate( + { muted: false, volume: 1, committedVolume: 1 }, + (state, event) => + event === "toggle mute" + ? { ...state, muted: !state.muted } + : event === "commit" + ? { ...state, committedVolume: state.volume } + : // Volume adjustment + event === 0 + ? // Dragging the slider to zero should have the same effect as + // muting: reset the volume to the committed volume, as if it were + // never dragged + { ...state, muted: true, volume: state.committedVolume } + : { ...state, muted: false, volume: event }, + ), + map(({ muted, volume }) => (muted ? 0 : volume)), + this.scope.state(), + ); + + /** + * Whether this participant's audio is disabled. + */ + public readonly locallyMuted: Observable = this.localVolume.pipe( + map((volume) => volume === 0), + this.scope.state(), + ); public constructor( id: string, @@ -253,22 +283,24 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { ) { super(id, member, participant, callEncrypted); - // Sync the local mute state and volume with LiveKit - combineLatest([this._locallyMuted, this._localVolume], (muted, volume) => - muted ? 0 : volume, - ) + // Sync the local volume with LiveKit + this.localVolume .pipe(this.scope.bind()) - .subscribe((volume) => { - (this.participant as RemoteParticipant).setVolume(volume); - }); + .subscribe((volume) => + (this.participant as RemoteParticipant).setVolume(volume), + ); } public toggleLocallyMuted(): void { - this._locallyMuted.next(!this._locallyMuted.value); + this.locallyMutedToggle.next(); } public setLocalVolume(value: number): void { - this._localVolume.next(value); + this.localVolumeAdjustment.next(value); + } + + public commitLocalVolume(): void { + this.localVolumeCommit.next(); } } diff --git a/src/tile/GridTile.tsx b/src/tile/GridTile.tsx index a46ff472..a85abe23 100644 --- a/src/tile/GridTile.tsx +++ b/src/tile/GridTile.tsx @@ -227,6 +227,7 @@ const RemoteUserMediaTile = forwardRef< (v: number) => vm.setLocalVolume(v), [vm], ); + const onCommitLocalVolume = useCallback(() => vm.commitLocalVolume(), [vm]); const VolumeIcon = locallyMuted ? VolumeOffIcon : VolumeOnIcon; @@ -250,10 +251,10 @@ const RemoteUserMediaTile = forwardRef< label={t("video_tile.volume")} value={localVolume} onValueChange={onChangeLocalVolume} - min={0.1} + onValueCommit={onCommitLocalVolume} + min={0} max={1} step={0.01} - disabled={locallyMuted} /> From d901045e55a29454cf69ed3ea7be734fff94ad2f Mon Sep 17 00:00:00 2001 From: Robin Date: Tue, 22 Oct 2024 17:23:40 -0400 Subject: [PATCH 2/2] Address review comments --- src/Slider.tsx | 6 ++++++ src/state/MediaViewModel.ts | 38 +++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/Slider.tsx b/src/Slider.tsx index 750836f6..aa9309e7 100644 --- a/src/Slider.tsx +++ b/src/Slider.tsx @@ -16,6 +16,12 @@ interface Props { label: string; value: number; onValueChange: (value: number) => void; + /** + * Event handler called when the value changes at the end of an interaction. + * Useful when you only need to capture a final value to update a backend + * service, or when you want to remember the last value that the user + * "committed" to. + */ onValueCommit?: (value: number) => void; min: number; max: number; diff --git a/src/state/MediaViewModel.ts b/src/state/MediaViewModel.ts index 60e2a061..26894f9e 100644 --- a/src/state/MediaViewModel.ts +++ b/src/state/MediaViewModel.ts @@ -248,22 +248,28 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel { this.localVolumeAdjustment, this.localVolumeCommit.pipe(map(() => "commit" as const)), ).pipe( - accumulate( - { muted: false, volume: 1, committedVolume: 1 }, - (state, event) => - event === "toggle mute" - ? { ...state, muted: !state.muted } - : event === "commit" - ? { ...state, committedVolume: state.volume } - : // Volume adjustment - event === 0 - ? // Dragging the slider to zero should have the same effect as - // muting: reset the volume to the committed volume, as if it were - // never dragged - { ...state, muted: true, volume: state.committedVolume } - : { ...state, muted: false, volume: event }, - ), - map(({ muted, volume }) => (muted ? 0 : volume)), + accumulate({ volume: 1, committedVolume: 1 }, (state, event) => { + switch (event) { + case "toggle mute": + return { + ...state, + volume: state.volume === 0 ? state.committedVolume : 0, + }; + case "commit": + // Dragging the slider to zero should have the same effect as + // muting: keep the original committed volume, as if it were never + // dragged + return { + ...state, + committedVolume: + state.volume === 0 ? state.committedVolume : state.volume, + }; + default: + // Volume adjustment + return { ...state, volume: event }; + } + }), + map(({ volume }) => volume), this.scope.state(), );