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

Say all, move caret per word instead of per line/sentence #9937

Closed
wants to merge 24 commits into from
Closed

Say all, move caret per word instead of per line/sentence #9937

wants to merge 24 commits into from

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jul 16, 2019

Link to issue number:

Fixes #9179
Fixes #3287

Summary of the issue:

When using say all, the caret moves per sentence or per line, not per word. This is not very helpful when using magnification or braille to track the current position of say all. More importantly though, when reviewing text with say all, exiting say all makes you end up at the start of the line or sentence, not at the current word's position.

Say all also doesn't follow in browse mode.

Description of how this pull request fixes the issue:

Find white space using regex, and bring the caret to it accordingly. This avoids fetching text info per word, which can be pretty expensive.

This option can be found in the speech settings: "Increase caret updates during say all"

Testing performed:

Tested in Word, Notepad and browse mode that the caret follows read words. Note that there is a minimal 0.5 seconds interval between every caret update to avoid the caret going mad on higher speechrates.

Known issues with pull request:

  1. With espeak, I've heard speech stuttering on higher rates with rate boost on. There may also be performance implications in browse mode. That's why this option is off by default.
  2. No changes to the documentation yet. I want to be sure about the option wording and its default state first.

Change log entry:

  • Changes
    • During say all, the caret position can now be updated more frequently to better resemble the current reading position. For this, enable the new "Increase caret updates during say all" option in NVDA's speech settings. #(9937)

@LeonarddeR LeonarddeR changed the title Say all, move caret per word instead per line/sentence Say all, move caret per word instead of per line/sentence Jul 16, 2019
@DrSooom
Copy link

DrSooom commented Jul 17, 2019

Wrong branche?

@LeonarddeR
Copy link
Collaborator Author

@DrSooom: Could you elaborate?

@DrSooom
Copy link

DrSooom commented Jul 17, 2019

"nvaccess:master" instead of "nvaccess:threshold". Compare with PR #9946.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 17, 2019 via email

@DrSooom
Copy link

DrSooom commented Jul 26, 2019

GitHub still shows 145 commits for this PR after Threshold was merged into Master. Keep an eye on it. (I thought that GitHub respectively git would be more intelligent.)

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 26, 2019 via email

@LeonarddeR
Copy link
Collaborator Author

Issue should now be fixed.

Copy link
Collaborator

@derekriemer derekriemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increase caret updates during say all sounds wordy to me. How about "Move cursor by word during say all"

@@ -37,6 +37,7 @@
outputDevice = string(default=default)
autoLanguageSwitching = boolean(default=true)
autoDialectSwitching = boolean(default=false)
increaseSayAllCaretUpdates = boolean(default=false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a speech synth dependent setting? For example, users may have many synths, and some might work better with this feature than others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derekriemer wrote:

Increase caret updates during say all sounds wordy to me. How about "Move cursor by word during say all"

Because while the caret is intended to move per word, the movement is also limited to once per half a second.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, sorry for the comment above, it ended up at the wrong place.

Anyway, making this a per synth setting makes sense to me. I will do that.

@@ -253,12 +260,27 @@ class CallbackCommand(BaseCallbackCommand):
otherwise it will block production of further speech and or other functionality in NVDA.
"""

def __init__(self, callback):
def __init__(self, callback: Union[SUPPORTED_CALLBACK_TYPES]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a class with a call supported by this union?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The only way to acoomplish this seems to be the use of collections.abc.Callable, but that also applies to classes itself. I'm reluctant to do this.

LeonarddeR and others added 2 commits August 5, 2019 13:24
Fixed typo

Co-Authored-By: Derek Riemer <[email protected]>
@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit 23b707621c

@derekriemer
Copy link
Collaborator

derekriemer commented Aug 10, 2019 via email

@derekriemer
Copy link
Collaborator

derekriemer commented Aug 10, 2019 via email

@LeonarddeR LeonarddeR marked this pull request as ready for review August 22, 2019 07:25
@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit ada9083620

@LeonarddeR
Copy link
Collaborator Author

I wrote this pull request on behalf of @BabbageCom. As I'm leaving @BabbageCom after the 29th of November, I can no longer afford maintaining this pr other than applying very basic review actions. If this pull request requires major changes, they will have to be applied by someone else, e.g. @sjfbol or whoever else is willing to take it.

@AppVeyorBot
Copy link

See test results for failed build of commit 20cf56ec97

@AppVeyorBot
Copy link

See test results for failed build of commit 4092c7d318

@feerrenrut
Copy link
Contributor

It would be worth considering how "vision enhancement providers" fit into this. It would be good if focus highlight followed the current word being spoken.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Apr 8, 2020 via email

@LeonarddeR
Copy link
Collaborator Author

This has merge conflicts. I'm currently in a situation where I neither have the time nor interest to fix them, it just requires too much attention and thorough testing. Apart from @BabbageCom, is there anyone who think this change would be a huge benefit and can take this over?

@LeonarddeR LeonarddeR added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Jul 16, 2020
@Adriani90
Copy link
Collaborator

cc: @Andre9642, @CyrilleB79, @JulienCochuyt Imo this would bring some benefits for braille users but also for people using focus highlighting.

@feerrenrut
Copy link
Contributor

This is certainly something I would be interested in working on. I have a vague plan to use a mechanism like this to improve the speech viewer. I don't think I can prioritize this immediately, so if anyone else has capacity and interest, feel free to take it on. But if not, we will get to this eventually. 😄

@dawidpieper
Copy link
Contributor

I can take it. What is the procedure? Should I create yet another PR?

@feerrenrut
Copy link
Contributor

I can take it. What is the procedure? Should I create yet another PR?

Yes, thanks @dawidpieper. Make a copy of this branch, and create a new PR from that branch, comment on this PR with the new PR number and I'll close this one. That will make it clear to everyone that you intend to take over the development.

@LeonarddeR
Copy link
Collaborator Author

Closing as superseded by #11658

@LeonarddeR LeonarddeR closed this Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. BabbageWork Pull requests filed on behalf of Babbage B.V. component/speech component/speech-synth-drivers component/vision Framework for assisting users with low vision feature/say-all feature
Projects
None yet
7 participants