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

Refactored the COM Registration Fixing Tool, to make it fail more gracefully, and improve its UX #12349

Closed

Conversation

XLTechie
Copy link
Collaborator

@XLTechie XLTechie commented Apr 28, 2021

Link to issue number:

Fixes #12345
Fixes #12351

Summary of the issue:

The COM Registration Fixing Tool still showed its completion dialog if it failed.
In particular, it showed it if the user initially said yes, but then selected NO or pressed escape in the UAC dialog.
That seemed disconcerting and incorrect.

Additionally, the code needed improvement and linting; and the UX of the tool was not all it could be.

Description of how this pull request fixes the issue:

Refactored this code to be in line with current coding standards, fix this problem, and to give the tool a better over all UX.

(More changes to come)

  • No longer shows success dialog on failure, or when the UAC is canceled or declined.
  • 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 mainly based on suggested text in COM Registration Fixing Tool shouldn't start with a warning dialog and off-putting language #12351 (comment)
  • Switched from using YES and NO buttons, to YES_NO.
  • Added a CANCEL button for visual closure and escape key use.
  • Added a Help button which opens the manual to the section describing the tool.
  • Catch the Windows error that signals user cancel of UAC, and return (as if "NO" had been initially chosen).
  • Added a docstring. Linted and added more logging.

Testing strategy:

  • Ran with extra debug logging calls, to test logic flow.
  • 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 WindowsError other than user cancel, to prove that a message would appear to show the error to the user.

Known issues with pull request:

  1. 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.
  2. 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)]

Change log entries:

New features

  • The COM Registration Fixing Tool will now show a message to the user, including the error, in the rare event of a Windows error while attempting COM re-registrations.
  • The COM Registration Fixing Tool initial dialog now has a visual close button, and can be exited with the escape key.

Bug fixes
The COM Registration Fixing Tool no longer shows a completion dialog if it was canceled by closing the UAC dialog without accepting. Instead it behaves as if "No" had been selected in the initial dialog.

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

… gracefully and improve code.

No longer shows success dialog on failure, or when the UAC is canceled or declined.
If there is a Windows error other than cancel of UAC, alert the user and show the error in a dialog.
Slight changes to warning message wording.
Switched from using YES and NO buttons, to YES_NO.
Added a CANCEL button for visual closure and escape key use.
Catch the Windows error that signals user cancel of UAC, and return (as if "NO" had been initially chosen).
Added a docstring.
Added some more logging.
@CyrilleB79
Copy link
Collaborator

Hello

Glad that you submit such a PR. I had it in mind already for a little time.
I can see it is still in Draft so you probably want to add additional work in it.

Here are my thoughts:

  • The errors that may occur in COMRegistrationFixes\__init__.py should not only be logged but also notified to the user when running the COM reg fixing tool. This way, we may clarify some comments that we can find on GitHub or on mailing lists such as "I have run the COM reg fixing tool two times because the first time I had strange results."
  • But what should be done when such an error occurs during NVDA installation?
  • I think you have changed the dialog asking confirmation prior to COM reg fixing tool execution. Could you check if it is now possible to add context help support to it. At the time context help was implemented, I had checked this but it was not supported in simple message box.

@XLTechie
Copy link
Collaborator Author

XLTechie commented Apr 29, 2021 via email

@CyrilleB79
Copy link
Collaborator

  • I think you have changed the dialog asking confirmation prior to COM reg fixing tool execution. Could you check if it is now possible to add context help support to it. At the time context help was implemented, I had checked this but it was not supported in simple message box.
    Maybe with the Help button? I will try to build it. I intend to improve the dialog further in any case, so it may end up being more helpful than the manual anyway.

No, there is no matter of help button. It depends on if you inherit from wx.Dialog or wx.MessageBox/gui.MessageBox. In the latter case, it does not seem possible to me.

@XLTechie XLTechie changed the title Refactored gui.onRunCOMRegistrationFixesCommand, to make it fail more gracefully, and to improve its code generally Refactored the COM Registration Fixing Tool, to make it fail more gracefully, and improve its UX Apr 29, 2021
@XLTechie XLTechie closed this Apr 30, 2021
@XLTechie
Copy link
Collaborator Author

This grew bigger than I originally intended; I will open a new PR from a more appropriate branch.

SaschaCowley pushed a commit that referenced this pull request Sep 20, 2024
…dows errors, improve UX (#12355)

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 (#12345).
* The initial dialog didn't have a visual close button (red X), and was not responsive to the `Escape` or `Alt+F4` keys (#10799).
* It didn't report errors to the user as pointed out by @CyrilleB79 in #12349, and
* Its initial warning dialog was not serving users well with its strong warning and presentation (#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:
1. 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.
2. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants