Skip to content

Add-on store: add warning dialog#15246

Merged
seanbudd merged 6 commits into
betafrom
addWarningText
Aug 4, 2023
Merged

Add-on store: add warning dialog#15246
seanbudd merged 6 commits into
betafrom
addWarningText

Conversation

@seanbudd

@seanbudd seanbudd commented Aug 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

None

Summary of the issue:

Users should be informed of the risk of installing add-ons

Description of user facing changes

When opening the add-on store, a warning dialog is shown.
There is a checkbox to not show the warning again.

Description of development approach

Uses similar approach to the user stats dialog
Move banner creation to helper function

Added typing to guiHelper.BoxSizerHelper. Fix bug where if a StaticBoxSizer is used with an intflag of buttons, BoxSizerHelper.addDialogDismissButtons wouldn't work correctly

Add support for creating labelled checkboxes using the BoxSizerHelper, LabelledControlHelper and associateElements

Testing strategy:

Test banner using --disable-addons
Test with sample code in source code for addDialogDismissButtons bug.
Test the warning nag - ensure the checkbox to hide the warning works

Known issues with pull request:

None

Change log entries:

N/A

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.

@seanbudd seanbudd requested a review from Qchristensen August 2, 2023 05:05
@seanbudd seanbudd requested a review from a team as a code owner August 2, 2023 05:05
@seanbudd seanbudd requested review from michaelDCurran and removed request for a team August 2, 2023 05:05
@seanbudd seanbudd added this to the 2023.2 milestone Aug 2, 2023
Qchristensen
Qchristensen previously approved these changes Aug 2, 2023
View reviewed changes

@Qchristensen Qchristensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work, thanks Sean!

@AppVeyorBot

AppVeyorBot commented Aug 2, 2023

Copy link
Copy Markdown

See test results for failed build of commit 707222d8fa

michaelDCurran
michaelDCurran reviewed Aug 2, 2023
View reviewed changes
Comment thread source/gui/_addonStoreGui/controls/storeDialog.py Outdated
@XLTechie

XLTechie commented Aug 2, 2023 via email

Copy link
Copy Markdown
Collaborator

@XLTechie

XLTechie commented Aug 2, 2023 via email

Copy link
Copy Markdown
Collaborator

@michaelDCurran

michaelDCurran commented Aug 2, 2023 via email

Copy link
Copy Markdown
Member

@CyrilleB79

CyrilleB79 commented Aug 2, 2023

Copy link
Copy Markdown
Contributor

I also support the one-time dialog. A disclaimer at each use of the add-on store is too invasive IMO. more so when it adds a tab keypress to the navigation flow.

In addition, I think that the disclaimer in the User Guide should be more explicit (and longer than the one of the GUI if needed). A disclaimer paragraph located just after the introduction of the add-on store's global description would be the best IMO, i.e. a 13.1 paragraph inserted after the introduction of section 13 and before the current paragraph 13.1.

At last, if you stick with the disclaimer in the add-on store's GUI:

  • do not reduce the message to one line, because you cannot know if the message will fit on one line on all systems, e.g. in case the message is translated or if the user uses different screen parameters (screen size, font size, resizing the Window, etc.)
  • do not use a control that can be focused or consumes a tab keypress while navigating. Rather use something such as panel descriptions in some setting panels such as "Object presentation" or "Document formatting" panels.

@seanbudd seanbudd changed the title Add-on store: add warning text Add-on store: add warning dialog Aug 3, 2023
@seanbudd seanbudd requested a review from michaelDCurran August 3, 2023 05:12
@CyrilleB79

CyrilleB79 commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

@seanbudd, I have tested the new version of this code. I have various remarks:

  1. The UX of this dialog is not very standard. A dialog with an OK button and a "Don't show this dialog again" would be much more standard.

  2. "Acknowledge" button is not suitable. Because if I do not press it, it would mean that I have not acknowledged the message, what is not expected.

  3. "Remind me later" is usually used to postpone an action more important than just acknowledging, e.g. installing an update or restarting the application.

  4. Saving in the config is not suitable because:

    • if my config is not saved automatically when NVDA exits I will be shown again this dialog after next restart
    • What happens if I launch the add-on store when a manual profile is active? Will the acknowledgement be saved in the profile? This is obviously not expected.

@michaelDCurran

michaelDCurran commented Aug 3, 2023

Copy link
Copy Markdown
Member

I second the comment from @CyrilleB79
that it should be an Okay button with a checkbox. However, I think saving in the config is fine (at least for NVDA 2023.2) as other dialogs like the Welcome dialog also save in the config. Worst case is that the user is asked again next time, and has to eventually save their config.

@seanbudd seanbudd merged commit b8efec7 into beta Aug 4, 2023
@seanbudd seanbudd deleted the addWarningText branch August 4, 2023 03:33
@CyrilleB79

CyrilleB79 commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

@seanbudd:
When the warning dialog is open (even in background), we cannot activate NVDA's menu with NVDA+N, but there is not the usual message indicating "Action unavailable while a dialog is expecting an answer".
Could you have a look to it?

@CyrilleB79 CyrilleB79 mentioned this pull request Aug 5, 2023
Closed
@seanbudd seanbudd mentioned this pull request Aug 7, 2023
Merged
6 tasks
seanbudd added a commit that referenced this pull request Aug 7, 2023
@seanbudd
Create helper function to show custom modal dialogs safer (#15265)
ab2b985 
Raised in #15246 (comment)

Summary of the issue:
Sometimes a custom modal dialog is required, instead of just using gui.message.messageBox.
For example, adding additional buttons or controls instead of just message text and ok/cancel/yes/no buttons.
However gui.message.messageBox adds special handling to ensure users are warned if actions are blocked by an open modal dialog.

Description of user facing changes
Users are warned when trying to perform a task (e.g opening the NVDA menu) when modal dialogs from the add-on store are waiting a response.
Other modal dialogs have also been addressed, unless they are non-blocking (i.e. started in a separate thread).
These might be worth addressing too, on a case-by-case basis.

Description of development approach
Create a helper function to show a dialog as a modal dialog, while performing the necessary handling of messageBox

Testing strategy:
Test opening various Add-on store dialogs and performing nvda+n
@seanbudd seanbudd mentioned this pull request Aug 9, 2023
Closed
@hwf1324 hwf1324 mentioned this pull request Sep 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

@Qchristensen Qchristensen Qchristensen left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2023.2

Development

Successfully merging this pull request may close these issues.

6 participants

@seanbudd @AppVeyorBot @XLTechie @michaelDCurran @CyrilleB79 @Qchristensen