Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP22909: Fix hotkey events synching with hotkey state.

Summary

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

The problem, which was not understood correctly by elexis here, and is that the hotkey state is changed in HotkeyInputHandler, so if that isn't run before handling the SDL_HOTKEYxxxx event, the state will be desynchronised.

There are still some cases where it is desynchronised though, namely all of "handleInputBeforeGUI" since that's called before the patch in rP19028 has any chance to trigger - and even then it's by sheer luck that it does.

I've looked at this some more as I needed handleInputBeforeGUI to have proper sync for D1837, and I believe the best fix is to add a new, top-priority handler in the event dispatch that does the state sync. This should ensure that our state is always synched, unless someone goes and change InitInput().

This thus completes rP14057.

Incidentally, I'm adding some simple tests for this that also show how to interact with JS code from C++ in a somewhat compact manner.

(I've had to change a number of other files to make the test actually test something).

Test Plan

Review the test. I'm wondering if I should just call this test_hotkeys.h instead.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

hotkey_fix

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 7231
Build 11786: Vulcan Build	Jenkins
Build 11785: arc lint + arc unit

Event Timeline

wraitii created this revision.Apr 21 2019, 4:19 PM

wraitii added inline comments.Apr 21 2019, 4:23 PM

source/gui/CGUI.cpp
82	This revert rP19028 (and also improves it I think? There's no guarantee that handlers would return IN_HANDLED).
source/gui/GUIManager.cpp
49	Added to use InitInput in tests.
source/lib/file/common/trace.cpp
97	This is needed because the tests failed as the timestamp was printfed with a ',' instead of a '.' when I added the GUIManager tests - seems linked with polling but I couldn't find a call to set locale there. Tracing seems pretty useless regardless so maybe we could just delete all of this.
source/ps/CConsole.cpp
638	Added so that InitInput can be called from tests without initialising the console.
source/ps/GameSetup/GameSetup.cpp
558	This is the key item, actually fixing the issue.
source/ps/Hotkey.cpp
253	Likewise added to allow using InitInput in testing.

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

Linter detected issues:
Executing section Source...

source/ps/Hotkey.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"

source/ps/CConsole.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"

source/gui/GUIManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/lib/file/common/trace.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/ps/GameSetup/GameSetup.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/Hotkey.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/gui/CGUI.cpp
|1671| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1671| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1671| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...

binaries/data/mods/_test.gui/gui/hotkey.js
|   4| function·handleInputBeforeGui(ev)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/_test.gui/gui/hotkey.js
|  10| function·handleInputAfterGui(ev)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1245/display/redirect

Harbormaster completed remote builds in B7231: Diff 7791.Apr 21 2019, 4:25 PM

elexis added inline comments.Apr 25 2019, 1:09 PM

binaries/data/mods/_test.gui/gui/gui.rng
882	Having two copies of these files will mean the files will become out of sync and it will be more maintenance work for future developers without benefit, am I wrong? We ought not to have to provide a copy if Pyrogenesis engine requires the file to be identical to the existing file. If we want this to be separate testing data, then the irrelevant parts of the file should be deleted instead.
source/ps/GameSetup/GameSetup.cpp
558	Why is it correct to split the handler into two, and why is this the correct line of insertion of the new handler? Doing an error here will break a lot of stuff, code comment L547 pointing into one possible direction of error.

wraitii marked an inline comment as done.Apr 25 2019, 1:53 PM

wraitii added inline comments.

binaries/data/mods/_test.gui/gui/gui.rng
882	Mh, I will do `If we want this to be separate testing data, then the irrelevant parts of the file should be deleted instead.` then. The engine expects XML validation files for these, so I just copied over the ones from mod-mod and didn't give it much more thought. Note that we could re-use this mod for other GUI tests in the future, and then the syntax validation would need to be re-extended until it might look like svn's, but I guess that's a big "if".
source/ps/GameSetup/GameSetup.cpp
558	With regards to 'split in two': A. `HotkeyInputHandler` takes 'basic' SDL input (key down, key up, which key), and generates a custom 'HOTKEY[down/up]' event, which gets pushed on the priority queue and dispatched next in the polling loop. B. If `HotkeyInputHandler` is passed a custom-created 'HOTKEY[down/up]' event, it updates `g_HotkeyStatus`, which `Engine.HotkeyIsPressed` uses. C. Originally, `g_HotkeyStatus` was updated when creating the 'HOTKEY[down/up]' events. But then any further check on `g_HotkeyStatus` would return the hotkey-updated status, event if there were events to be processed before. Imagine a queue [Z-up, mouse-click], where Z-up triggers a hotkey. If we now have a queue [mouse-click, Z-hotkey], the 'mouse-click' will see Z-hotkey as being active despite the hotkey-event not having been handled yet. However, rP14057 also introduced a priority queue which means we would actually get [Z-hotkey, mouse-click], and then things would work, but only coincidentally. D. The safest thing to do is to update `g_HotkeyStatus` when actually processing the Hotkey event. This is what rP14057 did. However, rP14057 did that in `HotkeyInputHandler`, probably because it was easy, which comes after the `gui_handler`. E. Thus events handled in the `gui_handler` won't see the updated `g_HotkeyStatus`. This was the cause behind rP19028, which called `HotkeyInputHandler` again. F. However the simpler thing to do is just update `g_HotkeyStatus` right away when processing a 'HOTKEY[down/up]' event, and then any downstream handlers will see the correct state. With regards to L547: We don't want to trigger hotkeys if `gui_handler` stops the event first (the main reason is indeed things like input boxes, but it's a more general concern, and for that matter I think the hotkey handler could be moved to be _last_). Thus we need to put `gui_handler` upstream of `HotkeyInputHandler`. We can't run the state-update part without running `HotkeyInputHandler` unless the functionalities are split. Thus I split them. The new `HotkeyStateChange` handler only updates the hotkey state when receiving a 'HOTKEY[down/up]' event, which by definition is already a hotkey event and is always triggered on top of the real input, so the concern about `gui_handler`input boxes doesn't apply

Add a simpler gui.rng for the _test.gui folders.
I removed the .rnc files since I don't have trang (it's annoying to install) and to be honest the regular syntax seems easier to from to me than the compact one.

Ready to merge.

Couldn't you just edit the files manually ? :)

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

Linter detected issues:
Executing section Source...

source/ps/GameSetup/GameSetup.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/ps/Hotkey.cpp
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/lib/file/common/trace.cpp
|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

source/ps/CConsole.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"

source/ps/Hotkey.h
|   1| /*·Copyright·(C)·2014·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2014"

source/gui/GUIManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/gui/CGUI.cpp
|1671| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1671| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1671| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...

binaries/data/mods/_test.gui/gui/hotkey.js
|   4| function·handleInputBeforeGui(ev)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/_test.gui/gui/hotkey.js
|  10| function·handleInputAfterGui(ev)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1325/display/redirect

Harbormaster completed remote builds in B7361: Diff 7906.May 4 2019, 6:21 PM

Years.

Build failure - The Moirai have given mortals hearts that can endure.

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

Harbormaster failed remote builds in B9386: Diff 9779!Sep 15 2019, 12:56 PM

Fix tests, needed a rebase.

Build failure - The Moirai have given mortals hearts that can endure.

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

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

Linter detected issues:
Executing section Source...

source/gui/tests/test_GuiManager.h
|  29| class·TestGuiManager·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestGuiManager:' is invalid C code. Use --std or --language to configure the language.

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStrW:' is invalid C code. Use --std or --language to configure the language.

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStr:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...

binaries/data/mods/_test.gui/gui/hotkey.js
|   4| function·handleInputBeforeGui(ev)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/_test.gui/gui/hotkey.js
|  10| function·handleInputAfterGui(ev)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.
Executing section cli...

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

This revision was not accepted when it landed; it landed in state Needs Review.Sep 15 2019, 2:16 PM

This revision was landed with ongoing or failed builds.

Closed by commit rP22909: Fix hotkey events synching with hotkey state. (authored by wraitii). · Explain Why

This revision was automatically updated to reflect the committed changes.

wraitii added a commit: rP22909: Fix hotkey events synching with hotkey state..

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

Build was aborted.

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

Harbormaster failed remote builds in B9387: Diff 9780!Sep 16 2019, 6:31 PM

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 mentioned this in rP24358: Properly clean up _testcache folder..Dec 10 2020, 9:18 AM

Fix hotkey event sync with hotkey state.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 7791

binaries/data/mods/_test.gui/gui/gui.rnc

binaries/data/mods/_test.gui/gui/gui.rng

binaries/data/mods/_test.gui/gui/gui_page.rnc

binaries/data/mods/_test.gui/gui/gui_page.rng

binaries/data/mods/_test.gui/gui/hotkey.js

binaries/data/mods/_test.gui/gui/hotkey.xml

binaries/data/mods/_test.gui/gui/page_hotkey.xml

source/gui/CGUI.cpp

source/gui/GUIManager.cpp

source/gui/tests/test_GuiManager.h

source/lib/file/common/trace.cpp

source/ps/CConsole.cpp

source/ps/GameSetup/GameSetup.h

source/ps/GameSetup/GameSetup.cpp

source/ps/Hotkey.h

source/ps/Hotkey.cpp

Fix hotkey event sync with hotkey state.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 7791

binaries/data/mods/_test.gui/gui/gui.rnc

binaries/data/mods/_test.gui/gui/gui.rng

binaries/data/mods/_test.gui/gui/gui_page.rnc

binaries/data/mods/_test.gui/gui/gui_page.rng

binaries/data/mods/_test.gui/gui/hotkey.js

binaries/data/mods/_test.gui/gui/hotkey.xml

binaries/data/mods/_test.gui/gui/page_hotkey.xml

source/gui/CGUI.cpp

source/gui/GUIManager.cpp

source/gui/tests/test_GuiManager.h

source/lib/file/common/trace.cpp

source/ps/CConsole.cpp

source/ps/GameSetup/GameSetup.h

source/ps/GameSetup/GameSetup.cpp

source/ps/Hotkey.h

source/ps/Hotkey.cpp

Fix hotkey event sync with hotkey state.
ClosedPublic
Actions

Revision Contents
Changeset List