Rebuild the hotkey lookup whenever bindings change

[burnhamrobertp]

The reverse "action -> keys" lookup (what Spring.GetActionHotKeys and the command-icon hotkey labels use) can be stale, because it's only rebuilt at the tail of ExecuteCommand. At startup the default and user bindings are loaded by Game.cpp calling LoadDefaults() and Load() directly, which skip that path, so the cache sits empty until the first keybinding command runs. Checked in a local build: GetActionHotKeys("pause") returns nothing at boot and is populated after any command. It doesn't surface in BAR because its cmd_bar_hotkeys widget does a keyreload on load, which rebuilds the cache.

This tracks whether bindings actually changed (hotkeysDirty, set on real bind/unbind/etc.) and rebuilds the cache once at the end of the outermost operation when something changed, so LoadDefaults()/Load() now refresh it on their own and the startup gap is gone. As a side effect it also stops the needless rebuild on commands that change nothing (keydebug, redundant binds).

Verified with a small game: GetActionHotKeys("pause") is now correct at startup, and a normal bind still updates it.

This came out of review on #3081 (the KeyBindingsChanged callin), where badosu pointed out the cache should be rebuilt whenever bindings change. It's a pre-existing issue independent of that PR, so it's split out here. The two touch the same spot in ExecuteCommand, so whichever lands second will have a small conflict, and the hotkeysDirty flag is the accurate "did anything change" signal that #3081's event could later use instead of its own.

Used Claude to help with this.

[sprunk]

Wouldn't doing the change below (and in the other tmp in line -826/+838) let us avoid introducing the new "dirty" bool and replace it with the existing buildHotkeyMap?

Suggested change

	buildHotkeyMap = tmpBuildHotkeyMap;
	MaybeRebuildHotkeyMap();
	buildHotkeyMap |= tmpBuildHotkeyMap;
	MaybeRebuildHotkeyMap();

[burnhamrobertp]

Not quite - that line wouldn't do anything on its own. buildHotkeyMap is already false at that point (Load sets it false for the whole batch), so |= and = end up the same there.

To actually drop the extra flag and reuse buildHotkeyMap, Bind/UnBind would have to set buildHotkeyMap true whenever they change a binding. The catch is that Load runs the keys file one line at a time through ExecuteCommand, and the MaybeRebuildHotkeyMap call at the bottom of ExecuteCommand runs for every one of those lines. So the moment a line binds something and sets the flag, that same line rebuilds the whole map, and you end up rebuilding once per binding in the file instead of once at the end. Setting buildHotkeyMap false during Load is what avoids that today.

So I think it needs to stay two separate things: buildHotkeyMap for "are we in the middle of a load" and hotkeysDirty for "did a binding actually change". That's what lets it skip rebuilding on every line and just rebuild once when the load finishes.

[burnhamrobertp]

Going incremental would sidestep this and simplify things - each bind/unbind would keep its own entry current and the rebuild flags could go away. I don't think it's worth it here though: keybindings get read constantly (icon labels, GetActionHotKeys) but changed rarely, so rebuilding the whole map from the current bindings when something changes stays simple and can't leave it half-updated or out of sync with the real bindings, since it's always derived fresh. Incremental would spread that bookkeeping across every path that changes a binding, which is more room for exactly the stale-map problem this fixes. So I'd rather keep the full rebuild and just make sure it runs at the right times, which is what this does.

[sprunk]

I was thinking something like this master...sprunk-patch-11

where there's only calls to MaybeRebuild at the end of every public function, and nothing calls those public functions internally (instead everything calls an Internal equivalent that looks the same and just doesn't have MaybeRebuild). So evey possible call path from the outside has exactly one call to MaybeRebuild at the end. Then you can have just one bool, everything sets it when anything changes, and then it's only checked once at the end.

[burnhamrobertp]

Ah I see now, yeah I don't see any reason why this wouldn't work, and its cleaner from an interface perspective, relies less on convention (I've not looked around but I assume the notion of having methods suffixed with Internal occurs elsewhere)