Add new commands for synth ring by rmcpantoja · Pull Request #16095 · nvaccess/nvda

rmcpantoja · 2024-01-25T12:34:32Z

Link to issue number:

Fixes #13768
It's very related to this issue, since it adds these same suggested commands in it for easier navigation in certain cases.

Summary of the issue:

More efficient navigation in the synth settings ring by adding new commands for it.

Description of user facing changes

Added scripts:

Set the current value to the first one as unassigned gesture.
CTRL+shift+NVDA+page up (laptop) and CTRL+NVDA+page up (desktop) to jump forward the current value by 4x.
CTRL+shift+NVDA+page down (laptop) and CTRL+NVDA+page down (desktop) to jump backward the current value by 4x.
Set the current value to the last one as unassigned gesture.

Description of development approach

In the synth ring, the first and last functions, as increase or decrease by 4x defs as well, (which handles self.min and self.max) were added to both the numeric ring and the global ring, in their respective classes. I consider this approach to be better, because by jumping in this way, focusing on the minimum and maximum values, it can respect the settings of other synth drivers and thus not cause problems with them.

Testing strategy:

Set the synth ring to a specific setting. EX: speed.
press control+shift+NVDA+page up or page down (laptop) or control+NVDA+page up or page down (desktop) to jump forward or backward through the currently selected setting. It should increase the value from 20 by 20. Also, assigning a gesture to jump to the first and last value, for example in the rate option, it should set the rate to 100 or 0.

Known issues with pull request:

None

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.

…end.

AppVeyorBot · 2024-01-25T13:24:39Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8unkrrvy49k2nq8y/artifacts/output/nvda_snapshot_pr16095-30812,59ea297e.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 28.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 17.0,
FINISH_END 0.2

See test results for failed build of commit 59ea297e82

…into update_synth_ring

AppVeyorBot · 2024-01-25T15:16:30Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/5eupt552hpkynmgr/artifacts/output/nvda_snapshot_pr16095-30814,582669b4.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.2,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 29.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 17.1,
FINISH_END 0.2

See test results for failed build of commit 582669b4d5

cary-rowen · 2024-01-26T04:18:28Z

Are these gestures really necessary?

In the original feature request @TheQuinbox wrote:

It isn't necessarily a problem, but would definitely make work more efficient.

Can there be an example or two of this?

Some of my concerns:
If the user accidentally adjusts the volume to the maximum value, it may cause hearing discomfort.

If the user accidentally sets the speech rate to the maximum value, it may be difficult to restore without knowing these gestures.

We can hardly assume that the user knows nvda+ctrl+r.

There are no such worries in the voice settings dialog, because it is real-time interaction and contextual.

AppVeyorBot · 2024-01-26T04:42:20Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/q6oc4wv1rxstoldv/artifacts/output/nvda_snapshot_pr16095-30828,8a570b62.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 25.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.6,
FINISH_END 0.2

See test results for failed build of commit 8a570b62bf

rmcpantoja · 2024-01-26T05:40:30Z

Are these gestures really necessary?

In the original feature request @TheQuinbox wrote:

It isn't necessarily a problem, but would definitely make work more efficient.

Can there be an example or two of this?

Some of my concerns: If the user accidentally adjusts the volume to the maximum value, it may cause hearing discomfort.

If the user accidentally sets the speech rate to the maximum value, it may be difficult to restore without knowing these gestures.

We can hardly assume that the user knows nvda+ctrl+r.

There are no such worries in the voice settings dialog, because it is real-time interaction and contextual.

Hi,
Well, finally the purpose of this PR is the same as the linked issue, to make efficient browsing into synth ring.
As for your concerns, that's a good point. In mi opinion, the user might already have prior knowledge about these keyboard shortcuts. But if we're talking about something that happens accidentally (such as the similar command to know the status bar in terms of keystrokes), I think leaving the start/end gestures unassigned would be ideal. This being so, the user could be responsible for using that shortcut and avoiding the accident, but I would like to hear more opinions.

CyrilleB79 · 2024-01-26T08:04:58Z

IMO, these gestures are not very useful because no one use the min and max values of almost all parameters.

What would be much more useful would be to have long step commands, e.g. move 20 by 20, or determine the step according to the number of available values. Would be much more useful for numeric parameters, but also for long lists such as eSpeak Voices (languages).

Also, gestures allowing to keep the hand on the arrow pad would be better than the use of home/end. What about NVDA+control+windows+up/downArrow (desktop keyboard) and NVDA+shift+control+windows+up/downArrow (laptop)? These gestures may seem a bit complex but they can be reassigned for people having difficulties using gestures with many modifiers.

rmcpantoja · 2024-01-26T11:48:17Z

IMO, these gestures are not very useful because no one use the min and max values of almost all parameters.

What would be much more useful would be to have long step commands, e.g. move 20 by 20, or determine the step according to the number of available values. Would be much more useful for numeric parameters, but also for long lists such as eSpeak Voices (languages).

Also, gestures allowing to keep the hand on the arrow pad would be better than the use of home/end. What about NVDA+control+windows+up/downArrow (desktop keyboard) and NVDA+shift+control+windows+up/downArrow (laptop)? These gestures may seem a bit complex but they can be reassigned for people having difficulties using gestures with many modifiers.

Hi @CyrilleB79
Yeah I know that setting the first and last value may not be very popular and useful, but there will be people like me who find a use for it, E.G. quickly go to the first available voice; quickly set the speed to 0 (with rate boost on) useful for maintaining a natural reading profile when interpreting text, although it depends a lot on the synthesizer used and how this synthesizer uses the rate command, and although this may be possible with configuration profiles, some people do not handle them.
On the other hand, long step commands (20*20, 4x increment/decrement) are already implemented in this pull request, which uses these keyboard accompaniments, but page down and page up instead.
The suggested commands sound good! Thank you for pointing out this.

CyrilleB79 · 2024-01-26T14:25:22Z

Sorry, I had read the description too quickly.

I think that the commands to jump to first or last value should not be assigned any gesture because I think that only few people will use them.

Regarding the long jump, a step of 4x is nice for values which vary between 0 and 100. For long lists such as eSpeak voices, 4x = 4 items in a list of many items is a too short step: there are 133 voices for eSpeak and 105 variants. In this case, long steps should allow to go through the whole list pressing 5 to 8 times the long step gesture.

rmcpantoja · 2024-01-26T15:19:02Z

Sorry, I had read the description too quickly.

I think that the commands to jump to first or last value should not be assigned any gesture because I think that only few people will use them.

Regarding the long jump, a step of 4x is nice for values which vary between 0 and 100. For long lists such as eSpeak voices, 4x = 4 items in a list of many items is a too short step: there are 133 voices for eSpeak and 105 variants. In this case, long steps should allow to go through the whole list pressing 5 to 8 times the long step gesture.

Thanks again, good suggestion! In fact, I think it is the right thing to do to prevent certain incidents from occurring with a beginner user.
Jumping forward and backward: Now that I think about it, setting.largeStep hasn't been used much in NVDA that I know of, so I can take advantage to use it for these two scripts, just multiplied by two so it's still 4x magnification. The difference is that in the lists (voice, variant, etc) it should also make long jumps like the same behavior in the slider settings. I will apply these improvements soon! 👍🏻

AppVeyorBot · 2024-01-26T16:41:31Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/9ognqtgt5v4d8y4o/artifacts/output/nvda_snapshot_pr16095-30832,ed195b11.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 30.4,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 17.3,
FINISH_END 0.1

See test results for failed build of commit ed195b118d

AppVeyorBot · 2024-01-27T03:30:55Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/yt7cxn79d4wuxsoe/artifacts/output/nvda_snapshot_pr16095-30838,b71f7b64.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 24.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 10.3,
FINISH_END 0.1

See test results for failed build of commit b71f7b64b2

rmcpantoja · 2024-01-27T15:06:55Z

Hi,
I applied these suggestions according to your touts and points. Lint is also fixed.

The latest failure in system tests appears to be independent of these new features.

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

AppVeyorBot · 2024-02-02T13:18:56Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/lekoyw60m5alb9n5/artifacts/output/nvda_snapshot_pr16095-30929,c2459a08.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 27.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.2,
TEST_START 0.0,
TEST_END 0.6,
FINISH_END 0.2

See test results for failed build of commit c2459a0849

AppVeyorBot · 2024-02-02T16:54:56Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/72q3pgkmf02vksyn/artifacts/output/nvda_snapshot_pr16095-30931,d49a6f54.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 30.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 10.8,
FINISH_END 0.2

See test results for failed build of commit d49a6f5450

rmcpantoja · 2024-02-08T12:19:55Z

Hi,
This feature is ready again.

seanbudd

Thanks, this is almost ready

Improved type hints. Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd · 2024-02-09T01:16:52Z

 import queueHandler
 from autoSettingsUtils.driverSetting import BooleanDriverSetting, NumericDriverSetting

-


can you please restore this new line? otherwise this is ready to go

Done👍🏻

Qchristensen

User guide reads fine.

cary-rowen · 2024-02-12T02:18:58Z

Congratulations on this PR being merged, please update the description of the original PR to be consistent with the actual changes. This helps translators or people interested in this PR review it in the future.

CTRL+shift+NVDA+home (laptop) and CTRL+NVDA+home (desktop) to set the current value to the first one.
CTRL+shift+NVDA+end (laptop) and CTRL+NVDA+end (desktop) to set the current value to the last one.

These two commands are not actually assigned, but the functionality is there.

rmcpantoja · 2024-02-12T14:42:19Z

Congratulations on this PR being merged, please update the description of the original PR to be consistent with the actual changes. This helps translators or people interested in this PR review it in the future.

CTRL+shift+NVDA+home (laptop) and CTRL+NVDA+home (desktop) to set the current value to the first one.

CTRL+shift+NVDA+end (laptop) and CTRL+NVDA+end (desktop) to set the current value to the last one.

These two commands are not actually assigned, but the functionality is there.

Hi @cary-rowen,
Thank you to pointing out this.
I updated the PR description according to these last advances.

Fixes nvaccess#13768 It's very related to this issue, since it adds these same suggested commands in it for easier navigation in certain cases. Summary of the issue: More efficient navigation in the synth settings ring by adding new commands for it. Description of user facing changes Added scripts: CTRL+shift+NVDA+home (laptop) and CTRL+NVDA+home (desktop) to set the current value to the first one. CTRL+shift+NVDA+page up (laptop) and CTRL+NVDA+page up (desktop) to jump forward the current value by 4x. CTRL+shift+NVDA+page down (laptop) and CTRL+NVDA+page down (desktop) to jump backward the current value by 4x. CTRL+shift+NVDA+end (laptop) and CTRL+NVDA+end (desktop) to set the current value to the last one. Description of development approach In the synth ring, the first and last functions, as increase or decrease by 4x defs as well, (which handles self.min and self.max) were added to both the numeric ring and the global ring, in their respective classes. I consider this approach to be better, because by jumping in this way, focusing on the minimum and maximum values, it can respect the settings of other synth drivers and thus not cause problems with them.

fisher729 · 2024-06-19T08:11:10Z

Hi.

In the user guide, I notice it says:
Added commands to jump first, last, forward and backward through the synth settings ring.
Shouldn't it say:
Added commands to jump to the first and last items, as well as forward and backward, in the synth settings ring.
This makes more sense in English. Don't think it would impact translations.

rmcpantoja · 2024-06-19T14:05:02Z

Hi.

In the user guide, I notice it says: Added commands to jump first, last, forward and backward through the synth settings ring. Shouldn't it say: Added commands to jump to the first and last items, as well as forward and backward, in the synth settings ring. This makes more sense in English. Don't think it would impact translations.

Hi @fisher729.
Thanks for your suggestions! This helps me to improve my English writing.
You said that it couldn't impact translations from other languages, but this still worries me a little, especially since it would be a hassle. In any case, I don't know if the deadline for translations has already ended, since we are currently in version 2024.2 rc2. If so, it could be improved in the future. Anyway, I would like to hear thinks from the community.
Sorry for the inconvenience!
Cheers.

seanbudd · 2024-06-20T00:00:52Z

@fisher729 - this PR was closed 4 months ago. If you'd like to propose user guide changes for 2024.3 please open a new issue/PR

Qchristensen · 2024-06-25T06:14:38Z

Hi.

In the user guide, I notice it says: Added commands to jump first, last, forward and backward through the synth settings ring. Shouldn't it say: Added commands to jump to the first and last items, as well as forward and backward, in the synth settings ring. This makes more sense in English. Don't think it would impact translations.

Just to be clear, while @fisher729 is correct on the grammar of this sentence, this text was in the what's new / changes file, not the user guide. From memory, I saw it, but as my approval was the last step required to approve the PR, I didn't make an issue of fixing it in the changes file. If it had been in the user guide I would have flagged it.

For reference, in the User Guide, it states: "It is also possible to set the first or last value of the current synth setting by assign custom gestures in Input Gestures dialog, under the speech category. This means, for example, when you're on a rate setting, it will set the rate to 0 or 100. When you're on a voice setting, it will set the first or last voice." https://www.nvaccess.org/files/nvda/documentation/userGuide.html#TheSynthSettingsRing

So, @rmcpantoja always worth learning, especially on a language as complex as English can be - but no further changes required on this PR, great work! 😀👍

rmcpantoja and others added 2 commits January 25, 2024 06:53

Added cappability to set the current Synth Ryng value at the home or …

e64f98e

…end.

Merge branch 'nvaccess:master' into update_synth_ring

fa05ccd

rmcpantoja mentioned this pull request Jan 25, 2024

Add more settings ring commands. #13768

Closed

rmcpantoja added 3 commits January 25, 2024 08:42

Fix lint.

92551cf

Merge branch 'update_synth_ring' of https://github.com/rmcpantoja/nvda …

4288831

…into update_synth_ring

Added cappability to jump forward and bacuard the current value by 4x.

05e4557

Fix lind.

f94e86e

rmcpantoja changed the title ~~Add new comands for synth ryng~~ Add new commands for synth ring Jan 25, 2024

Add docs.

ab63606

rmcpantoja marked this pull request as ready for review January 26, 2024 03:59

rmcpantoja requested review from a team as code owners January 26, 2024 03:59

rmcpantoja requested review from Qchristensen and seanbudd January 26, 2024 03:59

rmcpantoja added 2 commits January 26, 2024 10:23

Adding some code and doc improvements according to the feedback.

548b0e2

Fix docs.

a71477f

Fixing self.setting.

358a305

Use largeStep for backward script.

cfaff0c

seanbudd reviewed Feb 2, 2024

View reviewed changes

seanbudd marked this pull request as draft February 2, 2024 05:45

rmcpantoja and others added 3 commits February 2, 2024 07:30

Apply suggestions from code review.

1ea2fc7

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Added more type hints.

71707f6

Final fixes: NO_SETTINGS_MSG to ui.message.

fa5bf1c

rmcpantoja added 2 commits February 2, 2024 10:05

Added missing import.

189dc45

Final fix: decreaseLarge and increaseLarge scripts.

f6d69eb

rmcpantoja marked this pull request as ready for review February 8, 2024 12:12

seanbudd reviewed Feb 9, 2024

View reviewed changes

Comment thread source/synthSettingsRing.py Outdated

Comment thread source/synthSettingsRing.py Outdated

Comment thread source/synthSettingsRing.py Outdated

seanbudd marked this pull request as draft February 9, 2024 00:00

rmcpantoja and others added 2 commits February 8, 2024 19:57

Code-quality improvements.

d028c99

Improved type hints. Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Code-quality improvements.

6def707

seanbudd reviewed Feb 9, 2024

View reviewed changes

Minor fix.

938193c

rmcpantoja marked this pull request as ready for review February 11, 2024 20:18

Qchristensen approved these changes Feb 11, 2024

View reviewed changes

seanbudd approved these changes Feb 11, 2024

View reviewed changes

seanbudd merged commit b03f7cb into nvaccess:master Feb 11, 2024

nvaccessAuto added this to the 2024.2 milestone Feb 11, 2024

		import queueHandler
		from autoSettingsUtils.driverSetting import BooleanDriverSetting, NumericDriverSetting

Uh oh!

Conversation

rmcpantoja commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jan 25, 2024

Uh oh!

AppVeyorBot commented Jan 25, 2024

Uh oh!

cary-rowen commented Jan 26, 2024

Uh oh!

AppVeyorBot commented Jan 26, 2024

Uh oh!

rmcpantoja commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrilleB79 commented Jan 26, 2024

Uh oh!

rmcpantoja commented Jan 26, 2024

Uh oh!

CyrilleB79 commented Jan 26, 2024

Uh oh!

rmcpantoja commented Jan 26, 2024

Uh oh!

AppVeyorBot commented Jan 26, 2024

Uh oh!

AppVeyorBot commented Jan 27, 2024

Uh oh!

rmcpantoja commented Jan 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Feb 2, 2024

Uh oh!

AppVeyorBot commented Feb 2, 2024

Uh oh!

rmcpantoja commented Feb 8, 2024

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

rmcpantoja Feb 11, 2024

Choose a reason for hiding this comment

Uh oh!

Qchristensen left a comment

Choose a reason for hiding this comment

Uh oh!

cary-rowen commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmcpantoja commented Feb 12, 2024

Uh oh!

fisher729 commented Jun 19, 2024

Uh oh!

rmcpantoja commented Jun 19, 2024

Uh oh!

seanbudd commented Jun 20, 2024

Uh oh!

Qchristensen commented Jun 25, 2024

Uh oh!

Reviewers

rmcpantoja commented Jan 25, 2024 •

edited

Loading

rmcpantoja commented Jan 26, 2024 •

edited

Loading

cary-rowen commented Feb 12, 2024 •

edited

Loading