Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from Jan 15, 2018

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Aug 11, 2017

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.

Copy link
Collaborator

@dkager dkager left a comment

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.

@@ -961,7 +961,7 @@ def event_foreground(self):
def event_becomeNavigatorObject(self):
"""Called when this object becomes the navigator object.
"""
braille.handler.handleReviewMove()
braille.handler.handleReviewMove(shouldAutoTether=not eventHandler.lastReviewMoveDueToFollowing)
Copy link
Collaborator

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?

source/api.py Outdated
@@ -177,13 +177,14 @@ def getReviewPosition():
globalVars.reviewPosition,globalVars.reviewPositionObj=review.getPositionForCurrentMode(obj)
return globalVars.reviewPosition

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

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.

source/api.py Outdated
"""Sets a TextInfo instance as the review position. if clearNavigatorObject is true, It sets the current navigator object to None so that the next time the navigator object is asked for it fetches it from the review position.
"""
globalVars.reviewPosition=reviewPosition.copy()
globalVars.reviewPositionObj=reviewPosition.obj
if clearNavigatorObject: globalVars.navigatorObject=None
braille.handler.handleReviewMove()
eventHandler.lastReviewMoveDueToFollowing = isCaret
Copy link
Collaborator

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?

@@ -28,6 +28,7 @@
from collections import namedtuple
import re
import scriptHandler
import eventHandler
Copy link
Collaborator

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.

return
reviewPos = api.getReviewPosition()
#if reviewPos.obj == api.getFocusObject() and config.conf["reviewCursor"]["followFocus"]:
Copy link
Collaborator

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.

@@ -61,6 +61,7 @@
noMessageTimeout = boolean(default=false)
messageTimeout = integer(default=4,min=0,max=20)
tetherTo = string(default="focus")
autoTether= boolean(default=false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Space.

Copy link
Member

@michaelDCurran michaelDCurran left a comment

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?

try:
return func(*args, **self.kwargs)
except TypeError:
return extensionPoints.callWithSupportedKwargs(func, *args, **self.kwargs)
Copy link
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.

@@ -151,7 +151,7 @@ def readTextHelper_generator(cursor):
if cursor==CURSOR_CARET:
updater.updateCaret()
if cursor!=CURSOR_CARET or config.conf["reviewCursor"]["followCaret"]:
api.setReviewPosition(updater)
api.setReviewPosition(updater, isCaret=True)
Copy link
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?


def event_appModule_loseFocus(self):
if not config.conf["reviewCursor"]["followFocus"]:
api.setReviewPosition(self._oldReviewPos)
api.setReviewPosition(self._oldReviewPos, isCaret=False)
Copy link
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
Copy link
Collaborator Author

LeonarddeR commented Nov 23, 2017

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
Copy link
Member

michaelDCurran commented Nov 23, 2017 via email

@josephsl
Copy link
Collaborator

josephsl commented Nov 23, 2017 via email

@jcsteh
Copy link
Contributor

jcsteh commented Nov 23, 2017 via email

@michaelDCurran
Copy link
Member

michaelDCurran commented Nov 23, 2017 via email

@jcsteh
Copy link
Contributor

jcsteh commented Nov 23, 2017 via email

@michaelDCurran
Copy link
Member

michaelDCurran commented Nov 24, 2017 via email

@LeonarddeR
Copy link
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
Copy link
Contributor

bramd commented Nov 24, 2017

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
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
Copy link
Collaborator Author

LeonarddeR commented Dec 7, 2017

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
Copy link
Contributor

aaclause commented Dec 8, 2017

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
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
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
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
Copy link
Contributor

aaclause commented Dec 14, 2017

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
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
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
Copy link
Collaborator Author

LeonarddeR commented Dec 14, 2017 via email

@LeonarddeR
Copy link
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
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
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
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
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
Copy link
Contributor

aaclause commented Jan 4, 2018

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
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
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
8 participants