Skip to content

Fix #20148: set disabled state if starting magnifier failed#20184

Merged
seanbudd merged 1 commit into
betafrom
fix-20148
May 21, 2026
Merged

Fix #20148: set disabled state if starting magnifier failed#20184
seanbudd merged 1 commit into
betafrom
fix-20148

Conversation

@seanbudd

@seanbudd seanbudd commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Fix #20148

Summary of the issue:

If the magnifier failed to start (E.g. if screen curtain or win mag was running), the enabled state was still set to True

Description of user facing changes:

magnifier state now accurately reflects if magnifier failed to start

Description of developer facing changes:

none

Description of development approach:

check if running when setting state

Testing strategy:

tested steps from #20148

Known issues with pull request:

none

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.

@seanbudd seanbudd added this to the 2026.2 milestone May 20, 2026
@seanbudd seanbudd requested a review from michaelDCurran May 20, 2026 07:29
@seanbudd seanbudd requested a review from a team as a code owner May 20, 2026 07:29
Copilot AI review requested due to automatic review settings May 20, 2026 07:29
Copilot AI reviewed May 20, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes NVDA Magnifier’s persisted “enabled” state getting out of sync with the actual runtime state when magnifier startup/stop does not succeed, which in turn causes the settings checkbox state to be incorrect.

Changes:

  • Update start() to persist enabled based on isActive() after attempting to start.
  • Update stop() (when persisting) to persist enabled based on isActive() after attempting to stop.

Comment thread source/_magnifier/__init__.py
@seanbudd seanbudd merged commit 63c72b2 into beta May 21, 2026
50 of 56 checks passed
@seanbudd seanbudd deleted the fix-20148 branch May 21, 2026 04:13
@CyrilleB79

CyrilleB79 commented May 21, 2026

Copy link
Copy Markdown
Contributor

@seanbudd, the checked/unchecked seems to be OK now. But all the comments of #20148 haven't been taken into account.

As written in #20148 (comment), NVDA still reports "Starting Fullscreen magnifier with 10.0x zoom level, Normal filter, and Center full-screen mode". This is not correct.

Also the 3rd bullet of expected result described in this same comment reads:

Since this is a GUI action, the "Cannot start magnifier" should rather be in a message box, as done with screen curtain, rather than only spoken.

You haven't taken this into account. Is it on purpose, and if yes, why? If not, can you also take this point into account when fixing the erroneous message? Thanks.

@seanbudd seanbudd mentioned this pull request May 26, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@michaelDCurran michaelDCurran michaelDCurran approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2026.2

Development

Successfully merging this pull request may close these issues.

4 participants

@seanbudd @CyrilleB79 @michaelDCurran