Merge pull request #2681 from robintown/volume-slider

Make the volume slider less silly

src/Slider.tsx

@@ -16,6 +16,13 @@ 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;
   step: number;
@@ -30,6 +37,7 @@ export const Slider: FC<Props> = ({
   label,
   value,
   onValueChange: onValueChangeProp,
+  onValueCommit: onValueCommitProp,
   min,
   max,
   step,
@@ -39,12 +47,17 @@ export const Slider: FC<Props> = ({
     ([v]: number[]) => onValueChangeProp(v),
     [onValueChangeProp],
   );
+  const onValueCommit = useCallback(
+    ([v]: number[]) => onValueCommitProp?.(v),
+    [onValueCommitProp],
+  );
 
   return (
     <Root
       className={classNames(className, styles.slider)}
       value={[value]}
       onValueChange={onValueChange}
+      onValueCommit={onValueCommit}
       min={min}
       max={max}
       step={step}

src/state/MediaViewModel.test.ts

@@ -13,43 +13,47 @@ import {
   withTestScheduler,
 } from "../utils/test";
 
-test("set a participant's volume", async () => {
+test("control a participant's volume", async () => {
   const setVolumeSpy = vi.fn();
   await withRemoteMedia({}, { setVolume: setVolumeSpy }, (vm) =>
     withTestScheduler(({ expectObservable, schedule }) => {
-      schedule("-a|", {
+      schedule("-ab---c---d|", {
-        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|", {
         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", {
+      expectObservable(vm.localVolume).toBe("ab(cd)(ef)g", {
-        a: false,
+        a: 1,
-        b: true,
+        b: 0,
-        c: false,
+        c: 0.6,
+        d: 0.8,
+        e: 0.2,
+        f: 0,
+        g: 0.8,
       });
     }),
   );

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,51 @@ export class LocalUserMediaViewModel extends BaseUserMediaViewModel {
  * A remote participant's user media.
  */
 export class RemoteUserMediaViewModel extends BaseUserMediaViewModel {
-  private readonly _locallyMuted = new BehaviorSubject(false);
+  private readonly locallyMutedToggle = new Subject<void>();
-  /**
+  private readonly localVolumeAdjustment = new Subject<number>();
-   * Whether we've disabled this participant's audio.
+  private readonly localVolumeCommit = new Subject<void>();
-   */
-  public readonly locallyMuted: Observable<boolean> = this._locallyMuted;
 
-  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<number> = this._localVolume;
+  public readonly localVolume: Observable<number> = merge(
+    this.locallyMutedToggle.pipe(map(() => "toggle mute" as const)),
+    this.localVolumeAdjustment,
+    this.localVolumeCommit.pipe(map(() => "commit" as const)),
+  ).pipe(
+    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(),
+  );
+
+  /**
+   * Whether this participant's audio is disabled.
+   */
+  public readonly locallyMuted: Observable<boolean> = this.localVolume.pipe(
+    map((volume) => volume === 0),
+    this.scope.state(),
+  );
 
   public constructor(
     id: string,
@@ -253,22 +289,24 @@ export class RemoteUserMediaViewModel extends BaseUserMediaViewModel {
   ) {
     super(id, member, participant, callEncrypted);
 
-    // Sync the local mute state and volume with LiveKit
+    // Sync the local volume with LiveKit
-    combineLatest([this._locallyMuted, this._localVolume], (muted, volume) =>
+    this.localVolume
-      muted ? 0 : volume,
-    )
       .pipe(this.scope.bind())
-      .subscribe((volume) => {
+      .subscribe((volume) =>
-        (this.participant as RemoteParticipant).setVolume(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();
   }
 }
 

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}
             />
           </MenuItem>
         </>