Page MenuHomeWildfire Games
Differential D2260

Support setting global hotkeys
ClosedPublic
Actions

Authored by elexis on Sep 4 2019, 9:53 PM.

Details

Reviewers
None
Commits
rP22851: Support assigning functions to hotkeys from the JS GUI without involving a GUI…
Summary

During the course of D2257, nani has noticed that it would leave cleaner hotkey code if the hotkeys could be assigned directly, globally, without having to specify an XML object.

Test Plan

The attached patch demonstrates it for gamesetup.xml.
Notice that the approach differs from the object setting hotkey, because one can assign the same hotkey to multiple objects, and currently all of them would be triggered.
What hinders us from assigning multiple functions to one hotkey is that we cant distinguish them from JS, unless we'd introduce a second identifier.
At least the identification would be necessary if one wants to be able to remove / change them again from JS.

Notice that hitting a hotkey still triggers all of the subscribed onpress events / global hotkeys.
Notice that the code looks really nicer, especially since having to specify global XML objects means that they would be repeated, then limited to a fixed number, and mods would still have to insert a custom XML file;
whereas here they can just call the JS function.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 4 2019, 9:53 PM
Vulcan added a comment.Sep 4 2019, 9:55 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/73/display/redirect

elexis mentioned this in D2240: Move Mainmenu from XML to JS and use object oriented JS.Sep 4 2019, 9:57 PM
Vulcan added a comment.Sep 4 2019, 10:02 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/scripting/JSInterface_GUIManager.h
|  24| namespace·JSI_GUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceJSI_GUIManager{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  62|  62| var g_RomanNumbers = [undefined, "I", "II", "III", "IV", "V", "VI", "VII", "VIII"];
|  63|  63| 
|  64|  64| var g_PlayerTeamList = prepareForDropdown([{
|  65|    |-		"label": translateWithContext("team", "None"),
|    |  65|+	"label": translateWithContext("team", "None"),
|  66|  66| 		"id": -1
|  67|  67| 	}].concat(
|  68|  68| 		Array(g_MaxTeams).fill(0).map((v, i) => ({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  63|  63| 
|  64|  64| var g_PlayerTeamList = prepareForDropdown([{
|  65|  65| 		"label": translateWithContext("team", "None"),
|  66|    |-		"id": -1
|    |  66|+	"id": -1
|  67|  67| 	}].concat(
|  68|  68| 		Array(g_MaxTeams).fill(0).map((v, i) => ({
|  69|  69| 			"label": i + 1,
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  64|  64| var g_PlayerTeamList = prepareForDropdown([{
|  65|  65| 		"label": translateWithContext("team", "None"),
|  66|  66| 		"id": -1
|  67|    |-	}].concat(
|    |  67|+}].concat(
|  68|  68| 		Array(g_MaxTeams).fill(0).map((v, i) => ({
|  69|  69| 			"label": i + 1,
|  70|  70| 			"id": i
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  65|  65| 		"label": translateWithContext("team", "None"),
|  66|  66| 		"id": -1
|  67|  67| 	}].concat(
|  68|    |-		Array(g_MaxTeams).fill(0).map((v, i) => ({
|    |  68|+	Array(g_MaxTeams).fill(0).map((v, i) => ({
|  69|  69| 			"label": i + 1,
|  70|  70| 			"id": i
|  71|  71| 		}))
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  66|  66| 		"id": -1
|  67|  67| 	}].concat(
|  68|  68| 		Array(g_MaxTeams).fill(0).map((v, i) => ({
|  69|    |-			"label": i + 1,
|    |  69|+		"label": i + 1,
|  70|  70| 			"id": i
|  71|  71| 		}))
|  72|  72| 	)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  67|  67| 	}].concat(
|  68|  68| 		Array(g_MaxTeams).fill(0).map((v, i) => ({
|  69|  69| 			"label": i + 1,
|  70|    |-			"id": i
|    |  70|+		"id": i
|  71|  71| 		}))
|  72|  72| 	)
|  73|  73| );
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  68|  68| 		Array(g_MaxTeams).fill(0).map((v, i) => ({
|  69|  69| 			"label": i + 1,
|  70|  70| 			"id": i
|  71|    |-		}))
|    |  71|+	}))
|  72|  72| 	)
|  73|  73| );
|  74|  74| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  69|  69| 			"label": i + 1,
|  70|  70| 			"id": i
|  71|  71| 		}))
|  72|    |-	)
|    |  72|+)
|  73|  73| );
|  74|  74| 
|  75|  75| /**
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  78|  78| var g_RelicCountList = Object.keys(g_CivData).map((civ, i) => i + 1);
|  79|  79| 
|  80|  80| var g_PlayerCivList = g_CivData && prepareForDropdown([{
|  81|    |-		"name": translateWithContext("civilization", "Random"),
|    |  81|+	"name": translateWithContext("civilization", "Random"),
|  82|  82| 		"tooltip": translate("Picks one civilization at random when the game starts."),
|  83|  83| 		"color": g_ColorRandom,
|  84|  84| 		"code": "random"
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  79|  79| 
|  80|  80| var g_PlayerCivList = g_CivData && prepareForDropdown([{
|  81|  81| 		"name": translateWithContext("civilization", "Random"),
|  82|    |-		"tooltip": translate("Picks one civilization at random when the game starts."),
|    |  82|+	"tooltip": translate("Picks one civilization at random when the game starts."),
|  83|  83| 		"color": g_ColorRandom,
|  84|  84| 		"code": "random"
|  85|  85| 	}].concat(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  80|  80| var g_PlayerCivList = g_CivData && prepareForDropdown([{
|  81|  81| 		"name": translateWithContext("civilization", "Random"),
|  82|  82| 		"tooltip": translate("Picks one civilization at random when the game starts."),
|  83|    |-		"color": g_ColorRandom,
|    |  83|+	"color": g_ColorRandom,
|  84|  84| 		"code": "random"
|  85|  85| 	}].concat(
|  86|  86| 		Object.keys(g_CivData).filter(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  81|  81| 		"name": translateWithContext("civilization", "Random"),
|  82|  82| 		"tooltip": translate("Picks one civilization at random when the game starts."),
|  83|  83| 		"color": g_ColorRandom,
|  84|    |-		"code": "random"
|    |  84|+	"code": "random"
|  85|  85| 	}].concat(
|  86|  86| 		Object.keys(g_CivData).filter(
|  87|  87| 			civ => g_CivData[civ].SelectableInGameSetup
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  82|  82| 		"tooltip": translate("Picks one civilization at random when the game starts."),
|  83|  83| 		"color": g_ColorRandom,
|  84|  84| 		"code": "random"
|  85|    |-	}].concat(
|    |  85|+}].concat(
|  86|  86| 		Object.keys(g_CivData).filter(
|  87|  87| 			civ => g_CivData[civ].SelectableInGameSetup
|  88|  88| 		).map(civ => ({
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  83|  83| 		"color": g_ColorRandom,
|  84|  84| 		"code": "random"
|  85|  85| 	}].concat(
|  86|    |-		Object.keys(g_CivData).filter(
|    |  86|+	Object.keys(g_CivData).filter(
|  87|  87| 			civ => g_CivData[civ].SelectableInGameSetup
|  88|  88| 		).map(civ => ({
|  89|  89| 			"name": g_CivData[civ].Name,
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  84|  84| 		"code": "random"
|  85|  85| 	}].concat(
|  86|  86| 		Object.keys(g_CivData).filter(
|  87|    |-			civ => g_CivData[civ].SelectableInGameSetup
|    |  87|+		civ => g_CivData[civ].SelectableInGameSetup
|  88|  88| 		).map(civ => ({
|  89|  89| 			"name": g_CivData[civ].Name,
|  90|  90| 			"tooltip": g_CivData[civ].History,
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  85|  85| 	}].concat(
|  86|  86| 		Object.keys(g_CivData).filter(
|  87|  87| 			civ => g_CivData[civ].SelectableInGameSetup
|  88|    |-		).map(civ => ({
|    |  88|+	).map(civ => ({
|  89|  89| 			"name": g_CivData[civ].Name,
|  90|  90| 			"tooltip": g_CivData[civ].History,
|  91|  91| 			"color": g_ColorRegular,
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  86|  86| 		Object.keys(g_CivData).filter(
|  87|  87| 			civ => g_CivData[civ].SelectableInGameSetup
|  88|  88| 		).map(civ => ({
|  89|    |-			"name": g_CivData[civ].Name,
|    |  89|+		"name": g_CivData[civ].Name,
|  90|  90| 			"tooltip": g_CivData[civ].History,
|  91|  91| 			"color": g_ColorRegular,
|  92|  92| 			"code": civ
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  87|  87| 			civ => g_CivData[civ].SelectableInGameSetup
|  88|  88| 		).map(civ => ({
|  89|  89| 			"name": g_CivData[civ].Name,
|  90|    |-			"tooltip": g_CivData[civ].History,
|    |  90|+		"tooltip": g_CivData[civ].History,
|  91|  91| 			"color": g_ColorRegular,
|  92|  92| 			"code": civ
|  93|  93| 		})).sort(sortNameIgnoreCase)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  88|  88| 		).map(civ => ({
|  89|  89| 			"name": g_CivData[civ].Name,
|  90|  90| 			"tooltip": g_CivData[civ].History,
|  91|    |-			"color": g_ColorRegular,
|    |  91|+		"color": g_ColorRegular,
|  92|  92| 			"code": civ
|  93|  93| 		})).sort(sortNameIgnoreCase)
|  94|  94| 	)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  89|  89| 			"name": g_CivData[civ].Name,
|  90|  90| 			"tooltip": g_CivData[civ].History,
|  91|  91| 			"color": g_ColorRegular,
|  92|    |-			"code": civ
|    |  92|+		"code": civ
|  93|  93| 		})).sort(sortNameIgnoreCase)
|  94|  94| 	)
|  95|  95| );
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  90|  90| 			"tooltip": g_CivData[civ].History,
|  91|  91| 			"color": g_ColorRegular,
|  92|  92| 			"code": civ
|  93|    |-		})).sort(sortNameIgnoreCase)
|    |  93|+	})).sort(sortNameIgnoreCase)
|  94|  94| 	)
|  95|  95| );
|  96|  96| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|  91|  91| 			"color": g_ColorRegular,
|  92|  92| 			"code": civ
|  93|  93| 		})).sort(sortNameIgnoreCase)
|  94|    |-	)
|    |  94|+)
|  95|  95| );
|  96|  96| 
|  97|  97| /**
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
|1328|1328| 			offset = -Math.min(slideSpeed * dt, maxOffset);
|1329|1329| 	}
|1330|1330| 
|1331|    |-	updateSettingsPanelPosition(offset);	
|    |1331|+	updateSettingsPanelPosition(offset);
|1332|1332| }
|1333|1333| 
|1334|1334| /**
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/582/display/redirect

Harbormaster completed remote builds in B9223: Diff 9629.Sep 4 2019, 10:02 PM
elexis added inline comments.Sep 5 2019, 12:25 PM
source/gui/CGUI.cpp
85 ↗(On Diff #9629)

For the record, I inserted a return IN_HANDLED; here and in the handling below, but decided to remove it again, just so as to stay consistent.

If we want only one hotkey function to trigger, then it should become distinctly visible as a separate design choice, and not appear like this would be a necessity for this patch (i.e. the patch would appear restrictive to the devils advocate).

85 ↗(On Diff #9629)

JS::HandleValueArray::empty()

508 ↗(On Diff #9629)

Catch empty hotkey

516 ↗(On Diff #9629)

(If someone wants to see SpiderMonkey gone wild, change this to = function;)

source/gui/CGUI.h
647 ↗(On Diff #9629)

There will be maybe 20 of these values per GUI page, so garbage collection effects are more than negligible.
Can be beautified with SM60+ (put on the heap without adding tracing code).

This revision was not accepted when it landed; it landed in state Needs Review.Sep 5 2019, 12:42 PM
Closed by commit rP22851: Support assigning functions to hotkeys from the JS GUI without involving a GUI… (authored by elexis). · Explain Why
This revision was automatically updated to reflect the committed changes.
elexis added a commit: rP22851: Support assigning functions to hotkeys from the JS GUI without involving a GUI….
Owners added a subscriber: Restricted Owners Package.Sep 5 2019, 12:42 PM
elexis mentioned this in rP22854: Rewrite Main Menu..Sep 6 2019, 3:21 AM
elexis mentioned this in D2273: Fix structree / history civ button rebase error in D2240 / rP22854 and global hotekys triggering upon release in rP22851.Sep 15 2019, 11:20 AM
wraitii mentioned this in D2295: Improve js glue code in D1839/rP22909 (hotkey state fix) and deal with rebase issues..Sep 15 2019, 4:35 PM
wraitii mentioned this in rP22909: Fix hotkey events synching with hotkey state..Sep 15 2019, 6:39 PM
elexis mentioned this in rP22920: Fix struc tree / civ info hotkeys following rebase error in rP22854 / D2240 and….Sep 18 2019, 12:43 AM
elexis mentioned this in D2355: Rewrite session chat to use class syntax / object-oriented programming.Oct 5 2019, 12:13 PM
elexis mentioned this in rP23062: Rewrite session chat to use hierarchical object oriented design using class….Oct 6 2019, 4:43 PM
nani mentioned this in D1785: Custom building placement hotkeys + minor fix..Oct 9 2019, 2:09 PM
elexis mentioned this in D2391: OverlayCounter classes.Oct 26 2019, 8:21 PM
elexis mentioned this in D2410: Send NetClient GUIMessages to subscribing GUIObjects instead of pulling them onTick.Nov 26 2019, 9:32 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
gamesetup/
19 lines
12 lines
source/
gui/
12 lines
38 lines
scripting/
4 lines
14 lines

Diff 9635

View Options

ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.xml

Loading...
View Options

ps/trunk/source/gui/CGUI.h

Loading...
View Options

ps/trunk/source/gui/CGUI.cpp

Loading...
View Options

ps/trunk/source/gui/scripting/JSInterface_GUIManager.h

Loading...
View Options

ps/trunk/source/gui/scripting/JSInterface_GUIManager.cpp

Loading...
Wildfire Games · Phabricator