Skip to content
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

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

XLTechie
Copy link
Collaborator

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.

  • Input Composition
  • Review Cursor
  • browse mode
  • Document Nav
  • Add-on Store
  • Windows OCR

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:

  • 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.

@coderabbitai summary

@XLTechie XLTechie requested a review from a team as a code owner September 11, 2024 11:16
@XLTechie
Copy link
Collaborator Author

@josephsl, if you can, please verify the description of Input Composition.
@Qchristensen You may want to look at these settings panels when it builds.

Copy link
Member

@SaschaCowley SaschaCowley left a 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 Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
Comment on lines 3202 to 3203
"Adjust certain aspects of the Add-on Store, including whether you want to be notified "
"when your add-ons have updates available.",
Copy link
Member

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.

@@ -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.",
Copy link
Member

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:

Suggested change
"Configure some elements of the Windows optical character recognition feature which NVDA makes available.",
"Configure NVDA's Windows optical character recognition support.",

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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:

  1. 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 to False to maintain backwards compatibility, or True 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 its panelDescription.
  2. Add a helper function to SettingsPanel that will handle adding the description from panelDescription 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.

@SaschaCowley SaschaCowley marked this pull request as draft September 12, 2024 01:07
Copy link
Collaborator

@CyrilleB79 CyrilleB79 left a 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?

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Sep 17, 2024
@SaschaCowley
Copy link
Member

@XLTechie what is the status of this PR?

@XLTechie
Copy link
Collaborator Author

XLTechie commented Oct 28, 2024 via email

@SaschaCowley
Copy link
Member

I'm happy to keep this open, I just wanted to check it wasn't abandoned. Best of luck with the repairs!

@XLTechie
Copy link
Collaborator Author

XLTechie commented Oct 29, 2024 via email

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.

More config dialogs in NVDA settings should have descriptive text
4 participants