Correct internal gesture ID for Seika Notetaker by school510587 · Pull Request #17047 · nvaccess/nvda

school510587 · 2024-08-23T19:29:18Z

Link to issue number:

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.

Testing strategy:

Open Input gestures dialog.
Select an item and press Add.
Press (the right) space with dots 1, 3, 4, and 5 on Seika Notetaker.
d1+d3+d4+d5+space (Seika Notetaker) should be present.

If both spaces are pressed, the result should be backspace+d1+d3+d4+d5+space.

Known issues with pull request:

3 entries will be required to build a new item in gesture map. For example, br(seikantk):BACKSPACE+d1+d3+d4+d5, br(seikantk):d1+d3+d4+d5+SPACE, and br(seikantk):BACKSPACE+d1+d3+d4+d5+SPACE must be present to completely define the action for space with dots 1, 3, 4, and 5.

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 gesture handling logic to improve user interaction with the input system.
- Updated space parameter representation to reflect key values, improving clarity and functionality.
Bug Fixes
- Ensured consistency in the interpretation of the space parameter across different methods.

coderabbitai · 2024-08-23T19:29:24Z

Walkthrough

The changes involve updates to the _handleKeys method and the constructor of the InputGesture class within the seikantk.py file. The handling of the space parameter has been modified to reflect the key value rather than a boolean state, enhancing the clarity of its usage. Additionally, the default value of the space parameter in the constructor has been changed from False to 0, ensuring consistency in its interpretation across the code.

Changes

File	Change Summary
`source/brailleDisplayDrivers/seikantk.py`	Modified `_handleKeys` method to set `space` as the key value instead of a boolean; changed `InputGesture` constructor default value of `space` from `False` to `0`.

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>, please review it.
-- 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 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 using 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.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

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.

SaschaCowley

This seems to be fine to me, though I don't have a Seika Notetaker to test with. I just have a question about the internal determination of the space key which I would like clarification on (I understand you haven't changed the lines of code I'm asking about, but I reckon we can clarify this small matter while we're here). Also, could you please write a changelog entry for this fix?

SaschaCowley · 2024-08-26T05:41:59Z

@school510587 have you thoroughly tested this yourself?

school510587 · 2024-08-26T07:14:29Z

Hi @SaschaCowley,

Why are we using (1, 2, 3) for space at line 310, but (1, 2) for space at line 316?

(1, 2, 3) in line 310 is quite easy to understand. It represents that at least one of BACKSPACE and SPACE are pressed.

BACKSPACE + SPACE, i.e. value 3 of line 316, is defined as sayAll, rather than ordinary space. So, brailleDots = 0 and key = 3 fails both tests in line 309 and 316, and finally forms br(seikantk):BACKSPACE+SPACE at line 319. It is not a braille input, because both dots and space properties are not set.

school510587 · 2024-08-26T08:01:16Z

Hi @SaschaCowley,

@school510587 have you thoroughly tested this yourself?

Yes, here is a detailed result.

Key	Input Help	Input Gesture options
Both BACKSPACE and SPACE	SPACE+BACKSPACE	backspace+space (Seika Notetaker)
Single BACKSPACE	space	backspace (Seika Notetaker), any dots
Single SPACE	space	space (Seika Notetaker), any dots
BACKSPACE, SPACE and dot 1	space with dot 1	backspace+d1+space (Seika Notetaker), space with dot 1, space with any dots
Both BACKSPACE and dot 1	space with dot 1	backspace+d1 (Seika Notetaker), space with dot 1, space with any dots
Both SPACE and dot 1	space with dot 1	d1+space (Seika Notetaker), space with dot 1, space with any dots
Single dot 1	dot 1	d1 (Seika Notetaker), any dots

A problem appears above. Key name in the input help is not precise enough, but the following description of its function is correct. That is, if different functions are bound to BACKSPACE and SPACE respectively, they share the same key name space with different descriptions in input help.

Another notable phenomenon is that the key name of BACKSPACE + SPACE is precise. The difference is that it is not a braille input. So far, I don't know why the key name of braille input gesture is not precise enough.

seanbudd · 2024-08-30T05:59:02Z

cc @moyanming - any thoughts on this PR and the issues shown in input help?

seanbudd

Approved. If we get further feedback from @moyanming or other users/testers/devs we can address issues.
Please make sure this gets entries in the change log under bug fixes and API breaking changes

SaschaCowley

@school510587, please add changes entries.

school510587 · 2024-09-09T18:00:56Z

Hi,

Sorry, which is correct?

* Correct internal gesture ID for Seika Notetaker. (#16828, @school510587)

or

* Correct internal gesture ID for Seika Notetaker. (#17047, @school510587)

The PR 17047 attempts to solve the known issue 3 of 16828. Both are not issue number, and which should I use? Thanks.

seanbudd · 2024-09-09T23:44:36Z

Either the issue number or this PR number is fine

AppVeyorBot · 2024-09-11T08:45:36Z

PASS: Translation comments check.
PASS: License check.
PASS: Unit tests.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/c376gia9dnrr09ni/artifacts/output/nvda_snapshot_pr17047-33962,db460f03.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.2,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 15.7,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 18.8,
FINISH_END 0.2

See test results for failed build of commit db460f0346

school510587 · 2024-09-11T11:52:12Z

Hi,

What's up? The merge seems to be failed, but all tests passed. What can I do to help this?

SaschaCowley · 2024-09-12T01:23:19Z

@school510587 looks like it was an issue on AppVayer's side.

…ikantk_internal_gesture_name

school510587 · 2024-09-16T11:18:55Z

Hi,

However, I have another problem. My previous PR 16828 was accepted in NVDA 2024.4, but the IDs of single space and space with dots are not correct. They are corrected by this PR now, and where should I denote this change? Whether should I need to write this into changes.md?

SaschaCowley · 2024-09-17T03:38:49Z

@school510587 said:

However, I have another problem. My previous PR 16828 was accepted in NVDA 2024.4, but the IDs of single space and space with dots are not correct. They are corrected by this PR now, and where should I denote this change? Whether should I need to write this into changes.md?

I am not sure I understand what you are asking. PR #16828 works as expected, doesn't it? The only user facing change here is that the space key in use is displayed correctly, but NVDA's actual behaviour is unchanged.

school510587 · 2024-09-17T11:55:06Z

There are several IDs for a braille display gesture. For example, bk:space+dot1, bk:space+dots, and br(seikantk):SPACE+d1 refer to the same gesture.
#16828 fixes the behavior in bk level, i.e. bk:space or bk:space+dots. However, the behavior in br(seikantk) level is incorrect.

PR #16828 works as expected, doesn't it?

It does achieve my first goal - to generate the corresponding bk IDs, and the default braille input behaviors are also bound to them. The displayed name in input help changes from SPACE+d1 to space with dot 1. However, a user will find that br(seikantk) are wrong, i.e. br(seikantk):BACKSPACE+d1 on input gestures dialog.

The only user facing change here is that the space key in use is displayed correctly, but NVDA's actual behaviour is unchanged.

In this PR, information of the pressed space key(s) is passed into InputGesture class. Therefore, br(seikantk) IDs involving BACKSPACE and SPACE are fixed, and actions bound into br(seikantk) IDs are also mapped correctly. However, I surprisingly find no change in the input help hints of gestures that map to the actions by their br(seikantk) IDs.

With the initial NVDA settings, the behavior is unchanged from #16828 to this PR. A user must try to alter some setting through input gestures dialog to perceive what described above

SaschaCowley · 2024-09-18T00:16:02Z

Nevertheless, this is an API breaking change, so has to be in a 20xy.1 release. And if I am understanding correctly, your previous PR works as expected and does not introduce any regressions, so there is no need to change anything there.

In future, if you want changes to be introduced into the same NVDA version, please either note this explicitly in all PRs, or open them as a single PR.

school510587 · 2024-09-18T13:59:00Z

Nevertheless, this is an API breaking change, so has to be in a 20xy.1 release. And if I am understanding correctly, your previous PR works as expected and does not introduce any regressions, so there is no need to change anything there.

Ok, I think this is correct.

In future, if you want changes to be introduced into the same NVDA version, please either note this explicitly in all PRs, or open them as a single PR.

Got it, thanks.

# Conflicts: # user_docs/en/changes.md

Correct internal gesture ID for Seika Notetaker

d312b47

school510587 requested a review from a team as a code owner August 23, 2024 19:29

school510587 requested a review from SaschaCowley August 23, 2024 19:29

SaschaCowley requested changes Aug 26, 2024

View reviewed changes

Comment thread source/brailleDisplayDrivers/seikantk.py

SaschaCowley added component/braille-display-drivers api-breaking-change labels Aug 26, 2024

SaschaCowley added this to the 2025.1 milestone Aug 26, 2024

seanbudd self-requested a review August 27, 2024 00:09

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Aug 27, 2024

seanbudd added the blocked/needs-info The issue can not be progressed until more information is provided. label Aug 30, 2024

seanbudd approved these changes Sep 9, 2024

View reviewed changes

SaschaCowley approved these changes Sep 9, 2024

View reviewed changes

SaschaCowley reviewed Sep 9, 2024

View reviewed changes

SaschaCowley marked this pull request as draft September 10, 2024 02:11

SaschaCowley requested changes Sep 11, 2024

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Comment thread user_docs/en/changes.md

school510587 force-pushed the fix_seikantk_internal_gesture_name branch from c487c07 to 50ca6e4 Compare September 11, 2024 07:30

SaschaCowley requested changes Sep 12, 2024

View reviewed changes

Comment thread user_docs/en/changes.md

Merge branch 'master' of https://github.com/nvaccess/nvda into fix_se…

df5cae0

…ikantk_internal_gesture_name

school510587 force-pushed the fix_seikantk_internal_gesture_name branch from 50ca6e4 to 27164c5 Compare September 12, 2024 16:11

seanbudd reviewed Sep 15, 2024

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

Add entries to describe the above merge

bd338c2

school510587 force-pushed the fix_seikantk_internal_gesture_name branch from 27164c5 to bd338c2 Compare September 16, 2024 11:05

seanbudd approved these changes Sep 19, 2024

View reviewed changes

seanbudd marked this pull request as ready for review September 19, 2024 00:38

seanbudd requested a review from SaschaCowley September 19, 2024 00:38

SaschaCowley reviewed Sep 19, 2024

View reviewed changes

Comment thread user_docs/en/changes.md Outdated

SaschaCowley added 2 commits September 19, 2024 11:20

Improve clarity of user changes entry.

69134a2

Merge branch 'master' into fix_seikantk_internal_gesture_name

d05b5da

# Conflicts: # user_docs/en/changes.md

SaschaCowley approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'master' into fix_seikantk_internal_gesture_name

5a433b9

seanbudd merged commit 23caa79 into nvaccess:master Sep 19, 2024

nishimotz mentioned this pull request Apr 14, 2025

2025.1 本家の翻訳 nvdajp/nvdajp#498

Closed

Uh oh!

Conversation

school510587 commented Aug 23, 2024 • edited by coderabbitai Bot 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:

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SaschaCowley commented Aug 26, 2024

Uh oh!

school510587 commented Aug 26, 2024

Uh oh!

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

Uh oh!

Uh oh!

seanbudd commented Aug 30, 2024

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

SaschaCowley left a comment

Choose a reason for hiding this comment

Uh oh!

school510587 commented Sep 9, 2024

Uh oh!

seanbudd commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Sep 11, 2024

Uh oh!

school510587 commented Sep 11, 2024

Uh oh!

SaschaCowley commented Sep 12, 2024

Uh oh!

Uh oh!

Uh oh!

school510587 commented Sep 16, 2024

Uh oh!

SaschaCowley commented Sep 17, 2024

Uh oh!

school510587 commented Sep 17, 2024

Uh oh!

SaschaCowley commented Sep 18, 2024

Uh oh!

school510587 commented Sep 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

school510587 commented Aug 23, 2024 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 23, 2024 •

edited

Loading

school510587 commented Aug 26, 2024 •

edited

Loading