[PATCH] local.cfg adds keys instead of replacing them

[vts]

When defining custom keys in local.cfg, the keys appear to work in addition to the existing ones from default.cfg, rather than instead of them.

For instance. Specifying

hotkey.camera.left = Q, LeftArrow

in local.cfg causes the Q key to activate both camera.left and the existing camera.rotate.cw from default.cfg:

hotkey.camera.rotate.cw = "Ctrl+LeftArrow", "Ctrl+A", Q

The same is true for e.g. trying to change hotkey.console.toggle; the keys specified in local.cfg will work in addition to the ones from default.cfg, not instead of.

Also, using local.cfg to try and set the Escape key to certain commands does not work. hotkey.menu.toggle = Escape does not toggle the menu ingame when Escape is pressed, but the F10 key from default.cfg still does.

[Bogdan Lazic]

I will try to solve it.

[Bogdan Lazic]

Idea is to use set to detect all hotkey shortcut which is already used. Order in which config file is applied in Config.cpp is changed. So if shortcut already exist then do not add it again. Exception is Shift and Ctrl, because they can be used multiple time.

[historic_bruno]

Replying to Bogi:

Idea is to use set to detect all hotkey shortcut which is already used. Order in which config file is applied in Config.cpp is changed. So if shortcut already exist then do not add it again. Exception is Shift and Ctrl, because they can be used multiple time.

First, thanks for the patch and sorry it took a while to review :)

You should probably use the definitions in KeyName.cpp instead of hard-coding e.g. "Ctrl".

The patch's logic still allows for multiple identical hotkeys because if a hotkey has a modifier in it, it's instantly excluded from testing the used set, meaning Ctrl+Foo could still be used multiple times.

There's another bigger complication: any modifier key can be used multiple times, not just Ctrl and Shift, but Alt and also multiple combinations, in fact you can use combinations of almost any keys (I almost think you need multiple used sets to handle all these weird cases). In other words, the engine doesn't require that Ctrl and Shift come first in the argument. Consider the valid hotkeys "Shift+Alt+Ctrl+C" or "F9+Alt".

I'm not sure what the best solution is. We need something like the parsing logic of Hotkey.cpp so that we can break the hotkey strings into distinct keys, and then they have to be compared against all previously defined combinations of keys, but ignoring argument order, and that has to be done respecting the "priority" of config files. Feel free to discuss :)

[Philip Taylor]

I think a better solution would be to change CConfigDB::GetValuesWithPrefix. Currently it returns every config key-value pair whose key matches the prefix, regardless of config namespace. Instead, if a key occurs in two or more namespaces, it should only return the key-value pairs for that key from the highest-priority one of those namespaces.

[Bogdan Lazic]

It is another patch, it should be a better solution then previous one.

[Philip Taylor]

Hmm, RemoveOverlapingHotkeys() seems to break things: It's intentional that the same key sequence might be used for multiple hotkeys, e.g. ctrl+rightarrow is both "text.move.right" (when a GUI text input is focused) and "camera.rotate.ccw" (when not handled by the GUI), but that function removes one of those bindings. So I think the original bug report about the Q key is not a bug - it's perfectly legitimate to bind one key to multiple actions, in either a single config file or multiple. If we had a hotkey-editing UI then it probably ought to warn you if you add a binding that conflicts with another binding, but that's a problem for the UI and not for the config system.

The change in GetValuesWithPrefix seems to work, but I'll simplify it by returning a std::map to guarantee uniqueness of keys, instead of returning a std::vector and having to manually check for duplicates.

[philip]

In 11535:

Fix #953 (local.cfg adds keys instead of replacing them)