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

Message Dialog API take 2 #17582

Merged
merged 221 commits into from
Jan 8, 2025
Merged

Message Dialog API take 2 #17582

merged 221 commits into from
Jan 8, 2025

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Jan 6, 2025

Link to issue number:

Closes #13007
Closes #12344
Closes #12353

Summary of the issue:

This is the second PR to merge the message dialog API into NVDA. The previous merge was at f437723.
See #17304 for full implementation details.
The original PR was reverted by #17561, due to #17553 and #17560.

Description of user facing changes

This PR does not, in itself, introduce any end-user facing changes. However, it lays the groundwork for closing a number of issues.

Tasks

Description of development approach

Restored gui.nvdaControls.MessageDialog inheriting from ContextHelpMixin, which was accidentally lost.

Changed updateCheck.UpdateAskInstallDialog.onUpdateButton and onPostponeButton to simply end the modal, returning the appropriate code. Added a static method to UpdateAskInstallDialog which returns a callback function which, when given the return code from UpdateAskInstallDialog, performs the appropriate action. Added a property method to generate this callback function given the attributes of the instance on which it is called.

Testing strategy:

Created a portable copy of NVDA (scons dist), and tested running the CRFT:

  • Running and granting permission - works as expected
  • Running and denying permission - works as expected
  • Cancelling - works as expected

Added updateVersionType="snapshot:alpha" to source/versionInfo.py`, and tested updating with NVDA running from source:

  1. Attempt updating via NVDA menu -> Help -> Check for update... -> Download update -> Update - works as expected
  2. Update by downloading then postponing update, then exit NVDA -> Install pending update
    1. Without incompatible updates installed - works as expected
    2. With incompatible updates installed - works as expected
  3. Update by downloading then postponing update, restarting NVDA, then NVDA menu -> Install pending update - works as expected
  4. Update by downloading then postponing update, restarting NVDA, then accepting the prompt to install the update - works as expected

Known issues with pull request:

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.

@coderabbitai summary

@SaschaCowley SaschaCowley marked this pull request as ready for review January 6, 2025 04:43
@SaschaCowley SaschaCowley requested a review from a team as a code owner January 6, 2025 04:43
@SaschaCowley SaschaCowley requested a review from seanbudd January 6, 2025 04:43
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.

New changes look good

source/updateCheck.py Outdated Show resolved Hide resolved
source/updateCheck.py Outdated Show resolved Hide resolved
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 6, 2025
@SaschaCowley SaschaCowley merged commit bd45d8e into master Jan 8, 2025
5 checks passed
@SaschaCowley SaschaCowley deleted the messageDialogApi branch January 8, 2025 03:55
@github-actions github-actions bot added this to the 2025.1 milestone Jan 8, 2025
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
2 participants