Skip to content

WIP: fix(content): fix problems arising from rapid settings changes #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
WofWca wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

WIP: fix(content): fix problems arising from rapid settings changes #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 54 additions & 16 deletions src/content/AllMediaElementsController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,20 +118,28 @@ async function importAndCreateController<T extends ControllerKind>(
return controller;
}

// There should be only 1 controller attached to an element.
// However currently AllMediaElementsController acts so that there's only 1 Controller at all.
const controllersMap = new WeakMap<HTMLMediaElement, SomeController>();

// TODO dont create / use the new `AllMediaELementsController` before the old one has been destroyed.
// Can also do this with promises. Maybe this needs to be done in `init.ts`.

export default class AllMediaElementsController {
activeMediaElement: HTMLMediaElement | undefined;
activeMediaElementSourceIsCrossOrigin: boolean | undefined;
unhandledNewElements = new Set<HTMLMediaElement>();
handledElements = new WeakSet<HTMLMediaElement>();
elementLastActivatedAt: number | undefined;
controller: SomeController | undefined;
timeSavedTracker: TimeSavedTracker | undefined;
// I guess it's best to completely get rid of these fields so it's all nice and clean.
// controller: SomeController | undefined;
// timeSavedTracker: TimeSavedTracker | undefined;
private settings: Settings | undefined;
private _resolveDestroyedPromise!: () => void;
private _destroyedPromise = new Promise<void>(r => this._resolveDestroyedPromise = r);
// Whatever is added to `_destroyedPromise.then` doesn't need to be added to `_onDetachFromActiveElementCallbacks`,
// it will be called in `destroy`.
private _onDetachFromActiveElementCallbacks: Array<() => void> = [];
// // Whatever is added to `_destroyedPromise.then` doesn't need to be added to `_onDetachFromActiveElementCallbacks`,
// // it will be called in `destroy`.
private detachFromActiveElement: (() => void) | undefined;

constructor() {
if (process.env.NODE_ENV !== 'production') {
Expand All @@ -151,22 +159,24 @@ export default class AllMediaElementsController {
this._destroyedPromise.then(() => removeOnStorageChangedListener(reactToStorageChanges));
}
private destroy() {
this.detachFromActiveElement();
this.detachFromActiveElement?.();
this._resolveDestroyedPromise();

if (process.env.NODE_ENV !== 'production') {
allMediaElementsControllerActive = false;
}
}
private detachFromActiveElement() {
// COmment
// TODO It is possible to call this function before the `_onDetachFromActiveElementCallbacks` array has been filled
// and `controller` has been assigned.
// Also keep in mind that it's possible to never attached to any elements at all, even if `onNewMediaElements()`
// has been called (see that function).
this.controller?.destroy();
this.controller = undefined;
this._onDetachFromActiveElementCallbacks.forEach(cb => cb());
this._onDetachFromActiveElementCallbacks = [];
// this._onDetachFromActiveElementCallbacks = [];
this.detachFromActiveElement = undefined;
}

public broadcastStatus(): void {
Expand Down Expand Up @@ -195,6 +205,11 @@ export default class AllMediaElementsController {
assertDev(this.controller);

if (controllerTypeDependsOnSettings.some(key => key in newValues)) {
// TODO what if this happens after a new controller started initializing but before it finished?
// For example, if we started importing a controller of the old type.
// Then at this point it must be clear that we're already doing it and that we need to destroy it, then
// run this code.
// Oh, I think we need to DRY this stuff, yeah.
const currentController = this.controller;
const el = currentController.element;
assertDev(typeof this.activeMediaElementSourceIsCrossOrigin === 'boolean');
Expand All @@ -219,6 +234,8 @@ export default class AllMediaElementsController {
})();
}
} else {
// This will also require some work. Consider actually rewriting this so it con't return a new instance,
// create a new instance manually instead. that would be DRYer.
// See the `updateSettingsAndMaybeCreateNewInstance` method - `this.controller` may be uninitialized after that.
// TODO maybe it would be more clear to explicitly reinstantiate it in this file, rather than in that method?
this.controller = this.controller.updateSettingsAndMaybeCreateNewInstance(
Expand All @@ -238,6 +255,9 @@ export default class AllMediaElementsController {
throw new Error('Unsupported message type')
}
}
// TODO how about don't reference the whole controller here, but just the `getTelemetry` function.
// And set that function upon controller initialization.
// Same for timeSavedTracker?
if (this.controller?.initialized && this.timeSavedTracker) {
assertDev(typeof this.activeMediaElementSourceIsCrossOrigin === 'boolean');
assertDev(this.activeMediaElement);
Expand Down Expand Up @@ -363,13 +383,15 @@ export default class AllMediaElementsController {
this.detachFromActiveElement();
}
this.activeMediaElement = el;
let detachFromThisElement: () => void;
const detachFromThisElementP = new Promise<void>(r => detachFromThisElement = r);

assertDev(this._onDetachFromActiveElementCallbacks.length === 0, 'I think `_onDetachFromActiveElementCallbacks` '
+ `should be empty here. Instead it it is ${this._onDetachFromActiveElementCallbacks.length} items long`);
// assertDev(this._onDetachFromActiveElementCallbacks.length === 0, 'I think `_onDetachFromActiveElementCallbacks` '
// + `should be empty here. Instead it it is ${this._onDetachFromActiveElementCallbacks.length} items long`);

// Currently this is technically not required since `this.activeMediaElement` is immediately reassigned
// in the line above after the `detachFromActiveElement` call.
this._onDetachFromActiveElementCallbacks.push(() => this.activeMediaElement = undefined);
// // Currently this is technically not required since `this.activeMediaElement` is immediately reassigned
// // in the line above after the `detachFromActiveElement` call.
// this._onDetachFromActiveElementCallbacks.push(() => this.activeMediaElement = undefined);

await this.ensureLoadSettings();
assertDev(this.settings)
Expand All @@ -390,8 +412,11 @@ export default class AllMediaElementsController {
// without firing the 'loadstart' event.
// So this is reliable.
el.addEventListener('loadstart', onMaybeSourceChange, { passive: true });
this._onDetachFromActiveElementCallbacks.push(() => el.removeEventListener('loadstart', onMaybeSourceChange));
detachFromThisElementP.then(() => el.removeEventListener('loadstart', onMaybeSourceChange));

// TODO detachController fromOldElement, and await in case the old element === new element, which could happen
// if a new `AllMediaElementsController` is being initialized.
// Wait. Beter don't handle this case, but instead ensure that the old `AllMediaElementsController` is destroyed.
const controllerP = importAndCreateController(
getAppropriateControllerType(this.settings, elCrossOrigin),
() => [
Expand All @@ -401,11 +426,20 @@ export default class AllMediaElementsController {
timeSavedTrackerPromise,
]
).then(async controller => {
this.controller = controller;
// this.controller = controller;
await controller.init();
// Controller destruction is done in `detachFromActiveElement`.
return controller;
});
// TODO put this inside the above block?
const ensureDetachControllerFromThisEl = once(async () => {
// TODO here we actually don't need to wait for the controller to finish initializing before calling
// its `destroy()` method (it's said in its comment).
// Also we could cancel `importAndCreateController`.
await (await controllerP).destroy();
});
// TODO add to the map
// TODO `detachFromThisElementP.then(ensureDetachControllerFromThisEl)`?

let hotkeyListenerP;
if (this.settings.enableHotkeys) {
Expand All @@ -419,14 +453,17 @@ export default class AllMediaElementsController {
'./TimeSavedTracker'
);
await controllerP; // It doesn't make sense to measure its effectiveness if it hasn't actually started working yet.
const timeSavedTracker = this.timeSavedTracker = new TimeSavedTracker(
// Don't have to `await` for the possible previously attached to this element `TimeSavedTracker` to finish
// its destruction as it doesn't interfere with a new one.
const timeSavedTracker /* = this.timeSavedTracker */ = new TimeSavedTracker(
el,
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this.settings!,
addOnStorageChangedListener,
removeOnStorageChangedListener,
);
this._onDetachFromActiveElementCallbacks.push(() => timeSavedTracker.destroy());
detachFromThisElementP.then(() => timeSavedTracker.destroy());
// const ensureDestroyTimeSavedTracker

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
resolveTimeSavedTrackerPromise!(timeSavedTracker);
Expand Down Expand Up @@ -500,6 +537,7 @@ export default class AllMediaElementsController {
*/
public onNewMediaElements(...newElements: HTMLMediaElement[]): void {
newElements.forEach(el => this.unhandledNewElements.add(el));
// TODO cancel debounce upon destruction.
this.debouncedHandleNewElements();
}
}
85 changes: 83 additions & 2 deletions src/content/CloningController/CloningController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,44 @@ class SeekDurationProphet {
}
}

// TODO use this everywhere we do `_destroyedPromise.then`. Put it in a library.
// type Instance<Destroyed extends boolean, OnDestroyFieldName extends string = 'onDestroy'> = {
type Instance<Destroyed extends boolean> = {
// [P in OnDestroyFieldName]: Destroyed extends true
onDestroy: Destroyed extends true
? undefined
: (() => Awaited<void>)
}
// type InstanceDestroyed<OnDestroyFieldName extends string = 'onDestroy'> = Instance<true, OnDestroyFieldName>;
// type InstanceNotDestroyed<OnDestroyFieldName extends string = 'onDestroy'> = Instance<false, OnDestroyFieldName>;
// type InstanceMaybeDestroyed<OnDestroyFieldName extends string = 'onDestroy'> =
// InstanceDestroyed<OnDestroyFieldName>
// | InstanceNotDestroyed<OnDestroyFieldName>;
// type InstanceDestroyed = Instance<true, 'onDestroy'>;
// type InstanceNotDestroyed = Instance<false, 'onDestroy'>;
type InstanceDestroyed = Instance<true>;
type InstanceNotDestroyed = Instance<false>;
type InstanceMaybeDestroyed =
// InstanceDestroyed<OnDestroyFieldName>
// | InstanceNotDestroyed<OnDestroyFieldName>;
InstanceDestroyed
| InstanceNotDestroyed;
// function isDestroyed<
function isNotDestroyed<
// T extends { [P in OnDestroyFieldName]: (() => Awaited<void>) },
// T extends Instance<boolean, OnDestroyFieldName>,
// T extends InstanceMaybeDestroyed<OnDestroyFieldName>,
T extends InstanceMaybeDestroyed,
// OnDestroyFieldName extends string = 'onDestroy',
OnDestroyFieldName extends string,
>(
instance: InstanceMaybeDestroyed,
onDestroyFieldName: OnDestroyFieldName,
// ): instance is Instance<false, OnDestroyFieldName> {
): instance is InstanceNotDestroyed {
return !!instance[onDestroyFieldName];
}

// TODO a lot of stuff is copy-pasted from StretchingController.
export default class Controller {
static controllerType = ControllerKind.CLONING;
Expand Down Expand Up @@ -163,8 +201,21 @@ export default class Controller {
this.timeSavedTracker = timeSavedTracker;
}

this.seekDurationProphet = new SeekDurationProphet(element);
this._destroyedPromise.then(() => this.seekDurationProphet.destroy());
// One approach
// This ensures that we're not gonna initialize anything that would need destruction if `destroy` has been called.
if (isNotDestroyed(this)) {
// All the code inside the `if (isNotDestroyed(this)) {` block must be synchronous, so it's impossible to call
// `destroy` after we've initialized something, but before we called `this.onDestroy`.
// Don't TypeScript's type guards work like this? Or do they say that if it's NotDestroyed once, it's
// NotDestroyed forever?
this.seekDurationProphet = new SeekDurationProphet(element);
// We must remove all the usage of `this._destroyedPromise.then`
this.onDestroy(() => this.seekDurationProphet.destroy());
// this._destroyedPromise.then(() => this.seekDurationProphet.destroy());
}
// TODO but if the `init` function is async, would TypeScript still say that `this` is always NotDestroyed
// if we checked it just once? Because after we `await` for something, it could change, and we would need to
// check again, but we may forget as TypeScript could say that `onDestroy` is still defined.

// We don't need a high sample rate as this context is currently only used to volume on the chart,
// so consider setting it manually to a lower one. But I'm thinking whether it woruld actually
Expand All @@ -176,6 +227,36 @@ export default class Controller {
audioContext.close();
})
}
// One approach
private onDestroy = (callback: () => Awaited<void>) => {
this._onDestroyCallbacks.psuh(callback);
}
private destroy(): Awaited<void> {
this.onDestroy = undefined; // So `isNotDestroyed` returns `false`, so we don't initialize any more stuff.
// `Promise.all` is to wait for all the stuff to get destroyed.
return Promise.all(
this._onDestroyCallbacks.map(async cb => await cb())
);
}

// Another approach.
// May be inconvenient to work with? Or is it the same?
private destroyable<T>(init: () => Awaited<T>, destroy: (initResult: T) => Awaited<void>): void {
if (this.destroyed) {
// Just don't init the stuff.
return;
}
const initP = init();
this._onDestroyCallbacks.push(async () => {
await destroy(await initP);
});
}
public destroy(): Awaited<void> {
// `Promise.all` is to wait for all the stuff to get destroyed.
return Promise.all(
this._onDestroyCallbacks.map(async cb => await cb())
);
}

isInitialized(): this is ControllerInitialized {
return this.initialized;
Expand Down