Texture fills show in the vector tab as soon as they load (#196)
After signing out and back in, a texture fill stayed in its base colour until something else redrew the vector tab. The texture library is a singleton that outlives the editor. Four widgets subscribed to it and never unsubscribed. After a new sign-in the first loaded texture called a destroyed canvas, which threw and stopped the loop before the vector tab heard about the image. - TextureCanvas, TileCanvas, ShaderPreview and WorldViewport now unsubscribe in destroy(). This is the pattern FontPreview and VectorArtCanvas already use. WorldViewport leaves both the texture and the font library. - FileLibrary.notifyChange catches errors per listener. A broken listener no longer starves the others. The error is still logged as "[textures] a listener failed". - e2e 47i no longer forces a redraw. It waits up to 3 s for the tab to show the texture on its own. Findings: - The trigger was e2e module 45 (sign out, sign in), not the texture fill. My note in #196 that the browser showed no exception was wrong. The counter-check shows it clearly. Tests: - ./check.sh: 137 files, 2051 tests, exit 0 (before 136 / 2049) - 2 new unit tests in tests/viewport/file-library.test.ts - GM_STEPS=45,47 after a fresh seed: ALL PASSED, 71 checks - Counter-check with the old source and the new check: 1 FAIL (shape stays #0000ff) plus the TypeError. Old FileLibrary.ts fails the first unit test. - No full e2e run locally. The check count is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
66d6aafbb4
commit
6a2b06dd4b
7 files changed
+114
-14
No files matched your search
@@ -32,6 +32,7 @@ export class ShaderPreview extends Widget {
|
|||||||
private clockOrigin = 0;
|
private clockOrigin = 0;
|
||||||
/** Null: the clock runs. A number: the preview stands at that second. */
|
/** Null: the clock runs. A number: the preview stands at that second. */
|
||||||
private held: number | null = null;
|
private held: number | null = null;
|
||||||
|
private readonly stopPictures: () => void;
|
||||||
|
|
||||||
constructor(private readonly state: EditorState) {
|
constructor(private readonly state: EditorState) {
|
||||||
super();
|
super();
|
||||||
@@ -39,7 +40,7 @@ export class ShaderPreview extends Widget {
|
|||||||
this.content.addChild(this.mesh);
|
this.content.addChild(this.mesh);
|
||||||
this.content.mask = this.clip;
|
this.content.mask = this.clip;
|
||||||
this.addChild(this.bg, this.content, this.clip);
|
this.addChild(this.bg, this.content, this.clip);
|
||||||
textureLibrary.onChange(() => this.invalidate());
|
this.stopPictures = textureLibrary.onChange(() => this.invalidate());
|
||||||
}
|
}
|
||||||
|
|
||||||
override measure(): Size {
|
override measure(): Size {
|
||||||
@@ -107,6 +108,7 @@ export class ShaderPreview extends Widget {
|
|||||||
}
|
}
|
||||||
|
|
||||||
override destroy(): void {
|
override destroy(): void {
|
||||||
|
this.stopPictures();
|
||||||
disposeSpriteMesh(this.mesh);
|
disposeSpriteMesh(this.mesh);
|
||||||
super.destroy();
|
super.destroy();
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -136,6 +136,7 @@ export class TextureCanvas extends Widget {
|
|||||||
private collision = false;
|
private collision = false;
|
||||||
/** The outline a click adds corners to; for another frame, or one gone, the frame's last one. */
|
/** The outline a click adds corners to; for another frame, or one gone, the frame's last one. */
|
||||||
private active = { frameId: '', index: -1 };
|
private active = { frameId: '', index: -1 };
|
||||||
|
private readonly stopPictures: () => void;
|
||||||
|
|
||||||
constructor(private readonly state: EditorState) {
|
constructor(private readonly state: EditorState) {
|
||||||
super();
|
super();
|
||||||
@@ -159,7 +160,13 @@ export class TextureCanvas extends Widget {
|
|||||||
this.redraw();
|
this.redraw();
|
||||||
});
|
});
|
||||||
// A picture arrives asynchronously; until then there is nothing to fit the view to.
|
// 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<Widget['destroy']>[0]): void {
|
||||||
|
this.stopPictures();
|
||||||
|
super.destroy(options);
|
||||||
}
|
}
|
||||||
|
|
||||||
override measure(): Size {
|
override measure(): Size {
|
||||||
|
|||||||
@@ -68,6 +68,7 @@ export class TileCanvas extends Widget {
|
|||||||
private board = { w: 0, h: 0 };
|
private board = { w: 0, h: 0 };
|
||||||
private info: TileInfo | null = null;
|
private info: TileInfo | null = null;
|
||||||
private needsFit = true;
|
private needsFit = true;
|
||||||
|
private readonly stopPictures: () => void;
|
||||||
|
|
||||||
constructor(private readonly state: EditorState) {
|
constructor(private readonly state: EditorState) {
|
||||||
super();
|
super();
|
||||||
@@ -90,7 +91,13 @@ export class TileCanvas extends Widget {
|
|||||||
state.on('project', () => this.redraw());
|
state.on('project', () => this.redraw());
|
||||||
state.on('load', () => this.show(''));
|
state.on('load', () => this.show(''));
|
||||||
// A picture arrives asynchronously; the board is empty until it does.
|
// 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<Widget['destroy']>[0]): void {
|
||||||
|
this.stopPictures();
|
||||||
|
super.destroy(options);
|
||||||
}
|
}
|
||||||
|
|
||||||
override measure(): Size {
|
override measure(): Size {
|
||||||
|
|||||||
@@ -61,6 +61,7 @@ export abstract class WorldViewport extends Widget {
|
|||||||
private visibleCount = 0;
|
private visibleCount = 0;
|
||||||
private animated = false;
|
private animated = false;
|
||||||
protected lastItems: DrawItem[] = [];
|
protected lastItems: DrawItem[] = [];
|
||||||
|
private readonly stopFiles: (() => void)[];
|
||||||
|
|
||||||
protected constructor(protected readonly state: EditorState) {
|
protected constructor(protected readonly state: EditorState) {
|
||||||
super();
|
super();
|
||||||
@@ -78,8 +79,16 @@ export abstract class WorldViewport extends Widget {
|
|||||||
if (this.visible && !this.animated) this.redraw();
|
if (this.visible && !this.animated) this.redraw();
|
||||||
});
|
});
|
||||||
// Unconditional: a picture or a typeface arriving fires once per file, not per frame.
|
// Unconditional: a picture or a typeface arriving fires once per file, not per frame.
|
||||||
textureLibrary.onChange(() => this.redraw());
|
this.stopFiles = [
|
||||||
fontLibrary.onChange(() => this.redraw());
|
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<Widget['destroy']>[0]): void {
|
||||||
|
for (const stop of this.stopFiles) stop();
|
||||||
|
super.destroy(options);
|
||||||
}
|
}
|
||||||
|
|
||||||
override measure(): Size {
|
override measure(): Size {
|
||||||
|
|||||||
@@ -30,7 +30,13 @@ export abstract class FileLibrary<T> {
|
|||||||
|
|
||||||
/** Runs the listeners: something they draw from has changed without a file arriving here. */
|
/** Runs the listeners: something they draw from has changed without a file arriving here. */
|
||||||
notifyChange(): void {
|
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<T>;
|
protected abstract fetchFile(url: string, file: string): Promise<T>;
|
||||||
|
|||||||
@@ -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<string> {
|
||||||
|
readonly kept = new Map<string, string>();
|
||||||
|
private readonly pending: (() => void)[] = [];
|
||||||
|
|
||||||
|
constructor() {
|
||||||
|
super('/files', 'test');
|
||||||
|
}
|
||||||
|
|
||||||
|
ask(file: string): void {
|
||||||
|
if (!this.kept.has(file)) this.startLoading(file);
|
||||||
|
}
|
||||||
|
|
||||||
|
land(): Promise<void> {
|
||||||
|
for (const resolve of this.pending.splice(0)) resolve();
|
||||||
|
return new Promise((resolve) => setTimeout(resolve, 0));
|
||||||
|
}
|
||||||
|
|
||||||
|
protected fetchFile(url: string): Promise<string> {
|
||||||
|
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();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -843,15 +843,17 @@ export async function vectorArt(t) {
|
|||||||
const halvesEntry = Boolean(halvesName) && filteredOffers.includes(halvesName);
|
const halvesEntry = Boolean(halvesName) && filteredOffers.includes(halvesName);
|
||||||
await press(halvesEntry ? 'Enter' : 'Escape');
|
await press(halvesEntry ? 'Enter' : 'Escape');
|
||||||
await settle();
|
await settle();
|
||||||
// The picture loads asynchronously and the shape keeps its plain fill until it is there. In the
|
// The picture loads asynchronously and the shape keeps its plain fill until it is there; the tab
|
||||||
// full run the tab then waits for the next redraw instead of drawing when the picture lands
|
// redraws by itself when it lands. Nothing here asks for that redraw: after module 45 had signed
|
||||||
// (#196), so the check asks for that redraw itself. Remove the preview() with #196.
|
// out and in, a canvas of the old editor used to swallow the news (#196).
|
||||||
await sleep(300);
|
let leftHalf = await tabPixel(60, 100);
|
||||||
await evaluate('__gm.state.preview()');
|
let rightHalf = await tabPixel(196, 100);
|
||||||
await settle();
|
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 tiled = (await sky())?.paint;
|
||||||
const leftHalf = await tabPixel(60, 100);
|
|
||||||
const rightHalf = await tabPixel(196, 100);
|
|
||||||
check(
|
check(
|
||||||
'Texture offers the uploaded pictures but no drawing, and fills the shape with the one picked',
|
'Texture offers the uploaded pictures but no drawing, and fills the shape with the one picked',
|
||||||
halvesEntry &&
|
halvesEntry &&
|
||||||
|
|||||||
Reference in new issue
Block a user