From 427cddb8e5542f1369d22d7daad82570f97a9499 Mon Sep 17 00:00:00 2001 From: R Midhun Suresh Date: Mon, 25 Aug 2025 14:49:24 +0530 Subject: [PATCH] MVVM - Introduce the concept of disposables to track event listeners, sub vms and so on (#30475) * Introduce disposables to track sub vms and event listeners * Remove old code * Use disposable in BaseViewModel * Update vm so that the listener is tracked through disposable * No-op on dispose call instead of throwing error * Throw error in trackListener as well * Fix audio player vm * Expose isDisposed through base vm * Dispose AudioPlayerViewModel --- src/components/views/messages/MAudioBody.tsx | 1 + src/viewmodels/audio/AudioPlayerViewModel.ts | 12 +--- src/viewmodels/base/BaseViewModel.ts | 48 +++++-------- src/viewmodels/base/Disposables.ts | 70 +++++++++++++++++++ src/viewmodels/base/ViewModelSubscriptions.ts | 18 +---- .../event-tiles/TextualEventViewModel.ts | 9 +-- test/viewmodels/base/Disposables-test.ts | 57 +++++++++++++++ 7 files changed, 150 insertions(+), 65 deletions(-) create mode 100644 src/viewmodels/base/Disposables.ts create mode 100644 test/viewmodels/base/Disposables-test.ts diff --git a/src/components/views/messages/MAudioBody.tsx b/src/components/views/messages/MAudioBody.tsx index d96a9368d3..56c162b0d0 100644 --- a/src/components/views/messages/MAudioBody.tsx +++ b/src/components/views/messages/MAudioBody.tsx @@ -74,6 +74,7 @@ export default class MAudioBody extends React.PureComponent public componentWillUnmount(): void { this.state.playback?.destroy(); + this.state.audioPlayerVm?.dispose(); } protected get showFileBody(): boolean { diff --git a/src/viewmodels/audio/AudioPlayerViewModel.ts b/src/viewmodels/audio/AudioPlayerViewModel.ts index 208cd7e907..e028ba7860 100644 --- a/src/viewmodels/audio/AudioPlayerViewModel.ts +++ b/src/viewmodels/audio/AudioPlayerViewModel.ts @@ -77,6 +77,9 @@ export class AudioPlayerViewModel public constructor(props: Props) { super(props, AudioPlayerViewModel.computeSnapshot(props.playback, props.mediaName)); + this.disposables.trackListener(props.playback, UPDATE_EVENT, this.setSnapshot); + // There is no unsubscribe method in SimpleObservable + this.props.playback.clockInfo.liveData.onUpdate(this.setSnapshot); // Don't wait for the promise to complete - it will emit a progress update when it // is done, and it's not meant to take long anyhow. @@ -97,15 +100,6 @@ export class AudioPlayerViewModel } } - protected addDownstreamSubscription(): void { - this.props.playback.on(UPDATE_EVENT, this.setSnapshot); - // There is no unsubscribe method in SimpleObservable - this.props.playback.clockInfo.liveData.onUpdate(this.setSnapshot); - } - protected removeDownstreamSubscription(): void { - this.props.playback.off(UPDATE_EVENT, this.setSnapshot); - } - /** * Sets the snapshot and emits an update to subscribers. */ diff --git a/src/viewmodels/base/BaseViewModel.ts b/src/viewmodels/base/BaseViewModel.ts index 942cb6d210..a9ed6d2dc9 100644 --- a/src/viewmodels/base/BaseViewModel.ts +++ b/src/viewmodels/base/BaseViewModel.ts @@ -6,6 +6,7 @@ Please see LICENSE files in the repository root for full details. */ import { type ViewModel } from "../../shared-components/ViewModel"; +import { Disposables } from "./Disposables"; import { Snapshot } from "./Snapshot"; import { ViewModelSubscriptions } from "./ViewModelSubscriptions"; @@ -13,13 +14,11 @@ export abstract class BaseViewModel implements ViewModel { protected subs: ViewModelSubscriptions; protected snapshot: Snapshot; protected props: P; + protected disposables = new Disposables(); protected constructor(props: P, initialSnapshot: T) { this.props = props; - this.subs = new ViewModelSubscriptions( - this.addDownstreamSubscriptionWrapper, - this.removeDownstreamSubscriptionWrapper, - ); + this.subs = new ViewModelSubscriptions(); this.snapshot = new Snapshot(initialSnapshot, () => { this.subs.emit(); }); @@ -29,37 +28,24 @@ export abstract class BaseViewModel implements ViewModel { return this.subs.add(listener); }; - /** - * Wrapper around the abstract subscribe callback as we can't assume that the subclassed method - * has a bound `this` context. - */ - private addDownstreamSubscriptionWrapper = (): void => { - this.addDownstreamSubscription(); - }; - - /** - * Wrapper around the abstract unsubscribe callback as we can't call pass an abstract method directly - * in the constructor. - */ - private removeDownstreamSubscriptionWrapper = (): void => { - this.removeDownstreamSubscription(); - }; - - /** - * Called when the first listener subscribes: the subclass should set up any necessary subscriptions - * to call this.subs.emit() when the snapshot changes. - */ - protected abstract addDownstreamSubscription(): void; - - /** - * Called when the last listener unsubscribes: the subclass should clean up any subscriptions. - */ - protected abstract removeDownstreamSubscription(): void; - /** * Returns the current snapshot of the view model. */ public getSnapshot = (): T => { return this.snapshot.current; }; + + /** + * Relinquish any resources held by this view-model. + */ + public dispose(): void { + this.disposables.dispose(); + } + + /** + * Whether this view-model has been disposed. + */ + public get isDisposed(): boolean { + return this.disposables.isDisposed; + } } diff --git a/src/viewmodels/base/Disposables.ts b/src/viewmodels/base/Disposables.ts new file mode 100644 index 0000000000..77df53d097 --- /dev/null +++ b/src/viewmodels/base/Disposables.ts @@ -0,0 +1,70 @@ +/* +Copyright 2025 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import type { EventEmitter } from "events"; + +/** + * Something that needs to be eventually disposed. This can be: + * - A function that does the disposing + * - An object containing a dispose method which does the disposing + */ +export type DisposableItem = { dispose: () => void } | (() => void); + +/** + * This class provides a way for the view-model to track any resource + * that it needs to eventually relinquish. + */ +export class Disposables { + private readonly disposables: DisposableItem[] = []; + private _isDisposed: boolean = false; + + /** + * Relinquish all tracked disposable values + */ + public dispose(): void { + if (this.isDisposed) return; + this._isDisposed = true; + for (const disposable of this.disposables) { + if (typeof disposable === "function") { + disposable(); + } else { + disposable.dispose(); + } + } + } + + /** + * Track a value that needs to be eventually relinquished + */ + public track(disposable: T): T { + this.throwIfDisposed(); + this.disposables.push(disposable); + return disposable; + } + + /** + * Add an event listener that will be removed on dispose + */ + public trackListener(emitter: EventEmitter, event: string, callback: (...args: unknown[]) => void): void { + this.throwIfDisposed(); + emitter.on(event, callback); + this.track(() => { + emitter.off(event, callback); + }); + } + + private throwIfDisposed(): void { + if (this.isDisposed) throw new Error("Disposable is already disposed"); + } + + /** + * Whether this disposable has been disposed + */ + public get isDisposed(): boolean { + return this._isDisposed; + } +} diff --git a/src/viewmodels/base/ViewModelSubscriptions.ts b/src/viewmodels/base/ViewModelSubscriptions.ts index 97373fcf9f..a713782aec 100644 --- a/src/viewmodels/base/ViewModelSubscriptions.ts +++ b/src/viewmodels/base/ViewModelSubscriptions.ts @@ -6,20 +6,11 @@ Please see LICENSE files in the repository root for full details. */ /** - * Utility class for view models to manage suscriptions to their updates + * Utility class for view models to manage subscriptions to their updates */ export class ViewModelSubscriptions { private listeners = new Set<() => void>(); - /** - * @param subscribeCallback Called when the first listener subscribes. - * @param unsubscribeCallback Called when the last listener unsubscribes. - */ - public constructor( - private subscribeCallback: () => void, - private unsubscribeCallback: () => void, - ) {} - /** * Subscribe to changes in the view model. * @param listener Will be called whenever the snapshot changes. @@ -27,15 +18,8 @@ export class ViewModelSubscriptions { */ public add = (listener: () => void): (() => void) => { this.listeners.add(listener); - if (this.listeners.size === 1) { - this.subscribeCallback(); - } - return () => { this.listeners.delete(listener); - if (this.listeners.size === 0) { - this.unsubscribeCallback(); - } }; }; diff --git a/src/viewmodels/event-tiles/TextualEventViewModel.ts b/src/viewmodels/event-tiles/TextualEventViewModel.ts index 40121ecf2b..dd8132e7ae 100644 --- a/src/viewmodels/event-tiles/TextualEventViewModel.ts +++ b/src/viewmodels/event-tiles/TextualEventViewModel.ts @@ -17,18 +17,11 @@ export class TextualEventViewModel extends BaseViewModel { const content = textForEvent(this.props.mxEvent, MatrixClientPeg.safeGet(), true, this.props.showHiddenEvents); this.snapshot.set({ content }); }; - - protected addDownstreamSubscription = (): void => { - this.props.mxEvent.on(MatrixEventEvent.SentinelUpdated, this.setTextFromEvent); - }; - - protected removeDownstreamSubscription = (): void => { - this.props.mxEvent.off(MatrixEventEvent.SentinelUpdated, this.setTextFromEvent); - }; } diff --git a/test/viewmodels/base/Disposables-test.ts b/test/viewmodels/base/Disposables-test.ts new file mode 100644 index 0000000000..577374a644 --- /dev/null +++ b/test/viewmodels/base/Disposables-test.ts @@ -0,0 +1,57 @@ +/* +Copyright 2025 New Vector Ltd. +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial +Please see LICENSE files in the repository root for full details. +*/ + +import { EventEmitter } from "events"; + +import { Disposables } from "../../../src/viewmodels/base/Disposables"; + +describe("Disposable", () => { + it("isDisposed is true after dispose() is called", () => { + const disposables = new Disposables(); + expect(disposables.isDisposed).toEqual(false); + disposables.dispose(); + expect(disposables.isDisposed).toEqual(true); + }); + + it("dispose() calls the correct disposing function", () => { + const disposables = new Disposables(); + + const item1 = { + foo: 5, + dispose: jest.fn(), + }; + disposables.track(item1); + + const item2 = jest.fn(); + disposables.track(item2); + + disposables.dispose(); + + expect(item1.dispose).toHaveBeenCalledTimes(1); + expect(item2).toHaveBeenCalledTimes(1); + }); + + it("Throws error if acting on already disposed disposables", () => { + const disposables = new Disposables(); + disposables.dispose(); + expect(() => { + disposables.track(jest.fn); + }).toThrow(); + }); + + it("Removes tracked event listeners on dispose", () => { + const disposables = new Disposables(); + const emitter = new EventEmitter(); + + const fn = jest.fn(); + disposables.trackListener(emitter, "FooEvent", fn); + emitter.emit("FooEvent"); + expect(fn).toHaveBeenCalled(); + + disposables.dispose(); + expect(emitter.listenerCount("FooEvent", fn)).toEqual(0); + }); +});