Paths

Table of Contentst

Diffusion 0 A.D. Public Repository rP22909

Fix hotkey events synching with hotkey state.
AuditedrP22909
Actions

Description

Fix hotkey events synching with hotkey state.

This is a semi-revert of rP19028 and a correct fix for #3194.

The core issue is that the GUI handler must come behore the hotkey handler, otherwise typing in boxes can set off hotkeys, and the hotkey handler is repsonsible for updating the hotkey state.
Thus the GUI handler never has an up-to-date hotkey state, since that's done later. rP19028 fixed that by calling HotkeyInputHandler manually, but that was still broken in some (unused) cases and was hacky (indeed, it even looked hacky as noted by elexis).

The simplest fix is to split the 'hotkey creator' handler from the 'hotkey state change' handler, and run the 'hotkey state change handler' before any other handler. Thus the gui handler remains in front of the 'hotkey creator' handler, but it has a correct hotkey state at any time.

Differential Revision: https://code.wildfiregames.com/D1839

Details

Auditors

Silier
elexis

Committed

wraitii

Sep 15 2019, 2:16 PM

Differential Revision

D1839: Fix hotkey event sync with hotkey state.

Parents

rP22908: Workaround differences between libc and libstd string stream parsing of…

Branches

Unknown

Tags

Unknown

Build Status

Buildable 9397
Build 15614: Trigger Windows Autobuild
Build 15613: Post-Commit Build	Jenkins

Event Timeline

wraitii committed rP22909: Fix hotkey events synching with hotkey state..Sep 15 2019, 2:16 PM

wraitii added an edge: D1839: Fix hotkey event sync with hotkey state..

Harbormaster completed building B9397: rP22909: Fix hotkey events synching with hotkey state..Sep 15 2019, 2:27 PM

(I don't remember what was up with that hotkey behavior)

/ps/trunk/binaries/data/mods/_test.gui/gui/hotkey.js
14	(The function could return the value to avoid obtaining the global via eval)
/ps/trunk/source/gui/CGUI.cpp
77	^This does the same as was removed below
/ps/trunk/source/gui/tests/test_GuiManager.h
1	Adds support for GUI tests, perhaps not bad to split it from a hotkey change diff
61	Should the test coverage be changed by user configuration?
66	Eval calls had been removed for ScriptInterface::CreateObject, CreateArray
81	13 occurrences of `g_GUI->GetActiveGUI()->GetScriptInterface()` could be shortened with a const&
85	Eval avoidable with GetGlobal GetProperty
106	static method
/ps/trunk/source/lib/file/common/trace.cpp
98	refs rP22378 (might be independent of the hotkey change)

elexis added inline comments.Sep 15 2019, 3:23 PM

/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	HotkeyInputHandler depends on g_mouse_buttons set in GlobalsInputHandler

wraitii added inline comments.Sep 15 2019, 3:57 PM

/ps/trunk/source/lib/file/common/trace.cpp
98	I do believe this was needed for the tests to pass. Somehow.
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	And? This doesn't call HotkeyInputHandler?

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

elexis added inline comments.Sep 15 2019, 6:33 PM

/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	I suppose you added HotkeyStateChange so that the hotkey state has the correct value when an SDL event is processed.

See D2295 for the misc improvements.

/ps/trunk/source/gui/CGUI.cpp
77	Introduced in D2260 so this was missed.
/ps/trunk/source/gui/tests/test_GuiManager.h
1	Well it mostly tests the hotkeys.
61	It doesn't, we don't have different namespaces in test and in production, so this just uses the user one 'at random'
66	Also posterior to this patch...
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	Indeed, this is the point of the diff, splitting the hotkey event generation from the state change.

There also seems to be a contradiction, I don't know if its also the case in other places:

The key combination of a hotkey is pressed, therefore the hotkey is pressed, the event is triggered, and the hotkey press state is set to true.
But what if a different handler says the key combination was caught before the hotkey event handler sets the thing to true?
Then the hotkey is claimed to be pressed but there was no hotkey press event.
It means hotkeys can become held down (JS function says pressed = true) without having been pressed or without getting pressed in the future (no onPress event).

/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	Does it have the correct hotkey state if the hotkey depends on the mouse event when it is called prior to GlobalsInputHandler if the SDL event is a mouse event?

In rP22909#37808, @elexis wrote:

There also seems to be a contradiction, I don't know if its also the case in other places:

The key combination of a hotkey is pressed, therefore the hotkey is pressed, the event is triggered, and the hotkey press state is set to true.
But what if a different handler says the key combination was caught before the hotkey event handler sets the thing to true?
Then the hotkey is claimed to be pressed but there was no hotkey press event.

As you noted on the ticket, no, because hotkey events are crafted by 0 A.D. Either the 'hotkey combination' is caught before it gets to HotkeyInputHandler (e.g. textbox) and then no hotkey event is sent, or it isn't, and then a hotkey event is sent and the state must be changed when processing that.

With this change, a hotkey event == a hotkey state change. Before that, the equivalency did not stand.

/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	The hotkey is created on the preceding call (the mouse event), so at this point that doesn't matter whatsoever. So 'yes' to answer your question.

elexis added inline comments.Sep 16 2019, 11:54 AM

/ps/trunk/binaries/data/mods/_test.gui/gui/hotkey.js
4	\n{
14	(It could not, since `CGUIManager::HandleEvent` expects the return value to be bool)
/ps/trunk/binaries/data/mods/_test.gui/gui/hotkey.xml
4	I suppose there will be more than one GUI test, and there are multiple files per GUI page needed for one test, and they will all end up in the same folder unless there is a subfolder for each page as in mod/gui, public/gui/.
/ps/trunk/source/gui/CGUI.cpp
94	Indeed more correct to not state ret = IN_HANDLED if there is no handler for the event type!
/ps/trunk/source/gui/tests/test_GuiManager.h
1	(It seems like the entire JS and GUI code could have been avoided and test the C++ hotkey code directly if it wasn't for `CGUIManager::HandleEvent` `handleInputBeforeGui`, `handleInputAfterGui` mechanism being tested.)
58	(-\n)
61	Code ok: The VFS is initialized with `g_VFS = CreateVfs();`, not `InitVfs`, hence the `user.cfg` file never comes into contact with the testscript. In the `CGUI` page constructor there is a call to `GuiScriptingInit`. That includes `JSInterface_ConfigDB.cpp` and `JSInterface_VFS.cpp`, `JSInterface_Mod.cpp` etc, so it's not so far away that it may end up using `user.cfg`. (`globalscripts/` are loaded too but they exclude GUI files, otherwise they'd violate the idea of globalscripts) So as long as no mod/gui/ or public/gui/ file is loaded in the testscripts it will probably be safe enough. Possible improvement: Might perhaps be nicer to exclude JSInterfaces by default, only include them as needed in testscripts (Maybe fake -> mock)
65	Missing AutoRequest
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	Sorry for the noise, I confused HotkeyInputHandler and HotkeyStateChange handler! (I notice its the only handler that doesnt have handler in the name)
/ps/trunk/source/ps/Hotkey.cpp
162	Would it be cleaner and more performant to not have this a separate event handler called for every event, but to insert this one assignment in the place where the hotkey event is constructed (150 lines below from here at `SDL_Event_ hotkeyNotification;`)? (The tests cover this theory I guess)
205	(probably correct to remove, didn't check for side effects or pitfalls)

wraitii added inline comments.Sep 16 2019, 12:01 PM

/ps/trunk/source/gui/tests/test_GuiManager.h
1	yes, but that was actually the buggy part, if you run the tests without the fix, you'll see them failing. Then again one could probably have created a simpler case that failed too, but this took me a fairly long time to understand, and it also functions as a simple example of how to interact with JS.
/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	Well I named it so because per so it doesn't "handle" much... I guess that can be renamed though.
/ps/trunk/source/ps/Hotkey.cpp
162	I could be misunderstanding you, but if you suggest updating the state when one creates the hotkey event, you can't, otherwise if there are multiple hotkey events the state would be changed too quickly (see rP14057)

For the contradiction, I'm mostly thinking about a model where the JS GUI wants to perform some init upon Press, and then use that initialized data if Engine.HotkeyIsPressed() returns true) and destroy the data upon release. Or just any JS GUI code that requires a state model where the JS GUI Press event happens each time prior to the function retuning true.
So the conditions for such a contradiction in the JS GUI side (i.e. Engine.HotkeyIsPressed() returning true but no on="Press" GUI event for the hotkey being sent) is that a handler is registered after (i.e. called before) gui_handler that processes the hotkey event; and that it returns IN_HANDLED. Since that's not the case currently, no such contradiction arises for any of the current hotkeys; and the hypothetical one who introduces such a handler in C++ will have to care).

Re "hotkey combination" - I guess there are no hotkey combinations (multiple hotkeys combined), only key combinations (which includes virtual keys such as mouse presses), and key combinations which are the values assigned to hotkeys. Or is there better wording? (Its relevant when writing hotkey code).

So it seems like the hotkey behavior change is correct. It sets the thing to true everytime the hotkey is pressed, keeps the state on true until a release event occurs.

So I must accept the commit after the garbage collection and rebase error is fixed.

/ps/trunk/source/ps/GameSetup/GameSetup.cpp
575	(offtopic, but `CProfileViewer::InputThunk` is the other one not having handler in the name, also the lower_case_under_score notation seems ugly.)
/ps/trunk/source/ps/Hotkey.cpp
162	True, setting that would be too early because the event is pushed, not processed immediately.

broke test.exe build with --jenkins-tests and so jenkins

>	test.exe!Path::operator/(Path rhs) Line 234	C++
 	test.exe!TestGuiManager::setUp() Line 38	C++
 	test.exe!CxxTest::RealTestDescription::setUp() Line 76	C++
 	test.exe!CxxTest::TestRunner::runTest(CxxTest::TestDescription & td) Line 103	C++
 	test.exe!CxxTest::TestRunner::runSuite(CxxTest::SuiteDescription & sd) Line 85	C++
 	test.exe!CxxTest::TestRunner::runWorld() Line 65	C++
 	test.exe!CxxTest::TestRunner::runAllTests(CxxTest::TestListener & listener) Line 44	C++
 	[Inline Frame] test.exe!CxxTest::XmlFormatter::run() Line 279	C++
 	test.exe!CxxTest::Main<CxxTest::XmlPrinter>(CxxTest::XmlPrinter & tmp, int argc, char * * argv) Line 147	C++
 	test.exe!SDL_main(int argc, char * * argv) Line 20	C++
 	test.exe!main_utf8(int argc, char * * argv) Line 126	C
 	test.exe!main(int argc, char * * argv) Line 134	C

Path _testcache/, separator /
path.h(234): Function call failed: return value was -100303 (path contains both slash and backslash separators)

This commit now has outstanding concerns.Sep 16 2019, 9:01 PM

Silier mentioned this in D2299: Fix test.exe build failing with --jenkins-tests flag.Sep 16 2019, 9:25 PM

Itms mentioned this in rP22917: Fix rP22909 on Windows..Sep 17 2019, 9:27 PM

Silier accepted this commit.Sep 17 2019, 9:58 PM

All concerns with this commit have now been addressed.Sep 17 2019, 9:59 PM

Moving the GUI test page into a separate folder and the ScriptInterface cleanup would be good (D2295).
The missing JS_AutoRequest and the HotkeyInputHandler(ev); are strict defects, so I mark it that it's not forgotton (especially since its currently marked as green).
(The hotkey behavior change itself seemed correct from above analysis, thanks for that!)

This commit now has outstanding concerns.Sep 18 2019, 2:01 AM

wraitii mentioned this in rP22963: Improve js glue code in D1839/rP22909 (hotkey state fix) and deal with rebase….Sep 22 2019, 9:38 AM

wraitii requested verification of this commit.Sep 22 2019, 11:19 AM

This commit now requires verification by auditors.Sep 22 2019, 11:19 AM

The commit was audited and found to be correct, albeit I evidently missed https://code.wildfiregames.com/rP22909#37878
Thanks for the fix.

All concerns with this commit have now been addressed.Sep 22 2019, 12:06 PM

wraitii mentioned this in rP24358: Properly clean up _testcache folder..Dec 10 2020, 9:18 AM