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.

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

Enable debug logging when restarting NVDA with add-ons #17043

Merged
merged 9 commits into from
Aug 29, 2024

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Aug 22, 2024

This PR fixes the initial request of #11538.

The discussion in #11538 and a follow up PR by @mzanm (#13214) have led to the proposal of a more elaborated solution with a re-design of NVDA's exit dialog. Though this redesign has not seen the light of day, initially due to discussions on the best UX for this dialog and then due to a technical difficulty with the chosen technical design. The result is that 4 year later, the initial and main concern described in #11538 is not solved yet 4 year later. The best is the enemy of the good.

The proposal in this PR is to resolve the main concern initially described in #11538, i.e. addressing the most common use cases. On this basis and depending on feedback after it is merged, the remaining required UX improvements in this dialog can be described and implemented in a subsequent PR if needed.

Link to issue number:

Fixes the initial request of #11538.

Summary of the issue:

When a user encounters an issue with NVDA, a dev diagnosing the issue may need to ask them a log. In this situation, the most common use case is to ask a log with add-ons disabled (to eliminate possible add-on interferences) and log level set to debug (to get the maximum information to help debugging). Though, NVDA does not provide a handy way to restart with add-ons disabled and log level set to debug.

Description of user facing changes

NVDA's exit dialog now provides the 4 following permanent options:

  • "Exit" (as before)
  • "Restart" (as before)
  • "Restart with add-ons disabled and debug logging", replacing "Restart with add-ons disabled". This option may be used to discriminate if a bug comes from NVDA or from add-ons, and in case it comes from NVDA, help a developer to investigate and fix it.
  • "Restart with debug logging", just renamed from "Restart with debug logging enabled". This option is useful to get a log to investigate a bug in an add-on.

The non-permanent option to install pending updates remains unchanged.

And the options in secure mode remain unchanged. More specifically, "Restart with add-ons disabled" remains without enabling debug logging because logging is disabled in secure mode.

Description of development approach

Added the RESTART_WITH_ADDONS_DISABLED_AND_DEBUG_LOGGING item in gui.exit._ExitAction enum. And removed the unneeded items of this enum to display the combo-box. More specifically, RESTART_WITH_ADDONS_DISABLED_AND_DEBUG_LOGGING and RESTART_WITH_ADDONS_DISABLED cannot coexist in the allowed values of the combo-box.

Also renamed RESTART_WITH_DEBUG_LOGGING_ENABLED to RESTART_WITH_DEBUG_LOGGING so that RESTART_WITH_ADDONS_DISABLED_AND_DEBUG_LOGGING does not become a longer RESTART_WITH_ADDONS_DISABLED_AND_DEBUG_LOGGING_ENABLED; same (and more importantly) for the displayed string. Moreover, technically there is no debug logging that we can enable or disable in NVDA, but a debug level that may be set to "disabled", to "debug" or other intermediate levels.

At last, "Restart with debug logging" is not named "Restart with add-ons enabled and debug logging", because the add-ons can still be disabled all individually in the add-on store.

Testing strategy:

Manual tests:

  • Tried the 4 items of the menu in normal mode and checked if the log and the add-on availability are as expected
  • Tried the 3 items of the menu in secure mode and checked that no log is produced and that the add-ons are disabled in the case when it is expected.

No test of Windows Store app (config.isAppX) since the Windows Store version of NVDA is not supported anymore.

Known issues with pull request:

Does not address the dialog redesign discussed in #11538. This can be done in a subsequent PR if needed.

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.

Summary by CodeRabbit

  • New Features

    • Added an option to restart NVDA with add-ons disabled and debug logging enabled, enhancing user control for troubleshooting.
  • Documentation

    • Updated user guide to clarify new exit options, including the addition of debug logging during restarts.
  • Bug Fixes

    • Improved the naming conventions and logic associated with exit actions for better usability and understanding.

@CyrilleB79 CyrilleB79 requested review from a team as code owners August 22, 2024 14:24
Copy link
Contributor

coderabbitai bot commented Aug 22, 2024

Walkthrough

The changes introduce a new exit action in the NVDA application, allowing users to restart with add-ons disabled and debug logging enabled. Modifications to the exit dialog enhance the clarity of available options, including updates to documentation for user guidance. The exit action logic has been adjusted to accommodate these new functionalities, streamlining user control over the restart behavior of the application.

Changes

Files Change Summary
source/gui/exit.py Added a new exit action RESTART_WITH_ADDONS_DISABLED_AND_DEBUG_LOGGING to the _ExitAction class. Updated the onOk method to handle the new action. Renamed RESTART_WITH_DEBUG_LOGGING_ENABLED to RESTART_WITH_DEBUG_LOGGING.
user_docs/en/changes.md Documented the new feature that allows users to restart with add-ons disabled and debug logging enabled in the exit dialog.
user_docs/en/userGuide.md Updated the user guide to clarify checkbox options in the exit dialog, explicitly mentioning the inclusion of debug logging during the restart process.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
user_docs/en/userGuide.md (1)

1756-1756: Clarify the exit options description.

The changes improve clarity by mentioning debug logging options.

Consider rephrasing for better readability:

When checked, a dialog will appear when you attempt to exit NVDA, asking whether you want to exit, restart, restart with add-ons disabled and debug logging, restart with debug logging, or install pending updates (if any).

@AppVeyorBot
Copy link

See test results for failed build of commit 26683355a2

Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Thanks Cyrille, this will definitely make users' and developers' jobs easier when attempting to resolve issues. I'm just a bit unsure about the changes in wording.

source/gui/exit.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
CyrilleB79 and others added 3 commits August 26, 2024 09:25
Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Thanks Cyrille, this looks good to me now.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 27, 2024
@seanbudd seanbudd added this to the 2025.1 milestone Aug 27, 2024
user_docs/en/changes.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

This will be really handy, as setting debug logging AND disabling add-ons is something we commonly request of users when troubleshooting, great work!

@seanbudd seanbudd merged commit 7d247ee into nvaccess:master Aug 29, 2024
2 checks passed
@CyrilleB79 CyrilleB79 deleted the restartDebug branch September 3, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants