Skip to content

changes magnifier types and function name for clarity#19882

Merged
seanbudd merged 14 commits intonvaccess:masterfrom
France-Travail:typeChanges
Apr 28, 2026
Merged

changes magnifier types and function name for clarity#19882
seanbudd merged 14 commits intonvaccess:masterfrom
France-Travail:typeChanges

Conversation

@Boumtchack
Copy link
Copy Markdown
Contributor

@Boumtchack Boumtchack commented Mar 31, 2026

Link to issue number:

pre - #19473
parts of #19810

Summary of the issue:

#19810 was still to big so I focused on types changes and some functions renaming to go accordingly.
All these changes are nescessary for futur magnifier implementations.

Description of user facing changes:

"default"s have been removed so it can be seen in the doc

Description of developer facing changes:

replaced MagnifierPosition with MagnifierParameters for better handling of size, place and filter of the magnifier.
added a MagnifierType to handle magnifiers different types.

Description of development approach:

This work was split out from PR #19473 to make review and integration easier.
We first added the new structure: MagnifierParameters for size/position/filter, and MagnifierType for futur mode switching.

Testing strategy:

Unit

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.

@Boumtchack Boumtchack marked this pull request as ready for review March 31, 2026 15:21
@Boumtchack Boumtchack requested a review from a team as a code owner March 31, 2026 15:21
@Boumtchack Boumtchack requested review from Copilot and seanbudd March 31, 2026 15:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the magnifier configuration and type model to improve clarity and enable future magnifier mode work (e.g., fixed/docked/lens).

Changes:

  • Renames magnifier config keys/functions to drop the “default” wording (e.g., getDefaultZoomLevelgetZoomLevel) and updates UI bindings accordingly.
  • Reworks magnifier geometry/type types (introduces MagnifierParameters, Size, Coordinates, expands MagnifierType).
  • Updates unit tests to align with the refactored APIs and fullscreen magnifier parameter calculation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
tests/unit/test_magnifier/test_magnifier.py Updates baseline magnifier tests to reflect removed/abstracted position logic and type changes.
tests/unit/test_magnifier/test_fullscreenMagnifier.py Updates fullscreen magnifier tests to use _getMagnifierParameters and explicit start behavior.
source/gui/settingsDialogs.py Renames magnifier settings controls/config interactions and updates help IDs/labels.
source/config/configSpec.py Renames magnifier config keys in the config spec (drops default* keys).
source/_magnifier/utils/types.py Refactors magnifier-related types (parameters/size/coordinates/type/filter).
source/_magnifier/magnifier.py Updates base magnifier to use renamed config getters and makes parameters computation abstract.
source/_magnifier/fullscreenMagnifier.py Implements _getMagnifierParameters for fullscreen magnifier and adjusts lifecycle behavior.
source/_magnifier/config.py Renames config accessor functions to match new key names and API names.
source/_magnifier/commands.py Updates command messaging to use renamed config accessors.
Comments suppressed due to low confidence (1)

source/_magnifier/utils/types.py:145

  • Coordinates and Filter are defined twice in this module (e.g., Coordinates at ~22 and again at ~140; Filter at ~107 and again at ~171). The later definitions override the earlier ones and can lead to inconsistent types (e.g., MagnifierParameters.filter vs config.getFilter()), plus it’s confusing for maintainers. Remove the duplicate definitions and keep a single canonical Coordinates / Filter (and update references accordingly).
class MagnifierParameters(NamedTuple):
	"""Named tuple representing the size and position of the magnifier"""

	magnifierSize: Size
	coordinates: Coordinates
	filter: Filter


class Coordinates(NamedTuple):
	"""Named tuple representing x and y coordinates"""

	x: int
	y: int


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread source/gui/settingsDialogs.py
Comment thread source/gui/settingsDialogs.py
Comment thread source/gui/settingsDialogs.py
Comment thread source/config/configSpec.py
Comment thread source/_magnifier/fullscreenMagnifier.py
Comment thread source/_magnifier/magnifier.py
Comment thread source/_magnifier/fullscreenMagnifier.py Outdated
Comment thread source/_magnifier/config.py Outdated
Comment thread source/gui/settingsDialogs.py
Comment thread source/gui/settingsDialogs.py
@hwf1324
Copy link
Copy Markdown
Contributor

hwf1324 commented Mar 31, 2026

Hi,

Regarding the types, I have a question: could we use locationHelper.Point instead of Coordinates?

Thanks!

@seanbudd seanbudd marked this pull request as draft April 1, 2026 02:51
@seanbudd seanbudd marked this pull request as draft April 1, 2026 02:51
@hwf1324
Copy link
Copy Markdown
Contributor

hwf1324 commented Apr 1, 2026

locationHelper.POINT is actually ctypes.wintypes.POINT, and locationHelper.Point inherits from locationHelper._Point, which provides some helper methods.

@hwf1324
Copy link
Copy Markdown
Contributor

hwf1324 commented Apr 1, 2026

Regarding Size, there is actually also wx.Size, but would this cause confusion?

@seanbudd
Copy link
Copy Markdown
Member

seanbudd commented Apr 1, 2026

I think we should be using Coordinates.

@Boumtchack Boumtchack marked this pull request as ready for review April 7, 2026 09:15
@Boumtchack Boumtchack requested a review from a team as a code owner April 7, 2026 09:15
@Boumtchack Boumtchack requested a review from Qchristensen April 7, 2026 09:15
Comment thread tests/unit/test_magnifier/test_magnifier.py
Comment thread tests/unit/test_magnifier/test_focusManager.py Outdated
Comment thread source/_magnifier/utils/types.py Outdated
Comment thread source/_magnifier/utils/types.py Outdated
Comment thread source/_magnifier/utils/types.py Outdated
Comment thread source/_magnifier/utils/focusManager.py Outdated
@seanbudd seanbudd marked this pull request as draft April 7, 2026 23:18
@seanbudd seanbudd changed the title changes types and function name for clarity changes magnifier types and function name for clarity Apr 10, 2026
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 10, 2026
@Boumtchack Boumtchack marked this pull request as ready for review April 14, 2026 13:21
@seanbudd seanbudd marked this pull request as draft April 16, 2026 00:29
@Boumtchack
Copy link
Copy Markdown
Contributor Author

I deleted the dulpicates with the merge of master

@Boumtchack Boumtchack marked this pull request as ready for review April 21, 2026 09:16
Comment thread source/_magnifier/utils/types.py Outdated
Copy link
Copy Markdown
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.

Please ensure the diff doesn't contain any unnecessary changes, like moving classes around arbitrarily

Comment thread source/_magnifier/utils/types.py Outdated
Comment thread source/_magnifier/utils/types.py Outdated
@seanbudd seanbudd marked this pull request as draft April 21, 2026 23:18
@Boumtchack
Copy link
Copy Markdown
Contributor Author

I didn’t know about the quoted type annotation for forward references, which is why I moved the classes around.

@Boumtchack Boumtchack marked this pull request as ready for review April 22, 2026 09:41
Comment thread source/_magnifier/config.py Outdated
Comment thread source/_magnifier/config.py Outdated
Comment thread user_docs/en/userGuide.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
Comment thread user_docs/en/userGuide.md Outdated
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@seanbudd
Copy link
Copy Markdown
Member

Can you also please make sure to resolve conflicts from #19732 being merged?

Copy link
Copy Markdown
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.

Thanks @Boumtchack

Copy link
Copy Markdown
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

Looks good.

@seanbudd seanbudd merged commit 8b9c773 into nvaccess:master Apr 28, 2026
35 of 37 checks passed
@github-actions github-actions Bot added this to the 2026.2 milestone Apr 28, 2026
@Boumtchack Boumtchack deleted the typeChanges branch April 28, 2026 06:38
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

Development

Successfully merging this pull request may close these issues.

6 participants