Skip to content

Make gui.MainFrame.popupSettingsDialog part of the public API#15121

Merged
seanbudd merged 3 commits into
masterfrom
fixuUpPopupSettingsDialog
Jul 14, 2023
Merged

Make gui.MainFrame.popupSettingsDialog part of the public API#15121
seanbudd merged 3 commits into
masterfrom
fixuUpPopupSettingsDialog

Conversation

@seanbudd

@seanbudd seanbudd commented Jul 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #15120

Summary of the issue:

If add-ons want to provide a gesture or similar to open their config dialogs, they currently have two choices:

  • Call gui.mainFrame._popupSettingsDialog, which is a private method and could thus change signature or mechanism at any moment without breakage warning. Several add-ons currently live on the edge by using this approach, including one of mine.
  • Replicate the internals of gui.MainFrame._popupSettingsDialog to achieve the same thing, which involves some imports and careful management of parents and exceptions.

Description of user facing changes

None

Description of development approach

Mark API as public

Update AddonStore command to use the function

Testing strategy:

Tested opening and closing add-on store

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 a team as a code owner July 11, 2023 06:24
@seanbudd seanbudd requested a review from michaelDCurran July 11, 2023 06:24
XLTechie
XLTechie reviewed Jul 11, 2023
View reviewed changes

@XLTechie XLTechie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @seanbudd! I suggest a note in the changes for developers that this has become public.

@AppVeyorBot

AppVeyorBot commented Jul 11, 2023

Copy link
Copy Markdown

See test results for failed build of commit 3aca986134

@seanbudd seanbudd requested a review from michaelDCurran July 12, 2023 01:36
@seanbudd

seanbudd commented Jul 12, 2023

Copy link
Copy Markdown
Member Author

@michaelDCurran - re-requesting review as the lint fix was non-trivial

@XLTechie

XLTechie commented Jul 12, 2023 via email

Copy link
Copy Markdown
Collaborator

@seanbudd seanbudd merged commit a8b79c7 into master Jul 14, 2023
@seanbudd seanbudd deleted the fixuUpPopupSettingsDialog branch July 14, 2023 07:13
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jul 14, 2023
@cary-rowen cary-rowen mentioned this pull request Jul 14, 2023
Closed
@jmdaweb

jmdaweb commented Jul 15, 2023

Copy link
Copy Markdown
Contributor

Hi,
Since the add-ons mailing list is not the right place to discuss these things (as stated by the moderator), I suggest adding an alias for this method to keep compatibility until NVDA 2024.1. Although the function was not public, many add-ons make use of it and will break on 2023.2. This is the relevant thread: https://nvda-addons.groups.io/g/nvda-addons/topic/100136959
Regards.

@nvdaes

nvdaes commented Jul 15, 2023

Copy link
Copy Markdown
Collaborator

Hello: Seems that this PR has been merged even wrongly. When I see the log, this appears:

commit a8b79c7
Author: Sean Budd sean@nvaccess.org
Date: Fri Jul 14 17:13:13 2023 +1000

Make gui.MainFrame.popupSettingsDialog part of the public API (#15121)

Fixes #15096

Summary of the issue:
When an active profile has the synthesizer set to silence, and a user switches profiles to a synthesizer which fails to load, NVDA fails back to the silence synthesizer.

Also, as mentioned in issue #15141 , this breaks compatibility with add-ons. Additionally, in issue #15142 , vision provided are broken.
I think it maybe better to revert this and, since it's not urgent, and NVDA 2023.2 is almost complete, and we need to test relevant things like support for localizations in the store, I think this maybe reverted and set the milestone to 2024.1.

Author: Sean Budd sean@nvaccess.org
Date: Fri Jul 14 17:13:13 2023 +1000

Make gui.MainFrame.popupSettingsDialog part of the public API (#15121)

Fixes #15096

Summary of the issue:
When an active profile has the synthesizer set to silence, and a user switches profiles to a synthesizer which fails to load, NVDA fails back to the silence synthesizer.

@XLTechie

XLTechie commented Jul 15, 2023 via email

Copy link
Copy Markdown
Collaborator

@ABuffEr

ABuffEr commented Jul 15, 2023

Copy link
Copy Markdown
Contributor

I think it maybe better to revert this and, since it's not urgent, and NVDA 2023.2 is almost complete, and we need to test relevant things like support for localizations in the store, I think this maybe reverted and set the milestone to 2024.1.

I completely agree with this proposal.

@XLTechie XLTechie mentioned this pull request Jul 15, 2023
Closed
6 tasks
seanbudd added a commit that referenced this pull request Jul 17, 2023
@seanbudd
Retain aliases lost in #15121
2bee31a
@seanbudd seanbudd mentioned this pull request Jul 17, 2023
Merged
6 tasks
seanbudd added a commit that referenced this pull request Jul 17, 2023
@seanbudd
Retain aliases lost in #15121 (#15151)
c4ca763 
Follow up to #15121

Fixes #15142
Fixes #15141
Fixes #15148
Fixes #15147

Summary of the issue:
PR #15121 to fix #15120 had various unexpected side effects.
This is due to symbols that were previously star imported into gui/__init__.py being removed.
Add-ons and the vision enhancements core module relied on these symbols being imported.
While there was no API breaking changes, these changes affected over 30 add-ons

Description of user facing changes
Fix up issue with vision enhancement providers, such as opening the relevant settings panels.

Description of development approach
Add aliases for SettingsPanel, AutoSettingsMixin, _popupSettingsDialog in gui
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

+1 more reviewer

@XLTechie XLTechie XLTechie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2023.2

Development

Successfully merging this pull request may close these issues.

Make gui.MainFrame._popupSettingsDialog part of the public API so add-ons can safely use it, or create a similar public function

8 participants

@seanbudd @AppVeyorBot @XLTechie @jmdaweb @nvdaes @ABuffEr @michaelDCurran @nvaccessAuto