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

[burmancomp]

Link to issue number:

15115

Summary of the issue:

I think use case is mainly with terminal software like Putty or Teraterm when remotely managing Linux systems from command line using non-graphical user interface (typically shell like bash).

Caret is not always "real cursor", and following caret may be useless or harmful because it may lead to wrong location in window. Content of terminal window is often explored with review cursor.

When using review cursor, so that it does not follow caret, gives better possibilities to review window content because key presses or other things which trigger cursor movement do not affect on review position. However, although content which braille display is currently showing would change for example due to key press, braille display is not updated automatically. Automatic update would facilitate detection of changes and thus remote management.

Description of user facing changes

When braille is explicitly tethered to review, and review cursor does not follow system caret, braille display content is updated if:

• focus object is navigator object and
• content which should be shown on braille display changes or updates due to caret movement.

At the moment I think a setting for this would be somewhat redundant. Currently there is no changes in user guide because it does not seem to define braille behavior when braille is tethered to review and review cursor does not follow system caret.

Description of development approach

Implementation is in BrailleHandler.handleCaretMove function in braille.py.

Testing strategy:

Tested with Putty and Albatross 80 model.

Known issues with pull request:

none

Change log entries:

New features
Changes
When braille is explicitly tethered to review and review cursor does not follow system caret, braille display content is updated if:

• focus object is navigator object and
• content which should be shown on braille display changes or updates due to caret movement.
  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.

[LeonarddeR]

I'm honestly not sure about this feature. I'd say that if someone has review follows system caret disabled and they yet want their display to update for caret updates, they should just enable review cursor follows system caret option at least temporarily.

[burmancomp]

I'm honestly not sure about this feature. I'd say that if someone has review follows system caret disabled and they yet want their display to update for caret updates, they should just enable review cursor follows system caret option at least temporarily.

This is useful when one wants to monitor changes in given line caused or updated by caret move which occurs somewhere else within navigator object or its descendants.

This solution has its lack because it depends on caret move but it is more than nothing.

[LeonarddeR]

Ah, I think I understand what you're saying now and that makes perfect sense actually. As long as we don't make caret movement change the position that's currently being reviewed, I think it is OK.

Could you however move the logic from the review module to the braille module? I think you can change braille.BrailleHandler.handleCaretMove, search for if self._tether != TetherTo.FOCUS.value: within that method. Adding braille specific stuff to the review module should be avoided.

[burmancomp]

@LeonarddeR do we agree no setting for this?

[LeonarddeR]

Nice one. I don't think this needs a config option, indeed.
I think that after my suggestions are applied, the behavior will still be the same and the code is somewhat cleaner.cleaner.

[burmancomp]

What do you think should there be something in user guide?

[burmancomp]

@LeonarddeR why logic could not be in braille module pumpAll function? Does it use too much resources there for example? I ask because then display would be updated also when there is no caret movement. Then for example, selecting check box "[ ]" with spacebar would be updated on braille display to "[x]" automatically although there is no cursor movement.

[LeonarddeR]

@LeonarddeR do we agree no setting for this?

Don't think that's necessary no.

What do you think should there be something in user guide?

An entry in the changelog's changes section is enough.

@LeonarddeR why logic could not be in braille module pumpAll function? Does it use too much resources there for example? I ask because then display would be updated also when there is no caret movement. Then for example, selecting check box "[ ]" with spacebar would be updated on braille display to "[x]" automatically although there is no cursor movement.

Using pumpAll would mean an update is performed way too often.
Now the issue you raise about check boxes not being updated, I assume you mean in Browse mode when using the review cursor? As when in object review, I'm pretty sure an update check box should reflect its state when updated also when braille is tethered to review and review mode is set to object.

[burmancomp]

@LeonarddeR why logic could not be in braille module pumpAll function? Does it use too much resources there for example? I ask because then display would be updated also when there is no caret movement. Then for example, selecting check box "[ ]" with spacebar would be updated on braille display to "[x]" automatically although there is no cursor movement.

Using pumpAll would mean an update is performed way too often. Now the issue you raise about check boxes not being updated, I assume you mean in Browse mode when using the review cursor? As when in object review, I'm pretty sure an update check box should reflect its state when updated also when braille is tethered to review and review mode is set to object.

Sorry my comment was not clear. I referred to "check boxes" I have encountered in Linux using Putty. But I think now that problem is not actually given information like this "check boxes", in stead problem seems to be that for some reason there may be information which is in the region braille line is displaying but braille line is not always updated to show that information.

I compiled local test version where I removed what I added to BrailleHandler.handleCaretMove, and put the following to pumpAll:

if handler.getTether() != TetherTo.REVIEW.value: handler.handleUpdate(api.getFocusObject()) else: handler.handleUpdate(api.getNavigatorObject())

Result was that those "check boxes" which are checked and unchecked with spacebar: "[ ]", "[x]" changed their state also in braille line when pressing spacebar.

I think that there are situations where content which is being displayed on braille line may change but it is not updated to braille line automatically at the moment.

There should be mechanism that ensures that changed content is always updated to braille line. I think that solution this pull request implements at this moment is only partial and I think that solution should be more generic.

Where to put such code which ensures that all changes are updated?

[burmancomp]

Referring to my previous comment here are some additional examples.

Use gestures to these:

• set braille tethered to automatically, select text in editor, and disable/enable "braille show selection" with gesture
• when braille shows cursor, disable/enable it with gesture.

As you should note your changes (disabling/enabling showing selection or braille cursor) are not automatically updated to braille line.

I think that reason is same as with "check boxes" in Linux using Putty.

But when pumpAll contains code (code in my previous comment is not optimized) these changes (disabling/enabling showing selection or braille cursor) are updated to display automatically as well as "Linux check boxes" state.

[LeonarddeR]

Use gestures to these:

* set braille tethered to automatically, select text in editor, and disable/enable "braille show selection" with gesture

* when braille shows cursor, disable/enable it with gesture.

As you should note your changes (disabling/enabling showing selection or braille cursor) are not automatically updated to braille line.

I think this is a separate issue. could you please open a new issue for this?

[LeonarddeR]

Using something like that in pumpAll, while it sounds effective, is is really way to heavy. It will try to update objects way to often.

I think the cases you mention are pretty valid though. I think you should be able to fix them pretty easily.

1. In NVDAObjects.behaviours, in the LiveText class, in the _reportNewLines method, add braille.handler.handleUpdate(self) to the end of the method.
2. In NVDAObjects.Uia.winConsoleUIA, in the _NotificationsBasedWinTerminalUIA class, in the event_UIA_notification method, add braille.handler.handleUpdate(self) to the end of the method.

Pretty sure you will have covered all the terminal cases for braille. I think you can even avoid the current implementation in this pr, at least for terminals, however I think it still makes sense to keep it because it deals with caret updates.
The only concern I have that braille updates still might be expensive when lots of text is coming in on a terminal. We could consider moving the last part of braille.handler.handleUpdate to a separate method that is executed in pumpAll when necessary.
May be this should be done in a separate pr. I'm happy to do that, but it needs wide testing and certainly shouldn't go in 2023.2.

[burmancomp]

As you should note your changes (disabling/enabling showing selection or braille cursor) are not automatically updated to braille line.

I think this is a separate issue. could you please open a new issue for this?

I think that these and what I have encountered with Putty in Linux are both special cases of same issue.

Do you think that issue #15115 should be closed as well as this pull request and then open a new issue?

[burmancomp]

Using something like that in pumpAll, while it sounds effective, is is really way to heavy. It will try to update objects way to often.

It is not important to do it in pumpAll but it should be done somewhere not too often but however frequently enough.

I think the cases you mention are pretty valid though. I think you should be able to fix them pretty easily.

Thank you.

However, I think that whole problem (some display updates, which should be done, are not done) should be solved not just given special case.

[LeonarddeR]

Do you think that issue #15115 should be closed as well as this pull request and then open a new issue?

No, I think this pr is fine as is. We really should call handleUpdate when tethered to review and the caret is updated on an object.
I will open a new pr tomorrow that will complement this one and will make handleUpdate less expensive, as the major part of handleUpdate will be moved to the core pump.

[burmancomp]

As to pumpAll code it was not optimal and it did not handle all cases (at least browse mode was not properly handled). However, I am quite sure that number of redundant updates could be notably reduced with code optimization because most of time all needed updates are done.

Maybe one approach could be to check in pumpAll if update is already done during this cycle.

I think it is important that:

• whole problem (some lacking display updates) should be solved.

It is not essential if required logic is in pumpAll or elsewhere as long as it is somewhere.

Unfortunately I am somewhat thinking that this pull request should be modified to more generic before it is ready for review.

What do you think:

• is it possible to optimize pumpAll code so that it is not too heavy? I have local version which seems to somewhat handle all display update problems I have encountered during this process. Local PumpAll alternative has 10 code lines at this moment and 1 in BrailleHandler.handleUpdate. It checks if update is already done. Hopefully effects on cpu usage would be ass small as possible.
• if it is not possible with pumpAll where logic should be located and whatkind it should be?

[burmancomp]

Replaced with pull request #15163.