Register a dialog when it is shown, not when it is constructed

packages/editor/src/UI/ToolsPanel.ts

@@ -17,7 +17,7 @@ import { colors, styles } from './style'
 class WireSlot extends Slot<string> {
     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()

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) {

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 */

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,

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)
                 }