Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Log WindowErrors for easeOfAccess and config #13242

Merged
merged 6 commits into from
Jan 31, 2022
Merged

Log WindowErrors for easeOfAccess and config #13242

merged 6 commits into from
Jan 31, 2022

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jan 17, 2022 •
edited
Loading

Link to issue number:

Relates to #13155

Summary of the issue:

In order to investigate #13155, further logging is needed.

Description of how this pull request fixes the issue:

Adds logging for WindowErrors raised in easeOfAccess and config. This includes improvements to functions unrelated to #13155, which access the registry, by adding logging to prevent similar issues.

Testing strategy:

  • General alpha testing is needed to ensure the warnings and errors raised aren't noisy.
  • Manually test installing NVDA
  • Manually test toggling NVDA start after logon (both on and off)
    • ensure changing the setting persists across instances
      • from the installer
      • [from an installed copy
  • Manually test toggling NVDA start on logon screen (both on and off)
    • ensure changing the setting persists across instances
      • from the installer
      • from an installed copy
  • Additional testing is needed to try and reproduce Start NVDA after I sign in option is sometimes not preserved #13155, and get a clear picture of where the problem could be occurring.
    • This is yet to be reliably reproduced. The following has been tested:
      • Running the installer under a profile with special characters which can cause os.stat to fail (Testé ç ş ğ).
      • Start NVDA after I sign in option is sometimes not preserved #13155 general re-attempt
        • download a copy of the installer from this PR
        • install this PR build
        • turn on "start after logon", save the config
        • create a copy of the installer from this PR, identical except with a higher build number
        • run the identical, but newer installer to cause an update to occur
        • capture the installer log
        • start the newly installed copy
        • open the settings, check if "start after logon" is enabled
        • if not, capture the latest NVDA log
        • upload both logs

Known issues with pull request:

None

Change log entries:

For Developers

- ```config.RUN_REGKEY``, ``config.NVDA_REGKEY`` are deprecated, please use ``config.RegistryKey.RUN``, ``config.RegistryKey.NVDA`` instead. These will be removed in 2023. (#13242)
- ``easeOfAccess.ROOT_KEY``, ``easeOfAccess.APP_KEY_PATH`` are deprecated, please use``easeOfAccess.RegistryKey.ROOT``, ``easeOfAccess.RegistryKey.APP`` instead. These will be removed in 2023. (#13242)
- ``easeOfAccess.APP_KEY_NAME`` has been deprecated, to be removed in 2023. (#13242)

Code Review Checklist:

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

@AppVeyorBot
Copy link

See test results for failed build of commit 63b06a1044

@seanbudd seanbudd force-pushed the log-exceptions-for-easeOfAccess-config branch from 0244a69 to 879bca1 Compare January 19, 2022 02:48
@AppVeyorBot
Copy link

See test results for failed build of commit c80b4ceb72

@AppVeyorBot
Copy link

See test results for failed build of commit 1c290c47bc

@seanbudd seanbudd force-pushed the log-exceptions-for-easeOfAccess-config branch from 2635d90 to fde810b Compare January 19, 2022 04:22
@AppVeyorBot
Copy link

See test results for failed build of commit b7da7b6c48

@AppVeyorBot
Copy link

See test results for failed build of commit b7da7b6c48

@seanbudd seanbudd force-pushed the log-exceptions-for-easeOfAccess-config branch from fde810b to 4c8686d Compare January 20, 2022 06:02
@seanbudd seanbudd force-pushed the log-exceptions-for-easeOfAccess-config branch from 4c8686d to 9ac7087 Compare January 20, 2022 06:16
@seanbudd seanbudd requested a review from feerrenrut January 24, 2022 01:02
@seanbudd seanbudd marked this pull request as ready for review January 24, 2022 01:02
@seanbudd seanbudd requested a review from a team as a code owner January 24, 2022 01:02
@seanbudd seanbudd mentioned this pull request Jan 24, 2022
Closed
feerrenrut
feerrenrut suggested changes Jan 28, 2022
View reviewed changes
source/config/__init__.py Show resolved Hide resolved
source/easeOfAccess.py Show resolved Hide resolved
source/easeOfAccess.py Outdated Show resolved Hide resolved
seanbudd
seanbudd commented Jan 28, 2022
View reviewed changes
source/config/__init__.py Outdated Show resolved Hide resolved
seanbudd and others added 2 commits January 28, 2022 14:53
@AppVeyorBot
Copy link

See test results for failed build of commit 2cf98f73b2

@seanbudd seanbudd requested a review from feerrenrut January 31, 2022 00:30
feerrenrut
feerrenrut approved these changes Jan 31, 2022
View reviewed changes
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Looks good @seanbudd

@seanbudd seanbudd merged commit e37b9a0 into master Jan 31, 2022
@seanbudd seanbudd deleted the log-exceptions-for-easeOfAccess-config branch January 31, 2022 02:30
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Jan 31, 2022
seanbudd added a commit that referenced this pull request Jan 31, 2022
@seanbudd
Add changes from #13242
263a42f
@seanbudd seanbudd mentioned this pull request Jan 31, 2022
Merged
seanbudd added a commit that referenced this pull request Jan 31, 2022
@seanbudd
Add changes from #13242 (#13291)
443bf19
@lukaszgo1 lukaszgo1 mentioned this pull request Feb 23, 2022
Merged
5 tasks
seanbudd pushed a commit that referenced this pull request Feb 23, 2022
@lukaszgo1
No longer try to access non-existing registry key in getStartOnLogonS…
3b08819 
…creen (#13384)

Fix-up of PR #13242
Originally reported by @Stefan-Kliesch-FHP in #13348

Summary of the issue:
getStartOnLogonScreen can fail with UnboundLocalError when the autostart key does not exist.

Description of how this pull request fixes the issue:
If we failed to open autostart key we no longer try to access it.
@seanbudd seanbudd mentioned this pull request Mar 30, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feerrenrut feerrenrut feerrenrut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.
4 participants
@seanbudd @AppVeyorBot @feerrenrut @nvaccessAuto