Fix duration of voice message in timeline (#30973)

* fix: duration of voice message in timeline

* Revert "Fix clocks rendering at 00:00 when playback had not begun."

This reverts commit 68bcfbed3e37e0a8c9529e92a3e17a5bc70bf7ed.

* refactor: cleaner clock states check

* refactor: cleaner `onPlaybackStateChange` condition

* fix: `timeSeconds` is always a number

* refactor: allow playing and paused state to update clock state

* test: add test

* test: add moar test

* refactor: use `currentClockState`

src/audio/PlaybackClock.ts

@@ -74,7 +74,7 @@ export class PlaybackClock implements IDestroyable {
         // remainder of the division operation, we're assuming that playback is
         // incomplete or stopped, thus giving an accurate position within the active
         // clip segment.
-        return (this.context.currentTime - this.clipStart) % this.clipDuration;
+        return (this.context.currentTime - this.clipStart) % this.clipDuration || 0;
     }
 
     public get liveData(): SimpleObservable<number[]> {

src/audio/PlaybackQueue.ts

@@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
 Please see LICENSE files in the repository root for full details.
 */
 
-import { type MatrixEvent, type Room, EventType } from "matrix-js-sdk/src/matrix";
+import { EventType, type MatrixEvent, type Room } from "matrix-js-sdk/src/matrix";
 import { logger } from "matrix-js-sdk/src/logger";
 
 import { type Playback, PlaybackState } from "./Playback";
@@ -76,6 +76,12 @@ export class PlaybackQueue {
         const val = localStorage.getItem(`mx_voice_message_clocks_${this.room.roomId}`);
         if (!!val) {
             this.clockStates = new Map<string, number>(JSON.parse(val));
+            // Clean out any null values (from older versions)
+            for (const [key, value] of this.clockStates.entries()) {
+                if (value == null) {
+                    this.clockStates.delete(key);
+                }
+            }
         }
     }
 
@@ -90,14 +96,10 @@ export class PlaybackQueue {
         // Remember where the user got to in playback
         const wasLastPlaying = this.currentPlaybackId === mxEvent.getId();
         const currentClockState = this.clockStates.get(mxEvent.getId()!);
+
         if (newState === PlaybackState.Stopped && currentClockState !== undefined && !wasLastPlaying) {
-            if (currentClockState > 0) {
+            // noinspection JSIgnoredPromiseFromCall
-                // skipTo will pause playback, which causes the clock to render the current
+            playback.skipTo(currentClockState);
-                // playback seconds. If the clock state is 0, then we can just ignore
-                // skipping entirely.
-                // noinspection JSIgnoredPromiseFromCall
-                playback.skipTo(currentClockState);
-            }
         } else if (newState === PlaybackState.Stopped) {
             // Remove the now-useless clock for some space savings
             this.clockStates.delete(mxEvent.getId()!);
@@ -207,10 +209,8 @@ export class PlaybackQueue {
     }
 
     private onPlaybackClock(playback: Playback, mxEvent: MatrixEvent, clocks: number[]): void {
-        if (playback.currentState === PlaybackState.Decoding) return; // ignore pre-ready values
+        if (playback.currentState !== PlaybackState.Playing && playback.currentState !== PlaybackState.Paused) return; // ignore pre-ready values
 
-        if (playback.currentState !== PlaybackState.Stopped) {
+        this.clockStates.set(mxEvent.getId()!, clocks[0]); // [0] is the current seek position
-            this.clockStates.set(mxEvent.getId()!, clocks[0]); // [0] is the current seek position
-        }
     }
 }

test/unit-tests/audio/PlaybackQueue-test.ts

@@ -7,57 +7,50 @@ Please see LICENSE files in the repository root for full details.
 
 import { type Mocked } from "jest-mock";
 import { type MatrixEvent, type Room } from "matrix-js-sdk/src/matrix";
-import { SimpleObservable } from "matrix-widget-api";
 
 import { PlaybackQueue } from "../../../src/audio/PlaybackQueue";
-import { PlaybackState, type Playback } from "../../../src/audio/Playback";
+import { type Playback, PlaybackState } from "../../../src/audio/Playback";
-import { MockEventEmitter } from "../../test-utils";
 import { UPDATE_EVENT } from "../../../src/stores/AsyncStore";
+import { MockedPlayback } from "./MockedPlayback";
 
 describe("PlaybackQueue", () => {
     let playbackQueue: PlaybackQueue;
+    let mockRoom: Mocked<Room>;
 
     beforeEach(() => {
-        const mockRoom = {
+        mockRoom = {
             getMember: jest.fn(),
         } as unknown as Mocked<Room>;
         playbackQueue = new PlaybackQueue(mockRoom);
     });
 
-    it("does not call skipTo on playback if clock advances to 0s", () => {
+    it.each([
+        [PlaybackState.Playing, true],
+        [PlaybackState.Paused, true],
+        [PlaybackState.Preparing, false],
+        [PlaybackState.Decoding, false],
+        [PlaybackState.Stopped, false],
+    ])("should save (or not) the clock PlayBackState=%s expected=%s", (playbackState, expected) => {
         const mockEvent = {
             getId: jest.fn().mockReturnValue("$foo:bar"),
         } as unknown as Mocked<MatrixEvent>;
-        const mockPlayback = new MockEventEmitter({
+        const mockPlayback = new MockedPlayback(playbackState, 0, 0) as unknown as Mocked<Playback>;
-            clockInfo: {
-                liveData: new SimpleObservable<number[]>(),
-            },
-            skipTo: jest.fn(),
-        }) as unknown as Mocked<Playback>;
 
         // Enqueue
         playbackQueue.unsortedEnqueue(mockEvent, mockPlayback);
 
         // Emit our clockInfo of 0, which will playbackQueue to save the state.
-        mockPlayback.clockInfo.liveData.update([0]);
+        mockPlayback.clockInfo.liveData.update([1]);
 
-        // Fire an update event to say that we have stopped.
+        // @ts-ignore
-        // Note that Playback really emits an UPDATE_EVENT whenever state changes, the types are lies.
+        expect(playbackQueue.clockStates.has(mockEvent.getId()!)).toBe(expected);
-        mockPlayback.emit(UPDATE_EVENT as any, PlaybackState.Stopped);
-
-        expect(mockPlayback.skipTo).not.toHaveBeenCalled();
     });
 
-    it("does call skipTo on playback if clock advances to 0s", () => {
+    it("does call skipTo on playback if clock advances to 1s", () => {
         const mockEvent = {
             getId: jest.fn().mockReturnValue("$foo:bar"),
         } as unknown as Mocked<MatrixEvent>;
-        const mockPlayback = new MockEventEmitter({
+        const mockPlayback = new MockedPlayback(PlaybackState.Playing, 0, 0) as unknown as Mocked<Playback>;
-            clockInfo: {
-                liveData: new SimpleObservable<number[]>(),
-            },
-            skipTo: jest.fn(),
-        }) as unknown as Mocked<Playback>;
 
         // Enqueue
         playbackQueue.unsortedEnqueue(mockEvent, mockPlayback);
@@ -71,4 +64,24 @@ describe("PlaybackQueue", () => {
 
         expect(mockPlayback.skipTo).toHaveBeenCalledWith(1);
     });
+
+    it("should ignore the nullish clock state when loading", () => {
+        const clockStates = new Map([
+            ["a", 1],
+            ["b", null],
+            ["c", 3],
+        ]);
+        localStorage.setItem(
+            `mx_voice_message_clocks_${mockRoom.roomId}`,
+            JSON.stringify(Array.from(clockStates.entries())),
+        );
+        playbackQueue = new PlaybackQueue(mockRoom);
+
+        // @ts-ignore
+        expect(playbackQueue.clockStates.has("a")).toBe(true);
+        // @ts-ignore
+        expect(playbackQueue.clockStates.has("b")).toBe(false);
+        // @ts-ignore
+        expect(playbackQueue.clockStates.has("c")).toBe(true);
+    });
 });