Refactor COM Registration Fixing Tool part 2: remove bugs, return Windows errors, improve UX by XLTechie · Pull Request #12355 · nvaccess/nvda

XLTechie · 2021-04-30T09:06:46Z

Link to issue number:

Fixes #10799
Fixes #12345
Fixes #12351
Replaces #12349

Summary of the issue:

The COM Registration Fixing Tool still showed its completion dialog if it failed, or if the user said no to the UAC dialog (If the UAC is cancelled, the COM Reg Fixing Tool still claims to have completed successfully, though an error is logged #12345).
The initial dialog didn't have a visual close button (red X), and was not responsive to the Escape or Alt+F4 keys (Run COM Registration fixing tool Dialog can't be closed by ALT+F4 or ESC #10799).
It didn't report errors to the user as pointed out by @CyrilleB79 in Refactored the COM Registration Fixing Tool, to make it fail more gracefully, and improve its UX #12349, and
Its initial warning dialog was not serving users well with its strong warning and presentation (COM Registration Fixing Tool shouldn't start with a warning dialog and off-putting language #12351).

User facing changes:

Initial dialog given a more friendly message, with a more beginner-themed explanation.
User may now press Escape/Alt+F4 to leave the dialog, as well as hitting Cancel.
There is now a visual close control (red X).
F1 context help is now available.
Errors are reported to the user by error code.
The tool no longer reports success if it fails.
The tool no longer reports success if the process is cancelled by UAC.

Development details:

Refactored this code to be in line with current coding standards, fix the above issues, and to give the tool a better over all UX.
Catch the Windows error generated on UAC cancel, and log that the process was stopped at the UAC.
Show other Windows errors to the user via a dialog, instead of just in the log.
Switched from using YES and NO buttons, to Continue and CANCEL.
Added a docstring. Linted and added more logging.
In order to implement the Continue and Cancel paradigm, created gui.nvdaControls._ContinueCancelDialog.

Testing strategy:

Tested all buttons by tabbing and pressing, and by keyboard shortcuts.
Tested that all windows appear when and where they should.
Confirmed that the "completed" message no longer appears on failure or cancellation.
Simulated a Windows error to prove that a message would appear to show the error to the user.

Known issues with pull request:

Upon UAC cancel, or upon successful completion, focus is moved from desktop or where ever it was, to the NVDA systray icon. That seems a little strange to me, although it is not new behavior. gui.postPopup is supposed to fix this, but it doesn't.
Even after being deleted, the progress window's speech appears to hang around until after the completion window is cleared, although this speech seems to be cancelled or interrupted. Again, not new behavior. Log fragment showing this below:

IO - speech.speak (02:07:07.616) - MainThread (8368):
Speaking ['COM Registration Fixing Tool', 'dialog', 'The COM Registration Fixing Tool has completed successfully.', CancellableSpeech (still valid)]
IO - speech.speak (02:07:07.621) - MainThread (8368):
Speaking ['OK', 'button', CancellableSpeech (still valid)]
IO - inputCore.InputManager.executeGesture (02:07:09.326) - winInputHook (6992):
Input: kb(desktop):enter
IO - speech.speak (02:07:09.406) - MainThread (8368):
Speaking ['COM Registration Fixing Tool', 'dialog', "Please wait while NVDA attempts to fix your system's COM registrations...", CancellableSpeech (still valid)]
IO - speech.speak (02:07:09.411) - MainThread (8368):
Speaking ['COM Registration Fixing Tool', CancellableSpeech (still valid)]

Code Review Checklist:

Summary by CodeRabbit

New Features
- Introduced a custom dialog with "Continue" and "Cancel" buttons for better user interaction.
User Experience Improvements
- Updated messaging in the COM Registration Fixing Tool for better clarity and guidance.
- Enhanced error handling and logging for a more reliable user experience.
- Users can now exit the initial window using the escape key or alt+f4.

michaelDCurran · 2021-07-12T21:43:41Z

What is the status of this pr now that pr #12560 has been merged? Has this one been replaced, or is there extra work in this pr?

XLTechie · 2022-03-15T10:08:33Z

@michaelDCurran, to answer your old question: There is extra work in this PR. #12560 did the back-end. This one does the front-end (UX), and picks up errors that are not currently being handled or shown to the user. However I have been unavailable for several months, and this has fallen enough behind master that it can no longer merge/rebase as-is. I am working to bring it up to a compilable state, and then finish anything left to go on it. I think it will be tested and good to go for 2022.3.

LeonarddeR · 2023-08-25T16:43:03Z

@XLTechie any chance you can continue this any time soon?

AppVeyorBot · 2023-09-27T11:20:52Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/h0ga1sxagrfdb9jr/artifacts/output/nvda_snapshot_pr12355-29445,054795cb.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.2,
TEST_START 0.0,
TEST_END 0.7,
FINISH_END 0.1

See test results for failed build of commit 054795cbe5

CyrilleB79 · 2024-01-25T14:04:38Z

Ping @XLTechie:
Do you plan to push forward this PR soon?

More specifically, I was about to implement a fix for #12345 when I remembered of your PR.

CyrilleB79 · 2024-01-26T08:46:36Z

For issues with compile setup, feel free to ask on nvda-devel mailing list where such issues are usually discussed and addressed.

AppVeyorBot · 2024-04-12T12:04:34Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/kxhhr2g7x6tp2gsj/artifacts/output/nvda_snapshot_pr12355-31588,b7653daa.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.1,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 30.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 2.4,
FINISH_END 0.2

See test results for failed build of commit b7653daab8

seanbudd · 2024-09-02T00:03:52Z

@XLTechie - do you plan to finish this work? Otherwise I think it should be closed

XLTechie · 2024-09-02T05:42:02Z

@seanbudd yes. I put it aside because I kept running into wx issues with text formatting being very strange. I'll look at it again, and if I can't make it work as-is, I'll split it into separate PRs addressing the major portions, so the important bits can get merged. Sorry for letting it lay for so long--time flies.

seanbudd · 2024-09-02T05:48:35Z

No problem - draft PRs are totally fine - we just want to make sure theyre not fully abandoned

gui.onRunCOMRegistrationFixesCommand: - No longer shows success dialog on failure, or when the UAC is canceled or declined (#12345).. - We now catch the Windows error that signals user cancel of UAC, and return as if "NO" had been initially chosen. - If there is a Windows error other than cancel of UAC, alert the user and show the error in a dialog. - Rewrote initial dialog message to be more friendly and helpful, and removed warning icon (#12351). - Switched from using YES and NO buttons, to OK and Cancel (CANCEL button provides visual closure and escape key use). - Added a docstring and fully linted code. Added more debug logging.

coderabbitai · 2024-09-03T01:06:23Z

Walkthrough

The changes introduce enhancements to the NVDA software, focusing on the COM Registration Fixing Tool. Key updates include improved user interface elements, refined error handling, and better user guidance. The tool's dialog now allows for closure via keyboard shortcuts, enhances logging for cancellations, and provides clearer messaging regarding its functionality and implications.

Changes

Files	Change Summary
`source/gui/__init__.py`	Enhanced `onRunCOMRegistrationFixesCommand` method with improved docstring, type hints, and error handling.
`source/gui/utilityDialogs.py`	Introduced `ContinueCancelDialog` class with custom button logic for a tailored user experience.
`user_docs/en/changes.md`	Updated documentation to reflect user interface improvements and bug fixes related to NVDA functionality.

Assessment against linked issues

Objective	Addressed	Explanation
Allow dialog to be closed with ALT+F4 or ESC (#[10799])	✅
Prevent success message if UAC is cancelled (#[12345])	✅
Improve messaging in the COM Registration Fixing Tool (#[12351])	✅

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>, please review it.
-- 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
-- @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 using 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.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

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.

coderabbitai

Actionable comments posted: 1

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

XLTechie · 2024-09-19T01:48:48Z

@SaschaCowley I have addressed your latest review. I'm not sure I like the user guide reordered as you requested, but I do agree with your reason for wanting to do it. In the end, all of the same information is there, which ever order it is presented in, so I'm fine with whatever.

SaschaCowley · 2024-09-19T02:12:25Z

@XLTechie I agree it really doesn't read very well now. Perhaps we could rewrite it to be a little less technical and more to the point of what users are likely to care about? What do you think of something like this?

Installing and uninstalling programs on a computer can sometimes cause problems with the Windows registry that result in NVDA behaving abnormally.
This can happen, for example, after installing and uninstalling Adobe Reader, Math Player and other programs.
It can also happen after Windows updates, or during other events that access the registry.
THE COM Registration Fixing Tool attempts to fix these issues by repairing accessibility entries in the registry.

The types of issues this tool can fix include:

* NVDA reporting "unknown" or "pane" when navigating in browsers such as Firefox or Edge, mail programs such as Thunderbird, Windows Explorer, the task bar, and other programs.
* NVDA failing to switch between focus mode and browse mode when you expect it to.
* NVDA being very slow when navigating in browsers while using browse mode.

Because this tool corrects entries in the Windows registry, it requires administrative access to work, just like when installing a program.
If you have UAC (User Access Control) enabled, as most users do, you will need to follow whatever prompts are presented by UAC to run the tool successfully.

…y be used instead. This reverts commit 78a1398.

XLTechie · 2024-09-19T07:27:58Z

@SaschaCowley How about this? Not quite what you wrote, but closer. @Qchristensen you might want to get in on this user guide discussion.

Qchristensen

Updated user guide and dialog intro text reads well. Great work everyone

XLTechie mentioned this pull request Jun 9, 2021

COM Registration Fixing tool doesn't work #9039

Closed

lukaszgo1 mentioned this pull request Nov 3, 2021

Implement the gui.messageBox API #13007

Closed

XLTechie changed the title ~~Refactored the COM Registration Fixing Tool code to remove bugs and for better UX~~ Refactor COM Registration Fixing Tool part 2: remove bugs, return Windows errors, improve UX Mar 24, 2022

Adriani90 mentioned this pull request Mar 1, 2023

Firefox 57 and Later Unusable with NVDA #7778

Closed

This comment was marked as outdated.

Sign in to view

seanbudd added conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle labels Jun 17, 2024

Luke Davis and others added 5 commits September 2, 2024 19:55

changes

239810d

Pre-commit auto-fix

5467ff6

String reformatting

a293607

Remove old imports

5dfea2a

XLTechie marked this pull request as ready for review September 3, 2024 01:06

XLTechie requested a review from a team as a code owner September 3, 2024 01:06

XLTechie requested a review from SaschaCowley September 3, 2024 01:06

coderabbitai Bot reviewed Sep 3, 2024

View reviewed changes

Comment thread source/gui/__init__.py Outdated

XLTechie marked this pull request as draft September 3, 2024 01:13

Emitintro message using a method.

e15dedc

SaschaCowley requested review from Qchristensen and SaschaCowley September 19, 2024 00:26

seanbudd approved these changes Sep 19, 2024

View reviewed changes

SaschaCowley reviewed Sep 19, 2024

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Comment thread user_docs/en/userGuide.md Outdated

Comment thread user_docs/en/userGuide.md Outdated

Comment thread source/gui/__init__.py Outdated

Comment thread source/gui/__init__.py

Luke Davis added 2 commits September 18, 2024 21:23

Remove attribution from changes entries

5fd2344

Merge branch 'master' into CRFTRefactor

4e88743

XLTechie commented Sep 19, 2024

View reviewed changes

Comment thread source/gui/__init__.py Outdated

XLTechie commented Sep 19, 2024

View reviewed changes

Comment thread source/gui/__init__.py Outdated

XLTechie and others added 3 commits September 18, 2024 21:32

Apply suggestions from code review

10ea475

Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>

Update user guide per review suggestion.

78a1398

Add a clarifying comment

1a158f1

XLTechie commented Sep 19, 2024

View reviewed changes

Comment thread source/gui/__init__.py Outdated

XLTechie and others added 2 commits September 18, 2024 21:57

Change a word in the intro message for more clarity.

aba1ce7

Merge branch 'master' into CRFTRefactor

bad035a

Luke Davis added 3 commits September 19, 2024 02:18

Revert "Update user guide per review suggestion.": a newer version ma…

5bcf510

…y be used instead. This reverts commit 78a1398.

Merge branch 'master' into CRFTRefactor

894e820

Potential new user guide entry.

1e58fae

Fix a tiny bit of structure in the UG

f8befd5

Qchristensen approved these changes Sep 19, 2024

View reviewed changes

SaschaCowley merged commit 593d81f into nvaccess:master Sep 20, 2024

github-actions Bot added this to the 2025.1 milestone Sep 20, 2024

coderabbitai Bot mentioned this pull request Oct 1, 2024

Update translations from Crowdin #17233

Merged

coderabbitai Bot mentioned this pull request Nov 11, 2024

Merge beta to master #17383

Merged

coderabbitai Bot mentioned this pull request Dec 9, 2024

Create a MessageDialog API #17304

Merged

31 tasks

coderabbitai Bot mentioned this pull request Dec 19, 2024

Fix math not being read in Chromium-based browsers (#17421) #17548

Merged

5 tasks

coderabbitai Bot mentioned this pull request Jan 23, 2025

Remote Access #17580

Merged

5 tasks

nishimotz mentioned this pull request Apr 14, 2025

2025.1 本家の翻訳 nvdajp/nvdajp#498

Closed

Uh oh!

Conversation

XLTechie commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

User facing changes:

Development details:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Summary by CodeRabbit

Uh oh!

michaelDCurran commented Jul 12, 2021

Uh oh!

XLTechie commented Mar 15, 2022 via email

Uh oh!

LeonarddeR commented Aug 25, 2023

Uh oh!

This comment was marked as outdated.

AppVeyorBot commented Sep 27, 2023

Uh oh!

CyrilleB79 commented Jan 25, 2024

Uh oh!

This comment was marked as outdated.

CyrilleB79 commented Jan 26, 2024

Uh oh!

AppVeyorBot commented Apr 12, 2024

Uh oh!

seanbudd commented Sep 2, 2024

Uh oh!

XLTechie commented Sep 2, 2024 via email

Uh oh!

seanbudd commented Sep 2, 2024

Uh oh!

coderabbitai Bot commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Assessment against linked issues

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

XLTechie commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SaschaCowley commented Sep 19, 2024

Uh oh!

XLTechie commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

XLTechie commented Apr 30, 2021 •

edited

Loading

coderabbitai Bot commented Sep 3, 2024 •

edited

Loading

XLTechie commented Sep 19, 2024 •

edited

Loading

XLTechie commented Sep 19, 2024 •

edited

Loading