Configurable keyboard shortcuts

[Zuu]

A feature wished by many (including me) for long time...

I've looked a bit into it tonight, so it is far from finished. Though I have some participial lookup code implemented which let you activate the Pause-button though three different keys.

What I want to discuss here is the principals of how this feature is going to be implemented.


Processing of input today
After having looking around how keyboard events are processed I've found out that all windows in OpenTTD have their own event processing function which process key-events. Often they use a switch case block like this:

Code: Select all

case WE_KEYPRESS: {
    switch (e->we.keypress.keycode) {
    case WKC_F1: case WKC_PAUSE: ToolbarPauseClick(w); break;       
    case WKC_F2: ShowGameOptions(); break;
    case WKC_F3: MenuClickSaveLoad(0); break;                         
    // ...
    default: return;
    }
    e->we.keypress.cont = false;
} break;

I think this is the right level to add some lookup mechanism for configurable shortcuts. It could be implemented at lower level but then users would have to map "a" as "c" if they want auto-rail on the c-button , which is quite user unfriendly compared to map "auto-rail"-action on the "c"-button.


Lookup table
The idea I have is that there is a lookup table, an array, which uses pre-defined actions as index.Like this (a bit simplified):

Code: Select all

if (_key_lookup_table[AUTO_RAIL] == event.keycode) 
    ActivateAutoRails();

and somewhere else:

Code: Select all

_key_lookup_table[AUTO_RAIL] = 'A';

Adding some more complexity
For the moment I plan to not just store one keycode but a few to allow multiple key settings per action. (the pause-button can be activated by both F1 and Pause-button today)
Also I think this structure is a good place to store a short descriptive text such as "Auto rails" which can be used in the configuration GUI.

Because of the added complexity I'll use either a macro or an inline function for checking if an action should be activated in the event processing routines. If we have 3 possible keys per action that make 3 tests per action, so a macro or an inline is needed for code readability and flexibility (to be able to change 3 to 2 or 5 or whatever desired).

Code: Select all

// Macro:
IF_KEY_ACTION(e, KA_TOGGLE_PAUSE) 
    ToolbarPauseClick(w);

// Inline function:
if (IsKeyAction(e->we.keypress.keycode, KA_TOGGLE_PAUSE)) 
    ToolbarPauseClick(w);

Side by side
An important thing during the development is that the selected approach can work side by side with the current system. Only a few actions can be changed to use the lookup-table during the time the infrastructure is built up. This is good since declaring actions and action strings for every key shortcut in OpenTTD is a BIG task. But if we first have a working infrastructure it will at least not be wasted time.


Any (constructive) feedback?

A patch will be posted later when I have something with not too messy variable names.


EDIT:
Suggestions on variable names / names of terms are more than welcome! Such as what to call the lookup-table, is there a better term than key-actions? etc...

[Zojj]

This is a great idea and I think you are going in the right direction.

For names, how about

Code: Select all

_hotkeys[HK_AUTORAIL] = 'A';
_hotkeys[HK_CLEAR] = 'D';
...

I don't think you should rewrite the switches inside individual window processes; just replace the key with your global array.

Code: Select all

case WE_KEYPRESS: {
    switch (e->we.keypress.keycode) {
    case _hotkeys[HK_PAUSE]: ToolbarPauseClick(w); break;       
    // ...

If you are planning on this already, it was just your example that confused me.


I'm trying to think of a better way to eliminate duplicate code for mouse clicks and keystrokes that do the same thing... but I cant. =) Ideas on that?

[Rubidium]

Code: Select all

case _hotkeys[HK_PAUSE]: ToolbarPauseClick(w); break;

That's technically impossible.

There are several places where "hotkeys" are handled, so you (sadly enough) can't put all of them in a simple hashmap that points to some callback method.

So I think the best case would be to iterate over the table with hotkeys and once you've found the correct "index", you do a switch on that index. I think that there need to be several of these tables, one for each GUI that uses hotkeys.

Code: Select all

_hotkeys[HK_AUTORAIL] = 'A'

...

HotKey hk = INVALID_HK;
for (HotKey i; i != HK_END; i++) {
    if (_hotkeys[i] == e->we.keypress.keycode) { hk = i; break; }
}
switch (hk) {
    case HK_AUTORAIL: .....
}

[Zuu]

I've not changed the names to use the "hotkey" term. (I just saw your replies when I was to post the patch and did not want to delay that)

So here it is in it's current form. Far from useful. Except if you like to toggle pause with Ctrl+A, P and Pause-button

----

So I think the best case would be to iterate over the table with hotkeys and once you've found the correct "index", you do a switch on that index. I think that there need to be several of these tables, one for each GUI that uses hotkeys.

Code: Select all

_hotkeys[HK_AUTORAIL] = 'A'

...

HotKey hk = INVALID_HK;
for (HotKey i; i != HK_END; i++) {
    if (_hotkeys[i] == e->we.keypress.keycode) { hk = i; break; }
}
switch (hk) {
    case HK_AUTORAIL: .....
}

The idea I had was to have something like this

Code: Select all

IF_KEY_ACTION(e, KA_TOGGLE_PAUSE) ToolbarPauseClick(w);
IF_KEY_ACTION(e, KA_AUTO_RAILS) ShowBuildRailToolbar(_last_built_railtype, 4);
....

But that will result in uglier code than today since less code will be marked up. Also I don't know if many if-statements are slower than a switch case statement.

[Zojj]

Code: Select all

case _hotkeys[HK_PAUSE]: ToolbarPauseClick(w); break;

That's technically impossible.

Whoops. Thanks. =)

I like the loop as it is cleaner than a bunch of ifs, especially for a window with lots of hotkeys. What if you implemented it like this:

Code: Select all

Hotkey GetHotkey(uint16 keycode){
    for (Hotkey i; i != HK_END; i++) {
        if (_hotkeys[i] == keycode) return i;
    }
    return INVALID_HK;
}
...

switch (GetHotkey(e->we.keypress.keycode)) {
    case HK_AUTORAIL: .....
} 

[Zuu]

I like the loop as it is cleaner than a bunch of ifs, especially for a window with lots of hotkeys. What if you implemented it like this:

Code: Select all

Hotkey GetHotkey(uint16 keycode){
    for (Hotkey i; i != HK_END; i++) {
        if (_hotkeys[i] == keycode) return i;
    }
    return INVALID_HK;
}
...

switch (GetHotkey(e->we.keypress.keycode)) {
    case HK_AUTORAIL: .....
} 

I've had a discussion with Rubidium on IRC now and the conclusion was that there will not be massive amount of ifs. Either a single lookup-table with offsets for each GUI window or several lookup-tables, one for each GUI window. A for loop + switch case in some form will probably be the solution.