diff --git a/packages/editor/src/UI/ToolsPanel.ts b/packages/editor/src/UI/ToolsPanel.ts index a0f11f40..22af1018 100644 --- a/packages/editor/src/UI/ToolsPanel.ts +++ b/packages/editor/src/UI/ToolsPanel.ts @@ -17,7 +17,7 @@ import { colors, styles } from './style' class WireSlot extends Slot { public constructor(wireName: string) { super(wireName) - this.content = safeIcon(wireName, () => F.CreateIcon(wireName)) + this.content = F.SafeIcon(wireName, () => F.CreateIcon(wireName)) this.on('pointerdown', e => { if (e.button !== 0) return @@ -135,24 +135,13 @@ const WIRES = ['copper-wire', 'red-wire', 'green-wire'] */ const ROWS = 2 -/** - * An icon that failed to build costs this one slot rather than the whole - * panel - `F.CreateIcon`/`F.CreateUtilitySpriteIcon` throw by design for a - * name FD does not have (see `need()` in `core/need.ts`), and nothing above - * `generateSlots()` catches. Falls back to an empty, childless `Container` - - * not a drawn blank square, which this doc comment used to claim (#242 - * review): it has no graphic of its own at all, so the slot still exists and - * is still clickable, but nothing is visible where the icon would have been. - * Named in a warning rather than silently swallowed either way. - */ -function safeIcon(name: string, build: () => Container): Container { - try { - return build() - } catch (error) { - G.logger({ text: `Could not build the "${name}" icon: ${String(error)}`, type: 'warning' }) - return new Container() - } -} +/* + `safeIcon` used to live here. It is `F.SafeIcon` now, in + `controls/functions.ts` beside the two throwing icon builders it guards, + because `DisplayPanelEditor` needs the same guard for an icon a blueprint + names rather than one this file hardcodes (issue #280). Moved rather than + copied - what it does is unchanged, and its doc comment moved with it. +*/ export class ToolsPanel extends Panel { private slotsContainer: Container @@ -224,33 +213,33 @@ export class ToolsPanel extends Panel { altSlot, new WireSlot(WIRES[0]), new ActionSlot( - safeIcon('import_slot', () => + F.SafeIcon('import_slot', () => F.CreateUtilitySpriteIcon(FD.utilitySprites.import_slot) ), () => G.UI.toggleImportDialog() ), new WireSlot(WIRES[1]), new ActionSlot( - safeIcon('export_slot', () => + F.SafeIcon('export_slot', () => F.CreateUtilitySpriteIcon(FD.utilitySprites.export_slot) ), () => G.UI.toggleExportDialog() ), new WireSlot(WIRES[2]), new ActionSlot( - safeIcon('signal-anticlockwise-circle-arrow', () => + F.SafeIcon('signal-anticlockwise-circle-arrow', () => F.CreateIcon('signal-anticlockwise-circle-arrow') ), () => G.bp.history.undo() ), new ActionSlot( - safeIcon('signal-clockwise-circle-arrow', () => + F.SafeIcon('signal-clockwise-circle-arrow', () => F.CreateIcon('signal-clockwise-circle-arrow') ), () => G.bp.history.redo() ), new ActionSlot( - safeIcon('downloading', () => + F.SafeIcon('downloading', () => F.CreateUtilitySpriteIcon(FD.utilitySprites.downloading) ), () => G.quickActions.exportImage() diff --git a/packages/editor/src/UI/UIContainer.ts b/packages/editor/src/UI/UIContainer.ts index ec764a96..69ba2982 100644 --- a/packages/editor/src/UI/UIContainer.ts +++ b/packages/editor/src/UI/UIContainer.ts @@ -54,6 +54,26 @@ export class UIContainer extends Container { this.debugContainer.visible = visible } + /* + Deliberately still a bare call with no try/catch, and that is a + narrower statement than it looks (issue #280). + + A throwing editor constructor no longer damages anything outside + itself: `Dialog` registers on `added` rather than in its constructor, + so an editor that dies part-built is never added and never counted as + open. See the comment there. + + What a catch here would add is swallowing the throw, and that is the + wrong trade. `createEditor` is the point where a dialog is asked for + because the user clicked an entity; an editor that cannot be built is a + bug in that editor, and it should say so loudly - the failure is + already visible as one entity with no dialog, and the console keeps the + stack that names which one. Catching it here would turn every future + broken editor into a click that does nothing. + + The fix for a specific editor is in that editor - `DisplayPanelEditor` + and `F.SafeIcon` are the worked example. + */ public createEditor(entity: Entity): void { const editor = createEditor(entity) if (editor) { diff --git a/packages/editor/src/UI/controls/Dialog.ts b/packages/editor/src/UI/controls/Dialog.ts index 1eaf09a0..cad4c96f 100644 --- a/packages/editor/src/UI/controls/Dialog.ts +++ b/packages/editor/src/UI/controls/Dialog.ts @@ -33,7 +33,45 @@ export abstract class Dialog extends Panel { this.addLabel(12, 10, title, styles.dialog.title) } - Dialog.s_openDialogs.push(this) + /* + Registered when the dialog joins the display tree, not here. + + This line used to be a bare `Dialog.s_openDialogs.push(this)`, and + a constructor is the one place a dialog cannot safely claim to be + open from: `super()` runs before any subclass body, and `close()` + is the only thing that takes an entry back out. So a subclass + constructor that throws after `super()` returns leaves an entry + behind for a dialog that was never shown and that nothing holds a + reference to (issue #280). + + That is reachable rather than theoretical. Icon names come out of + the blueprint, `F.CreateIcon` throws for a name FD does not have, + and `UIContainer.createEditor` is a bare call with no try/catch + above it - `DisplayPanelEditor` drawing a planet icon is the + measured path. A try/catch at that one call site would have fixed + that one call site; every later `new SomethingEditor()` would be + free to repeat it. + + `added` is the point a subclass cannot skip and cannot reach early. + Pixi emits it from `addChild`/`addChildAt` on the child, and every + dialog in this codebase is constructed and then added by its + creator - the four sites are `UIContainer`'s `createEditor`, + `toggleImportDialog`, `toggleExportDialog` and `createInventory`, + each of which adds to `dialogsContainer` on the next line. A + constructor that throws never returns the object to be added, so + it never registers. Nothing here adds a dialog to anything else, + and `Panel`'s own `addChild` of its background emits on the + background, not on `this`. + + `once`, not `on`: a dialog is added exactly once and then closed, + and `once` means a re-add cannot register a second entry for the + same object, which would take two `closeLast()` presses to clear. + + The window this opens - constructed but not yet added, and so not + yet `anyOpen()` - is closed by every call site being synchronous. + Nothing can read the registry between the two statements. + */ + this.once('added', () => Dialog.s_openDialogs.push(this)) } /** Closes last open dialog */ diff --git a/packages/editor/src/UI/controls/functions.ts b/packages/editor/src/UI/controls/functions.ts index a37791c7..db41fc58 100644 --- a/packages/editor/src/UI/controls/functions.ts +++ b/packages/editor/src/UI/controls/functions.ts @@ -419,6 +419,37 @@ function CreateUtilitySpriteIcon(data: SpriteData, maxSize = 32, setAnchor = tru return sprite } +/** + * An icon that failed to build costs the one slot it was going in rather than + * whatever was drawing it - `CreateIcon` and `CreateUtilitySpriteIcon` above + * both throw by design for a name FD does not have (see `need()` in + * `core/need.ts`), and a name can come straight out of a blueprint. Falls back + * to an empty, childless `Container`: no graphic of its own at all, so the slot + * or row still exists and is still laid out, but nothing is visible where the + * icon would have been. Named in a warning either way rather than silently + * swallowed - note that reaches the user as a toast, not a console line. + * + * Takes a builder rather than a name so it covers both icon functions. Lives + * here, next to the two calls it is guarding, rather than in whichever caller + * needed it first: `ToolsPanel` wanted it for a hardcoded name it could not + * spell wrong twice, `DisplayPanelEditor` wants it for a name a blueprint + * chose, and a second copy in the second caller is how the two drift. + * + * **This is not a substitute for a try/catch above a whole feature.** It is the + * right tool only where the icon is one piece of something bigger that should + * still be drawn without it. Where a throw would cost the caller everything - + * see `TileContainer.generateSprite` in CLAUDE.md - the question is what to + * draw instead, not how to skip one sprite. + */ +function SafeIcon(name: string, build: () => Container): Container { + try { + return build() + } catch (error) { + G.logger({ text: `Could not build the "${name}" icon: ${String(error)}`, type: 'warning' }) + return new Container() + } +} + export default { ShadeColor, DrawRectangle, @@ -427,6 +458,7 @@ export default { CreateIconWithAmount, CreateRecipe, CreateUtilitySpriteIcon, + SafeIcon, applyTint, colorAndAlphaToColorSource, rgbToColorSource, diff --git a/packages/editor/src/UI/editors/DisplayPanelEditor.ts b/packages/editor/src/UI/editors/DisplayPanelEditor.ts index 522ec781..c2172ba0 100644 --- a/packages/editor/src/UI/editors/DisplayPanelEditor.ts +++ b/packages/editor/src/UI/editors/DisplayPanelEditor.ts @@ -18,12 +18,29 @@ const COMPARATOR_COL_WIDTH = 18 const VALUE_COL_WIDTH = 40 const CONDITION_WIDTH = ICON_COL_WIDTH + COMPARATOR_COL_WIDTH + VALUE_COL_WIDTH +/* + Every icon this file draws is named by the blueprint, in + `control_behavior.parameters`, so every one of them is a name FD may not + have - and `F.CreateIcon` ends in a bare `throw` for such a name. There is + no try/catch anywhere above here: `UIContainer.createEditor` is a bare call, + so a throw costs the whole dialog rather than one icon, and it costs it + after `Dialog`'s constructor has already registered the dialog as open + (issue #280). `F.SafeIcon` is what keeps the cost to the icon. + + Not hypothetical. `data.json` exports no planet prototype at all, so + `nauvis`, `vulcanus`, `fulgora` and `gleba` are in none of the five + collections `CreateIcon` searches and all four reach that throw - 19 icon + references in the committed corpus use one (issue #231). A panel captioned + for a planet is exactly the panel someone writes. +*/ + /** Icon(s) + comparator + value, laid out in fixed-width columns to line up across rows */ function createConditionDisplay(condition: ICondition | undefined): Container { const container = new Container() if (!condition || !condition.first_signal?.name) return container - const icon = F.CreateIcon(condition.first_signal.name, CONDITION_ICON_SIZE) + const firstName = condition.first_signal.name + const icon = F.SafeIcon(firstName, () => F.CreateIcon(firstName, CONDITION_ICON_SIZE)) icon.position.set(ICON_COL_WIDTH / 2, CONDITION_ICON_SIZE / 2) container.addChild(icon) @@ -35,8 +52,11 @@ function createConditionDisplay(condition: ICondition | undefined): Container { container.addChild(comparatorLabel) const valueX = ICON_COL_WIDTH + COMPARATOR_COL_WIDTH - if (condition.second_signal?.name) { - const secondIcon = F.CreateIcon(condition.second_signal.name, CONDITION_ICON_SIZE) + const secondName = condition.second_signal?.name + if (secondName) { + const secondIcon = F.SafeIcon(secondName, () => + F.CreateIcon(secondName, CONDITION_ICON_SIZE) + ) secondIcon.position.set(valueX + CONDITION_ICON_SIZE / 2, CONDITION_ICON_SIZE / 2) container.addChild(secondIcon) } else { @@ -91,8 +111,9 @@ export class DisplayPanelEditor extends Editor { const row = new Container() row.position.set(12, 210 + i * ROW_HEIGHT) - if (param.icon?.name) { - const icon = F.CreateIcon(param.icon.name, 20) + const iconName = param.icon?.name + if (iconName) { + const icon = F.SafeIcon(iconName, () => F.CreateIcon(iconName, 20)) icon.position.set(10, 10) row.addChild(icon) } diff --git a/tests/dialog-registry-leak.spec.ts b/tests/dialog-registry-leak.spec.ts new file mode 100644 index 00000000..b1c29a77 --- /dev/null +++ b/tests/dialog-registry-leak.spec.ts @@ -0,0 +1,269 @@ +import { test, expect } from '@playwright/test' +import { encodeBlueprint as encode, packVersion as version } from './helpers/encode-blueprint' +import { suppressOverlays } from './helpers/overlays' + +/* + A dialog whose constructor throws, and what it leaves behind (issue #280). + + Two separate things, and they are not alternatives. + + The NARROW half is `DisplayPanelEditor`. Every icon it draws is named by the + blueprint - `control_behavior.parameters` carries an icon per row and a + signal at each end of that row's condition - and `F.CreateIcon` ends in a + bare `throw` for a name FD does not have. `data.json` exports no planet + prototype at all, so `nauvis`, `vulcanus`, `fulgora` and `gleba` all reach + it (issue #231), and 19 icon references in the committed corpus use one. The + throw cost the whole dialog: clicking the panel did nothing at all. The fix + is `F.SafeIcon`, so the cost is the one icon and a warning naming it. + + The GENERAL half is `Dialog`'s registry. `Dialog`'s constructor used to push + `this` onto the static `s_openDialogs`, and `super()` runs before any + subclass body, so a subclass that threw afterwards left an entry for a + dialog that was never shown. `Dialog.anyOpen()` then answered true with + nothing on screen, and `E` - which reads exactly that to decide between + "close the top dialog" and "open the inventory" - went to the wrong branch. + Registration moved to the `added` event, which a constructor that throws + never reaches. + + Why the general half needs its own case, and its own entity. Once the narrow + half is in, no display panel can reach the throw any more, so the registry + fix would have nothing to prove. The third test uses an assembling machine + carrying a recipe name FD does not have, which is the same class of input - + a name out of the blueprint, reaching an unguarded `F.CreateIcon`, this time + through `Editor`'s own `Recipe` slot. That editor is still broken and this + spec does not fix it; it is the fixture precisely because it is broken. + + What can and cannot be observed. `window.__fbe_test.openDialogCount()` reads + the pixi child count of `dialogsContainer`, so it cannot see a phantom + registry entry at all - a leaked entry and a clean registry both read 0. The + only thing that can see one is the keybind that branches on it, so the third + test presses `E` and asks whether the inventory opened. That is also the + thing the user actually loses. + + One correction to the issue text, measured. The phantom is not permanent: + `closeLast()` calls `close()` on it, and `close()` filters the registry + before it destroys anything, so the entry clears itself. The cost is one + swallowed `E` (or `Escape`) per failed open, not a dead keybind for the + session. Two failed opens cost two presses, which is why the third test + clicks twice. + + MUTATION RECORD - each half reverted in turn, against this spec. + + 1. `F.SafeIcon(...)` -> `F.CreateIcon(...)` at all three sites in + `DisplayPanelEditor.ts` (the narrow half reverted, registry fix kept): + - test 1 FAILS: `expect(received).toEqual(expected)` on the page-error + list, `["Error: No item, fluid, recipe, signal or inventory group + named nauvis"]` against `[]`; the dialog count assertion that follows + would have failed too, 0 against 1. + - test 2 FAILS the same way, naming `vulcanus`. + - test 3 PASSES - it never opens a display panel. + + 2. `this.once('added', () => Dialog.s_openDialogs.push(this))` -> + `Dialog.s_openDialogs.push(this)` in `Dialog.ts` (the general half + reverted, `F.SafeIcon` kept): + - test 3 FAILS: `expect(received).toBe(expected)` on the dialog count + after `E`, 0 against 1. Both phantoms are still queued, so the first + `E` closes one instead of opening the inventory. + - tests 1 and 2 PASS - nothing in them throws any more, so nothing + leaks. + + 3. Both reverted (the pre-fix code): + - all three FAIL, each with its own message above. + + So neither half covers for the other, and neither test can pass for the + wrong reason. + + 4. The alternative the issue offers, measured rather than argued: the + registry push left in the constructor, and a bare try/catch wrapped + around `UIContainer.createEditor` instead (`F.SafeIcon` kept). + - test 3 FAILS, and it fails at the CONTROL rather than at the keybind: + `expect(received).toHaveLength(expected)`, 2 against 0, on the + page-error filter that opens test 3. Swallowing the throw is all + that catch does. The phantom entries are still queued and the `E` + press after them still goes to the wrong branch; the test never + gets that far, because the constructor's failure has stopped being + visible at all. That is the whole objection to it, written down in + `UIContainer.createEditor`'s own comment: it does not clear the + registry - `s_openDialogs` is `protected`, so clearing it from there + would need a new public escape hatch on `Dialog` - and it buys the + silence of every future broken editor for nothing. + + Runs against the dev server like the rest of tests/ - see CLAUDE.md for the + two servers that have to be up. +*/ + +type Page = import('@playwright/test').Page + +/* + `connect_to_logistic_network` rather than a wire, because + `Entity.generateConnector` is what puts the editor down its read-only + conditions branch and either input satisfies it. A wire would need a second + entity to run to; this needs one panel. +*/ +function displayPanel(parameters: Record[]): string { + return encode({ + item: 'blueprint', + version: version(2, 0, 55), + icons: [{ index: 1, signal: { type: 'item', name: 'display-panel' } }], + entities: [ + { + entity_number: 1, + name: 'display-panel', + position: { x: 0.5, y: 0.5 }, + control_behavior: { connect_to_logistic_network: true, parameters }, + }, + ], + }) +} + +/** A planet as the row's own icon - `DisplayPanelEditor`'s parameter loop. */ +const PLANET_ROW_ICON = displayPanel([ + { + icon: { type: 'space-location', name: 'nauvis' }, + text: 'home', + condition: { + first_signal: { type: 'item', name: 'iron-plate' }, + comparator: '>', + constant: 5, + }, + }, +]) + +/* + A planet at each end of the condition instead - `createConditionDisplay`. + Kept apart from the row icon so that reverting either call site on its own + is caught: with both planets in one blueprint, whichever threw first would + mask the other. +*/ +const PLANET_CONDITION = displayPanel([ + { + icon: { type: 'item', name: 'iron-plate' }, + text: 'away', + condition: { + first_signal: { type: 'space-location', name: 'vulcanus' }, + comparator: '<', + second_signal: { type: 'space-location', name: 'gleba' }, + }, + }, +]) + +/* + An assembling machine naming a recipe FD does not have. Decodes and loads - + the schema's `recipeName` keyword only warns - and then `Editor`'s `Recipe` + slot calls `F.CreateIcon` on it with nothing catching, so the constructor + throws after `super()` has run. The fixture for the registry half. +*/ +const UNKNOWN_RECIPE = encode({ + item: 'blueprint', + version: version(2, 0, 55), + icons: [{ index: 1, signal: { type: 'item', name: 'assembling-machine-1' } }], + entities: [ + { + entity_number: 1, + name: 'assembling-machine-1', + position: { x: 1.5, y: 1.5 }, + recipe: 'totally-not-a-recipe', + }, + ], +}) + +async function load(page: Page, source: string): Promise { + const errors: string[] = [] + page.on('pageerror', e => errors.push(String(e))) + + await suppressOverlays(page) + await page.goto('/') + await page.waitForFunction(() => window.__fbe_test !== undefined, { timeout: 60_000 }) + await page.evaluate(async (src: string) => { + const t = window.__fbe_test + await t.loadBp(await t.getBlueprintOrBookFromSource(src)) + }, source) + return errors +} + +/* + Hover then click, which is `openEntityGUI`. Steps away first because + hovering is driven by GridData's `update32` and only fires when the pointer + crosses a tile boundary - moving to a point it already occupies emits + nothing. Same reason as display-panel-editor.spec.ts and chest-editor.spec.ts. +*/ +async function clickEntity(page: Page, entityNumber: number): Promise { + const at = await page.evaluate( + (n: number) => window.__fbe_test.entityScreenPosition(n), + entityNumber + ) + if (!at) throw new Error(`no entity ${entityNumber} in the loaded blueprint`) + + await page.mouse.move(at.x, at.y + 240) + await page.mouse.move(at.x, at.y) + expect(await page.evaluate(() => window.__fbe_test.editorMode())).toBe('EDIT') + + await page.mouse.down() + await page.mouse.up() +} + +const dialogCount = (page: Page): Promise => + page.evaluate(() => window.__fbe_test.openDialogCount()) + +test('a display panel row icon naming a planet costs the icon, not the dialog', async ({ + page, +}) => { + const errors = await load(page, PLANET_ROW_ICON) + expect(await dialogCount(page)).toBe(0) + + await clickEntity(page, 1) + expect(errors, `page errors: ${errors.join(' | ')}`).toEqual([]) + expect(await dialogCount(page)).toBe(1) + + /* + The warning names the icon. It reaches the user as a toast rather than a + console line - `G.logger` is wired to the website's toasts - so a probe + listening on `page.on('console')` would see nothing and read a working + guard as a silent skip. + */ + await expect(page.locator('.toasts-warning', { hasText: 'nauvis' })).toBeVisible() + + await page.keyboard.press('Escape') + expect(await dialogCount(page)).toBe(0) + expect(errors, `page errors: ${errors.join(' | ')}`).toEqual([]) +}) + +test('a condition signal naming a planet costs that icon, not the dialog', async ({ page }) => { + const errors = await load(page, PLANET_CONDITION) + + await clickEntity(page, 1) + expect(errors, `page errors: ${errors.join(' | ')}`).toEqual([]) + expect(await dialogCount(page)).toBe(1) + + // Both ends of the condition are guarded, not just the first. + await expect(page.locator('.toasts-warning', { hasText: 'vulcanus' })).toBeVisible() + await expect(page.locator('.toasts-warning', { hasText: 'gleba' })).toBeVisible() + + await page.keyboard.press('Escape') + expect(await dialogCount(page)).toBe(0) +}) + +test('an editor constructor that throws leaves the E keybind alone', async ({ page }) => { + const errors = await load(page, UNKNOWN_RECIPE) + + /* + Twice. One phantom entry costs one `E` press, so a single failed open + would still let the second press through and a spec pressing twice would + pass against the leak. Two failed opens against one press cannot. + */ + await clickEntity(page, 1) + await clickEntity(page, 1) + + /* + The control for this whole test: the constructor really did throw, twice. + Without it a change that quietly made the machine editor open would leave + the assertion below passing while measuring nothing about the registry. + */ + expect(errors.filter(e => e.includes('totally-not-a-recipe'))).toHaveLength(2) + expect(await dialogCount(page)).toBe(0) + + // Nothing is open, so E opens the inventory. A leaked entry sends it to + // `Dialog.closeLast()` instead and nothing appears. + await page.keyboard.press('KeyE') + expect(await dialogCount(page)).toBe(1) +})