-
-
Notifications
You must be signed in to change notification settings - Fork 638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added descriptive text to more of the settings panels #17160
base: master
Are you sure you want to change the base?
Conversation
@josephsl, if you can, please verify the description of Input Composition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making a start on this @XLTechie. There's still a bit of work to go with regard to making the descriptions visible in the GUI, and I've also provided some thoughts on the wording.
source/gui/settingsDialogs.py
Outdated
"Adjust certain aspects of the Add-on Store, including whether you want to be notified " | ||
"when your add-ons have updates available.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels fairly self-explanatory to me.
source/gui/settingsDialogs.py
Outdated
@@ -3246,6 +3269,10 @@ def onSave(self): | |||
class UwpOcrPanel(SettingsPanel): | |||
# Translators: The title of the Windows OCR panel. | |||
title = _("Windows OCR") | |||
panelDescription = _( | |||
# Translators: This is descriptive text shown at the top of the Windows OCR settings panel. | |||
"Configure some elements of the Windows optical character recognition feature which NVDA makes available.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wording feels a little clunky, but I'm not sure I can come up with much better. How about:
"Configure some elements of the Windows optical character recognition feature which NVDA makes available.", | |
"Configure NVDA's Windows optical character recognition support.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, most of what I came up with was clunky too. I'd apply this, but GH won't let me for unknown reasons (the diff is up to date). I'll change it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panel descriptions are not visible in the UI. Take a look at ObjectPresentationPanel.makeSettings
to see how they're being added visually. Though given similar code is increasingly going to be repeated all over the place, I wonder if it's worth refactoring SettingsPanel
to visually add panelDescription
itself to keep things DRY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaschaCowley
That would be a heck of a refactor, given that makeSettings
is abstract.
I hadn't looked at that code in a while, and forgot how it worked.
It would break a lot of add-ons to change the way this is handled, although I suppose now is the time.
I thought of trying to add it to the sizer using the metaclass, and have it be dependent on whether makeSettings
does anything with the intro text, but inspect.getsource()
is not going to be able to read makeSettings
for one of the gui.SettingsPanel
subclasses. At least, as far as I know.
Not sure of any other "gentle" way of doing things.
It's either break API here and refactor the whole thing; or duplicate the code.
Unless you can think of an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right... Two thoughts come to mind, though I haven't thought through these at great length:
- Add the panel description in
SettingsPanel._buildGui
. Add a class attribute,automaticallyAddPanelDescription
(or similar) to override this behaviour if not desired. This could be set toFalse
to maintain backwards compatibility, orTrue
to make things "just work". This will be needed, at the very least, for the advanced settings panel, which has a warning banner and descriptive text that are added separately, but concatenated to form itspanelDescription
. - Add a helper function to
SettingsPanel
that will handle adding the description frompanelDescription
appropriately. Manually call the helper function whenever we want a visible description. This way, we aren't repeating all of the layout logic, but aren't changing the API in any backwards-incompatible way. Theoretically, I think this would only need to be passed the box sizer helper as it will be able to get everything else from instance attributes.
My inclination is to go the automatic route, as SettingsPanelAccessible
automatically adds panelDescription
from the screen-reader user's point of view, so why shouldn't it be there visually? I'm not sure how many things this would break though.
Co-authored-by: Sascha Cowley <[email protected]>
This reverts commit e994f06.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XLTechie.
Only some minor suggestions.
Also, I wonder if it wouldn't be useful to add a description in all panels for consistency?
Co-authored-by: Cyrille Bougot <[email protected]>
Co-authored-by: Cyrille Bougot <[email protected]>
@XLTechie what is the status of this PR? |
@SaschaCowley I don't think I can get back to this until December or January. I still plan to, but my office was partially demolished thanks to a toxic mold invasion.
Working out of a temporary location while rebuilding, with only a third of my usual infrastructure, has derailed anything not high priority.
If you need to close it to get it out of the way, it's fine.
|
I'm happy to keep this open, I just wanted to check it wasn't abandoned. Best of luck with the repairs! |
Thanks!
|
Link to issue number:
Fixes #13568
Summary of the issue:
Several settings panels were missing friendly introductory text.
While some have names that are so obvious they do not need introductory text (Speech, Braille, Keyboard, Mouse), others do need it (Input Composition, Browse Mode).
Description of user facing changes
The following settings panels now have some introductory text.
Description of development approach
Wrote, or mostly copied from the user guide with minor alterations, descriptions for the above listed settings panels.
Testing strategy:
Built, made sure the modified panels spoke correctly.
Some with vision may want to check that they show up right.
Known issues with pull request:
None, unless you count that i didn't label the more obvious panels.
Code Review Checklist:
@coderabbitai summary