Skip to content

Log messages when starting / ending freeze#14309

Merged
feerrenrut merged 10 commits into
masterfrom
logEndOfFreeze
Nov 4, 2022
Merged

Log messages when starting / ending freeze#14309
feerrenrut merged 10 commits into
masterfrom
logEndOfFreeze

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

Link to issue number:

As per #14289 (comment)

Summary of the issue:

When NVDA freezes its behavior changes, this can break assumptions, and make inferring its behavior difficult.
The logs do not clearly indicate the start / end / level of a freeze.

Description of user facing changes

None

Description of development approach

  • Refactor code to extract responsibilities to make loop conditions easier to reason about.
  • Add logging for different levels of freeze, start and end.

Testing strategy:

  • System tests.
  • Alpha testing.

Known issues with pull request:

  • It would be helpful to have clear names and a description of the implications of the different levels of freeze.

Change log entries:

For Developers

- More detailed logging for NVDA freezes to aid debugging.

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
  • Security precautions taken.

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut feerrenrut marked this pull request as ready for review November 1, 2022 04:21
@feerrenrut feerrenrut requested a review from a team as a code owner November 1, 2022 04:21
seanbudd
seanbudd reviewed Nov 2, 2022
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.

generally LGTM

Comment thread source/watchdog.py Outdated
Comment thread source/watchdog.py
Comment thread source/watchdog.py
Comment thread source/watchdog.py Outdated
Comment thread source/watchdog.py Outdated
Comment thread source/watchdog.py Outdated
Comment thread source/watchdog.py Outdated
seanbudd
seanbudd approved these changes Nov 2, 2022
View reviewed changes
Comment thread source/watchdog.py Outdated
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

feerrenrut commented Nov 2, 2022

Copy link
Copy Markdown
Contributor Author

Last system test failure:

Robot.startupShutdownNVDA.RestartsRobot
Error message:
Timed out waiting Welcome to NVDA to focus

Unlikely to be related to this change.

michaelDCurran
michaelDCurran requested changes Nov 2, 2022
View reviewed changes

@michaelDCurran michaelDCurran 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.

I'd like to see this change split into separate commits in the following order:

  • Switch to using performance counters
  • Add missing logging
  • Refactor code
    As much as I don't want to create extra work here, I'm finding it hard to follow all the changes and therefore can not be confident to give my assurance that the change looks correct. Especially also because the primary point of the pr was to add missing logging, not to necessarily refactor.

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

feerrenrut commented Nov 2, 2022

Copy link
Copy Markdown
Contributor Author

@michaelDCurran The changes have been split. The refactor is composed of several aspects, I also made these separate commits.

@feerrenrut feerrenrut self-assigned this Nov 3, 2022
feerrenrut added a commit that referenced this pull request Nov 4, 2022
@feerrenrut
Update changes file for PR #14309
0721011
@feerrenrut feerrenrut merged commit f674505 into master Nov 4, 2022
@feerrenrut feerrenrut deleted the logEndOfFreeze branch November 4, 2022 08:26
@nvaccessAuto nvaccessAuto added this to the 2023.1 milestone Nov 4, 2022
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

@michaelDCurran michaelDCurran michaelDCurran approved these changes

Assignees

@feerrenrut feerrenrut

Labels

None yet

Projects

None yet

Milestone

2023.1

Development

Successfully merging this pull request may close these issues.

5 participants

@feerrenrut @AppVeyorBot @seanbudd @michaelDCurran @nvaccessAuto