Update braille independently on caret #15163

burmancomp · 2023-07-18T07:28:37Z

Link to issue number:

Fixes #15115

Summary of the issue:

This extends pull request #15134 and includes pull request #15156. Collaborated with @LeonarddeR.

Description of user facing changes

Braille line is updated in terminal window when there is text change.

In addition braille line is updated in any application when:

braille cursor is turned on or off with gesture
if there is selected text in edit control or document, changing "show selection" state with gesture updates braille line when otherwise supported

Description of development approach

Terminal window updates are handled with:

NVDAObjects.behaviors.Terminal.event_textChange and
event_UIA_notification in winConsoleUIA._NotificationsBasedWinTerminalUIA.

Objects are passed to BrailleHandler.handleUpdate function.

When braille show selection state is changed with gesture, BrailleHandler.initialDisplay function is executed.

When braille cursor is toggled with gesture, BrailleHandler._updateDisplay function is executed.

Testing strategy:

Tested with Albatross 80 and little with Optelec Alva BC680.

Known issues with pull request:

none known

Change log entries:

New features
Changes

When the text in a terminal changes without updating the caret, the text on a braille display will now properly update when positioned on a changed line. This includes situations where braille is tethered to review.

Bug fixes

The braille cursor and selection indicators will now always be updated correctly after showing or hiding respective indicators with a gesture.

For Developers

braille.handler.handleUpdate and braille.handler.handleReviewMove have been changed in order not to update instantly. Before this change, when either of these methods was called very often, this would drain many resources. These methods now queue an update at the end of every core cycle instead. They should also be thread safe, making it possible to call them from background threads.

Deprecations:

braille.BrailleHandler.handlePendingCaretUpdate is now deprecated with no public replacement.

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.

…hanges

AppVeyorBot · 2023-07-18T08:28:52Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit b95da15196

AppVeyorBot · 2023-07-18T10:57:39Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 621db0d2a3

AppVeyorBot · 2023-07-18T20:48:21Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit d33cb7f891

AppVeyorBot · 2023-07-19T18:06:30Z

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/hg7kpgkw05fukcug/artifacts/output/nvda_snapshot_pr15163-28705,2165a402.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 18.8,
FINISH_END 0.1

See test results for failed build of commit 2165a40281

AppVeyorBot · 2023-07-19T20:25:06Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/fecmki3w222i41hd/artifacts/output/nvda_snapshot_pr15163-28706,513924ae.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.8,
FINISH_END 0.3

See test results for failed build of commit 513924ae0f

AppVeyorBot · 2023-07-19T21:26:42Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/nqn3km0vlj45cbvy/artifacts/output/nvda_snapshot_pr15163-28707,4c50ada9.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 25.4,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.6,
FINISH_END 0.2

See test results for failed build of commit 4c50ada91e

AppVeyorBot · 2023-07-23T18:19:07Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/udm2y4oyq5kvb1oe/artifacts/output/nvda_snapshot_pr15163-28715,cca30e20.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.7,
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 cca30e202a

AppVeyorBot · 2023-07-23T19:48:38Z

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

See test results for failed build of commit db74e0cddd

AppVeyorBot · 2023-07-23T22:03:14Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 02c14674f5

AppVeyorBot · 2023-07-24T06:07:57Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit c8f952a425

AppVeyorBot · 2023-07-24T09:33:43Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit a6183d1bc7

AppVeyorBot · 2023-07-24T19:25:04Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit b748d3f826

AppVeyorBot · 2023-07-25T07:39:31Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 0c530a1c6e

AppVeyorBot · 2023-07-26T08:46:59Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 1c7b399611

AppVeyorBot · 2023-07-26T10:09:28Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit ef8163f39d

LeonarddeR · 2023-07-26T10:57:39Z

While I like this pr as a proof of concept, I see several reasons why this doesn't fit for NVDA Core, as it is merely a workaround and obfuscator of other bugs that should have our attention instead:

Most notably, it has a polling/timer based approach instead of event driven.
A timer on the main thread like this may severely impact NVDA's performance in objects that are slow to update, such as Excel, some more complex UIA implementations, etc. A timer on another thread will probably result in issues with thread safety.

Again, I really like your investment in this, but I'd rather see #15134 being merged first and then have smaller follow ups like #15156, but even the latter is already to broad in scope and I decided to close it. The braille subsystem of NVDA is pretty complex and hard to oversee and it is very, very easy to break things

AppVeyorBot · 2023-07-26T11:28:23Z

PASS: Translation comments check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 19b4701ac9

burmancomp · 2023-07-26T12:14:20Z

Thank you @LeonarddeR for your comments.

1. Most notably, it has a polling/timer based approach instead of event driven.

What kind of event driven approach you are thinking?

2. A timer on the main thread like this may severely impact NVDA's performance in objects that are slow to update, such as Excel, some more complex UIA implementations, etc. A timer on another thread will probably result in issues with thread safety.

Currently it is run once in 0.5 seconds, and it tries to avoid redundant updates. There is also _cursorBlinkTimer polling in 0.25 seconds cycles as default in main thread. Is an additional timer a problem or its polling cycle or what it is doing?

I tried also with timer in its own thread but in browser there is _ctypes.COMError when switching from browse mode to focus mode.

This pull request should only do something with timer when:

current object is terminal window (and before update it is checked that braille content has been changed)
state of show selection is changed
braille cursor is toggled.

I suppose that most of time only above checks are done, and this is once in 0.5 seconds.

Again, I really like your investment in this, but I'd rather see #15134 being merged first and then have smaller follow ups like #15156, but even the latter is already to broad in scope and I decided to close it. The braille subsystem of NVDA is pretty complex and hard to oversee and it is very, very easy to break things

There may be problems but I suppose they are met in testing or maybe they can shown with calculations.

Could you tell whatkind of tests should be done?

It is not important that this implementation would be merged but these things are important.

When and if for example font attributes are presented in braille switching that state on and off is important. What I try to say is that these things should be solved anyway so why not earlier than later?

LeonarddeR · 2023-07-26T12:19:23Z

What kind of event driven approach you are thinking?

There is already the textChange event fired on terminal objects when the text changes. That event should be fired when anything changes in the text, including check boxes in terminals. If not, that's a bug that needs to be looked into first.
Currently, no braille updates are performed in event_textChange, so that would be a good starting point.

LeonarddeR · 2023-08-24T07:39:32Z

@burmancomp could you please add the following to tests/unit/__init__py, below braille.filter_displaySize.register(getFakeCellCount)?

_original_handleReviewMove = braille.handler.handleReviewMove


def _patched_handleReviewMove(shouldAutoTether=True):
	"""
	For braille unit tests that rely on updating the review cursor, ensure that we simulate a core cycle
	by executing BrailleHandler._handlePendingUpdate after Braillehandler.handleReviewMove
	"""
	_original_handleReviewMove(shouldAutoTether)
	braille.handler._handlePendingUpdate()


braille.handler.handleReviewMove = _patched_handleReviewMove

AppVeyorBot · 2023-08-24T07:43:02Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/0988qbs05kxl0gpr/artifacts/output/nvda_snapshot_pr15163-28941,7d68bd73.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 25.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.8,
FINISH_END 0.1

See test results for failed build of commit 7d68bd7314

AppVeyorBot · 2023-08-24T09:11:45Z

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/dlfjmrpuk9fhv2cs/artifacts/output/nvda_snapshot_pr15163-28942,860a4c4d.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 25.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.5,
FINISH_END 0.2

See test results for failed build of commit 860a4c4d5e

LeonarddeR · 2023-08-24T16:37:38Z

I'd like to suggest the following changelog entries:

Changes:

When the text in a terminal changes and braille is tethered to review, the text on a braille display will now properly update if the review cursor is positioned on a changed line. (braille: update display when tethered to review and review cursor does not follow caret #15115)

Bug fixes

The braille cursor and selection indicators will now always be updated correctly after showing or hiding respective indicators with a gesture.

Changes for developers

BrailleHandler.handleUpdate and BrailleHandler.handleReviewMove have been changed in order not to update instantly. Before this change, when either of these methods was called very often, this would drain many resources. These methods now queue an update at the end of every core cycle instead, behaving very much the same as BrailleHandler.handleCaretMove. They should also be thread safe, making it possible to call them from background threads.

Deprecations:

braille.BrailleHandler.handlePendingCaretUpdate is now deprecated with no public replacement.

AppVeyorBot · 2023-08-24T17:13:21Z

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/ajmq6vyvj1s1cyt8/artifacts/output/nvda_snapshot_pr15163-28943,9de47c2e.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 25.6,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.7,
FINISH_END 0.1

See test results for failed build of commit 9de47c2e25

burmancomp · 2023-08-24T17:44:39Z

Changes:

* When the text in a terminal changes and braille is tethered to review, the text on a braille display will now properly update if the review cursor is positioned on a changed line. ([braille: update display when tethered to review and review cursor does not follow caret #15115](https://github.com/nvaccess/nvda/issues/15115))

Braille line is also updated correctly when braille is tethered to focus. This is when there is text change but caret is not moved.

If you have suitable phrase for this, let me know, or if mentioning this is not necessary.

LeonarddeR · 2023-08-24T19:12:55Z

Good catch. SOmething like:;

When the text in a terminal changes without updating the caret, the text on a braille display will now properly update when positioned on a changed line. This includes situations where braille is tethered to review.

seanbudd

Thanks @burmancomp

…ndentlyOnCaret

burmancomp · 2023-08-29T05:29:14Z

Thanks @burmancomp

And thanks @LeonarddeR as well.

Fixes #15391 Fixup of #15163 Summary of the issue: Since #15163, it is much more likely that braille tries to update a region for an object that no longer exists. This is no problem at all, however in that case, the region should simply be ignored for updating and a debug warning should be logged instead. Description of user facing changes No errors in de log while browsing Description of development approach a try/except around region.update, with an extra check to avoid updating regions for tree interceptors that have died. Testing strategy: Browsed with Firefox for some minutes, wasn't able to reproduce the issue any longer.

Closes #15773 Fixup of #15163 Summary of the issue: Contracted braille input is broken since #15163 merge. Description of user facing changes Contracted braille input works properly again. Description of development approach braille.handler._doCursorMove no longer exists. braille.handler._regionsPendingUpdate.add and braille.handler._handlePendingUpdate are used instead.

LeonarddeR added 3 commits July 13, 2023 22:19

handle braille updates in pump all and include updates for terminal c…

fd366c0

…hanges

Better deprecation handling

f2bf00b

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

1c207b7

burmancomp mentioned this pull request Jul 18, 2023

Update braille when tethered to review but review cursor does not follow caret #15134

Closed

6 tasks

burmancomp marked this pull request as ready for review July 26, 2023 09:07

burmancomp requested a review from a team as a code owner July 26, 2023 09:07

burmancomp requested a review from seanbudd July 26, 2023 09:07

burmancomp force-pushed the updateBrailleIndependentlyOnCaret branch from 84be35d to 8c947d9 Compare July 26, 2023 10:28

LeonarddeR added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Jul 26, 2023

handle more braille line updates

6a5b4d7

burmancomp force-pushed the updateBrailleIndependentlyOnCaret branch from b3ef673 to 6a5b4d7 Compare July 27, 2023 20:48

suggested insertion to unit test

37c9a22

LeonarddeR approved these changes Aug 24, 2023

View reviewed changes

next time

864213b

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 29, 2023

seanbudd approved these changes Aug 29, 2023

View reviewed changes

seanbudd added 2 commits August 29, 2023 15:23

Merge remote-tracking branch 'origin/master' into updateBrailleIndepe…

f4fe574

…ndentlyOnCaret

update changes

0e11f30

seanbudd merged commit 7b31977 into nvaccess:master Aug 29, 2023
1 check was pending

nvaccessAuto added this to the 2023.3 milestone Aug 29, 2023

burmancomp deleted the updateBrailleIndependentlyOnCaret branch August 29, 2023 05:32

This was referenced Sep 6, 2023

Remove scheduled deprecations for 2024.1 #15385

Merged

Error logged when browsing to new page in Firefox #15391

Closed

Add checks for region updates #15393

Merged

This was referenced Sep 7, 2023

Review cursors don't see refreshed information #5771

Closed

Some notifications are not displayed to braille display. #5747

Closed

aaclause mentioned this pull request Nov 11, 2023

NVDA 2023.3 and alpha: contracted braille input doesn't work #15773

Closed

aaclause added a commit to aaclause/nvda that referenced this pull request Nov 12, 2023

Fixes nvaccess#15773 / Fixup of nvaccess#15163

f3cff4b

aaclause mentioned this pull request Nov 12, 2023

Fixes #15773 / Contracted braille input #15774

Merged

5 tasks

aaclause added a commit to aaclause/nvda that referenced this pull request Nov 22, 2023

Fixes nvaccess#15773 / Fixup of nvaccess#15163

7aa4585

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update braille independently on caret #15163

Update braille independently on caret #15163

burmancomp commented Jul 18, 2023 •

edited

AppVeyorBot commented Jul 18, 2023

AppVeyorBot commented Jul 18, 2023

AppVeyorBot commented Jul 18, 2023

AppVeyorBot commented Jul 19, 2023

AppVeyorBot commented Jul 19, 2023

AppVeyorBot commented Jul 19, 2023

AppVeyorBot commented Jul 23, 2023

AppVeyorBot commented Jul 23, 2023

AppVeyorBot commented Jul 23, 2023

AppVeyorBot commented Jul 24, 2023

AppVeyorBot commented Jul 24, 2023

AppVeyorBot commented Jul 24, 2023

AppVeyorBot commented Jul 25, 2023

AppVeyorBot commented Jul 26, 2023

AppVeyorBot commented Jul 26, 2023

LeonarddeR commented Jul 26, 2023 •

edited

AppVeyorBot commented Jul 26, 2023

burmancomp commented Jul 26, 2023

LeonarddeR commented Jul 26, 2023 •

edited

LeonarddeR commented Aug 24, 2023

AppVeyorBot commented Aug 24, 2023

AppVeyorBot commented Aug 24, 2023

LeonarddeR commented Aug 24, 2023 •

edited

AppVeyorBot commented Aug 24, 2023

burmancomp commented Aug 24, 2023

LeonarddeR commented Aug 24, 2023

seanbudd left a comment

burmancomp commented Aug 29, 2023

Update braille independently on caret #15163

Update braille independently on caret #15163

Conversation

burmancomp commented Jul 18, 2023 • edited

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:

AppVeyorBot commented Jul 18, 2023

AppVeyorBot commented Jul 18, 2023

AppVeyorBot commented Jul 18, 2023

AppVeyorBot commented Jul 19, 2023

AppVeyorBot commented Jul 19, 2023

AppVeyorBot commented Jul 19, 2023

AppVeyorBot commented Jul 23, 2023

AppVeyorBot commented Jul 23, 2023

AppVeyorBot commented Jul 23, 2023

AppVeyorBot commented Jul 24, 2023

AppVeyorBot commented Jul 24, 2023

AppVeyorBot commented Jul 24, 2023

AppVeyorBot commented Jul 25, 2023

AppVeyorBot commented Jul 26, 2023

AppVeyorBot commented Jul 26, 2023

LeonarddeR commented Jul 26, 2023 • edited

AppVeyorBot commented Jul 26, 2023

burmancomp commented Jul 26, 2023

LeonarddeR commented Jul 26, 2023 • edited

LeonarddeR commented Aug 24, 2023

AppVeyorBot commented Aug 24, 2023

AppVeyorBot commented Aug 24, 2023

LeonarddeR commented Aug 24, 2023 • edited

AppVeyorBot commented Aug 24, 2023

burmancomp commented Aug 24, 2023

LeonarddeR commented Aug 24, 2023

seanbudd left a comment

Choose a reason for hiding this comment

burmancomp commented Aug 29, 2023

burmancomp commented Jul 18, 2023 •

edited

LeonarddeR commented Jul 26, 2023 •

edited

LeonarddeR commented Jul 26, 2023 •

edited

LeonarddeR commented Aug 24, 2023 •

edited