Page MenuHomeWildfire Games

Fix hotkey event sync with hotkey state.
Needs ReviewPublic

Authored by wraitii on Apr 21 2019, 4:19 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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
D1839_hotkey_fix
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7361
Build 11989: Vulcan BuildJenkins
Build 11988: 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

elexis added inline comments.Apr 25 2019, 1:09 PM
binaries/data/mods/_test.gui/gui/gui.rng
883

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
883

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_handlerinput boxes doesn't apply

wraitii updated this revision to Diff 7906.Sat, May 4, 5:26 PM
wraitii marked an inline comment as done.

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.

Stan added a subscriber: Stan.Sat, May 4, 6:18 PM

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

Vulcan added a comment.Sat, May 4, 6:21 PM

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