fix(ui): PR #131 review round 3 — nits sweep
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 <noreply@anthropic.com>
This commit is contained in:
+13
-7
@@ -178,14 +178,19 @@ func _unhandled_key_input(event: InputEvent) -> void:
|
|||||||
return
|
return
|
||||||
var key_event := event as InputEventKey
|
var key_event := event as InputEventKey
|
||||||
# Registry-driven toggle: each manifest declares its own default_key.
|
# Registry-driven toggle: each manifest declares its own default_key.
|
||||||
for manifest: Variant in ImplantRegistry.get_manifests():
|
for manifest: ImplantAppManifest in ImplantRegistry.get_manifests():
|
||||||
if manifest.get("default_key") == key_event.keycode:
|
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(
|
HudGroups.toggle_app(
|
||||||
manifest.get("app_path"),
|
manifest.app_path,
|
||||||
ImplantRegistry.get_resolved_mode(manifest.get("app_path", ""))
|
ImplantRegistry.get_resolved_mode(manifest.app_path)
|
||||||
)
|
)
|
||||||
return
|
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 key_event.keycode == KEY_BRACKETLEFT:
|
||||||
if economics_app and HudGroups.is_app_active("implant/economics"):
|
if economics_app and HudGroups.is_app_active("implant/economics"):
|
||||||
economics_app.navigate(-1)
|
economics_app.navigate(-1)
|
||||||
@@ -195,8 +200,9 @@ func _unhandled_key_input(event: InputEvent) -> void:
|
|||||||
|
|
||||||
|
|
||||||
func _on_atlas_economics_link(system_id: String) -> 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
|
# #835 D-191: Pre-filter the economics monitor to the city's system and open
|
||||||
# the panel open. AtlasApp closes itself before emitting this signal.
|
# it as an insert panel. AtlasApp closes automatically when Economics opens
|
||||||
|
# (HudGroups single-active-app rule → app_changed signal).
|
||||||
if economics_app == null:
|
if economics_app == null:
|
||||||
return
|
return
|
||||||
economics_app.select_system(system_id)
|
economics_app.select_system(system_id)
|
||||||
|
|||||||
@@ -11,7 +11,6 @@ extends Control
|
|||||||
|
|
||||||
signal app_opened(mode: int)
|
signal app_opened(mode: int)
|
||||||
signal app_closed
|
signal app_closed
|
||||||
signal insert_deactivated
|
|
||||||
|
|
||||||
var manifest: ImplantAppManifest = null
|
var manifest: ImplantAppManifest = null
|
||||||
var nav: ImplantNavStack = null
|
var nav: ImplantNavStack = null
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ signal screen_changed(current_screen_id: String)
|
|||||||
var _stack: Array[String] = []
|
var _stack: Array[String] = []
|
||||||
var _payloads: Array[Dictionary] = []
|
var _payloads: Array[Dictionary] = []
|
||||||
var _default_screen_id: String = ""
|
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:
|
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:
|
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)
|
_stack.append(screen_id)
|
||||||
_payloads.append(payload)
|
_payloads.append(payload)
|
||||||
|
_mutating = true
|
||||||
screen_changed.emit(screen_id)
|
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:
|
func pop() -> void:
|
||||||
|
if _mutating:
|
||||||
|
push_error(
|
||||||
|
"ImplantNavStack: nested mutation detected — use call_deferred from screen_changed handler"
|
||||||
|
)
|
||||||
|
return
|
||||||
if _stack.is_empty():
|
if _stack.is_empty():
|
||||||
push_warning("ImplantNavStack: pop() on empty stack")
|
push_warning("ImplantNavStack: pop() on empty stack")
|
||||||
return
|
return
|
||||||
_stack.pop_back()
|
_stack.pop_back()
|
||||||
_payloads.pop_back()
|
_payloads.pop_back()
|
||||||
if not _stack.is_empty():
|
if not _stack.is_empty():
|
||||||
|
_mutating = true
|
||||||
screen_changed.emit(_stack.back())
|
screen_changed.emit(_stack.back())
|
||||||
|
_mutating = false
|
||||||
elif not _default_screen_id.is_empty():
|
elif not _default_screen_id.is_empty():
|
||||||
push(_default_screen_id)
|
push(_default_screen_id)
|
||||||
|
|
||||||
|
|
||||||
func replace(screen_id: String, payload: Dictionary = {}) -> void:
|
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():
|
if not _stack.is_empty():
|
||||||
_stack.pop_back()
|
_stack.pop_back()
|
||||||
_payloads.pop_back()
|
_payloads.pop_back()
|
||||||
_stack.append(screen_id)
|
_stack.append(screen_id)
|
||||||
_payloads.append(payload)
|
_payloads.append(payload)
|
||||||
|
_mutating = true
|
||||||
screen_changed.emit(screen_id)
|
screen_changed.emit(screen_id)
|
||||||
|
_mutating = false
|
||||||
|
|
||||||
|
|
||||||
func reset_to_default() -> void:
|
func reset_to_default() -> void:
|
||||||
|
|||||||
@@ -38,6 +38,9 @@ func _build_ui() -> void:
|
|||||||
_label.mouse_filter = Control.MOUSE_FILTER_IGNORE
|
_label.mouse_filter = Control.MOUSE_FILTER_IGNORE
|
||||||
add_child(_label)
|
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 client_ver := _read_client_version()
|
||||||
var proto_ver: int = Protocol.PROTOCOL_VERSION
|
var proto_ver: int = Protocol.PROTOCOL_VERSION
|
||||||
_version_label = Label.new()
|
_version_label = Label.new()
|
||||||
|
|||||||
@@ -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.
|
- **`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.
|
- **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.
|
- **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)
|
### Deferred (Phase 6+, not sprint-scoped)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user