From ea7bcfabfb265701eaa78ce0e9ffc1dc29bb9cb6 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 19 Apr 2026 14:47:05 +0200 Subject: [PATCH] =?UTF-8?q?fix(ui):=20PR=20#131=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20nits=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven nit-level fixes from PR review: - Remove dead signal insert_deactivated from ImplantApp; the hook method on_insert_deactivated() is the actual contract. - Rewrite _on_atlas_economics_link comment in main.gd to reflect the actual flow (AtlasApp closes as a consequence of HudGroups single- active-app, not before emitting anything). - Document the "pop never empties below default" invariant on ImplantNavStack.pop() with a pointer to reset_to_default. - Add _mutating re-entrancy guard on ImplantNavStack mutation methods. push_error + early return if called during a screen_changed emission. - main.gd registry loop now uses typed ImplantAppManifest property access (manifest.app_path, manifest.default_key) instead of dictionary-style .get() calls. Empty app_path triggers push_warning. - Document the economics [/] hotkey exception in main.gd and reference the planned handle_global_key lifecycle hook. Arch doc Follow-up section gains a bullet for the new hook. - Comment the independent-version-read rationale above client_ver and proto_ver in loading_screen.gd. Co-Authored-By: Claude Opus 4.6 --- client/scripts/main.gd | 20 +++++++++++++------- client/ui/implant/implant_app.gd | 1 - client/ui/implant/implant_nav_stack.gd | 24 ++++++++++++++++++++++++ client/ui/loading_screen.gd | 3 +++ docs/architecture/implant-app-pattern.md | 1 + 5 files changed, 41 insertions(+), 8 deletions(-) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 7ea1ec96f..734a109e7 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -178,14 +178,19 @@ func _unhandled_key_input(event: InputEvent) -> void: return var key_event := event as InputEventKey # Registry-driven toggle: each manifest declares its own default_key. - for manifest: Variant in ImplantRegistry.get_manifests(): - if manifest.get("default_key") == key_event.keycode: + for manifest: ImplantAppManifest in ImplantRegistry.get_manifests(): + if manifest.app_path.is_empty(): + push_warning("main.gd: manifest with empty app_path — skipping") + continue + if manifest.default_key == key_event.keycode: HudGroups.toggle_app( - manifest.get("app_path"), - ImplantRegistry.get_resolved_mode(manifest.get("app_path", "")) + manifest.app_path, + ImplantRegistry.get_resolved_mode(manifest.app_path) ) return - # [ / ] — cycle economics system selector (app-specific, not generic enough for manifest). + # [ / ] — in-app navigation for the economics monitor. Not manifest-declared because + # these control intra-app navigation (prev/next system), not app launch. A planned + # handle_global_key lifecycle hook will absorb this (see arch doc Follow-up). if key_event.keycode == KEY_BRACKETLEFT: if economics_app and HudGroups.is_app_active("implant/economics"): economics_app.navigate(-1) @@ -195,8 +200,9 @@ func _unhandled_key_input(event: InputEvent) -> void: func _on_atlas_economics_link(system_id: String) -> void: - # #835 D-191: Pre-filter the economics monitor to the city's system and pop - # the panel open. AtlasApp closes itself before emitting this signal. + # #835 D-191: Pre-filter the economics monitor to the city's system and open + # it as an insert panel. AtlasApp closes automatically when Economics opens + # (HudGroups single-active-app rule → app_changed signal). if economics_app == null: return economics_app.select_system(system_id) diff --git a/client/ui/implant/implant_app.gd b/client/ui/implant/implant_app.gd index 702969a87..1be95d6a6 100644 --- a/client/ui/implant/implant_app.gd +++ b/client/ui/implant/implant_app.gd @@ -11,7 +11,6 @@ extends Control signal app_opened(mode: int) signal app_closed -signal insert_deactivated var manifest: ImplantAppManifest = null var nav: ImplantNavStack = null diff --git a/client/ui/implant/implant_nav_stack.gd b/client/ui/implant/implant_nav_stack.gd index 1f051ad3c..670114b87 100644 --- a/client/ui/implant/implant_nav_stack.gd +++ b/client/ui/implant/implant_nav_stack.gd @@ -8,6 +8,7 @@ signal screen_changed(current_screen_id: String) var _stack: Array[String] = [] var _payloads: Array[Dictionary] = [] var _default_screen_id: String = "" +var _mutating: bool = false # re-entrancy guard: set while screen_changed is emitting func set_default(screen_id: String) -> void: @@ -15,30 +16,53 @@ func set_default(screen_id: String) -> void: func push(screen_id: String, payload: Dictionary = {}) -> void: + if _mutating: + push_error( + "ImplantNavStack: nested mutation detected — use call_deferred from screen_changed handler" + ) + return _stack.append(screen_id) _payloads.append(payload) + _mutating = true screen_changed.emit(screen_id) + _mutating = false +# pop() never empties the stack below the default screen — this app always +# has at least one screen visible. To truly reset, use reset_to_default(). func pop() -> void: + if _mutating: + push_error( + "ImplantNavStack: nested mutation detected — use call_deferred from screen_changed handler" + ) + return if _stack.is_empty(): push_warning("ImplantNavStack: pop() on empty stack") return _stack.pop_back() _payloads.pop_back() if not _stack.is_empty(): + _mutating = true screen_changed.emit(_stack.back()) + _mutating = false elif not _default_screen_id.is_empty(): push(_default_screen_id) func replace(screen_id: String, payload: Dictionary = {}) -> void: + if _mutating: + push_error( + "ImplantNavStack: nested mutation detected — use call_deferred from screen_changed handler" + ) + return if not _stack.is_empty(): _stack.pop_back() _payloads.pop_back() _stack.append(screen_id) _payloads.append(payload) + _mutating = true screen_changed.emit(screen_id) + _mutating = false func reset_to_default() -> void: diff --git a/client/ui/loading_screen.gd b/client/ui/loading_screen.gd index fad4e8222..8a149f368 100644 --- a/client/ui/loading_screen.gd +++ b/client/ui/loading_screen.gd @@ -38,6 +38,9 @@ func _build_ui() -> void: _label.mouse_filter = Control.MOUSE_FILTER_IGNORE add_child(_label) + # client_ver and proto_ver are independent — project.yaml version is the client release, + # Protocol.PROTOCOL_VERSION is the wire protocol. Mismatches between builds are visible + # only to the observer reading the loading-screen label; a future ticket will surface them. var client_ver := _read_client_version() var proto_ver: int = Protocol.PROTOCOL_VERSION _version_label = Label.new() diff --git a/docs/architecture/implant-app-pattern.md b/docs/architecture/implant-app-pattern.md index 4ef001460..549663b7f 100644 --- a/docs/architecture/implant-app-pattern.md +++ b/docs/architecture/implant-app-pattern.md @@ -297,6 +297,7 @@ Autoload order: `implant_registry` must scan before `main.gd` queries manifests - **`DataChannels` autoload** — manifest-declared snapshot subscriptions. Seam already reserved via `on_install` lifecycle position. - **Launcher UI** — once a fourth implant app exists, a chooser becomes necessary. Until then, key bindings suffice. - **Settings-UI key remapping** — resolves key-binding collisions beyond first-wins. +- **`handle_global_key` lifecycle hook** — new `ImplantApp` override; shells handle intra-app keys (e.g. economics `[`/`]` system navigation). Main.gd routes unhandled keydown to the active app instead of hardcoding per-app bindings. ### Deferred (Phase 6+, not sprint-scoped)