diff --git a/src/editor/shader/ShaderPreview.ts b/src/editor/shader/ShaderPreview.ts index 69117572..6e107e36 100644 --- a/src/editor/shader/ShaderPreview.ts +++ b/src/editor/shader/ShaderPreview.ts @@ -32,6 +32,7 @@ export class ShaderPreview extends Widget { private clockOrigin = 0; /** Null: the clock runs. A number: the preview stands at that second. */ private held: number | null = null; + private readonly stopPictures: () => void; constructor(private readonly state: EditorState) { super(); @@ -39,7 +40,7 @@ export class ShaderPreview extends Widget { this.content.addChild(this.mesh); this.content.mask = this.clip; this.addChild(this.bg, this.content, this.clip); - textureLibrary.onChange(() => this.invalidate()); + this.stopPictures = textureLibrary.onChange(() => this.invalidate()); } override measure(): Size { @@ -107,6 +108,7 @@ export class ShaderPreview extends Widget { } override destroy(): void { + this.stopPictures(); disposeSpriteMesh(this.mesh); super.destroy(); } diff --git a/src/editor/texture/TextureCanvas.ts b/src/editor/texture/TextureCanvas.ts index e3b79cb3..fc125b98 100644 --- a/src/editor/texture/TextureCanvas.ts +++ b/src/editor/texture/TextureCanvas.ts @@ -136,6 +136,7 @@ export class TextureCanvas extends Widget { private collision = false; /** The outline a click adds corners to; for another frame, or one gone, the frame's last one. */ private active = { frameId: '', index: -1 }; + private readonly stopPictures: () => void; constructor(private readonly state: EditorState) { super(); @@ -159,7 +160,13 @@ export class TextureCanvas extends Widget { this.redraw(); }); // A picture arrives asynchronously; until then there is nothing to fit the view to. - textureLibrary.onChange(() => this.redraw()); + this.stopPictures = textureLibrary.onChange(() => this.redraw()); + } + + /** The library outlives a signed-out editor; a stale listener would redraw destroyed graphics. */ + override destroy(options?: Parameters[0]): void { + this.stopPictures(); + super.destroy(options); } override measure(): Size { diff --git a/src/editor/tile/TileCanvas.ts b/src/editor/tile/TileCanvas.ts index ef689bf3..4364609b 100644 --- a/src/editor/tile/TileCanvas.ts +++ b/src/editor/tile/TileCanvas.ts @@ -68,6 +68,7 @@ export class TileCanvas extends Widget { private board = { w: 0, h: 0 }; private info: TileInfo | null = null; private needsFit = true; + private readonly stopPictures: () => void; constructor(private readonly state: EditorState) { super(); @@ -90,7 +91,13 @@ export class TileCanvas extends Widget { state.on('project', () => this.redraw()); state.on('load', () => this.show('')); // A picture arrives asynchronously; the board is empty until it does. - textureLibrary.onChange(() => this.redraw()); + this.stopPictures = textureLibrary.onChange(() => this.redraw()); + } + + /** The library outlives a signed-out editor; a stale listener would redraw destroyed graphics. */ + override destroy(options?: Parameters[0]): void { + this.stopPictures(); + super.destroy(options); } override measure(): Size { diff --git a/src/editor/viewport/WorldViewport.ts b/src/editor/viewport/WorldViewport.ts index 95f9e6a8..2e0fb170 100644 --- a/src/editor/viewport/WorldViewport.ts +++ b/src/editor/viewport/WorldViewport.ts @@ -61,6 +61,7 @@ export abstract class WorldViewport extends Widget { private visibleCount = 0; private animated = false; protected lastItems: DrawItem[] = []; + private readonly stopFiles: (() => void)[]; protected constructor(protected readonly state: EditorState) { super(); @@ -78,8 +79,16 @@ export abstract class WorldViewport extends Widget { if (this.visible && !this.animated) this.redraw(); }); // Unconditional: a picture or a typeface arriving fires once per file, not per frame. - textureLibrary.onChange(() => this.redraw()); - fontLibrary.onChange(() => this.redraw()); + this.stopFiles = [ + textureLibrary.onChange(() => this.redraw()), + fontLibrary.onChange(() => this.redraw()), + ]; + } + + /** The libraries outlive a signed-out editor; a stale listener would redraw destroyed graphics. */ + override destroy(options?: Parameters[0]): void { + for (const stop of this.stopFiles) stop(); + super.destroy(options); } override measure(): Size { diff --git a/src/viewport/FileLibrary.ts b/src/viewport/FileLibrary.ts index 1ab60701..b8a76c52 100644 --- a/src/viewport/FileLibrary.ts +++ b/src/viewport/FileLibrary.ts @@ -30,7 +30,13 @@ export abstract class FileLibrary { /** Runs the listeners: something they draw from has changed without a file arriving here. */ notifyChange(): void { - for (const listener of this.listeners) listener(); + // One listener that throws must not keep the file from the ones after it (#196). + for (const listener of this.listeners) + try { + listener(); + } catch (error) { + console.error(`[${this.tag}] a listener failed`, error); + } } protected abstract fetchFile(url: string, file: string): Promise; diff --git a/tests/viewport/file-library.test.ts b/tests/viewport/file-library.test.ts new file mode 100644 index 00000000..5332eb7b --- /dev/null +++ b/tests/viewport/file-library.test.ts @@ -0,0 +1,67 @@ +/** + * The base of the picture and font libraries: a file asked for is fetched once, and the listeners + * hear when it lands. The libraries are module singletons and outlive an editor that was signed out + * of, so a listener can belong to a widget that is gone (#196). + */ +import { describe, expect, it, vi } from 'vitest'; +import { FileLibrary } from '../../src/viewport/FileLibrary'; + +/** A library whose loads a test resolves by hand. */ +class HandLibrary extends FileLibrary { + readonly kept = new Map(); + private readonly pending: (() => void)[] = []; + + constructor() { + super('/files', 'test'); + } + + ask(file: string): void { + if (!this.kept.has(file)) this.startLoading(file); + } + + land(): Promise { + for (const resolve of this.pending.splice(0)) resolve(); + return new Promise((resolve) => setTimeout(resolve, 0)); + } + + protected fetchFile(url: string): Promise { + return new Promise((resolve) => this.pending.push(() => resolve(url))); + } + + protected keep(file: string, loaded: string): void { + this.kept.set(file, loaded); + } +} + +describe('the file library', () => { + it('tells every listener when a file lands, even after one of them threw', async () => { + const library = new HandLibrary(); + const error = vi.spyOn(console, 'error').mockImplementation(() => {}); + // First in line, as the canvas of an editor signed out of is: its graphics are destroyed. + library.onChange(() => { + throw new TypeError("Cannot read properties of null (reading 'clear')"); + }); + const later = vi.fn(); + library.onChange(later); + + library.ask('a.png'); + await library.land(); + + expect(library.kept.get('a.png')).toBe('/files/a.png'); + expect(later).toHaveBeenCalledTimes(1); + // Not swallowed: the broken listener still shows in the console. + expect(error).toHaveBeenCalledWith('[test] a listener failed', expect.any(TypeError)); + error.mockRestore(); + }); + + it('forgets a listener once its unsubscribe has run', async () => { + const library = new HandLibrary(); + const gone = vi.fn(); + const stop = library.onChange(gone); + stop(); + + library.ask('a.png'); + await library.land(); + expect(gone).not.toHaveBeenCalled(); + }); +}); diff --git a/tools/e2e/47-vectorart.mjs b/tools/e2e/47-vectorart.mjs index 426efb4b..3dcfa26a 100644 --- a/tools/e2e/47-vectorart.mjs +++ b/tools/e2e/47-vectorart.mjs @@ -843,15 +843,17 @@ export async function vectorArt(t) { const halvesEntry = Boolean(halvesName) && filteredOffers.includes(halvesName); await press(halvesEntry ? 'Enter' : 'Escape'); await settle(); - // The picture loads asynchronously and the shape keeps its plain fill until it is there. In the - // full run the tab then waits for the next redraw instead of drawing when the picture lands - // (#196), so the check asks for that redraw itself. Remove the preview() with #196. - await sleep(300); - await evaluate('__gm.state.preview()'); - await settle(); + // The picture loads asynchronously and the shape keeps its plain fill until it is there; the tab + // redraws by itself when it lands. Nothing here asks for that redraw: after module 45 had signed + // out and in, a canvas of the old editor used to swallow the news (#196). + let leftHalf = await tabPixel(60, 100); + let rightHalf = await tabPixel(196, 100); + for (let waited = 0; waited < 3000 && colourDistance(rightHalf, HALVES_RIGHT) >= 20; waited += 100) { + await sleep(100); + leftHalf = await tabPixel(60, 100); + rightHalf = await tabPixel(196, 100); + } const tiled = (await sky())?.paint; - const leftHalf = await tabPixel(60, 100); - const rightHalf = await tabPixel(196, 100); check( 'Texture offers the uploaded pictures but no drawing, and fills the shape with the one picked', halvesEntry &&