Skip to content

feat(nr-tlf): one keyboard binding table that the desktop and the web both read - #3337

Merged
rmstdope merged 7 commits into
mainfrom
nr-tlf
Oct 4, 2026
Merged

rmstdope merged 7 commits into
mainfrom
nr-tlf

Conversation

@rmstdope

@rmstdope rmstdope commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

Bead: nr-tlf. Refactoring: a key bound on desktop is silently missing on the web, which cost nr-use a third failed verification.

What changes

  • src/platform/key_bindings.rs now holds KEY_BINDINGS, the single table of which key drives which console input on which console, in both shells. When one key has several rows, the shell tries them in order and stops at the first the plugged device accepts. For example, W tries the Power Pad, then an SNES pad, then the joypad.
  • Desktop: controller_mapping.rs holds no bindings of its own any more. It turns a winit key into a table key and applies that key's rows. The Super Scope's 4 and 5 come from the table too.
  • Web: the new wasm export key_binding_table() provides the web rows, and web/src/input/key_bindings.ts applies them. keyToButtonController1/2, applyKeyboardMapping, keyboard_mapping.ts and superScopeKeyAction are deleted. The page reads the table on the first key event with an emulator loaded. wasm is initialised by then, so the start-up race from nr-di6 does not apply.
  • Parity: one_shell_bindings_are_exactly_the_declared_differences pins every DesktopOnly and WebOnly row.
    • A one-shell binding is now a deliberate edit to that list.
    • The wasm side drops a web row whose input the page cannot apply, and every_web_row_reaches_the_page then goes red. On the page, parseKeyBindingTable throws on such a row.

No key changes meaning

Every existing binding is kept, including the many places where the two shells already differed. Each difference is now listed in the pinned test:

  • Power Pad keys exist on desktop only.
  • The Vs. service button (-) exists on desktop only.
  • Arrow keys exist on desktop only.
  • On the NES and the Game Boy, T and R are swapped between the two shells.
  • The face buttons of an SNES pad plugged into an NES differ between the two shells.
  • GBA uses T/R/Q/E on desktop and G/F/V/B on the web.
  • The SNES has a keyboard player 2 on the web only.

Whether to unify any of these is a player-visible decision for the navigator, and it is not part of this bead.

Small deviations on the web, none of which a player can see:

  • The web Game Boy no longer calls preventDefault on keys it ignores. These are the player-2 keys (IJKLOP90), which went to controller 2, which the Game Boy ignores, and Y/G/Q/E, which only had SNES buttons. Neither set did anything before. Keeping the old behaviour would need rows in the table that do nothing.
  • SNES player-2 keys now go through the same shouldSuppressSnesJoypadInput check as player 1. Before, they sent joypad presses to a port holding a Mouse or Super Scope, which that port ignores.
  • An SNES-only key on a web NES joypad (Q/E/Y/G) is still the page's key. This was restored after the review.

Validation

  • ./scripts/gate-full.sh: passed on 68182bb9. It covers fmt, all three clippy runs, cargo test --all-features --lib (14619 passed), doctests, wasm-pack test including key_binding_table_has_exactly_the_web_rows, the Python suites, tsc, and Vitest (607 passed).
  • cargo test --all-features --lib key_bindings: passed. These are the table tests.
  • ./scripts/test-dir.sh src/frontends/native/keyboard: passed. All the existing desktop keyboard tests are unchanged. New sweeps drive every desktop pad row (NES, GB, GBA, SNES), every Power Pad and SNES-pad row with that device plugged in, and the Vs. service key.
  • npx vitest run web/src/input/key_bindings.test.ts: passed (15 tests).
  • The optional human spot check from the plan (NES WASD/T/R/4/5, Vs. coin on 6, GBA G/F on the web and T/R on desktop) is left to verification.

Web covered: yes. It is the point of the bead.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WcPjTUhFNUMPCdVvQEUMgj

rmstdope and others added 4 commits October 4, 2026 12:01
…e differences pinned

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WcPjTUhFNUMPCdVvQEUMgj
@rmstdope

rmstdope commented Oct 4, 2026

Copy link
Copy Markdown
Owner Author

Review (cold read, reviewed_head: b8bbdf0a08ae2fae411a828dd87b6e19361bb38a): reviewed before merge under the producer's review practice by an agent that was given the diff and the bead, but not the producer's reasoning.

The reviewer checked every row of KEY_BINDINGS against the old tables on main, desktop and web. Each key, each fallback order, the legacy set_snes_button ids and the port routing all match, including the case where both players go to port 2 with one gamepad. The findings below are about test coverage and one small behaviour change; no binding is wrong.

  1. The desktop safety net is narrower than the plan's desktop_honours_every_desktop_row. The sweeps cover NES/GB/GBA Pad rows only. Nothing drives the PowerPad rows, the SnesPadOnNes rows (with an SNES pad plugged in), VsService, or the desktop SNES Pad rows through apply_nes_rows. For example, if the SnesPadOnNes arm returned true without calling set_snes_button, every test would stay green. This should block the merge.
  2. Most rows both shells share are not pinned. Only 18 of the 49 Both rows are pinned, plus a count. Changing both(NES, KeyS, Pad(One, Down)) to Up would turn nothing red. Suggestion: pin the full Both list as a literal.
  3. The web GBA routing to port 1 is untested. applyKeyboardBindings hard-codes [1] for the GBA. Without that special case, the GBA keyboard goes dead when a gamepad is connected, and no test fails.
  4. preventDefault is no longer called on keys that have a mapping but do nothing. Two cases: web GB Y/G/Q/E, and web NES Q/E/Y/G with a plain joypad. Also, web SNES player-2 keys now go through shouldSuppressSnesJoypadInput, which has no visible effect. Not blocking; the PR body should mention both.
  5. Nit: every_row_a_shell_honours_is_reachable treats only Pad as terminal. VsCoin and VsService also end the chain.

…w, test web GBA routing

Answers review findings 1, 2, 3 and 5. An SNES-only key on a web NES
joypad is again the page's key (finding 4).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WcPjTUhFNUMPCdVvQEUMgj
@rmstdope

rmstdope commented Oct 4, 2026

Copy link
Copy Markdown
Owner Author

Answers to the cold review, in 91ac4da7:

  1. Fixed.
    • every_desktop_nes_power_pad_and_snes_pad_row_reaches_its_device plugs a real Power Pad or SNES controller into the row's port. It presses and releases every desktop PowerPad row (24) and SnesPadOnNes row (12), and checks the device's exact button bits.
    • test_minus_holds_the_vs_service_button reads $4016 bit 2 across a press and a release.
    • every_desktop_snes_pad_row_presses_its_button covers all 16 desktop SNES pad rows. For each row it checks that the key and a direct set_button(0, id) leave port 1 in the same state.
    • Mutation check: with the reviewer's example (the SnesPadOnNes arm returning true without calling the setter), the native keyboard suite goes red.
  2. Fixed. both_shells_share_exactly_these_bindings now pins all 49 shared rows as a literal, the same way the one-shell lists are pinned.
  3. Fixed. The port choice moved from app.ts into keyboardPorts() in web/src/input/key_bindings.ts. Vitest now pins that the GBA uses [1] with 0, 1 or 2 gamepads connected, and pins the other consoles' routing.
  4. Partly restored; the rest is accepted and noted in the PR body.
    • Web NES, SNES-only keys (Q/E/Y/G) on a plain joypad: a key that reaches a port is again the page's key and is preventDefaulted, even when no device takes it. Vitest covers this.
    • Web GB, Y/G/Q/E and the player-2 keys: these keys did nothing on the Game Boy before, and they no longer preventDefault. Keeping the old behaviour would mean adding rows to the table that do nothing, which would make the table say something untrue. The browser default for these letter keys only matters for features such as Firefox's type-ahead find.
    • Web SNES player-2 keys now go through shouldSuppressSnesJoypadInput. This has no visible effect, as the review says.
  5. Fixed. every_row_a_shell_honours_is_reachable now treats VsCoin and VsService as ending the chain, like Pad.

…ive tests that read them

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WcPjTUhFNUMPCdVvQEUMgj
@rmstdope

rmstdope commented Oct 4, 2026

Copy link
Copy Markdown
Owner Author

Review (delta, reviewed_head: 91ac4da7, previous: b8bbdf0a): reviewed before merge under the producer's review practice by an agent that was given the delta, the bead, the previous findings and the posted answers, but not the producer's reasoning.

The reviewer checked all five earlier findings against the code and confirmed each one fixed. The web GB change in finding 4 is documented and has no player-visible effect.

  1. Blocking: two gate clippy legs now fail. The test-only accessors Bus::controller_state (src/nes/bus/bus.rs) and InputPorts::port1_state (src/snes/input/mod.rs) are #[cfg(test)], but only the native keyboard tests call them. In the wasm build and in the frontend-only build they are therefore never used, and -D warnings rejects them.

Answer. Fixed in 68182bb9. Both accessors are now #[cfg(all(test, feature = "native"))], the same condition under which frontends::native is compiled, and both clippy legs are clean. The full gate is running again on the new head. The PR body's claim that the gate passed applied only to b8bbdf0a, and is updated once this run is green.

…rned two clippy legs red

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WcPjTUhFNUMPCdVvQEUMgj
@rmstdope
rmstdope merged commit dd8f325 into main Oct 4, 2026
12 checks passed
@rmstdope
rmstdope deleted the nr-tlf branch October 4, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant