Skip to content

Add KeyBindingsChanged callin#3081

Open
burnhamrobertp wants to merge 2 commits into
beyond-all-reason:masterfrom
burnhamrobertp:feature/keybindings-changed-callin
Open

Add KeyBindingsChanged callin#3081
burnhamrobertp wants to merge 2 commits into
beyond-all-reason:masterfrom
burnhamrobertp:feature/keybindings-changed-callin

Conversation

@burnhamrobertp

Copy link
Copy Markdown
Contributor

Reviving #2575, which stalled after review. Implements #2519. Built on Phireh's commits (kept as-is so the credit stays with them) with a couple of follow-ups from me for the review feedback.

Main change from #2575: the callin no longer passes anything. Per sprunk's point on the original PR, locking in a payload format seemed premature, and anything a listener needs is already available through Spring.GetKeyBindings. So KeyBindingsChanged() just fires as a signal and listeners re-read if they care. That also drops the half-finished table-building code that was in there.

Smaller things: the inline {} stub in EventClient.h is now a plain declaration with an empty definition in EventClient.cpp, matching the other callins in that section (badosu asked about the braces). The leftover @return and the typo sprunk flagged are gone with the removed block. Kept Phireh's sendEvents batching so keyreload/keyload/unbindall fire a single event instead of one per line.

I left buildHotkeyMap alone for now. badosu noted it isn't set on some mutating ops (e.g. unbindaction) and overlaps with the changedKeys flag, but reworking it looked bigger than this PR, and now that the payload is empty an occasional redundant event is harmless since listeners just re-read. Probably better as a separate follow-up. Right now only keydebug skips the event, and commands that fail already early-return before the emit.

Builds clean for both legacy and headless, and I ran it in a small game: KeyBindingsChanged fires once for a single bind and once for a single unbind, fires exactly once for keyreload and unbindall (not once per line), and doesn't fire at all for keydebug. @badosu does this match the direction you wanted? There's a keybind editor being built in BAR that would consume this, and it only needs the plain signal form.

Used Claude to help with the rework.

@sprunk

sprunk commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator
  • would be nice to squash commits 1-4 into one, and 5-6 into one.
  • seems to be missing an implementation in widget handler.
  • otherwise looks good.

Co-authored-by: Amadeus Folego <amadeusfolego@gmail.com>
@burnhamrobertp burnhamrobertp force-pushed the feature/keybindings-changed-callin branch from 1cfa2e3 to 3d6ac7a Compare July 1, 2026 13:31
@burnhamrobertp

Copy link
Copy Markdown
Contributor Author

Squashed into the two commits.

Good catch on the widget handler, it was missing there. I wired it the same way KeyMapChanged is (into the callin list, a widgetHandler:KeyBindingsChanged that loops the widgets, and the matching function in main.lua), so widgets can hook widget:KeyBindingsChanged now.

I ran the engine side in a small game to check it fires once on a single bind/unbind, once rather than per line on keyreload and unbindall, and not at all on keydebug.

Comment thread cont/LuaUI/main.lua Outdated
@burnhamrobertp burnhamrobertp force-pushed the feature/keybindings-changed-callin branch from 3d6ac7a to d8dca15 Compare July 1, 2026 15:12
Comment thread rts/Game/UI/KeyBindings.cpp Outdated
Comment thread rts/Lua/LuaHandle.cpp
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.

4 participants