Make list of speech modes in the NVDA+s cycling script configurable.#15873
Conversation
|
@XLTechie @CyrilleB79 @LeonarddeR Since you expressed interest in #15806 you may want to review this PR. |
See test results for failed build of commit b7abfe1b7c |
Oh yes, to all questions! |
| if not any( | ||
| self._allSpeechModes[i].producesSpeech for i in enabledSpeechModes | ||
| ): | ||
| log.debugWarning("None of the speech modes producing speech are enabled. This configuration is invalid.") | ||
| gui.messageBox( | ||
| # translators: Message shown when none of required speech modes are enabled. | ||
| _("One of speech mode talk or on-demand has to be enabled."), | ||
| # Translators: The title of the message box | ||
| _("Error"), | ||
| wx.OK | wx.ICON_ERROR, | ||
| self | ||
| ) |
There was a problem hiding this comment.
is this necessary? wouldn't there be some utility in switching between beeps and off for some (partially sighted) users or for particular application configuration profiles?
I can't think of a use case, but I'm just concerned about artificially limiting configurability where there's no technical need to do so.
There was a problem hiding this comment.
I'm actually looking at this from another view point. Note that the default speech mode is talk. If you add a possibility to disable the talk mode, there is a bit of an inconsistency of what's the cold start default and what's configurable. I'd personally limit the list to beeps and on demand and always show the off and talk modes.
There was a problem hiding this comment.
This has been discussed at lengths in #15806, where the following conclusions were reached:
- Some users do not need to have the 'off' mode enabled, since they either want full speech, or beeps, which are just an indication that something has happened. While @XLTechie explicitly requested an ability to disable the 'off' mode, I can also see some usefulness in this i.e. in a separate profile for terminals, where I'm either working with full speech, or just want to have an indication that something is happening, and 'beeps' mode is ideal for such things. In short it should be possible to disable the 'off' mode.
- It is also very likely that for someone using primarily magnification the 'talk' mode is not necessary, and is as annoying when switching between modes as the 'on-demand' mode for people who have no use for it. That is why the 'talk' mode is on the list and can be unchecked.
- The current validation has been implemented like this to make sure user is never in a situation in which they are left without any speech. Thinking about use cases for having only 'beeps' and 'off' modes enabled the only one I could came up with is a Braille only user who works without any speech most of the time, but uses 'beeps' mode similarly to what has been described above, i.e. as a quick indication that something has happened in the console when they're not looking at their Braille display. It seems there are two choices here:
- We can start by not limiting the configurability i.e. allow to disable both 'talk' and 'on-demand' modes, and if there is a lot of complaints about this design implement something better according to user's feedback. or
- Start with the current design, and if someone reports that it is too limiting for their clear defined use case make this more configurable later.
I personally prefer the first option, as it is much easier to react to user feedback and make something simpler, than start with a limited config which is not working for some.
- The inconsistency argument is certainly valid. This can also be addressed in two ways:
- We can assume that configurations where 'talk' mode is disabled are pretty rare, done by mostly advanced users, and simply ignore the inconsistency
- We can do something similar to what has been implemented in JAWS, i.e. add additional control where it is possible to select in which speech mode NVDA should start. In that case I propose to modify the validation for the list of enabled speech modes, so that it it would be impossible to disable the speech mode defined as the one in which NVDA starts.
There was a problem hiding this comment.
I think I'd prefer the following based on your thoughts:
- Start with not limiting the configurability, though it makes sense to ensure that at least one option is enabled.
- Allow configuring the start mode
| - it is now possible to exclude unwanted speech modes when cycling between them using ``NVDA+s`` from the Speech category in NVDA's settings. (#15806, @lukaszgo1) | ||
| - If you are currently using the NoBeepsSpeechMode add-on consider uninstalling it ,and disabling "beeps" and "on-demand" modes in the settings. | ||
| - |
There was a problem hiding this comment.
I don't think mentioning the add-on is relevant in the core change log. We haven't done this historically when other add-ons have been integrated (e.g. add-on store, OCR).
I've also move the sentence around to be clearer.
| - it is now possible to exclude unwanted speech modes when cycling between them using ``NVDA+s`` from the Speech category in NVDA's settings. (#15806, @lukaszgo1) | |
| - If you are currently using the NoBeepsSpeechMode add-on consider uninstalling it ,and disabling "beeps" and "on-demand" modes in the settings. | |
| - | |
| - In the NVDA's settings Speech category, it is now possible to exclude unwanted speech modes when cycling between them using ``NVDA+s``. (#15806, @lukaszgo1) |
There was a problem hiding this comment.
Does the add-on cause issues with this new feature? We did mention removing the add-on for highlighting but that's because they conflicted with each other.
There was a problem hiding this comment.
Well, I just tried, and it apparently gives no errors, but hides completely the new functionality and, moreover, the new on-demand speech mode.
There was a problem hiding this comment.
Thanks for testing. That sounds like a bad conflict! We should definitely encourage uninstalling
There was a problem hiding this comment.
note that the add-on will be marked incompatible anyway, so I'm not sure whether it should be addressed. People should no longer update it.
There was a problem hiding this comment.
I mentioned add-on in the change log since:
- it is pretty popular
- it has even been mentioned in the NV Access training materials, and I'm not sure how easy is to update them (especially the audiobook and printed Braille copy), therefore mentioning that NoBeepsSpeechMode is unnecessary somewhere which is localized seemed a right thing to do
- With the newly added ability to override compatibility for add-ons the fact that add-on is no longer compatible is much less important. Most users would override compatibility if no updated version is available, so mentioning in the change log that there is a core function which should be used instead is the least we can do to discourage forcefully enabling the add-on
Personally I think we may consider going even a step further i.e. detect NoBeepsSpeechMode in the core, if its enabled let user know that it is no longer needed, offer to configure NVDA in the same way it behaved with the add-on and uninstall it. While that would be a new paradigm in NVDA I'd say it is worth considering.
CyrilleB79
left a comment
There was a problem hiding this comment.
Many thanks @lukaszgo1 for this work that was requested after my previous PR.
One thing that I have noted:
When NVDA starts or when the config is reset, speech mode turns back to Talk, no matter if Talk is in available in the cycle speech mode command; this was already the case in NVDA 2023.3. I think it's not a problem, and it can be a security so that a user does not end with NVDA starting only with mode off and beep available.
When switching profile however, the speech mode is not changed, what makes sense.
| description=_( | ||
| # Translators: Input help mode message for cycle speech mode command. | ||
| "Cycles between the speech modes of off, beep, talk and on-demand." | ||
| "Cycles between enabled speech modes." |
There was a problem hiding this comment.
"enabled" may let one think that a speech mode is active.
Using "available" is less confusing IMO.
| "Cycles between enabled speech modes." | |
| "Cycles between available speech modes." |
or just:
| "Cycles between enabled speech modes." | |
| "Cycles between speech modes." |
There was a problem hiding this comment.
Changed in 927918c. I went wit your second suggestion, since it is much simpler. My original idea was to use 'available and 'enabled' in a similar sense they're used for add-ons i.e. each mode in NVDA is available, but only these which are configured are enabled.
| _("Error"), | ||
| wx.OK | wx.ICON_ERROR, | ||
| self | ||
| ) |
There was a problem hiding this comment.
I think that this part of code should go away, so that not to create an unneeded constraint.
Anyway, if it is chosen to keep it, a return statement is missing:
| ) | |
| ) | |
| return False |
|
@ABuffEr it may be safer to release a final version of the add-on, compatible
with 2024.1 once beta is reached, that detects the version and pops a message
suggesting uninstalling. It could even disable all other functionality if 2024.1
is running.
|
|
@lukaszgo1 I haven't reviewed this yet, but a thought I had while reading
@seanbudd's and other comments:
Instead of limiting which options may be chosen, since that is seeming
unpopular, what about a warning pop-up when applying/okaying the config, if no
speech options are selected?
That is, something like:
You did not choose either Talk or On-Demand as one of your speech mode options.
Please note that this may result in no speech output at all. Are you sure you
want to continue? Yes No.
With a no selection returning to the config screen.
|
|
I have updated the initial description of this PR. In short currently the only validations for the checked modes ensures that there are at least two checked.
It is also worth discussing if NVDA should not detect that users had NoBeepsSpeechMode installed, and propose to uninstall it, perhaps configuring NVDA to mimic the add-on's behavior. |
|
I think the lowest impact and simplest UX is the warning dialog - we can always change the UX further in the future, |
|
I have mentioned things which were proposed during review in the known issues of the initial comment, so that they're not lost. Otherwise this is ready to be reviewed. |
XLTechie
left a comment
There was a problem hiding this comment.
Great work @lukaszgo1! Some suggestions below, mainly on comments and docs.
Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net> Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>
|
@CyrilleB79 @XLTechie Many thanks for your review! I believe all your comments are now addressed. |
Qchristensen
left a comment
There was a problem hiding this comment.
Changes look good and will be a welcome feature for users, great work everyone!
|
@CyrilleB79 While this has been merged, I wanted to address your outstanding comment in response to me. You disagreed with my suggestion to add the warning dialog note to the user guide. There are a few other cases of warning dialogs being spoken of in the user guide, such as in relation to incompatible add-ons, and the screen curtain, though this is by no means consistent. I do understand your point of view, and I have no strong opinion either way about whether to include the note about the warning. But I wished you to understand why I made the suggestion in the first place. |
Link to issue number:
Closes #15806
Summary of the issue:
NVDA allows to change the currently used speech mode by pressing
NVDA+s. By default this gesture cycles between 'talk', 'beeps', 'on-demand' and 'no speech'. For many users having ability to switch to beeps or on demand is unnecessary, and they have no need for modes different than speech and no speech. For other beeps mode is certainly useful, but it is much more important to be able to quickly switch between speech and no speech, so beeps mode just makes the goal slower to achieve. With the introduction of fourth speech mode in #15804 it has been decided that users should be given ability to disable speech modes they do not need to use.Description of user facing changes
The new checkable list containing all speech modes (all are initially checked)has been added into the Speech settings panel. When switching between modes with NVDA+s only modes which are checked in the list are included. When applying settings selected modes are validated, to make sure at least two modes are checked. When the 'talk' mode is disabled user is warned that they may be left without any speech.
Description of development approach
Configuration spec has been expanded to store list of disabled modes - this ensures that if any new mode is added to NVDA it is enabled by default in the cycling script. Speech settings panel now contains a list of all the speech modes, only enabled one are checked. When introducing validation for the speech settings panel it has been discovered that code responsible for checking panel's validity displays a warning about it being invalid, prevents saving invalid config, yet after dismissing the warning settings dialog gets closed. Since this is not optimal validation for settings dialog has been modified, so that after warning is dismissed settings remain open, and the invalid control is focused. The cycling script has been modified to only cycle between enabled modes. Unit tests verifying its behavior, both when no modes are disabled (the default) and some are disabled, were added. To make them work it has been necessary to perform a small modification to the method which lists configuration profiles in the
configmodule, it assumed that profiles directory always exists, which is not true in unit tests. Now when the directory is missing appropriate warning is logged, and empty list is returned. User guide has been expanded to describe newly added control.Testing strategy:
Known issues with pull request:
Code Review Checklist: