Context help: support more controls and dialogs#11743
Conversation
- Added missing helpId in some dialogs/panels - Fixed some helpId - Added calls to self.bindHelpEvent - Fixed parameters (anchor) of some calls to self.bindHelpEvent
|
Cc @feerrenrut for general review, @Qchristensen for review of 3rd commit regarding user guide. |
See test results for failed build of commit fe7b24ce9a |
@CyrilleB79 I think it would be better to have a “No help available here” message for add-ons such as IBMTTS that don't have help context. |
- if the setting is a standard setting, go to the paragraph that describes this specific setting in User Guide - if the setting is not standard, i.e. specific to a third-party synth, go to speech settings in the User Guide
Thanks @OzancanKaratas for commenting. Opening the user guide at the beginning was actually not a good solution: it does not bring any additional information to the user.
For reviewers: |
|
Please do not review. |
|
With the User Guide, Is it worth changing the text from: "If checked this option allows you to significantly increase the rate of the current voice by multiplying the [regular rate value #SpeechSettingsRate] by a predetermined factor. To: It's more succinct and similar wording to other options. I thought if we can't specify the exact rate increase, it's probably best not to include something like "by a predetermined factor" which invites the question "what is that factor?". I must admit, when I made that video you referred to, I just worked out the speech rate with and without rate boost. Evidently in my test the increase was 300% but if that is not the same for all supported synthesizers, it's probably best, as you suggest, not to specify it at all. |
|
@Qchristensen you suggests the following rewriting in the UG:
What about shortening it still more to the shorter form as follows:
Not so much similar to "Variant" option (just above). If we would like to write something similar to the "Variant" option, we may write a longer form:
I fully agree with that in any case and I will not mention the term of factor or or multiplier. What do you prefer between shorter or longer form (or something else)? |
|
I like @Qchristensen's new wording much better than the original.
Indeed, if not supported by the synthesizer, this checkbox will be disabled.
The user still may wonder why he can't access that checkbox, and may waste time
looking for it, or trying to figure out why he can't use it. @Qchristensen's
wording avoids that.
|
See test results for failed build of commit 6004b35c7b |
|
I would tend to favour clear, concise wording. And as XLTechie noted, even though the option is disabled if not supported, if a user knows it exists, they may be confused why they can't find it. |
|
OK for the words "if supported..." Something still bothers me however: |
|
This PR is now ready for review. I have added context help for some more dialogs in a5860ce. This commit needs to be reviewed with the same care as the 2nd commit (ebcf8a1). The following dialogs remain not overloaded with context help:
Magically, message boxes opened from a dialog such as add-on info opened from the add-on manager support context help of the parent dialog. However, I do not understand exactly why... Any clarification is welcome. |
|
Something still bothers me however:
"When enabled, the speech rate is significantly increased, if supported by the current synthesizer. This option is disabled by default."
Grammatically this let think that the speech rate is enabled (or not).
I don't think most native English speakers would have that misunderstanding, but
I can see it as a far out possibility.
I would prefer something clearer:
"When enabled, this option significantly increases the speech rate, if supported by the current synthesizer.
This option is disabled by default."
I don't object to that, although it is getting long again.
Do we need the note about the default?
Perhaps:
"Enabling this option will significantly increase the speech rate, if supported by the current synthesizer."
|
@XLTechie, thanks for commenting agin. |
|
I also had copied the style of another option. I am happy either way, and certainly a small change which makes it clearer for non-native English speakers is a useful change, I am happy to take suggestions from translators on that. |
|
@feerrenrut do you plan to integrate this PR in 2020.4? This would make sense since #11456 is also part of 2020.4. |
|
@CyrilleB79 wrote:
While I don't have a solution for this, ideally we should redesign the way in which context help is presented so that we can check if there is a section in the documentation or not for a particular control. |
I agree with you that this way to do is not ideal. But I do not know any other way to do. As soon as you have an better idea to manage context help, please open a new issue with detailed description and links to this PR and to #11456, so that it is more visible. |
feerrenrut
left a comment
There was a problem hiding this comment.
Overall, this looks good. There are a lot of changes here, can you confirm you have tested help works for all of these now?
Please don't add more to this PR, lets get it finalized and merged, and add any further missed items in subsequent PR's.
Co-authored-by: Reef Turner <feerrenrut@users.noreply.github.com>
|
Thanks @feerrenrut for this review. I confirm that during the development, I have carefully tested all the controls of the modified dialogs or panels. The 2 last commits were just a merge commit to remove a conflict with master and the commit after review comments. I will not add anymore commit (except possible review comment addressing if required). |
Qchristensen
left a comment
There was a problem hiding this comment.
Reads well: concise and clear. Good work!
Link to issue number:
Fixes #7757
Follow-up of #11456
Summary of the issue:
#11456 provided a framework to context help in NVDA dialogs and added context help in the majority of NVDA dialogs and controls. However, this work being very teddious, some issue remained:
Description of how this pull request fixes the issue:
I have separated the fixes in 3 commits to ease the review:
1st commit (c940083)
Speech, Braille and Vision setting all use subpanels inheriting from AutoSettingsMixin. It was taken into account however that all these subpanels do not have the same level of details in the user guide and thus have been treated diferently:
2nd commit (ebcf8a1)
Make some more dialogs or frames inherit from gui.ContextHelpMixin.
Note:
I have made the modifications as per #11456. But I do not know in detail the use of DpiScalingHelperMixinWithoutInit and DpiScalingHelperMixIn. Thus this part of code (class inheritance and init function) should be carefully checked/reviewed.
3rd commit (dd72d09)
Added a paragraph for rate boost in the user guide.
Note: this video indicates at 1:33 that rate boost with Windows OneCore voice triples the speech rate. Also this page indicates that rate boost triples the regular reading rate of NVDA. However, looking at the code, it seems that only eSpeak uses the 3 factor; OneCore using 5.5 factor. I have also checked this by doing some tests (rate 100%, RB off vs rate 18% RB on). Thus, I did not specify the 3 factor in the user guide.
Testing performed:
Tested that pressing F1 in all dialogs and controls opens the User Guide at the expected paragraph. For some dialogs or controls however, these tests were made during development and not with the final commit of this PR. The tests were made sometimes with NVDA in French (and thus also the User Guide) and sometimes with NVDA in English.
Note that sometimes, the browser opens the UG at the correct position (visually), but NVDA reads from beginning of the UG, i.e. ignoring the anchor. I think this is already known and was commented somewhere (maybe in #11456); anyway, it's a browser issue and pressing F5 may allow to jump to the correct anchor. In any case, the URL was correct.
Known issues with pull request:
This PR does not provide a support for:
Change log entry:
None if this PR is merged in 2020.4 since #11456 belong also to 2020.4