Skip to content

Complete support of bk:space and bk:space+dots gestures for Seika Notetaker#16828

Merged
seanbudd merged 7 commits into
nvaccess:masterfrom
school510587:fix_issue16642
Jul 15, 2024
Merged

Complete support of bk:space and bk:space+dots gestures for Seika Notetaker#16828
seanbudd merged 7 commits into
nvaccess:masterfrom
school510587:fix_issue16642

Conversation

@school510587

@school510587 school510587 commented Jul 6, 2024

Copy link
Copy Markdown
Contributor

Link to issue number:

Closes #16642.

Summary of the issue:

Two drivers are mentioned in the issue, eurobraille and seikantk. The user can change an option of the former to solve the issue. However, there is no feasible approach for the user to solve the issue for the latter manually, which is the aim of the PR.

Description of user facing changes

The user can perceive the changes by three ways.

  1. Observe the input help. On pressing dots and/or space/backspace, names of braille input gestures are reported correctly.
  2. Run braille input translation. The result must be as expected, while the old seikantk driver behaves incorrectly.
  3. Use add-ons relying on braille input. In fact, this is the main demand that users reqest for the modification. Correct braille input leads to correct result.

Description of development approach

4 cases of input gestures are explicitly specified in seikantk.py, and the algorithm description is available in the commit log.

Testing strategy:

The most simple way to test is to run input help. Be sure that your working braille display is Seika Notetaker.

Case 1:

  1. Press any dot along with either space or backspace on the braille display.
  2. Expect "space with dot XXX" to be announced.

Case 2:

  1. Press either space or backspace on the braille display.
  2. Expect "Inputs braille dots via the braille keyboard" to be announced.

It is OK to test the change through the input translation table.

  1. Set the input translation table to Unicode.
  2. Press either space or backspace on the braille display.
  3. A braille space is generated, rather than an ordinary space character.

Known issues with pull request:

There are two notable issues currently.

  1. Entries in gestureMap for br(seikantk):d7 and br(seikantk):d8 may be redundant.
  2. With InputGestureRouting, the routing parameter of InputGesture.init seens useless.
  3. Incorrect internal br(seikantk) ID for some bk:space+dots gestures.

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
    • Enhanced input handling in braille displays by combining keys and dots into a single gesture, improving the user experience.
  • Bug Fixes
    • Corrected the input gesture logic to ensure space key mappings are no longer included erroneously, preventing conflicts.
  • Improvements
    • Updated default settings for input gestures, ensuring smoother operation and better default handling for users.

@school510587 school510587 requested a review from a team as a code owner July 6, 2024 16:22
@coderabbitai

coderabbitai Bot commented Jul 6, 2024

Copy link
Copy Markdown
Contributor

Walkthrough

The seikantk.py module has been updated to handle InputGesture objects more effectively. The _handleKeys method now appends gestures combining keys and dots, addressing a Braille display input issue. The _handleKeysRouting method has been streamlined by removing space key mappings. Additionally, the InputGesture constructor has been refined to set default values and streamline logic for handling dots and space based on keys and routing.

Changes

File Change Summary
source/brailleDisplayDrivers/seikantk.py Modified _handleKeys to append combined keys and dots, adjusted _handleKeysRouting by removing space key mappings, updated InputGesture constructor for defaults and new logic

Assessment against linked issues

Objective Addressed Explanation
Combine keys and dots in _handleKeys (16642)
Remove "kb:space" mappings from _handleKeysRouting (16642)
Update InputGesture constructor for default values and new logic (16642)

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 Configration 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.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 90e03e4c35

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit cad52d5ce2

@school510587

Copy link
Copy Markdown
Contributor Author

Hi @md_curran,

Sorry, I am not familiar to the lint check. I think I got nothing from the AppVeyorBot report. If I must do anymore before merge, please tell me. The PR is important for many Taiwanese users, because it determines whether BrlIMEHelper works for Seika Notetaker.

BrlIMEHelper is an add-on converting braille input into IME emulation, which can be thought of as another implementation of braille input handler. Seika Notetaker, called BPDA (亮點) by Taiwanese, is a majority of braille displays that students can borrow from the government organization. Thus, students with Seika Notetakers are eager for fix of the driver to facilitate their learning.

Thanks.

@hwf1324

hwf1324 commented Jul 10, 2024

Copy link
Copy Markdown
Contributor

Hi @md_curran,

Sorry, I am not familiar to the lint check. I think I got nothing from the AppVeyorBot report. If I must do anymore before merge, please tell me. The PR is important for many Taiwanese users, because it determines whether BrlIMEHelper works for Seika Notetaker.

BrlIMEHelper is an add-on converting braille input into IME emulation, which can be thought of as another implementation of braille input handler. Seika Notetaker, called BPDA (亮點) by Taiwanese, is a majority of braille displays that students can borrow from the government organization. Thus, students with Seika Notetakers are eager for fix of the driver to facilitate their learning.

Thanks.

See: https://ci.appveyor.com/project/NVAccess/nvda/builds/50160022#L5371

@seanbudd

Copy link
Copy Markdown
Member

I would encourage running runlint.bat before pushing any changes

if brailleDots:
  if key in (1, 2, 3):
    1. bk:space+dots. key is cleared.
  else:
    2. bk:dots. key may be not 0.
if key:
  if key in (1, 2):
    3. bk:space. If bk:space+dots is triggered, the case must not be true.
  else:
    4. br(seikantk):XXX, otherwise.
@seanbudd

Copy link
Copy Markdown
Member

Can you please add a changelog entry detailing this bug fix?

@school510587

Copy link
Copy Markdown
Contributor Author

Hi,

I have uploaded the new changelog entry. However, the md file now uses different symbol * from the changelog entry specification -. I think it would be better to update the documentation.

Comment thread user_docs/en/changes.md Outdated
### Bug Fixes

* NVDA once again relies on UIA events for caret movement in XAML and WPF text controls, rather than only on manual querying of the caret position. (#16817, @LeonarddeR)
* The Seika Notetaker driver now correctly generates braille input for braille space and space with dots gestures. (#16642, @school510587)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this correct? I think there's a typo

Suggested change
* The Seika Notetaker driver now correctly generates braille input for braille space and space with dots gestures. (#16642, @school510587)
* The Seika Notetaker driver now correctly generates braille input for space and backspace with dots gestures. (#16642, @school510587)

@seanbudd

Copy link
Copy Markdown
Member

@school510587 good catch with the markdown issue. Do you mind updating that file in a separate PR?

Comment thread user_docs/en/changes.md Outdated
@school510587

Copy link
Copy Markdown
Contributor Author

Hi @seanbudd,

I think I should clearly specify which gestures are influenced, and there are actually 5: space, backspace, space with dots, backspace with dots, and space with backspace with dots. So I use "space, backspace and space/backspace with dots". Please tell me if there is any better representation, thanks.

@seanbudd seanbudd merged commit 3aafc82 into nvaccess:master Jul 15, 2024
@school510587

Copy link
Copy Markdown
Contributor Author

@school510587 good catch with the markdown issue. Do you mind updating that file in a separate PR?

OK. Please see #16860.

seanbudd pushed a commit that referenced this pull request Sep 19, 2024
Fixes issue 3 of #16828 .

Summary of the issue:
The br(seikantk):XXX IDs of bk:space and bk:space+dots are incorrect. Although it has few influence to UX, it may bring trouble to add-on developers.

Description of user facing changes
There must be one option displayed when adding a new gesture in Input gestures dialog is about Seika Notetaker. Without this PR, the option is always backspace+dX, e.g. backspace+d1+d3+d4+d5. Now, backspace+d1+d3+d4+d5, d1+d3+d4+d5+space, and backspace+d1+d3+d4+d5+space are displayed correctly according to the actual space key(s) pressed by the user.

Description of development approach
Type of space parameter of class InputGesture becomes int. It may be 1(backspace), 2(space), or 3(both). The correct key name is obtained from _getKeyNames.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The input translation table option doesn't work for some braille displays

4 participants