Skip to content

Add the ability to automatically tether to focus or review#7489

Merged
michaelDCurran merged 20 commits into
nvaccess:masterfrom
BabbageCom:i2385
Jan 15, 2018
Merged

Add the ability to automatically tether to focus or review#7489
michaelDCurran merged 20 commits into
nvaccess:masterfrom
BabbageCom:i2385

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Aug 11, 2017

Copy link
Copy Markdown
Collaborator

Link to issue number:

Fixes #2385
fixes #5237

Summary of the issue:

Currently, using the review cursor requires one to explicitly tether to review and back. See also #2385 (comment)

Description of how this pull request fixes the issue:

This adds the ability to the GUI to enable auto tethering. If set, NVDA will automatically tether to review for review cursor changes, and tethers back to focus when changing the focus or caret.

The following implementation is used, based on the proposal in #2385 (comment)

  1. Ad a new boolean option to the config spec, autoTether

  2. Replace the getter and setter for braille.BrailleHandler.tether with getTether and setTether methods. This is because setTether needs an additional keyword argument to determine whether it was an automatic or manual tether action. Of course, keep the old tether auto property for backwards compatibility, but deprecate it

  3. Always show ui.reviewMessage in braille when auto tethering is on

  4. Tether to review for several scripts in globalCommands, namely:

    • reviewMode_next
    • reviewMode_previous
    • review_currentLine
    • review_currentWord
    • review_currentCharacter
  5. Add a new shouldAutoTether keyword argument to BrailleHandler.handleGainFocus, handleCaretMove and handleReviewMove. This argument should default to True, and can be set to False in places in the code base where this is explicitly required. Of course, shouldAutoTether should be ignored if the auto tethering option is disabled globally.

After some discussion with @dkager, we decided that using the several previous/next scripts to do auto tethering is ugly. Thus, to keep things in sync nicely, auto tethering is done by handleReviewMove instead. This has the following implications:

A. handleReviewMove should not auto tether when the move is caused due to the review cursor following the focus or the caret. Since most of the review following focus is done using api.setNavigatorObject with the isFocus parameter set to True, api.setNavigatorObject should somehow communicate this to handleReviewMove. However, setNavigatorObject doesn't execute handleReviewMove itself, but executes the becomeNavigatorObject event, which executes handleReviewMove. The isFocus parameter gets lost in this process. This is fixed by creating a caching variable, eventHandler.lastReviewMoveDueToFollowing. This new variable is also set properly by api.setReviewPosition based on a new parameter isCaret. At least for setNavigatorObject, I'd rather pass this info as an event parameter, but that means we break backwards compatibility.
B. There have been some edge cases that needed to be changed in setting the navigatorObject. For example, in the explorer appmodule on the SuggestionListItem class, the navigator object is set for event_UIA_elementSelected. This call of setNavigatorObject now has the extra isFocus parameter set to True.

Testing performed:

Various internal testing, but this needs to be tested more broadly. I'd like to request an initial review first, though.

Known issues with pull request:

None I'm currently aware of, except for the implications as noted above. And, no unit tests, as I really have no idea how to do this reliably.

Changelog entry

  • Changes
    • It is no longer necessary to explicitly tether braille to focus or review, as this will happen automatically by default. (Automatically switch braille tethering between review and focus #2385)
      • Note that automatic tethering to review will only occur when using a review cursor or object navigation command. Scrolling will not activate this new behavior.

@dkager dkager left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assuming it actually works (haven't tested this myself) I think it looks quite good. Just one or two small points.

Comment thread source/NVDAObjects/__init__.py Outdated
"""Called when this object becomes the navigator object.
"""
braille.handler.handleReviewMove()
braille.handler.handleReviewMove(shouldAutoTether=not eventHandler.lastReviewMoveDueToFollowing)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This variable name isn't very clear if you don't have the surrounding context of this PR. Maybe choose another name?

Comment thread source/api.py Outdated
return globalVars.reviewPosition

def setReviewPosition(reviewPosition,clearNavigatorObject=True):
def setReviewPosition(reviewPosition,clearNavigatorObject=True, isCaret=False):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comma police. :) Also, the docstring needs updating.

Comment thread source/api.py Outdated
globalVars.reviewPositionObj=reviewPosition.obj
if clearNavigatorObject: globalVars.navigatorObject=None
braille.handler.handleReviewMove()
eventHandler.lastReviewMoveDueToFollowing = isCaret

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we be sure that this line and the one below it are only ever executed in one thread at a time?

Comment thread source/braille.py Outdated
from collections import namedtuple
import re
import scriptHandler
import eventHandler

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not too happy with another circular dependency, but I know we discussed this before and all solutions we came up with were hacky.

Comment thread source/braille.py Outdated
if not shouldAutoTether and self._tether != self.TETHER_REVIEW:
return
reviewPos = api.getReviewPosition()
#if reviewPos.obj == api.getFocusObject() and config.conf["reviewCursor"]["followFocus"]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would say just remove these two lines.

Comment thread source/config/configSpec.py Outdated
noMessageTimeout = boolean(default=false)
messageTimeout = integer(default=4,min=0,max=20)
tetherTo = string(default="focus")
autoTether= boolean(default=false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Space.

@michaelDCurran michaelDCurran 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.

I feel that auto tethering is really what we would want the majority NVDA user to be using, Therefore we should set autoTether to True by default.
But something is bothering me a little with this particular option as a checkbox. What happens if it is on, but tetherTo is set to review?
I assume autoTether in this case pretty much has no meaning?

Comment thread source/eventHandler.py
try:
return func(*args, **self.kwargs)
except TypeError:
return extensionPoints.callWithSupportedKwargs(func, *args, **self.kwargs)

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.

Please log a clear message here (perhaps warning) so that we catch these issues pretty quick and fix them.

Comment thread source/sayAllHandler.py Outdated
updater.updateCaret()
if cursor!=CURSOR_CARET or config.conf["reviewCursor"]["followCaret"]:
api.setReviewPosition(updater)
api.setReviewPosition(updater, isCaret=True)

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.

Should perhaps isCaret be only true if cursor==CURSOR_CARET?

Comment thread source/appModules/lockapp.py Outdated
def event_appModule_loseFocus(self):
if not config.conf["reviewCursor"]["followFocus"]:
api.setReviewPosition(self._oldReviewPos)
api.setReviewPosition(self._oldReviewPos, isCaret=False)

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.

As isCaret is False by default, explicitly specifying isCaret=False here is not needed, and is perhaps a little confusing. I'd prefer that anywhere in the codebase that isCaret is only ever specified if it needs to be set to true. Mirroring that of isFocus for setNavigatorObject.

@LeonarddeR

LeonarddeR commented Nov 23, 2017

Copy link
Copy Markdown
Collaborator Author

But something is bothering me a little with this particular option as a checkbox. What happens if it is on, but tetherTo is set to review?
I assume autoTether in this case pretty much has no meaning?

It still has a meaning. When tethering is set to review and auto tethering is on, the initial tethering mode is review until auto tethering triggers focus. However, I agree that this is confusing.

@dkager, @bramd, @jcsteh: thoughts? I'd suggest having three options, focus, review and auto. I recall from one of our braille discussions that @jcsteh preferred separate options for tethering mode focus/review and auto tethering on/off, because it allows you to quickly override the decision made by auto tethering. I belief that's the reason I implemented it as it is now. However, after thought about it a little more, I think i agree with Mick that it is a bit confusing from an UX perspective.

@michaelDCurran

michaelDCurran commented Nov 23, 2017 via email

Copy link
Copy Markdown
Member

@josephsl

josephsl commented Nov 23, 2017 via email

Copy link
Copy Markdown
Contributor

@jcsteh

jcsteh commented Nov 23, 2017 via email

Copy link
Copy Markdown
Contributor

@michaelDCurran

michaelDCurran commented Nov 23, 2017 via email

Copy link
Copy Markdown
Member

@jcsteh

jcsteh commented Nov 23, 2017 via email

Copy link
Copy Markdown
Contributor

@michaelDCurran

michaelDCurran commented Nov 24, 2017 via email

Copy link
Copy Markdown
Member

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@michaelDCurran commented on 24 nov. 2017 02:36 CET:

There are a few options for the tether command script:

  1. Have it toggle between auto, focus and review. This is my personal
    choice.

I agree.

  1. Leave it as is. I.e. toggles between focus and review When auto is
    off, and temporarily between focus and review when auto is on, allowing
    braille scrolling to control both review or caret, but it to snap back
    when something else occurs.

This will probably be confusing for the end user.

I'm intending to set the tetherTo config parameter to focus when the autoTether config parameter is True. We can also use the last state of tetherTo before enabling autoTether, but that will result into undefined behavior.

@bramd

bramd commented Nov 24, 2017

Copy link
Copy Markdown
Contributor

Hmm, I haven't tested this yet, but making a toggle with three options seems the clearest interface to me as well.

Some other thoughts:

Given the use case that you want to read some text in a document or in a console and type at another location in the document or console, it's also needed to disable following of the caret by the review cursor. I know it is a lot of magic, but somehow having a toggle between auto/focus/review, it would make sense to turn review mode in a real review mode without being disturbed by caret/focus changes.

@michaelDCurran

Copy link
Copy Markdown
Member

This will need an update to the user guide for the tether setting. But looks good other than that.

…would result in the caret not being followed correctly. This also applies to the braille_toFocus script
@LeonarddeR

LeonarddeR commented Dec 7, 2017

Copy link
Copy Markdown
Collaborator Author

I just pushed another commit that fixes #5237, tethering from review back to focus failed in browse mode. Also, the braille_toFocus script is now working much more reliably in auto tethering cases.

@derekriemer reported issues with tethering on Gitter, but I can't reproduce them. Note that when using object review, it might look like you are tethered to focus even though you are tethered to review. You can check this by setting focus context representation to "always fill display" which gives you a more clear view of difference between focus and review tethering.

@michaelDCurran: To be specific, there are now 3 new commits since your last review I belief.

@AAClause

AAClause commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

I have a strange behavior since this function is implemented. To reproduce it, the simplest is:

  1. Open 2 instances of cmd;
  2. Enable "braille tether to review" in one of these windows by pressing NVDA+CTRL+t. At this point, we can read the content of the terminal (i.e. result of commands) with backward and forward buttons of braille display.
  3. Now, go to the second instance. At this stage, we can't read the contents of this terminal. We are forced to reswitch in review mode By pressing 3 times on CTRL+NVDA+t to find the expected behavior.

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

That's a very nice catch there. This, among other similar bugs, should have been fixed in the branch now, and will be visible as soon as review and incubation has taken place.

michaelDCurran added a commit that referenced this pull request Dec 11, 2017
michaelDCurran added a commit that referenced this pull request Dec 11, 2017
@AAClause

Copy link
Copy Markdown
Contributor

I am running on next-14726,ebbd6f4c.
I still have bugs with scrolling in some cases (e.g. in Microsoft Word).

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@Andre9642 commented on 12 dec. 2017 17:21 CET:

I am running on next-14726,ebbd6f4c.
I still have bugs with scrolling in some cases (e.g. in Microsoft Word).

Could you please provide steps to reproduce?

@AAClause

AAClause commented Dec 14, 2017

Copy link
Copy Markdown
Contributor

Sorry for the delayed response.

Could you please provide steps to reproduce?

Unfortunately, it's complicated to reproduce and list all cases because sometimes I do not understand the steps that caused the problem...
However, I spotted a case where it happens every time.

  1. Open Word, and choose "blank document".

  2. Type the next content (it is a simple example):

    111
    222
    333
    
  3. Restart NVDA with NVDA+q (with add-ons disabled preferably). Normally, you should fall back on the document.

  4. Go to the top of document (with Control+home). Now, we can read "111" on the braille display.

  5. Try to scroll forward with your braille display.

  • Expected behavior: you should move on the next line and the braille display should shows "222".
  • Actual behavior: NVDA switch to review mode (braille cursor = dot 8), cursor has not moved and the braille display shows "111".

I reproduced this on several machines, Word 2016 and NVDA version next-14726,ebbd6f4c.

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@Andre9642 commented on 14 dec. 2017 12:31 CET:

  1. Open Word, and choose "blank document".

  2. Type the next content (it is a simple example):

    111
    222
    333
    
  3. Restart NVDA with NVDA+q (with add-ons disabled preferably). Normally, you should fall back on the document.

  4. Go to the top of document (with Control+home). Now, we can read "111" on the braille display.

  5. Try to scroll forward with your braille display.

  • Expected behavior: you should move on the next line and the braille display should shows "222".
  • Actual behavior: NVDA switch to review mode (braille cursor = dot 8), cursor has not moved and the braille display shows "111".

For me, the result always is as expected. Could you do the following at a python console:
import config; print config.conf['braille']['tetherTo']

This parameter is reset to focus when explicitly thetering automatically, however when the tether setting has not been touched after an update and it was initially set to review, auto tethering is on and the config parameter is set to review, which should actually not be the case.
@michaeldcurran/@jcsteh: I think I yet prefer a config upgrade step for this as per the reason above. Furthermore, we can also convert tethering to an option based option in the config spec instead of a string based option. Thoughts? ANother reason is that using both a string based value and the autoTether boolean together feels quite ugly to me.

@AAClause

Copy link
Copy Markdown
Contributor
>>> import config; print config.conf['braille']['tetherTo']
focus

I have another problem, I'm wondering if it's related: when I start/restart NVDA in a virtual document (eg: in Firefox, in a message in Thunderbird...), the browse mode is not enabled automatically. I am forced to leave and return to the virtual content.

@LeonarddeR

LeonarddeR commented Dec 14, 2017 via email

Copy link
Copy Markdown
Collaborator Author

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@Andre9642: Also, just to make sure, do you have any configuration profiles active that might override the tethering setting? I've looked at the code again and, apart from what I mentioned in #7489 (comment) , I can't yet think of what might cause this behavior. The fact that I can't reproduce this makes it even more difficult to deal with it properly.

@AAClause

Copy link
Copy Markdown
Contributor

@LeonarddeR I've just tested with the last master version. And... I have the same issue. I'm sorry, so this isn't related to this new feature!

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@Andre9642 commented on 14 dec. 2017 21:45 CET:

@LeonarddeR I've just tested with the last master version. And... I have the same issue. I'm sorry, so this isn't related to this new feature!

Just to make sure, which issue are you now referring to? The browse mode issue or the scrolling issue in Word?

@michaelDCurran

Copy link
Copy Markdown
Member

@LeonarddeR: Are you happy for this to be merged to master and closed? Also, can someone provide a what's new entry in the description?

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@Andre9642: Could you please answer my last question? Are the issues you were experiencing related to auto tethering or not?
@derekriemer: What do you think of this? It seems you have found some instability, even though I wasn't able to reproduce.

@AAClause

AAClause commented Jan 4, 2018

Copy link
Copy Markdown
Contributor

Are the issues you were experiencing related to auto tethering or not?

After all, no. I have the same problem with NVDA 2017.4 concerning the scrolling issue in Word.
The scroll forward issue is present especially when I scroll a lot of times in a complex document and, sometimes, when the cursor is located on the first character of the document. Tested with addons disabled. If I have more precise details, I'll create an issue.

Regarding the browse mode issue, I have some instability (very) occasionally and randomly (= hard to reproduce). I don't know if it's related to auto tethering. :/ When this happens, the Braille display no longer follows if I move with arrow keys. I couldn't get this issue with NVDA 2017.4.

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@Andre9642 commented on 4 jan. 2018 01:55 CET:

Are the issues you were experiencing related to auto tethering or not?

After all, no. I have the same problem with NVDA 2017.4 concerning the scrolling issue in Word.
The scroll forward issue is present especially when I scroll a lot of times in a complex document and, sometimes, when the cursor is located on the first character of the document. Tested with addons disabled. If I have more precise details, I'll create an issue.

Regarding the browse mode issue, I have some instability (very) occasionally and randomly (= hard to reproduce). I don't know if it's related to auto tethering. :/ When this happens, the Braille display no longer follows if I move with arrow keys. I couldn't get this issue with NVDA 2017.4.

Thanks for the clarification. May be you could answer the following questions?

  1. Do you, by any chance, know whether switching between browse mode and focus mode could cause this?
  2. What is shown on the display when it does not follow?
  3. Does the non-following start after interacting with the review cursor or object navigation, or just randomly?

@LeonarddeR

Copy link
Copy Markdown
Collaborator Author

@michaelDCurran commented on 2 jan. 2018 00:55 CET:

@LeonarddeR: Are you happy for this to be merged to master and closed? Also, can someone provide a what's new entry in the description?

I just added a what's new entry.

@michaelDCurran michaelDCurran merged commit 841ee00 into nvaccess:master Jan 15, 2018
@nvaccessAuto nvaccessAuto added this to the 2018.1 milestone Jan 15, 2018
@LeonarddeR LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BabbageWork Pull requests filed on behalf of Babbage B.V. component/braille

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tethering braille to review and back to focus fails in browse mode Automatically switch braille tethering between review and focus

8 participants