Fix tracking of keyboard modifiers when watchdog is attempting recovery

[mltony]

Link to issue number:

Fixes: #12609

Summary of the issue:

NVDA doesn't correctly track state of keyboard modifiers (such as Control, or Insert) in some cases, e.g. when watchdog is recovering.

Description of how this pull request fixes the issue:

In winInputHook.py , in def keyboardHook(), currently there is a condition:

if watchdog.isAttemptingRecovery or code!=HC_ACTION:
return windll.user32.CallNextHookEx(0,code,wParam,lParam)

So when watchdog.isAttemptingRecovery is True, then this function returns early and doesn't process any keyboard events, including events related to keyboard modifiers. As a result, when a user happens to press modifier while watchdog is recovering, NVDA won't process this, thus getting out of sync with current keyboard state.
In this PR I propose to check whether current keyboard event is related to keyboard modifiers, and when it is related, then process it even when watchdog is recovering.
Please note, I reduplicate the list of keyboard modifiers in winInputHook.py, since importing the same list from keyboardHandler.py makes NVDA to crash on startup, probably due to circular import.

Testing strategy:

Performed thorough manual testing on locally built version. This PR was applied on top of 2021.1beta1.
I have been running my custom build for about a month and couldn't reproduce the problem even once. For comparison, without this PR< the problem would reproduce several times a day during my normal work process.
I also had tones.beep() inserted in multiple versions to verify my hypothesis, and it proved true that when Notepad++ window is stuck on network write, then indeed watchdog enters recovery mode very often, and also while this happens if user happens to press or release any modifiers, then I also verified, that this event is indeed being ignored in current master version.

Known issues with pull request:

None observed.

Change log entries:

Bug fixes:
Fixing tracking state of keyboard modifiers (such as Control, or Insert) when watchdog is recovering.

Code Review Checklist:

• Pull Request description is up to date.
• Unit tests (something unrelated fails regarding to Bulgarian braille table - but master fails too)
• System (end to end) tests - doesn't apply
• Manual testing.
• User Documentation - doesn't apply
• Change log entry.
• Context sensitive help for GUI changes - doesn't apply
• UX of all users considered - doesn't apply.

[mltony]

CC: @michaelDCurran @feerrenrut

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/3gxsdvhf9egmq9x8/artifacts/output/nvda_snapshot_pr12610-23271,6c4dbe10.exe

See test results for failed build of commit 6c4dbe10a7

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/6hhcfmqyovflwlcm/artifacts/output/nvda_snapshot_pr12610-23272,7fe12802.exe

See test results for failed build of commit 7fe12802a1

[michaelDCurran]

At first glance, I'm thinking it might be better to not check for watchdog recovery at all in winInputHook. Rather in keyboardHandler.internal_keyDownEvent: Just before the try block with executeGesture, if watchdog is attempting recovery, return True. Similarly, at the top of the finally block, if watchdog is attempting recovery, return true.
internal_keyUpEvent does not need to check at all I believe.
Essentially, we are trying to avoid queuing anything to the main thread, and or interrigating any NVDAObjects.
I agree the existing watchdog checks are too broad.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/s31983vjch96ydya/artifacts/output/nvda_snapshot_pr12610-23290,6d2da42e.exe

See test results for failed build of commit 6d2da42e67

[mltony]

@michaelDCurran, fixed this according to your suggestions.

[mltony]

@michaelDCurran, fixed!

[michaelDCurran]

Um... it really doesn't bother me either way. I think just importing watchdog is much more easier to understand... and directly addresses the issue. but @feerrenrut if you want to work with @mltony to implement your idea then that is okay with me.

[feerrenrut]

Whether it is easier to understand is a matter of perspective I think. If you aren't familiar with watchdog you'll have to go and understand the origin of that either way to understand this PR. My main reasoning for wanting this separation is to:

• prevent an 'import order dependency'. Currently the import for watchdog is later in core, however it is implicitly imported by inputCore before keyboardHandler. This makes the import order hard to reason about. This won't matter when/if we get to stage of having no import time behavior. But we aren't there yet.
• encode the intent, keyboardHandler is an observer of the watchdog, not a controller. It doesn't cause state changes.

I'm happy to work through this with @mltony

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/kmnkekvbd3eb8sor/artifacts/output/nvda_snapshot_pr12610-23324,89a45864.exe

See test results for failed build of commit 89a4586406

[mltony]

@feerrenrut, I updated this PR according to your suggestion - I created WatchdogObserver class.

[mltony]

I did some more testing, it seems this might also fix #10588 - at least pasting from clipboard history seems to work much better with this PR on my computer. Tagging FYI.

[CyrilleB79]

@mltony if you do not have any additional modification to do in this PR, please pass it in "Ready" state rather than letting it in "Draft". This way, it will not be forgotten by NVAccess.

[LeonarddeR]

For reference, the current code that checks the watchdog was introduced by me in #9208. I agree that it is to aggressive, though I hope #6291 won't be made worse this way again. May be @lukaszgo1 could have a look as well, as he performed some testing as part of #9208?

[mltony]

@LeonarddeR, addressed your comments.
Also, I marked this PR as ready for review - please let me know if I shouldn't have done that.

[seanbudd]

LGTM - @feerrenrut suggested these changes so I'm re-requesting review.

[feerrenrut]

Yep this looks good. I'd just like to add some type information for WatchdogObserver. I'll push a commit with this.

[feerrenrut]

@mltony I have pushed a commit to add some more type information to the changed areas. I tested only by ensuring NVDA runs from source.

Changes have been addressed

[feerrenrut]

I think the description for this PR is inaccurate. This could be misleading to someone re-visiting in the future. Could you please update it?