Skip to content

Eurobraille bnote and bbook automatic detection#14690

Merged
seanbudd merged 32 commits into
nvaccess:masterfrom
FalkoBabbage:eb_bnote_bbook
Jun 6, 2023
Merged

Eurobraille bnote and bbook automatic detection#14690
seanbudd merged 32 commits into
nvaccess:masterfrom
FalkoBabbage:eb_bnote_bbook

Conversation

@FalkoBabbage

@FalkoBabbage FalkoBabbage commented Mar 2, 2023

Copy link
Copy Markdown
Contributor

Eurobraille asked us to adjust the current braille display driver in NVDA. This includes the automatic detection of 2 devices as well as some changes in the gestures.

Link to issue number:

Not applicable

Summary of the issue:

  • The b.note "VID_28AC&PID_0012"/"VID_28AC&PID_0013" is not detected automatically
  • The b.book "VID_28AC&PID_0020"/"VID_28AC&PID_0021" is not detected automatically
  • the b.note joystick1Left/joystick1Right should do the action "braille_scrollBack/Forward" instead of "review_next/previousCharacter"
  • when connected to a b.note/b.book, a message should be send to the braille display with the format {"S", "n", "NVDA/XX"}, where "XX" is the number of the COM-port. A {"S", "n" ""} message should be sent when disconnecting.

Description of user facing changes

  • NVDA automatically detects the b.note and b.book
  • some gestures changed when a b.note is connected

Description of development approach

  • I have added the PD/VID for the b.note and b.book to bdDetect.py for automatic detection.
  • In the existing eurobraille.py display driver I have added the "deviceType" for the b.note and b.book
  • In the existing eurobraille.py, when a b.note is connected, another gesture map is used than the default one for the change in joystick1Left/joystick1Right.
  • I have changed the name to "eurobraille Displays"
  • In the existing eurobraille.py, when a device is connected and is confirmed to be a b.note or b.book, NVDA sends a identifier for the connection. When NVDA terminates it sends an empty message to reset this identifier to the display.

Testing strategy:

  • I tested the general working of the automatic detection of the b.note and the new joystick1left/right gesture.
  • I sent a alpha build of this branch to Eurobraille and they confirmed it works as intended.

Known issues with pull request:

  • When pressing NVDA+1 on the braille display, the help mode of NVDA cannot be turned off again via the same command. I am not sure whether this is an issue with this specific PR or whether it occurs in NVDA in general.

Change log entries:

New features

  • Added support for Eurobraille B.Note and B.Book braille displays.
    Changes
  • Gestures for the Eurobraille braille displays are updated. Please refer to the NVDA user guide for an overview.
    Bug fixes
    For Developers

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 901eae92bc

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit b7714634ce

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of using a different gesture map for the note to change gestures, it should be possible to use model based identifiers. Those take precedence over the generic ones. That means adding only two new joystick identifiers for the note displays in the format br(eurobraille.b.note). Both translate to the same model identifier according to self.model = display.deviceType.lower().split(" ")[0]

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit a55cfcf924

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 456c31ca91

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 6dea777dfa

@FalkoBabbage

Copy link
Copy Markdown
Contributor Author

I am not sure why appveyor failed the symbols tests. All symbol tests failed because of "Speech did not finish before timeout"
I ran runsystemtests.bat --include symbols locally and it all passed.

@CyrilleB79

Copy link
Copy Markdown
Contributor

Probably not linked to braille drivers. System tests sometimes are not very stable.

You may want to push a new commit to run tests again to have them passed, e.g.:

git commit --allow-empty -m "Empty commit to re-run CI tests."
git push

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit cbdbdd5bbd

@seanbudd

Copy link
Copy Markdown
Member

Is this ready for review? The test failures are irrelevant here

@FalkoBabbage

Copy link
Copy Markdown
Contributor Author

Hi @seanbudd, this is ready for review in that case.

@FalkoBabbage FalkoBabbage marked this pull request as ready for review April 5, 2023 06:00
@FalkoBabbage FalkoBabbage requested a review from a team as a code owner April 5, 2023 06:00
@FalkoBabbage FalkoBabbage requested a review from seanbudd April 5, 2023 06:00

@seanbudd seanbudd left a comment

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.

This generally looks good to me.
Let me know your thoughts on splitting the files.

Comment thread source/brailleDisplayDrivers/eurobraille.py Outdated
Comment thread source/brailleDisplayDrivers/eurobraille.py Outdated
Comment thread source/brailleDisplayDrivers/eurobraille.py Outdated
@FalkoBabbage

Copy link
Copy Markdown
Contributor Author

Thanks for the corrections @seanbudd .I have missed the formatting rules of the user guide.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 243e085211

@seanbudd seanbudd left a comment

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.

Hi @FalkoBabbage ,

I've pushed some changes fixing up the key formatting in the user guide.
Can you please review and make sure I didn't mess anything up?

Notably:

  • removing a group of keys that was repeated 3 times
  • changed to using lowerCamelCase consistently for keys, to match the rest of the user guide
  • using code formatting for entire key sequence, e.g. ``dot1+space``, not dot1+``space``

Comment thread user_docs/en/userGuide.t2t Outdated
Comment on lines +3589 to +3600
| Scroll braille display back | backward |
| Scroll braille display forward | Forward |
| Move to current focus | Backward + forward |
| Route to braille cell | routing |
| Scroll braille display back | backward |
| Scroll braille display forward | Forward |
| Move to current focus | Backward + forward |
| Route to braille cell | routing |
| Scroll braille display back | backward |
| Scroll braille display forward | Forward |
| Move to current focus | Backward + forward |
| Route to braille cell | routing |

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.

these are duplicated three times, is there a reason for it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a mistake I made while copying the provided documentation from Eurobraille to the t2t format. Thank you for noticing it!

@seanbudd seanbudd left a comment

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.

Hi @FalkoBabbage ,

I've pushed some changes fixing up the key formatting in the user guide.
Can you please review and make sure I didn't mess anything up?

Notably:

  • removing a group of keys that was repeated 3 times
  • changed to using lowerCamelCase consistently for keys, to match the rest of the user guide
  • using code formatting for entire key sequence, e.g. ``dot1+space``, not dot1+``space``

@seanbudd

seanbudd commented Jun 5, 2023

Copy link
Copy Markdown
Member

Could you also please fix the linting issues raised in the PR failure message?

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit dabd96a102

@FalkoBabbage

Copy link
Copy Markdown
Contributor Author

Hi @FalkoBabbage ,

I've pushed some changes fixing up the key formatting in the user guide. Can you please review and make sure I didn't mess anything up?

Notably:

* removing a group of keys that was repeated 3 times

* changed to using `lowerCamelCase` consistently for keys, to match the rest of the user guide

* using code formatting for entire key sequence, e.g. `` ``dot1+space`` ``, not `` dot1+``space`` ``

Thank you! I went through the documentation and it is correct now.
Is there a guidline for the userguide formatting? I have apparently missed some formatting rules.

Comment thread user_docs/en/userGuide.t2t Outdated
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 58662ec369

@seanbudd

seanbudd commented Jun 5, 2023

Copy link
Copy Markdown
Member

@seanbudd seanbudd left a comment

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.

Thanks @FalkoBabbage

@Qchristensen Qchristensen left a comment

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.

Good work.

@seanbudd seanbudd merged commit a0a146d into nvaccess:master Jun 6, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Jun 6, 2023
seanbudd added a commit that referenced this pull request Jul 3, 2023
Fixes #15079

Summary of the issue:
Eurobraille devices were not being detected as of #14690

Description of user facing changes
Eurobraille devices should now be detected

Description of development approach
Forcibly include brailleDisplayDrivers.eurobraille in packages like the albatross driver
@seanbudd seanbudd mentioned this pull request Jul 26, 2023
6 tasks
seanbudd added a commit that referenced this pull request Jul 27, 2023
Follow up to #14690

Summary of the issue:
The docs for the eurobraille display aren't entirely clear and could use some rewording

Description of user facing changes
Fix up docs
seanbudd pushed a commit that referenced this pull request Jul 28, 2023
None
Fix-up of #14690

Summary of the issue:
In input help mode, when pressing a dot key on the braille keyboard of Esys, I get the following in the log:

INFO - inputCore.InputManager._handleInputHelp (11:32:45.222) - MainThread (15120):
Input help: gesture br(eurobraille.esys):d+o+t+<+g+e+n+e+r+a+t+o+r+ +o+b+j+e+c+t+ +I+n+p+u+t+G+e+s+t+u+r+e+.+_+_+i+n+i+t+_+_+.+<+l+o+c+a+l+s+>+.+<+g+e+n+e+x+p+r+>+ +a+t+ +0+x+0+8+A+2+9+9+F+0+>, bound to script braille_dots on globalCommands.GlobalCommands
Description of user facing changes
Fixed the logged message

Description of development approach
Small fix in code (see diff)
seanbudd pushed a commit that referenced this pull request Jul 31, 2023
Fix-up for #14690

Summary of the issue:
f-string was written as "f ...." instead of f"...."
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.

7 participants