Clarity improvement in magnifier naming#20281
Conversation
|
@CyrilleB79 I started rewritting the user docs and I'll do the same in the settings when #19913 is closed. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the magnifier “follow focus” terminology to “tracking” across the codebase, user docs, and scripts, and renames the fullscreen mode action from “toggle” to “cycle” for clarity.
Changes:
- Renamed
MagnifierFollowFocusTypetoMagnifierFollowTrackingTypeand updated related call sites. - Updated user documentation to reflect the new “tracking” terminology and revised descriptions.
- Renamed the fullscreen magnifier mode command from
toggleFullscreenModetocycleFullscreenMode.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| user_docs/en/userGuide.md | Renames “focus tracking” language to “tracking” and adjusts magnifier documentation/anchors. |
| tests/unit/test_magnifier/test_focusManager.py | Updates unit tests to use the renamed follow/tracking enum. |
| source/gui/settingsDialogs.py | Renames magnifier UI “Focus” group to “Tracking” and updates follow checkbox plumbing; also changes review cursor config key usage. |
| source/globalCommands.py | Updates scripts to use the renamed enum and cycles fullscreen mode via the renamed command. |
| source/_magnifier/utils/types.py | Renames the follow enum type and updates translation contexts/labels. |
| source/_magnifier/utils/focusManager.py | Migrates focus manager to the renamed tracking enum type. |
| source/_magnifier/config.py | Updates follow-state config mapping and public API parameter naming to “tracking”. |
| source/_magnifier/commands.py | Updates toggle-follow API to accept tracking type and renames fullscreen mode function to “cycle”. |
|
FYI, I have made modifications in #20261 which update UI strings and even more sometimes. These may conflict with this PR, sorry (especially regarding "follow mode" / "tracking type"). |
|
Also are we sure that "tracking mode" (previously "focus mode" / "full-screen mode") only applies to full-screen? Couldn't a docked mode user choose between "center" and "Border"? This PR is probably not the place to change that, but at least keep it in mind when/if changing variable/options names to avoid "full-screen" if the option may be moved in the future. And another question: |
Yes I will wait for your modification, I reverted to draft.
for now it's a fullscreen thing, because It's a fullscreen thing for Windows Magnifier, but nothing is fixed and this could be applied to other types of view, I think we could keep it as it is for know and change it if we want to add this to other modes, like spotlight (wich would be change to overview)
sometimes I feel like removing relatives would just save us some times, ahah |
Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
CyrilleB79
left a comment
There was a problem hiding this comment.
Partial review.
I've not had time to give all feedback or provide more suggestions.
There was a problem hiding this comment.
As explained in the initial issue, "Relative" should be explained without referring to Windows Magnifier.
Since you indicate the goal of each mode:
I think that Relative mode is useful if one wants to figure where the tracked element / the zoomed area is located with respect to the whole screen.
There was a problem hiding this comment.
I thought I had changed it, my bad
There was a problem hiding this comment.
I thought I had changed it, my bad
Indeed, you had. But it has been lost somewhere, maybe while merging latest changes in beta.
Let's hope you have not lost other parts of your work.
|
Also, it may be interesting to rename "focus manager" to "tracking manager" (file name, class, etc.) |
|
should I rename the "follow" functions to "tracking" as well? like |
…avail/nvda into MagnifierNamingClarity
|
This is way too much unnecessary code churn for beta. Can we please avoid any large code changes and instead only keep updating user facing strings and other necessary beta changes? |
|
well I'm not sure this pr really is needed for the beta as the goal was to rename variables and not so much a userchange issue, I think @CyrilleB79 already covered a lot of this in #20261. Maybe I there's some changes that can be made to userSettings and Userguide, but the main goal of this pr was to rename all functions for developers more than users |
| fullscreenModeLabelText = _("Focus &mode:") | ||
| fullscreenModeLabelText = _("Tracking &mode:") | ||
| fullscreenModeChoices = [mode.displayString for mode in FullScreenMode] if FullScreenMode else [] | ||
| self.fullscreenModeList = fullscreenGroup.addLabeledControl( |
There was a problem hiding this comment.
I'd rename this one since it is part of the (public) API:
| self.fullscreenModeList = fullscreenGroup.addLabeledControl( | |
| self.trackingModeList = fullscreenGroup.addLabeledControl( |
Link to issue number:
Closes #20126
Summary of the issue:
At the moment, the magnifier module lacks clarity in naming, it uses several terms to talk about the same things and that can lead to confusions
Description of user facing changes:
Users will have a better and clearer desciption and settings of the magnifier
Description of developer facing changes:
Variables and naming will be more consistent and logical
Description of development approach:
Adapting code naming to the User guide terms
Testing strategy:
Known issues with pull request:
Code Review Checklist: