Skip to content

Log messages when starting / ending freeze #14309

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.

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

Merged
merged 10 commits into from
Nov 4, 2022
Merged

Log messages when starting / ending freeze #14309

merged 10 commits into from
Nov 4, 2022

Conversation

feerrenrut
Copy link
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.

@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
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

generally LGTM

@AppVeyorBot

This comment was marked as outdated.

@feerrenrut
Copy link
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.

Copy link
Member

@michaelDCurran michaelDCurran left a comment

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.

@feerrenrut
Copy link
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 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants