Add Braille and Tones extension points to avoid monkey patching by API consumers #14503

LeonarddeR · 2023-01-03T12:40:35Z

Link to issue number:

Replaces #9917

Summary of the issue:

Tools like NVDA remote that need to intercept speech and braille output currently rely on monkeypatching to do this.

Description of user facing changes

It is no longer possible to override the display size of the braille handler by setting braille.handler.displaySize. It is also no longer possible to enable/disable the handler by setting braille.handler.enabled.

Description of development approach

Added the following extension points:

inputCore.manager.decide_executeGesture: Decider for filtering gestures
tones.decide_beep
nvwave.decide_playWaveFile
braille.handler.pre_writeCells
braille.handler.filter_displaySize
braille.handler.decide_enabled
braille.handler.displayChange
braille.handler.displaySizeChanged

This means that NVDA Remote and similar tools can at least drop all monkeypatching related to braille, playing tones and waves. Speech is a separate subject and will be handled in a follow up.

Testing strategy:

Tested that NVDA will still work as normal.
Tested that lowering the display size programatically adds a full cell to the end of the text to indicate that there's a hard stop because of a lower cell count used.
New unit tests

Known issues with pull request:

None known

Change log entries:

For Developers

Added the following extension points: (Check them in source documentation for details).
inputCore.manager.decide_executeGesture
tones.decide_beep
nvwave.decide_playWaveFile
braille.handler.pre_writeCells
braille.handler.filter_displaySize
braille.handler.decide_enabled
braille.handler.displayChanged
braille.handler.displaySizeChanged

API breaking changes

it is no longer possible to enable/disable the braille handler by setting braille.handler.enabled or update the display size of the handler by setting braille.handler.displaySize.
To update the displaySize programatically, register a handler to braille.handler.filter_displaySize. See the brailleViewer code for an example on how to do this.
To disable the braille handler programatically, register a handler to braille.handler.decide_enabled.

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.

* inputCore.manager.decide_executeGesture: Decider for filtering gestures * tones.decide_beep * nvwave.decide_playWaveFile * braille.handler.pre_writeCells * braille.handler.filter_displaySize * braille.handler.decide_enabled

AppVeyorBot · 2023-01-03T13:38:02Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8r03cwdd1lrbwohp/artifacts/output/nvda_snapshot_pr14503-27403,c24a5615.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.2,
BUILD_START 0.0,
BUILD_END 31.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 22.9,
FINISH_END 0.1

See test results for failed build of commit c24a56155a

AppVeyorBot · 2023-01-03T14:43:18Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/l0y5h2wo4dykd9dq/artifacts/output/nvda_snapshot_pr14503-27404,4ff1ae2c.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 24.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.6,
FINISH_END 0.1

See test results for failed build of commit 4ff1ae2c34

AppVeyorBot · 2023-01-03T15:50:13Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/6v6btfoy9gts2d5s/artifacts/output/nvda_snapshot_pr14503-27405,c25aab6e.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 25.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.8,
FINISH_END 0.1

See test results for failed build of commit c25aab6ec4

CyrilleB79 · 2023-01-03T20:42:24Z

Small fix in change log:
(Check their in source documentation for details): replace "their" by "them"

source/tones.py

source/nvwave.py

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

AppVeyorBot · 2023-01-04T07:39:44Z

See test results for failed build of commit f4f92ae9da

seanbudd · 2023-01-04T23:22:21Z

It seems that the system tests are failing for a legitimate reason on this PR

AppVeyorBot · 2023-01-05T01:08:25Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/uuf3i60m6ue28oaq/artifacts/output/nvda_snapshot_pr14503-27422,a44cfee9.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 25.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 21.5,
FINISH_END 0.2

See test results for failed build of commit a44cfee9b9

LeonarddeR · 2023-01-05T06:49:27Z

I'l have a look and will add one additional extension point plus some unit tests as well.

AppVeyorBot · 2023-01-05T07:50:59Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/g1fojqgftp242uur/artifacts/output/nvda_snapshot_pr14503-27431,e1d27383.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.1,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.0,
FINISH_END 0.1

See test results for failed build of commit e1d27383a5

AppVeyorBot · 2023-01-05T15:28:53Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/klj9ndan1d2edavq/artifacts/output/nvda_snapshot_pr14503-27432,a43126df.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 26.2,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.5,
FINISH_END 0.1

See test results for failed build of commit a43126dff8

AppVeyorBot · 2023-01-05T16:37:12Z

PASS: Unit tests.
FAIL: System tests (tags: installer NVDA). See test results for more information.
PASS: Lint check.
PASS: Translation comments check.
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 25.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.0,
FINISH_END 0.1
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/la7qikbka8s8s04h/artifacts/output/nvda_snapshot_pr14503-27433,b44f589a.exe

See test results for failed build of commit b44f589a85

AppVeyorBot · 2023-01-06T19:19:53Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/eqt5h58rxhdeypb0/artifacts/output/nvda_snapshot_pr14503-27447,74f272c4.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.3,
BUILD_START 0.0,
BUILD_END 27.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 20.9,
FINISH_END 0.1

See test results for failed build of commit 74f272c4bd

LeonarddeR · 2023-01-09T06:24:52Z

The system tests are working and all the unit tests are in place. I think this is readdy for another review.

source/braille.py

source/extensionPoints/__init__.py

tests/unit/__init__.py

tests/unit/extensionPointTestHelpers.py

tests/unit/test_inputCore.py

tests/unit/test_nvwave.py

tests/unit/test_tones.py

tests/unit/test_braille.py

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd

Thanks @LeonarddeR , LGTM, I can fix up the docstring when updating the changes

seanbudd · 2023-01-11T01:07:25Z

source/braille.py

+		#: Internal cache for the displaySize property.
+		#: This attribute is used to compare the displaySize output by l{filterDisplaySize}
+		#: with its previous output.
+		#: If the value differs, L{displaySizeChanged} is notified.


can this be turned into a docstring like the others?

Fixup of #14503 Summary of the issue: When a handler was registered to decide_enabled and a cursor was blinking or a message with timeout was shown on the braille display, the display would still be updated. Found when working on NVDA Remote to integrate the extension points. The extensionPointTestHelpers wouldn't strictly test whether the handler was actually called. Also the handler argument for value was incorrect and using a class to store the filter value was overdone. Description of user facing changes When a handler decides to disable the braille handler, cursor blinking is now stopped and a message is dismissed.

…x FilterValueT name (#14549) Summary of the issue: FilterValueTypeT nneeded renaming to FilterValueT and had a wrong string in its TypeVar declaration. When experimenting with the extensionPoints, particularlyy with registering a handler to decide_executeGesture in a braille display driver, I discovered that inputCore was not yet initialized when initializing the driver and therefore extension point registration failed. Description of user facing changes Moved extension points, changes file is changed as part of this pr to reflect this. Description of development approach Just moving and renaming code

See <nvaccess/nvda#14503>

…ewer (#15214) Fixes regression from #14503 Summary of the issue: As part of #14503, braille extension points have been added that are used by the braille viewer. However when destroying the braille viewer by pressing alt+f4, the display size filter handler was never unregistered, resulting in the braille handler still assuming 40 braille cells even though no display was connected. Description of user facing changes Braille handler connectly restores to 0 cells again after disabling the braille viewer with alt+f4 when no braille display is connected. Description of development approach Changed if check on the dialog to is not None explicitly. wx gives a destroyed window a bool value of False, so the boolean check would work. Ensure braille.filter_displaySize is unregistered when destroyBrailleViewer is called when the window is already destroyed. Note that destroying the window will unregister braille.pre_writeCells properly, so that unregistration can be limited to an undestroyed window only.

…cide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

…ecide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

…cide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

Added the following extension points:

03e8f37

* inputCore.manager.decide_executeGesture: Decider for filtering gestures * tones.decide_beep * nvwave.decide_playWaveFile * braille.handler.pre_writeCells * braille.handler.filter_displaySize * braille.handler.decide_enabled

LeonarddeR requested a review from a team as a code owner January 3, 2023 12:40

LeonarddeR requested a review from seanbudd January 3, 2023 12:40

Fix unit tests

b50d000

Lint

f5ac9ba

seanbudd reviewed Jan 4, 2023

View reviewed changes

source/tones.py Outdated Show resolved Hide resolved

seanbudd reviewed Jan 4, 2023

View reviewed changes

source/nvwave.py Outdated Show resolved Hide resolved

LeonarddeR and others added 2 commits January 4, 2023 07:29

Apply suggestions from code review

a020b4a

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Fix remaining typos

bc8cfad

seanbudd marked this pull request as draft January 4, 2023 23:22

HOpefully fix system tests

77ffa3f

Merge remote-tracking branch 'origin/master' into extensionPoints_new

f8c3c50

Next attempt

1c273d1

LeonarddeR added 5 commits January 5, 2023 19:33

Fix tests

9eed06a

Add new extension points

102fb0a

Merge remote-tracking branch 'origin/master' into extensionPoints_new

1bd348e

Linting

da686b8

Move docs

613de23

seanbudd added this to the 2023.1 milestone Jan 9, 2023

seanbudd added the api-breaking-change label Jan 9, 2023

seanbudd changed the title ~~Add additional extension points to avoid monkey patching by NVDA remote and similar tools~~ Add Braille and Tones extension points to avoid monkey patching by API consumers Jan 9, 2023

seanbudd reviewed Jan 9, 2023

View reviewed changes

seanbudd marked this pull request as draft January 9, 2023 23:48

LeonarddeR and others added 5 commits January 10, 2023 07:42

First batch of review actions

5ca2d91

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Move variable doc strings to use epidoc style

96c5152

Rename sameDisplayReInit

c0792f1

Unit test review actions

d9c5b08

Finish renaming sameDisplayReInit

683252c

LeonarddeR requested a review from seanbudd January 10, 2023 08:53

LeonarddeR marked this pull request as ready for review January 10, 2023 08:53

seanbudd approved these changes Jan 11, 2023

View reviewed changes

update changes

19c1f27

seanbudd merged commit f62986e into nvaccess:master Jan 11, 2023

This was referenced Jan 11, 2023

Prepare for changes in NVDA 2023.1 BabbageCom/unicornNVDA#6

Closed

Follow up for decide_enabled extension point #14536

Merged

seanbudd added a commit that referenced this pull request Jan 19, 2023

Fix lint from #14503

180c9f2

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request Apr 15, 2023

Temporarily disable feature for NVDA >= 2023.1

6720eb6

See <nvaccess/nvda#14503>

LeonarddeR mentioned this pull request Jul 28, 2023

Fix braille extension points not properly unregistered for braille viewer #15214

Merged

6 tasks

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request Oct 7, 2023

monkeypatch: inputCore.InputManager.executeGesture -> `inputCore.de…

bf90e97

…cide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

LeonarddeR mentioned this pull request Feb 23, 2024

Add pre_filter_speechSequence and post_filter_speechSequence Action and use post_filter_speechSequence in speechViewer #16213

Closed

5 tasks

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request May 12, 2024

monkeypatch: inputCore.InputManager.executeGesture -> `inputCore.d…

782d07d

…ecide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request May 12, 2024

monkeypatch: inputCore.InputManager.executeGesture -> `inputCore.de…

b2de504

…cide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request May 12, 2024

monkeypatch: inputCore.InputManager.executeGesture -> `inputCore.de…

163e5b9

…cide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

AAClause added a commit to AAClause/BrailleExtender that referenced this pull request May 12, 2024

monkeypatch: inputCore.InputManager.executeGesture -> `inputCore.de…

dbe41ff

…cide_executeGesture` See <nvaccess/nvda#14503>, <LeonarddeR/rdAccess#14>

Uh oh!

Add Braille and Tones extension points to avoid monkey patching by API consumers #14503

Add Braille and Tones extension points to avoid monkey patching by API consumers #14503

Uh oh!

Conversation

LeonarddeR commented Jan 3, 2023 • edited by seanbudd 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:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jan 3, 2023

Uh oh!

AppVeyorBot commented Jan 3, 2023

Uh oh!

AppVeyorBot commented Jan 3, 2023

Uh oh!

CyrilleB79 commented Jan 3, 2023

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Jan 4, 2023

Uh oh!

seanbudd commented Jan 4, 2023

Uh oh!

AppVeyorBot commented Jan 5, 2023

Uh oh!

LeonarddeR commented Jan 5, 2023

Uh oh!

AppVeyorBot commented Jan 5, 2023

Uh oh!

AppVeyorBot commented Jan 5, 2023

Uh oh!

AppVeyorBot commented Jan 5, 2023

Uh oh!

AppVeyorBot commented Jan 6, 2023

Uh oh!

LeonarddeR commented Jan 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeonarddeR commented Jan 3, 2023 •

edited by seanbudd

Loading