Skip to content

Fix occasional error when the session is locked/unlocked#16121

Merged
seanbudd merged 2 commits into
nvaccess:betafrom
CyrilleB79:checkFocus
Feb 4, 2024
Merged

Fix occasional error when the session is locked/unlocked#16121
seanbudd merged 2 commits into
nvaccess:betafrom
CyrilleB79:checkFocus

Conversation

@CyrilleB79

@CyrilleB79 CyrilleB79 commented Feb 1, 2024

Copy link
Copy Markdown
Contributor

Link to issue number:

Fix #16120
Related to #15400.

Summary of the issue:

Sporadic errors still occur when locking/unlocking Windows session. As per #15400, this is due to the fact that api.getFocusObject returns an NVDAObjects.NVDAObject but we use it as it was an NVDAObjects.window.Window. But that's not the case when the focus is (or was) winAPI.secureDesktop._handleSecureDesktopChange._SecureDesktopNVDAObject.

Description of user facing changes

Probably none; these errors did not seem to have any visible impact.

Description of development approach

Check the class of the object returned by api.getFocusObject before using its properties.

Testing strategy:

  • Lock and unlock the session. However, this test is not complete since the issue only occurred sporadically.
  • When merged to beta, check that such errors are not there anymore.

Known issues with pull request:

Maybe there are other places where we use an NVDAObject as if it was a Window. We'll fix them the same way if we encounter them in the future.

Change log

None. Error appeared during 2024.1 dev cycle.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@CyrilleB79

CyrilleB79 commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

@seanbudd or others:

Regarding the error no 2 which is not yet fixed, I need some help since I do not fully understand what is in keyboardHandler.py.

I have the feeling that shouldUseToUnicodeEx should return False when the focus is not an instance of NVDAObjects.window.Window, since we need a real window to get the keyboard layout.

However, I do not know if I have also to modify internal_keyDownEvent in case the focus is an instance of winAPI.secureDesktop._handleSecureDesktopChange._SecureDesktopNVDAObject. And also if shouldUseToUnicodeEx returns False, does it mean that character typing is not processed?

A clarification on all this would help me add explaining comments (if needed) when modifying shouldUseToUnicodeEx. Thanks.

@seanbudd, also, let me know if you prefer me to target master branch. I have targeted beta since these errors are not present in NVDA 2023.3.

@AppVeyorBot

AppVeyorBot commented Feb 1, 2024

Copy link
Copy Markdown

See test results for failed build of commit 8fe411e436

@CyrilleB79

CyrilleB79 commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

Finally, I have forced shouldUseToUnicodeEx to return False for NVDAObjects that are not real windows, i.e. not instances of Window.

Let me know if it's OK and if I should do something else. The questions in #16121 (comment) are still pending though.

@CyrilleB79 CyrilleB79 marked this pull request as ready for review February 1, 2024 21:34
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner February 1, 2024 21:34
@CyrilleB79 CyrilleB79 requested review from SaschaCowley and removed request for a team February 1, 2024 21:34
seanbudd
seanbudd approved these changes Feb 1, 2024
View reviewed changes

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CyrilleB79

@seanbudd seanbudd added this to the 2024.1 milestone Feb 1, 2024
@seanbudd

seanbudd commented Feb 1, 2024

Copy link
Copy Markdown
Member

I think this was a known concern when this change was made.
The assumption was that very little code would be firing while sleep mode is enabled while the secure desktop copy is running instead.

I don't think we would be processing keyboard events, so I wouldn't change internal_keyDownEvent unless errors are discovered.

It might be worth asking for a review from @codeofdusk , who introduced handling for KeyboardHandlerBasedTypedCharSupport

@seanbudd seanbudd merged commit b7a695f into nvaccess:beta Feb 4, 2024
@CyrilleB79 CyrilleB79 deleted the checkFocus branch February 5, 2024 09:52
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
@CyrilleB79 @Adriani90
Fix occasional error when the session is locked/unlocked (nvaccess#16121
d2c1956 
)

Fix nvaccess#16120
Related to nvaccess#15400.

Summary of the issue:
Sporadic errors still occur when locking/unlocking Windows session. As per nvaccess#15400, this is due to the fact that api.getFocusObject returns an NVDAObjects.NVDAObject but we use it as it was an NVDAObjects.window.Window. But that's not the case when the focus is (or was) winAPI.secureDesktop._handleSecureDesktopChange._SecureDesktopNVDAObject.

Description of user facing changes
Probably none; these errors did not seem to have any visible impact.

Description of development approach
Check the class of the object returned by api.getFocusObject before using its properties.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@SaschaCowley SaschaCowley Awaiting requested review from SaschaCowley SaschaCowley is a code owner automatically assigned from nvaccess/developers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2024.1

Development

Successfully merging this pull request may close these issues.

3 participants

@CyrilleB79 @AppVeyorBot @seanbudd