Implements keyboard hotkey bindings

[TropicalBastos]

This PR implements keyboard bindings for The Powder Toy. It adds an option in the options window that opens up a new view to which the user can configure hot keys for the main switch block of hotkeys found in GameView::OnKeyPress. These are stored as prefs and whenever a key or combination of keys is pressed we look up if we have a binding for it and if we do it executes the identified function.

By default, all the current hotkeys in the GameView::OnKeyPress switch block are added and stored in prefs if the user does not have any keyboard binding prefs already.

New functions/routines that need to be added as a future default keyboard hotkey can be done so in KeyboardBindingsMap.h.

Reserved keys (and key combinations) can likewise be added in ReservedKeys.h.

Note that configuring a key that has already been added as a binding or is a reserved key will not work and the UI control will revert to the previous assigned hotkey.

[jacob1]

This looks awesome :o, I've wanted something like this for a while.

I will look at it more closely later. I only briefly scrolled through it now.

One suggestion I have is that defining stuff like REDO and REDO_2 is awkward. What if you could allow one shortcut to have multiple key bindings? I see you do the _2 thing in a few places. It would probably need some updates to the interface (I haven't looked at the interface yet).

Also, be sure to use tabs instead of 4 spaces.

I think this PR would be helpful in the future too when we add controller support (There's also a TODO to add that). We were trying to come up with optimal button mappings, allowing users to configure their own mappings would be best (I'll still have to come up with defaults though). Speaking of defaults, I do like that you are future proofing adding additional key shortcuts.

[TropicalBastos]

Thanks, yes so I guess I could implement a way to reduce the '_2' suffix and have multiple bindings under one function, I'd have to look into a workaround when time is more on my side.

Looks like the culprit for the spaces is my editor (as I'm always a tab guy). I'll change the indentation of all the new files to use tabs, thanks for pointing it out!

Anything else thats required just let me know, also feel free to make changes to this branch as you see fit.

[LBPHacker]

Going to have a look at this myself shortly.

[LBPHacker]

I have a bunch of suggestions which I would have implemented myself but I don't have the time right now and will likely be unable to work on them for some time to come, so I'll just dump what I think should be done here. Don't take the names (type names, variable names, identifiers in general) seriously.

We want multiple shortcuts to do the same thing, so we should implement a many-to-one mapping, not a one-to-one with a certain actions duplicated, as @jacob1 mentioned earlier.

There's also the problem that we don't really have any universal way of representing keys other than scancodes, and even these are not universal, but are the least likely to change on any given hardware setup. I don't trust SDL_GetKeyName to return a universal name, and the docs do say it returns a human-readable (!) name for the keycode (!). In light of this, I'd rather use scancodes for anything the user won't likely ever see and use SDL_GetScancodeName for displaying stuff, and only for displaying stuff.

In GameView, this would mean ditching GetFunctionIdForKeyboardBinding and just doing a map lookup of some sorts instead:

	// assuming we have this map somewhere in GameModel:
	std::map<std::tuple<int, int>, Action> key_binding_to_action;
	// Action is what you seem to have named KeyboardBindingFunction; it has
	// to stay an enum so we can switch on it.

	// GameView.cpp, before the action switch
	...
	if (repeat)
		return;

	auto action_it = key_binding_to_action.find(std::make_tuple(
		(shift ? MOD_SHIFT : 0) | // MOD_* are powers of 2, just make sure
		(ctrl ? MOD_CTRL : 0) |   // |-ing them together yields an int in the end
		(alt ? MOD_ALT : 0),      // so as to not confuse std::make_tuple
		scan   // excuse the formatting, this is the second field of the tuple
	));
	bool didKeyShortcut = false;
	if (action_it != key_binding_to_action.end())
	{
		didKeyShortcut = true;
		switch (action_it->second) // decltype(action_it->second) = Action
		{
			...
		}
	}
	...

This key_binding_to_action would be loaded from the preferences on startup and would be updated by KeyboardBindingsModel (which you don't yet seem to have?), much like how e.g. OptionsModel::SetMouseClickRequired updates mouseClickRequired, though in this case I guess GameModel could just be told to reload it from preferences instead of passing it directly.

Since key_binding_to_action is your everyday many-to-one mapping, it could be easily stored directly in the json config node KeyboardBindings. I'm thinking of a string -> int mapping where the string is actually some ugly way of gluing keyboard modifiers onto SDL scancodes, so maybe something like "int+int", and the int is an Action.

I also have a user interface in mind that could control this. There could be a row for every Action with as many KeyboardBindingTextboxes (KBTs) as many shortcuts are mapped to said Action. There'd be a - button next to every KBT and there'd be a + button somewhere in the first row that would add another KBT. These KBTs should enter sampling mode when focused (as currently do) but they should allow conflicting shortcuts to be set (as they currently do not). They would of course need to somehow let the user know when there are conflicts and disable the OK action in the window.

Excuse the ASCII mockup:

+-------------------------------------------------------+
|                           ___   ______________   ___  |
|  Reload Simulation       |_+_| |____Ctrl+R____| |_-_| |
|                                 ______________   ___  |
|                                |______F5______| |_-_| |
|                                                       |
|                           ___   ______________   ___  |
|  Open Element Search     |_+_| |______E_______| |_-_| |
|                                                       |
|                           ___                         |
|  Toggle Air Mode         |_+_|   (No shortcut found)  | /* in case someone   
|                                                       | hates this feature */
|                                                       |
~   ~   ~   ~   ~   ~   ~   ~   ~   ~   ~   ~   ~   ~   ~

@TropicalBastos, I hope all this doesn't differ too wildly from what you had in mind. Feel free to ask, I threw this comment together in a hurry and I may have left things out.

@jacob1, thoughts?

[jacob1]

I trust SDL_GetKeyName a little more than you do., and I actually don't trust SDL_GetScancodeName, because the documentation has a big warning on it "The returned name is by design not stable across platforms". I think it should be stable across platforms.

Problem is, I intentionally switched us from key codes to scan codes last year, to fix issues with keyboard layouts. Shortcuts need to remain consistent no matter what your keyboard layout looks like. So, we should continue using scancodes regardless of the potential issues. The differences in scancodes are probably minimal and shouldn't affect us, we don't have any key shortcuts based on windows key, and the other scancodes we use are probably the same across platforms (we should verify this).

Ascii mockup looks good to me. I haven't compiled this yet to see how it looks.

[TropicalBastos]

Mockup looks great and would fit really well into a many to one mapping. When I get some free time I'll look into implementing it - on the other hand if anyone else wants to implement the many-to-one system then feel free to as I don't have much free time this coming week but I'll certainly look into squeezing some time to get this feature out the door.

UI feedback for conflicting hotkey mappings is a good idea and I can put that on my todo list too. I also like the idea of disabling the OK button if there is a conflict and just let the user do all the configuring they need.

I'm really like the suggestion of having a KeyboardBindingsModel deal with data persistence and load the appropriate prefs into memory, definitely a good call for this feature. As for the mapping I suppose using scan codes + modifier combo would be more future proof than key names, thanks for the suggestions @LBPHacker

Regarding the Windows key, we could always add the it as a reserved key, the same goes for any keys that are not cross platform.

[jacob1]

I was talking with LBPHacker on IRC, and decided the part about scancode names was irrelevant anyway. We can store to powder.pref as scancode (not the name). When displaying to the user we can use SDL_GetScancodeName, and it won't matter whether or not it's the same across platforms.
So basically that part is irrelevant. LBPHacker was right originally that we should use scancodes instead of keycodes, and store to powder.pref as scancode (not as a name)

[TropicalBastos]

Thanks for the all the suggestions guys, I've started working on them and I'm planning to get it all done by the end of the week provided time is on my side. I've currently finished the many to one mapping on the backend via a KeyboardBindingsModel so its really just the UI and polishing.

[LBPHacker]

Could you please join IRC sometime? We've discussed a lot of things about this and just dumping it all here would not be too reasonable. We would also like to avoid a situation in which you and us take different directions in the development in this feature.

TPT is a huge project, and it's also fairly old. Certain features are just asking to be redesigned; keyboard bindings are one of those, and its redesign requires great care and coordination. While it would be nice to have all related conversation here, we'd still like it more if you joined IRC and continued there.

We're in #powder-dev on Freenode.

[TropicalBastos]

@LBPHacker Okay thanks, I have joined but I see no conversation history nor any conversation, is this normal?

[moonheart08]

@TropicalBastos Yes. IRC doesn't normally implement conversation history. A lot of channels have logs handled by a bot, so you just go to a website to view them. At the moment, this is not the case for #powder-dev.
Your best bet would be to keep a client open.

[TropicalBastos]

I'm going to start pushing some work in progress to the branch, so ignore the unfinished work.

@moonheart08 Thanks for the info, it's appreciated. I keep getting disconnected and losing the chat, but I guess I'll just have to be careful.

@LBPHacker I should have some more free time to discuss approaches and direction over IRC in the weekend.

[TropicalBastos]

Almost done, a few more tweaks on the frontend (and the gameview listener) and then a bit of a refactor to neaten it up and then it will be reviewable again (and open to improvement).

@jacob1 @LBPHacker Sorry my machine disconnected from the IRC the other night, but I managed to catch that you guys need some work on the Mac stuff and I'll be glad to help in that department when I get some time relief again.

[TropicalBastos]

View and backend done implementation wise, might need a clean up in parts (flattening include trees etc, maybe a refactor in parts?) but its working well.

Just the handler in the GameView left and thats pretty much it.

[TropicalBastos]

The following has been fully implemented:

• Many to one mapping of shortcuts to functions
• New UI that reflects @LBPHacker 's mockup
• Reset to defaults button added
• OK button disabled upon hotkey conflict
• Alert message tells the user that there is a conflict if there is one
• Can attach multiple or no bindings to a function. When a function has no bindings it displays "No shortcut"

Feel free to review/refactor/enhance to your heart's content, but its working really well so far from what I've tested.

[LBPHacker]

@TropicalBastos Finally got some time to look into this.

[TropicalBastos]

@LBPHacker I've been out of touch too, been very busy with work life unfortunately - so my apologies for not attending the IRC etc. Also, if you feel like refactoring anything, feel free to do so and if you have any questions let me know. Thanks.

[LBPHacker]

Took the liberty of rebasing your branch to master.

[LBPHacker]

And of squashing your commits.

[LBPHacker]

@TropicalBastos Hey, I'm still alive. I made a new branch for this because pushing this many changes to a PR just doesn't feel right, but your branch should be FF-able to that one. This feature is turning out to be a much bigger hassle than I initially thought, but it's going alright. Expect more stuff to come in the following week.