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

Add ability to speak character when cursor routing #16947

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Aug 2, 2024

Link to issue number:

Closes #8072

Summary of the issue:

When cursor routing, it can sometimes be handy if speech follows along and tells you what character you moved to, especially when the character in braille is unknown to you.

Description of user facing changes

  • Added a new braille option "Speak character when routing cursor in text", disabled by default. When enabled, NVDA will speak the character you routed to after a cursor routing action.

Description of development approach

Added a new option to the config spec, as well as a function to braille to speak the character at the given position. Also added an item to the settings panel as well as an unbound global command.

Testing strategy:

  • Tested that NVDA will speak the character ath the cursor when routing when the option is on
  • Tested that NVDA won't speak the character ath the cursor when routing when the option is off
  • Tested that the global command (when bound) appropriately toggles the option

Known issues with pull request:

None known

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.

Summary by CodeRabbit

  • New Features

    • Introduced a setting to automatically speak the character at the cursor during braille cursor routing, enhancing auditory feedback for improved accessibility.
    • Added a checkbox in settings for users to enable or disable the new speech feature.
    • Implemented a toggle command for the speech feature in braille mode.
  • Documentation

    • Updated user guide to include instructions for the new braille speech feature and customization options.
    • Added notes in the change log regarding default settings and operational benefits for users.
  • Bug Fixes

    • Enhanced configuration management for better user interaction with the new settings.

@LeonarddeR LeonarddeR requested review from a team as code owners August 2, 2024 17:41
Copy link
Contributor

coderabbitai bot commented Aug 2, 2024

Walkthrough

The changes enhance NVDA's braille functionality by introducing a feature that allows the application to announce the character at the cursor when routing with a braille display. This feature improves accessibility for users who rely on auditory feedback while navigating text. It includes a new configuration option, user interface adjustments, and support for toggling the feature through settings.

Changes

Files Change Summary
source/braille.py Added _speakOnRouting function to announce cursor characters; modified _routeToTextInfo to integrate speech functionality.
source/config/configSpec.py Introduced speakOnRouting boolean configuration option, defaulting to false, enhancing routing accessibility features.
source/globalCommands.py Added script_braille_toggleSpeakOnRouting method to toggle speech output for cursor routing characters, improving user interaction in braille mode.
source/gui/settingsDialogs.py Updated makeSettings to include a checkbox for the new speech feature and modified onSave to handle user preferences for auditory feedback during routing.
user_docs/en/changes.md Documented the new feature, emphasizing its auditory feedback capabilities for characters under the cursor when using braille displays.
user_docs/en/userGuide.md Added a section explaining how to enable the new speech feature for braille cursor routing, including customization options for users.

Assessment against linked issues

Objective Addressed Explanation
NVDA should report the letter by speech when moving the cursor to a letter (#[8072])
Users should be able to enable/disable the speech feature through settings (#[8072])
Speech output should provide auditory feedback corresponding to the cursor position (#[8072])

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (3)
source/braille.py (1)

3685-3689: Add docstring details.

The docstring should include more details about the function's behavior and the expected type for the info parameter.

"""
Speaks the character at the cursor position after routing.

This function checks if the `speakOnRouting` configuration option is enabled.
If enabled, it will spell out the character at the cursor position using speech.

:param info: The TextInfo object at the cursor position after routing.
:type info: textInfos.TextInfo
"""
source/gui/settingsDialogs.py (1)

4525-4525: Ensure the new configuration option is documented.

The new configuration option speakOnRouting is correctly integrated but is not documented in any markdown, reStructuredText, or plain text files. Please ensure that this new option is documented in the relevant documentation files.

  • source/config/configSpec.py
  • source/braille.py
  • source/globalCommands.py
  • source/gui/settingsDialogs.py
Analysis chain

Ensure the new configuration option is documented.

The new configuration option speakOnRouting is correctly integrated. Ensure that this new option is documented in the relevant documentation files.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new configuration option `speakOnRouting` is documented.

# Test: Search for the documentation of the new configuration option. Expect: Occurrences in documentation files.
rg --type markdown --type rst 'speakOnRouting'

Length of output: 46


Script:

#!/bin/bash
# Description: Verify that the new configuration option `speakOnRouting` is documented.

# Test: Search for the documentation of the new configuration option in plain text files.
rg --type txt 'speakOnRouting'

# Test: Search for the documentation of the new configuration option in other potential formats.
rg 'speakOnRouting'

Length of output: 1031

user_docs/en/changes.md (1)

18-18: Fix typographical error.

There is a typographical error: "ath" should be "at".

* When performing a braille cursor routing action, NVDA can now automatically speak the character at the cursor. (#8072, @LeonarddeR)

source/globalCommands.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
@XLTechie
Copy link
Collaborator

XLTechie commented Aug 3, 2024 via email

@LeonarddeR
Copy link
Collaborator Author

@XLTechie
wrote:

Given how many options for controlling small aspects there are (ever growing),

This is certainly something to consider.

wouldn't it make more sense to turn this on by default, let the new feature be exposed, and let those who don't like it figure out how to turn it off, instead of the other way around?

No feedback on cursor routing has been the default since the start of NVDA and none has ever bothered to implement this feature until now. Personally, I would only enable the feature if, for example, I was working with foreign language braille tables. So to keep it short, I'm really reluctant to enable this by default.
What do you think @Adriani90?

@seanbudd seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Aug 5, 2024
@LeonarddeR
Copy link
Collaborator Author

Given ... how few users go through after every update and look at new ones, or read through the changes: ...

Thinking more about this, may be after every update, we should consider having a message that reminds one to open the what's new after an update?

@Qchristensen
Copy link
Member

The how we should get new features in front of users is a very valuable discussion - although out of scope for this PR - so I created issue #16961 for it - @XLTechie and @LeonarddeR I copied your comments over and tagged you both in that issue.

@burmancomp
Copy link
Contributor

Not directly to this pr but somehat similar: I think it would be useful if when braille is tethered to focus, it would be possible that when caret moves from line to line (when braille up/down/forward/backward buttons are pressed), there would be option that content of new line would be spoken. This would be useful when using both braille and speech simultaneously.

@LeonarddeR
Copy link
Collaborator Author

@burmancomp could you please open a new issue for this? I think it could be an useful feature indeed, though much harder to implement.

@seanbudd seanbudd added conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. and removed blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. labels Aug 6, 2024
source/globalCommands.py Outdated Show resolved Hide resolved
source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
source/globalCommands.py Outdated Show resolved Hide resolved
source/globalCommands.py Outdated Show resolved Hide resolved
@LeonarddeR
Copy link
Collaborator Author

@Qchristensen Turns out there's some debate about wording. Could you chime in? Basically:

  1. Toggle on/off
  2. Turn on/off
  3. Enable/disable

@seanbudd
Copy link
Member

seanbudd commented Aug 7, 2024

I think enabled/disabled is generally what we use, I just couldn't think of it at the time

@XLTechie
Copy link
Collaborator

XLTechie commented Aug 7, 2024 via email

Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

User Guide reads well, just made one small fix in the changes text

user_docs/en/changes.md Outdated Show resolved Hide resolved
Co-authored-by: Quentin Christensen <quentin@nvaccess.org>
@seanbudd seanbudd merged commit 46e48b9 into nvaccess:master Aug 8, 2024
2 checks passed
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
5 participants