Transport Tycoon Forums

Posted: **07 Jul 2008 15:18**

This patch allows the customization of hotkeys. As a starting point, (nearly) all now existing hotkeys were made customizable. Some additional functions which can be assigned hotkeys were included.

There exists no GUI right now, so the customization has to be done in the config file. There are some new sections in the form of [hotkey_*] defined for the config file, which each hold the hotkeys for one toolbar/window. If one of the expected sections is not found, the defaults are loaded and on program exit, they are written to the config file. Invalid hotkeys are deleted from the config file.

It is possible to assign more than one command to one keycode, so it is possible to use kind of macros (e.g.: you could bind RailAuto and Remove to one key). The commands are executed in the order they are read from the config file.

If a keycode is handled by a toolbar, it is not passed on to a higher level. So you can define the same keycode in global/game and a toolbar (e.g. rail) and depending on wether the rail toolbar is open, the global/game or the rail function is used (this makes it possible to use 'A' as autorail and - if road toolbar is open - as autoroad (which was my primary motivation for writing this patch)).

For a detailed information on how to specify the hotkeys and a list of supported toolbars/windows, keys and commands please see the attached textfile.

There are also StringIDs defined for all commands. They are not yet used, but are placeholders for a planned GUI.

The mod-ignore version does ignore additionally pressed modifier keys. So for example if the user pressed STRG+ALT+T and a hotkey is only defined for STRG+T, STRG+T is executed. This is only experimental, and there are some issues with it. Stick with the main version, if you do not really need that behaviour.

Changelog:
v1.0:

Changed the patch to do not include the StringIDs.

v1.1:

Codechanges: splitted OnKeyPress event in order to avoid code repetitions

Codechange: cleaned constructor of the descendants of HotkeyWindow (repetitive code)

Added: command "ZoomToCursor" was added to global section

Documentation: Added a lot of documentation; Now conform to doxygen format codestyle

v1.2:

Fix: missing vehicle owner check on hotkey execution in order_gui.cpp

Codechange: using a defined constant for determining the modifier keys (less magic numbers)

v1.3 (2008-07-20):

Codechange: Splitted the terraform section into terraform_game and terraform_editor and defined commands for each section accordingly

Codechange: Defined an own enum for every section.

Codechange: Using the _build_*_proc arrays for executing the procedures for toolbars. This requires matching enums for those sections.

Codechange: Giving an int as parameter to HandleProcNr to be able to pass the different enums.

Added: Some commands were added:
- [game]: ToggleFastForward
- [editor]: ToggleFastForward

No mod-ignore version any more as there are too many conceptual problems.

v1.4 (2008-09-15):

Fix: Hotkeys were deleted on config save.

Codechange: since r14260, trunk merges keycode for "normal" 0-9 keys and keypad 0-9 keys (FS#2277). Therefore the special keys NUM_0 to NUM_9 are no longer valid.

Added: possibility to specify arbitrary keycodes by using "Kc%d" with "%d" being the numerical value of the keycode.

v1.5 (2008-09-15):

Fix: Specifying an keycode which would have also a string representation by numeric value did result in a duplicate hotkey entry on saving the config file.

v1.6 (2008-10-12):

Codechange: Due to changes in trunk code, the command Transfer in the hotkey_order section is no longer valid.

v1.7 (2009-02-09):

Codechange: In trunk code, all 'patches' references were removed. To stay consistent, the old "ShowPatchOptions" command is now "ShowAdvancedSettings" (like it is used in trunk too).

v1.8 (2009-05-14):

Made ToggleBoundingBoxes configurable in order to be able to use Ctrl+B for other commands. So in [global] section there is a new command: ToggleBoundingBoxes

Posted: **07 Jul 2008 15:44**

Nice job

From a brief look at it, it's very fine indeed.
I just wonder if it wold be required to have a GUI for that.
Might be a very (too) massive one, in my humble opinion.

Posted: **07 Jul 2008 17:10**

/me commits seppuku

Posted: **08 Jul 2008 14:40**

A GUI would certainly be rather large. And as it is really no difficult procedure to make some changes in the config file, you are probably right that no GUI is needed. Especially those, who are interested in changing the hotkeys are probably advanced players and might even have changed other things in the config file.

As a GUI design would need much additional time and lead to a large patch, I will for now release a version without the StringID definitions, which is suitable now and maybe later a patch based on this version which is then only responsible for the GUI (if a GUI is even needed (lets see how the responses are)).

@Wolf01:

the code is not that bad, is it?

Posted: **08 Jul 2008 15:01**

St256 wrote:@Wolf01: the code is not that bad, is it?

To my untrained eye it looks nice

and indeed, GUI is IMO not necessary. I guess you might get an idea on Wolf01's motivation for suicide by reading that: http://www.tt-forums.net/viewtopic.php?f=32&t=26105

Posted: **08 Jul 2008 17:10**

There are quite a few issues, to be honest.

IT's far better then most of the regular stuff been submitted, granted, but still..

One thing that bugs me is the multiple repetitions here and there.
I would like to see the loader of the hotkey list on each window
(read this code ->

Code: Select all

+		/* load the hotkeys from the configuration */
+		this->m_hotkeys = _settings_client.hotkey[HKL_AIRPORT];
+		/* ensure, that we have a HotkeyList */
+		assert(this->m_hotkeys != NULL);

)
be declared and used only once, like a parameter passed on tot he constructor.

Another repetition:

Code: Select all

+		EventState state = ES_NOT_HANDLED;
+		uint i = 0;
+		HotkeyProcs proc_nr;

The ifs cascade in strtokc is not very nice either.

This is your function comment

Code: Select all

+/* convert a string representation of a keycode to its numerical value */
+static uint16 strtokc(const char *str)

This is what it should look like

Code: Select all

+/** convert a string representation of a keycode to its numerical value
+  * @param str string that will  converted 
+  * @return  the converted value, the hotkey really */
+static uint16 strtokc(const char *str)

There are more, those are just out of a quick read (not just by me

)

Posted: **09 Jul 2008 14:17**

There's also some alignments problems

Code: Select all

+		while ((proc_nr = m_hotkeys->GetHotkeyProc(keycode, i)) != INVALID_HP) {
+			switch (proc_nr) {
+				case ...; break;
+				...
+				default: return ES_NOT_HANDLED;
+				} <-- remove a tab
+			state = ES_HANDLED;
+			} <-- remove a tab

and it happens for all handlers

Posted: **09 Jul 2008 22:34**

Hey, there are some advices from developers - seems there is interest in this patch

.

@wolf01: looks like we have done the same work twice - not really efficient.

switch/case statments: i guess have made this mistake in every switch/case statement of this patch

- fixed all of them.

code repetitions:
- i wrote an assign method and added a parameter to the constructor. instead of the duplicate code
- split the OnKeyPress handler to two separate functions to avoid the duplicate code

removed the mentioned if cascade

documented everything according to doxygen/openttd rules.

please see first post for updated patch version 1.1

Posted: **10 Jul 2008 09:56**

As being someone who previously made an attempt on hotkeys I've read through your patch and here is my comments (on version 1.1):

At line 38 you remove an empty line which I guess is only a mistake as you've not changed anything else next to that line.

[edit]Now it looks like I have no comments but I'm not very used to the OTTD coding style (or rather my style is quite different) so I can't really come with any comments on that. But I noticed what approaches you've taken and it's nice too see that you've got so far that you've taken the time to define all actions and change window event processing at most if not all places. Good job I would like to say![/edit]

[slightly off-topic]
I like seeing that you've taken an approach that will make it easy to change the current match between keycode and action. Currently exactly the right modifier state is needed, so if any extra modifier is used to accomplish a key-stroke that was not though of it will not work for the user. In my point of view it should be enough that you have at least the required modifiers down for an action to take place. - extra modifers should not matter.

For this to work SHIFT+DELETE need to be defined before DELETE for example. But that is in my opinion worth the gain of only demanding that at least the defined modifiers should be down rather than exactly as defined.

Code: Select all

//MOD_BITS = a mask that has ones for the bits used to store the modkeys
MOD_BITS = WKC_SHIFT | WKC_META | WKC_ALT | WKC_CTRL;

input_mod = MOD_BITS & input_keycode;
input_key = !MOD_BITS & input_keycode;

// to check if input_keycode matches with the hotkey hk_keycode
// with the criteria that
// - At least the modkeys of hk_keycode should be pressed (but also accept additional mod-keys)
// - The correct non-mod key should be pressed
if( ((MOD_BITS & hk_keycode) & input_mod == input_mod) && (!MOD_BITS & hk_keycode) == input_key)
{
	// execute hotkey
}

I've not actually applied your patch and made the above changes yet so it's more like a concept. And should probably still be left out of this patch and possible come later if the objections are not to big.
[/slightly off-topic]

Posted: **15 Jul 2008 10:13**

I guess not taking additional modifier keys into account is conceptually not as easy as it seems. Suppose that the user presses CTRL+ALT+T and there are two hotkeys defined - one for CTRL+T and one for ALT+T which one should get processed? There is no way to determine which one the user really wanted to press. You could execute both of them, but they may have assigned actions which cancel each other or do other strange things - so also no good idea.

[Edit]
For now, I have modified the code, to do what you suggested, and in the above mentioned case, the combination with the highest numerical value is executed. The order is as follows: SHIFT > CTRL > ALT > META.
To make this more clear: Whenever the user presses any combination including SHIFT, and there is defined a hotkey which uses SHIFT, this command gets executed;

Some examples:

User: CTRL+SHIFT+ALT+T
Defined: CTRL+T and ALT+T
Executed: CTRL+T

User: CTRL+SHIFT+ALT+T
Defined: SHIFT+T and CTRL+ALT+T
Executed: SHIFT+T

I hope that made clear what I was trying to explain.

The hotkeys get sorted by modifier when they are loaded, so the order of definition (in config file) does not matter.

Just an approach and far from perfect.
[/Edit]

Posted: **15 Jul 2008 14:59**

You're right, you brought up some conflicts that I did not though of. But having the program pick one of them (using a defined schema and not by random is probably good enough)

St256 wrote:I hope that made clear what I was trying to explain.

The hotkeys get sorted by modifier when they are loaded, so the order of definition (in config file) does not matter.

Your example makes it quite clear. One question through. You say when you load you sort the definitions sorted by mod-key order. For this to work I think it is essential that if there are two definitions and one is a subset of the other then the one with most mod-keys should be checked for match first. So in the case below CTRL+DELETE need to be checked before just DELETE.

User: CTRL+DELETE
Defined: CTRL+DELETE and DELETE
Executed: should be CTRL + DELETE

I don't know if that is already so, but to me you did not explicitly stated that. Better be sure than sorry.

----

I'm happy to see you've gone and implemented my suggestion in your patch. But if you aim to get your patch into trunk I think it is tactical clever to leave it out so that your first patch only implements configurable hotkeys and then you make a second that ignores extra mod-keys. I've not read your 1.1 patch but I guess it's mainly the detection that is changed, and not many other places that are affected, but I could be wrong.

-----

Edit: Some more review: (all line-numbers are line number in your patch-file.)

Starting from line 1631:

Code: Select all

+	virtual EventState HandleHotkeyProc(HotkeyProcs proc_nr)
+	{
+		switch(proc_nr) {
+			case HP_ORDERS_SKIP: OrderClick_Skip(this, -1); break;
+			case HP_ORDERS_DELETE: OrderClick_Delete(this, -1); break;
+			case HP_ORDERS_GOTO: OrderClick_Goto(this, -1); break;
+			case HP_ORDERS_NONSTOP: OrderClick_Nonstop(this, -1); break;
+			case HP_ORDERS_FULLLOAD: OrderClick_FullLoad(this, -1); break;
+			case HP_ORDERS_UNLOAD: OrderClick_Unload(this, -1); break;
+			case HP_ORDERS_TRANSFER: OrderClick_Transfer(this, -1); break;
+			case HP_ORDERS_SERVICE: OrderClick_Service(this, -1); break;
+			default: return ES_NOT_HANDLED;
+		}
+		return ES_HANDLED;
+	};
+
 	virtual void OnPaint()
 	{
 		bool shared_orders = this->vehicle->IsOrderListShared();
@@ -955,30 +968,6 @@

 		ResetObjectToPlace();
 	}
 
-	virtual EventState OnKeyPress(uint16 key, uint16 keycode)
-	{
-		static const KeyToEvent keytoevent[] = {
-			{'D', OrderClick_Skip},
-			{'F', OrderClick_Delete},
-			{'G', OrderClick_Goto},
-			{'H', OrderClick_Nonstop},
-			{'J', OrderClick_FullLoad},
-			{'K', OrderClick_Unload},
-			//{'?', OrderClick_Transfer},
-			//('?', OrderClick_Service},
-		};
-
-		if (this->vehicle->owner != _local_player) return ES_NOT_HANDLED;             // <-- line 1667 in patch-file
-
-		for (uint i = 0; i < lengthof(keytoevent); i++) {
-			if (keycode == keytoevent[i].keycode) {
-				keytoevent[i].proc(this, -1);
-				return ES_HANDLED;
-			}
-		}
-		return ES_NOT_HANDLED;
-	}

On line 1667 in trunk it makes a check for vehicle owner that I can't find a counter part in your code. Unless that is done before you call the HandleHotkeyProc.

Also one of the comments I got from a dev, I think it was Rubidium, was that in OpenTTD there are situations similar to this one where they define a list of keys and the actions and then use a for-loop. The comment I got with my previous work on hotkeys was that they wanted too keep the for loop structure of the code. In your case it looks like it is possible to do so and probably keep the devs a little more happy.

I see you've also rolled up the rail-toolbar loop which I think need to stay in the compact form used in trunk. (about line 1734)

You should know that getting big patches in trunk takes long time. At some point I guess it would be good if you get on IRC and talk friendly with them. But don't try to force it on them.

But have a look on the event handling procedures of all the windows and find where you've changed loops to switch-case structure and see if you can find a solution to get back to the old structure. Perhaps instead of defining keys as it is done in trunk define hotkey actions (such as HP_ROAD_BRIDGE) in the arrays.

Hope my feedback/review helps.

Posted: **16 Jul 2008 10:19**

Zuu wrote:For this to work I think it is essential that if there are two definitions and one is a subset of the other then the one with most mod-keys should be checked for match first.

It is done exactly like this.

Zuu wrote:On line 1667 in trunk it makes a check for vehicle owner that I can't find a counter part in your code. Unless that is done before you call the HandleHotkeyProc.

You are right, this was never checked in my implementation. It did not introduce bugs, however, as the actual action did check this constraint again (one was able to select the goto tool, but not to add an order). - Fixed in v1.2

Rubidium wrote: 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.

So I guess, the Implementation like it is now is what is liked best (at least by Rubidium) (source: this post).

Thanks for your feedback.

Posted: **17 Jul 2008 08:18**

St256 wrote:
Rubidium wrote: 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.
So I guess, the Implementation like it is now is what is liked best (at least by Rubidium) (source: this post).

I do think that Rubidiums main point is not that the if statement inside the loop only matches exact key code. Instead I do think his main point was the structure with the for-loop and then a switch case block.

The reason I do think it's strategically best to use the current exact match is that that is how it is in trunk. And it would be better if this patch only have one goal - configurable hotkeys. Not also changing how key codes are matched.

However what is a nice side effect of your code is that the if-statement that need to be changed for inexact matching is only at one single place in the entire code which makes a such shift possible, but should strategically come in a later patch.

Posted: **18 Jul 2008 21:22**

Hi,

I earlier proposed that for example rail GUI should use the old for-loop based key/hotkey handling code but using hotkeys instead of keys.

For this I've taken your patch 1.2 and r13707 of trunk and changed the rail_gui.cpp to what I think is a better solution. I've done "svn diff src/rail_gui.cpp > rail_gui.patch"

rail_gui.patch: (2.69 KiB) Downloaded 436 times

So my patch is against trunk and not your patch. And it only contains the rail_gui.cpp file.

Posted: **19 Jul 2008 11:20**

Zuu wrote:I think it is tactical clever to leave it out so that your first patch only implements configurable hotkeys and then you make a second that ignores extra mod-keys.

As the changes are indeed rather isolated (only in the hotkey detection and the additional sorting routine needed for the mod-ignore version), I am going to bugfix/improve both versions and let the devs decide which one they prefer.

Zuu wrote:I earlier proposed that for example rail GUI should use the old for-loop based key/hotkey handling code but using hotkeys instead of keys.

I even have a simpler solution in my head: why not declare a HotkeyProc enum for each window and use the enum values directly as indices for the _build_*_button_proc array? This way you can get rid of the additional for loop. (Currently working on implementing this...)

Concerning your ignore modifier idea, another problem came to my mind: if a user has a toolbar open (e.g.: build-rail) and presses something like CTRL+3 in order to toggle the transparency, this keycode would get handled by the rail toolbar already and be never passed on to the top level toolbar gui which handles the transparency. This is not an easy to solve problem, as it would require changes to the system of how the keypress events get handled now.

Posted: **20 Jul 2008 13:15**

Simplified code by using different enums for every hotkey section. See first post for updated patch.
Also the ToggleFastForward command can now be assigned to a hotkey (in [hotkey_game] and in [hotkey_editor]).

Posted: **06 Aug 2008 08:37**

Updated to revision r14004. Only codechanges to adapt to the changes that were made in trunk.

Posted: **28 Aug 2008 15:23**

Again an update to the current trunk revision. No functional changes.

Posted: **28 Aug 2008 22:51**

I could not find any info on this, but does it support hotkeys other than the regular 101 keys? There are many keyboards that come with extra keys to program macros. One specific example is the Logitech G15, which has 18 programmable keys. Can they be used for assignment as well?

Posted: **29 Aug 2008 09:02**

St256: Nice to see you've taken up the work again or at least updated the patch to current trunk.

satosphere wrote:I could not find any info on this, but does it support hotkeys other than the regular 101 keys? There are many keyboards that come with extra keys to program macros. One specific example is the Logitech G15, which has 18 programmable keys. Can they be used for assignment as well?

I doubt that. But you can for example check in the SDL docs if they are defined. Perhaps you know the names of these keys? Also have a look in for example the SDL-driver of OpenTTD to make sure they are not defined yet.

Myself I got no extra keys but I have a hardware programmable keyboard. Perhaps simply assign Ctrl+Alt+Shift+normal key in OpenTTD and then make you a hardware macro for that. But it would then be global for all programs. So if you like your idea, perhaps help out and do some research. For it to work in OpenTTD the keys need to be defined in SDL, DirectX and MacOSX.

Transport Tycoon Forums

Patch: configurable hotkeys

Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys

Re: Patch: configurable hotkeys