refactor(ui): PR #134 review — MetaScreen/ESC chain tightening, D-192 reword
Addresses Tyre's 9 architecture items from the sprint-36 client review. - decisions/architecture.md (Tyre #1): D-192 now says "deprecate; removal tracked in #868" instead of "remove". The branch does not remove the version field or guard — that belongs in the coordinated server+client PR. The decision text now matches the code on this branch. - meta_stack.gd (#2): handle_escape() on a screen with closable_by_escape=false now consumes the event unconditionally. Was returning whatever on_escape() returned, which default-returned false and leaked ESC into main.gd's implant/settings chain — opening the settings dialog behind the loading screen. - debug_console.gd (#4): drop the direct KEY_ESCAPE branch in _unhandled_input. ESC now falls through to main.gd → MetaStack, which finds the console on top of the stack and closes it via the normal path. Other keys are still consumed so movement/action can't leak. - main.gd (#6, #10): extract the ESC priority chain into _handle_menu_key() so "MetaStack → implant → settings" is a named thing. Add a comment near connect_to_sim explaining that GameState.bookmark_catalog survives the Option A scene transition via the autoload. - main_menu.gd (#7): header comment documenting the double LoadingScreen lifecycle — safe today because main_menu.tscn and main.tscn never co-exist, noted for future promotion to autoload if that changes. - meta_screen.gd (#8): apply captures_input symmetrically in open()/ close() — was set in open() only, so a screen changing the flag between open+close kept the opened value forever. - meta_screen.gd (#9): on_escape() docstring clarifies the tri-state (consume-and-hold / consume-and-close / ignore) — and that closable_by_escape=false is the screen-wide way to say "consume-and-hold". - bug_report_dialog.gd (#11): capture_cancelled now emits from on_close() (covers any close path — ESC, MetaStack pop, programmatic close) rather than only on_escape(). A new _completed flag distinguishes completion from cancel so the two signals stay mutually exclusive.
This commit is contained in:
@@ -751,7 +751,7 @@ Technical foundation decisions that constrain implementation: engine, client-ser
|
||||
|
||||
### D-192: Drop PROTOCOL_VERSION lockstep handshake
|
||||
|
||||
- **Decision:** Remove the `version` field from the snapshot envelope, the `PROTOCOL_VERSION` constants on both server (`server/src/bridge/types.rs`) and client (`client/scripts/protocol/protocol.gd`), and the version-mismatch guard in `Protocol.decode_snapshot()`. Genuine schema mismatches surface as MessagePack decode errors or missing-field errors at the consumer; that signal is sufficient for our deployment model.
|
||||
- **Decision:** Deprecate the snapshot envelope `version` field, the `PROTOCOL_VERSION` constants on both server (`server/src/bridge/types.rs`) and client (`client/scripts/protocol/protocol.gd`), and the version-mismatch guard in `Protocol.decode_snapshot()`. Removal is tracked in ticket **#868** (server + client coordinated, sprint 37 or later). Once removed, genuine schema mismatches will surface as MessagePack decode errors or missing-field errors at the consumer; that signal is sufficient for our deployment model. Until #868 lands, the field and guard stay in place — they are no longer load-bearing, but removing them requires coordinated edits on both sides and fresh fixture regeneration.
|
||||
- **Rationale:** The version constants were designed for a network deployment where client and server can ship out of sync. Our actual deployment is a subprocess: the Godot client launches the Rust server it was built with. They are *always* in sync at runtime — the version check has never caught a real mismatch in the field, only dev-time forgetfulness. The cost has been measurable: every protocol-shaping sprint requires bumping two constants in lockstep, and we accumulated tautological tests asserting `PROTOCOL_VERSION == N` (deleted in sprint 36 — see ticket from this D-record). Removing the handshake makes the per-sprint cost zero. **Reversibility:** When/if networked multiplayer arrives (no firm date — see [D-005](#d-005-architecture-godot-client--rust-server-via-subprocess)), the natural fit is a one-time handshake at connection time (a single client-version vs. server-version exchange in the connection protocol), not a per-snapshot version stamp. So even the multiplayer path doesn't argue for keeping the per-snapshot field — that field would be doubly redundant once a connection-time check exists. The design space hasn't been narrowed.
|
||||
- **What we lose:** A single eager, human-readable error at connect time ("client v22 ↔ server v23"). A genuine dev-time schema drift will now surface as a downstream decode/missing-field error, possibly seconds into a session rather than at handshake.
|
||||
- **What we keep:** All field-presence and roundtrip tests in `test_protocol_bridge.gd`, `test_signal_sprint24.gd`, etc. — these cover the *behavior* the version constant was meant to gate. Decode failure in `Messagepack.decode()` still rejects malformed payloads.
|
||||
|
||||
Reference in New Issue
Block a user