Correctly follow selection in braille when extending beyond display width by dkager · Pull Request #6775 · nvaccess/nvda

dkager · 2017-01-21T14:18:04Z

This PR fixes #5770.

What

When extending a selection beyond the braille display width, braille keeps showing the unmoving end of the selection. It thus makes it difficult to track what is actually being selected without using speech. This is also true for multi-line selections, where braille will continue to show the top line when you press Shift+downArrow.

How

This patch adds an attribute to NVDAObject to indicate whether the selection is anchored at the start. That is, when the right/bottom end of the selection is moving, the selection is anchored at the start. Otherwise it is not. This attribute is set by CursorManager and EditableText (with and without auto selection detection).

Discussion

There was some discussion about how multi-line selection should be handled. In particular, the question was what should happen when you select entire lines with Shift+downArrow.
The question can be generalized: do you want to see the next character to be selected, or should braille follow the very last character that is still inside the selection?

To avoid having to deal with corner cases, I have chosen the latter approach in this patch. This is easiest to implement, but is it intuitive? Especially when selecting entire lines it may be useful to see the next line you are about to select.

Example

Say you have the text:

Hello
World
How are you?

When you are on the top line and press Shift+downArrow, braille would show "Hello" with dots 7&8. Another Shift+downArrow shows "World" with dots 7&8, etc.

Testing

Tested so far in Notepad, WordPad, Word 2016, Notepad++, Firefox (browse mode).

CC @LeonarddeR, @tuukkao

…TreeInterceptor. The property defaults to False. Also use this to have braille scroll the selection's end into view if the anchor is at the start. re nvaccess#5770

- The copy of the braille cursor TextInfo in braille.TextInfoRegion.update(). - Duplicate call to detectPossibleSelectionChange(). This is now handled in EditableTextWithAutoSelectDetection.

…ensate for that. As a consequence, braille will always show the last selected character, both for single and multi-line selections.

dkager · 2017-01-21T14:21:41Z

 		if eventHandler.isPendingEvents('valueChange',self):
 			self.hasContentChangedSinceLastSelection=True
 		super(Edit,self).event_caret()
-		self.detectPossibleSelectionChange()


For justification of this removal, see #5770 (comment)

dkager · 2017-01-25T15:57:05Z

 				newInfo.setEndPoint(oldInfo, "startToStart")
+		# _isSelectionAnchoredAtStart is not the same as _lastSelectionMovedStart,
+		# i.e. their default values are opposite and _isSelectionAnchoredAtStart is only updated if the selection has changed.
+		self._isSelectionAnchoredAtStart = not self._lastSelectionMovedStart


@jcsteh I think we need to make sure this is compatible with 62f42c6

Did some testing and that looked good.

LeonarddeR · 2017-06-22T05:52:48Z

 		speech.speakObject(self,reason=controlTypes.REASON_FOCUS)

+	def _get_isSelectionAnchoredAtStart(self):
+		"""Determine if the selection is anchored at the start.


It might help here if you give a brief explanation of what you mean with a selection being anchored at the start.

LeonarddeR · 2017-06-22T05:58:11Z

 		if eventHandler.isPendingEvents('valueChange',self):
 			self.hasContentChangedSinceLastSelection=True
 		super(Edit,self).event_caret()
-		self.detectPossibleSelectionChange()


This is explained in #5770 (comment)

LeonarddeR · 2017-06-22T06:07:33Z

+	def scrollToCursorOrSelection(self, region):
+		if region.brailleCursorPos is not None:
+			self.mainBuffer.scrollTo(region, region.brailleCursorPos)
+		elif not hasattr(region, "obj") or not region.obj.isSelectionAnchoredAtStart:


Probably a matter of style, but instead of the hasatr, I'd do a isinstance(region,(NVDAObjectRegion,TextInfoRegion)) here

LeonarddeR · 2017-06-22T06:10:21Z

 			self.reportSelectionChange(oldInfo)
 		except:
-			return
+			return


I don't get how this one got into the diff, they look identical

dkager · 2017-06-22T09:19:27Z

@LeonarddeR I tweaked the pydoc. Also, I believe hasattr is more future-proof than hard-coding subclasses.

jcsteh

Thanks for your work. This looks pretty good overall.

jcsteh · 2017-06-23T04:55:31Z

 		"""
 		speech.speakObject(self,reason=controlTypes.REASON_FOCUS)

+	def _get_isSelectionAnchoredAtStart(self):


Could you please rename this to isTextSelectionAnchoredAtStart? It occurred to me that selection might be a bit ambiguous here; i.e. selection of items in a container vs text selection.

jcsteh · 2017-06-23T04:57:43Z


+	def _get_isSelectionAnchoredAtStart(self):
+		"""Indicates if the selection is anchored at the start.
+		The anchored position is the end that doesn't move when extending or shrinking the selection.


Perhaps you could add an example here:

For example, if you have no selection and you press shift+rightArrow to select the next character, this will return True. In contrast, if you have no selection and you press shift+leftArrow to select the previous character, this will return False.

jcsteh · 2017-06-23T05:01:49Z

+		If the selection is anchored at the end or there is no information this is C{False}.
+		@return: C{True} if the selection is anchored at the start else C{False}
+		@rtype: bool
+		"""


It strikes me as odd that we create this private variable here which gets set in a completely different class. That is, when you're reading this code, it's not clear where this gets set. I'm thinking the base implementation should just be a class variable with a default value and then the classes which implement this can either implement their own property or override with a simple instance variable.

jcsteh · 2017-06-23T05:08:48Z

+			# Collapse the selection to the point that is moving.
+			readingInfo.collapse(end=self.obj.isSelectionAnchoredAtStart)
+			# Get the reading unit at the end of the selection.
+			readingInfo.expand(unit)


As noted in the comment that was previously here:

# HACK: Some TextInfos only support UNIT_LINE properly if they are based on POSITION_CARET, # so use the original cursor TextInfo for line and copy for cursor.

I think this is going to break for certain TextInfos. Affected implementations will only let you get the line if you're working with the caret or selection range. Once you copy the range, that ability is lost. This used to be true for Word, though I think we've since rectified that. However, I'm pretty certain this is still true for MSHTML. Long story short: let readingInfo be the TextInfo you create from POSITION_SELECTION and then copy it for sel.

jcsteh · 2017-06-23T05:17:05Z

 				newInfo.setEndPoint(oldInfo, "startToStart")
+		# _isSelectionAnchoredAtStart is not the same as _lastSelectionMovedStart,
+		# i.e. their default values are opposite and _isSelectionAnchoredAtStart is only updated if the selection has changed.
+		self._isSelectionAnchoredAtStart = not self._lastSelectionMovedStart


I get that they're reversed, but I don't follow the bit about it only being updated if the selection changes. Would it really matter if isSelectionAnchoredAtStart just always returned the reverse of _lastSelectionMovedStart? I'm thinking we should get rid of _lastSelectionMovedStart and just flip everything to use the new attribute.

It looks like this was valid before the refactor a while ago, but not anymore.

On second thought there is a difference. But maybe it is actually the desired behavior.
Flipping around _lastSelectionMovedStart to _isTextSelectionAnchoredAtStart assumes that the end of the seletion is moving unless proven otherwise. This means that if the anchor position is unknown, braille will default to showing the end of the selection.
I'll give it a try and push an update when I'm satisfied.

jcsteh · 2017-06-23T05:20:26Z

 					review.setCurrentMode('document',True)
 			braille.handler.handleGainFocus(self)

+	def _get_isSelectionAnchoredAtStart(self):


This should be in DocumentTreeInterceptor, since text is not valid for non-document TreeInterceptors. Only DocumentTreeInterceptors have makeTextInfo.

Same suggested changes as for this property in NVDAObject.

This should be in DocumentTreeInterceptor, since text is not valid for non-document TreeInterceptors.

I believe I put it in the superclass to avoid having to check for that specific subclass when using this attribute. See my previous comment. This would of course not be necessary if TextInfoRegion is always paired with a DocumentTreeInterceptor, but the documentation doesn't make that clear.

jcsteh · 2017-06-23T05:23:11Z

@dkager commented on 22 Jun 2017, 19:19 GMT+10:

Also, I believe hasattr is more future-proof than hard-coding subclasses.

More future proof, but potentially less clear and more error prone. I think i agree here; I'd prefer to see this as an isinstance. Note that you'll need to test for DocumentTreeInterceptor, not just TreeInterceptor.

dkager · 2017-06-23T09:16:35Z

With regard to hasattr versus isinstance, if I understand correctly we would end up with code like:

		elif not (isinstance(region, NVDAObjectRegion) or (isinstance(region, TextInfoRegion) and isinstance(region.obj, DocumentTreeInterceptor))) or not region.obj.isTextSelectionAnchoredAtStart:

I'm not sure I find that clearer. :)

LeonarddeR · 2017-06-23T12:59:08Z

  elif not (isinstance(region, NVDAObjectRegion) or (isinstance(region, TextInfoRegion) and isinstance(region.obj, DocumentTreeInterceptor))) or not region.obj.isTextSelectionAnchoredAtStart:

This is identical to

  elif not (isinstance(region, NVDAObjectRegion) or (isinstance(region, TextInfoRegion) and isinstance(region.obj, DocumentTreeInterceptor)) or region.obj.isTextSelectionAnchoredAtStart):

dkager · 2017-06-23T13:03:47Z

@LeonarddeR You mean equivalent.
And yes, it is. I find both mind-boggling, haha.

dkager · 2017-06-23T15:17:33Z

+			self._readingInfo = readingInfo = sel.copy()
+			if self.obj.isSelectionAnchoredAtStart:
+				# The end of the range is exclusive, so make it inclusive first.
+				readingInfo.move(textInfos.UNIT_CHARACTER, -1, "end")


I'm still not happy about this. For a downward multi-line selection it is more practical to see the next line to be selected rather than the very right edge of the last selected line. But there is no good way to detect the difference between the result of pressing shift+downArrow and shift+end. The only solution I know of is to rely on cursor position even when there is a selection, but that doesn't work in all applications.

So there are two options:

Rely on the cursor where possible. Provides great usability in some programs (Notepad++, Eclipse) and average usability in others (Notepad, Word, browse mode).

Don't try to be clever and always show the very edge of a selection. This is consistent and predictable, but usability is so-so.

dkager

Addressed comments from James and Leonard and made some small improvements.

dkager · 2017-06-24T12:45:10Z

 			newInfo=self.makeTextInfo(toPosition)
 			if oldInfo.isCollapsed:
-				self._lastSelectionMovedStart = newInfo.compareEndPoints(oldInfo, "startToStart") < 0
+				self.isTextSelectionAnchoredAtStart = newInfo.compareEndPoints(oldInfo, "startToStart") == 0


@jcsteh Could you please double check this line?

newInfo is usually a collapsed destination position. Its start isn't yet set to the cursor. For example, if you press control+end, newInfo will be collapsed at the last character in the document at this point in the code. So, I think this should be an exact reversal; i.e. this should be >= 0. Having said that, I can't seem to figure out right now how this would actually break things. It seems to work fine, I guess because it gets set later. Still, I think I'd be more comfortable with an exact reversal, but do run the unit tests to make sure I'm not missing something. :)

dkager · 2017-06-24T12:46:12Z

 				# Starting a new selection, so set the selection direction
 				# based on the direction of this movement.
-				self._lastSelectionMovedStart = direction < 0
+				self.isTextSelectionAnchoredAtStart = direction > 0


@jcsteh Could you double check this line too?

This one looks fine, since direction can't be 0 if unit is not specified.

dkager · 2017-06-25T14:53:03Z

+	def scrollToCursorOrSelection(self, region):
+		if region.brailleCursorPos is not None:
+			self.mainBuffer.scrollTo(region, region.brailleCursorPos)
+		elif not isinstance(region, (NVDAObjectRegion, TextInfoRegion)) or not region.obj.isTextSelectionAnchoredAtStart:


Motivation for this test:

If a region is an NVDAObjectRegion it will have an NVDAObject, which has the isTextSelectionAnchoredAtStart attribute.

If a region is a TextInfoRegion then it has a TextInfo and it will be based on a TreeInterceptor. DocumentTreeInterceptor is the only TreeInterceptor with a TextInfo, so the region must have one of those as its object. This also means the selection anchor attribute is present.

Unless I'm missing something, I think selection anchoring is only relevant for TextInfoRegion. Even if we're dealing with an NVDAObject (not a DocumentTreeInterceptor), if it has text, it'll also have a TextInfoRegion... and it is this region that we'll be dealing with. So, I think you only need to test for TextInfoRegion.

jcsteh · 2017-06-29T07:00:59Z

+	def scrollToCursorOrSelection(self, region):
+		if region.brailleCursorPos is not None:
+			self.mainBuffer.scrollTo(region, region.brailleCursorPos)
+		elif not isinstance(region, (NVDAObjectRegion, TextInfoRegion)) or not region.obj.isTextSelectionAnchoredAtStart:


Unless I'm missing something, I think selection anchoring is only relevant for TextInfoRegion. Even if we're dealing with an NVDAObject (not a DocumentTreeInterceptor), if it has text, it'll also have a TextInfoRegion... and it is this region that we'll be dealing with. So, I think you only need to test for TextInfoRegion.

jcsteh · 2017-06-29T07:19:52Z

 			newInfo=self.makeTextInfo(toPosition)
 			if oldInfo.isCollapsed:
-				self._lastSelectionMovedStart = newInfo.compareEndPoints(oldInfo, "startToStart") < 0
+				self.isTextSelectionAnchoredAtStart = newInfo.compareEndPoints(oldInfo, "startToStart") == 0


newInfo is usually a collapsed destination position. Its start isn't yet set to the cursor. For example, if you press control+end, newInfo will be collapsed at the last character in the document at this point in the code. So, I think this should be an exact reversal; i.e. this should be >= 0. Having said that, I can't seem to figure out right now how this would actually break things. It seems to work fine, I guess because it gets set later. Still, I think I'd be more comfortable with an exact reversal, but do run the unit tests to make sure I'm not missing something. :)

jcsteh · 2017-06-29T07:21:24Z

 				# Starting a new selection, so set the selection direction
 				# based on the direction of this movement.
-				self._lastSelectionMovedStart = direction < 0
+				self.isTextSelectionAnchoredAtStart = direction > 0


This one looks fine, since direction can't be 0 if unit is not specified.

jcsteh · 2017-06-30T00:32:27Z

 		self.hasContentChangedSinceLastSelection=False
 		speech.speakSelectionChange(oldInfo,newInfo,generalize=hasContentChanged)

+	def _findSelectionAnchor(self,oldInfo,newInfo):


There's a slight chance someone might see this method name and expect it to return something, rather than taking action. I think renaming this to _updateSelectionAnchor or similar might make this clearer.

jcsteh · 2017-06-30T00:35:09Z

+				sel.setEndPoint(readingInfo, "endToEnd")
+		else:
+			# If there is no selection, use the cursor.
+			self._readingInfo = readingInfo = self._getCursor()


Sorry; I missed this in my first round of review. If there's no selection, POSITION_SELECTION will be collapsed at the cursor, so this is redundant. That also suggests we should get rid of _getCursor altogether. ReviewTextInfoRegion will need to be updated to just return the review position for _getSelection.

Does that makes sense to you, semantically speaking? If it does then I'm happy to change it.

Would you discard POSITION_CARET? Intuitively, the following order looks good to me:

POSITION_SELECTION

POSITION_CARET

POSITION_FIRST

Where POSITION_FIRST seems a poor fallback, but better than nothing I guess.

…ere is no selction, _getSelection() returns the cursor.

dkager · 2017-07-01T12:05:41Z

I think all review comments have been resolved.
My only comment now is that it seems slightly wasteful to pass a collapsed TextInfo to _addTextWithFields. All it does is establish where the cursor is located. Still, it looks like splitting the reading unit into chunks can't be avoided. So the extra overhead should be small enough.

jcsteh · 2017-07-03T03:08:28Z

@dkager commented on 1 Jul 2017, 22:05 GMT+10:

My only comment now is that it seems slightly wasteful to pass a collapsed TextInfo to _addTextWithFields.

I assume you're referring to the case where there's no selection, only a cursor. It's true that this will call getTextWithFields unnecessarily. I guess we could pass in an additional hint specifying that the TextInfo is collapsed (since we already checked that) and thus eliminate this call, but this code is already complex enough, so I'd prefer to avoid that unless we can prove there's a noticeable perf improvement. In any case, if we're going to do that, let's deal with that later. :)

jcsteh

Last tiny change. Thanks for your work and your patience. :)

jcsteh · 2017-07-03T03:12:30Z

 		except:
+#			try:
+#				return self.obj.makeTextInfo(textInfos.POSITION_CARET)
+#			except:


Please remove these commented lines. To answer your question, you can discard POSITION_CARET because POSITION_SELECTION should return the caret if there's no selection. Speech relies on this assumption as well.

…idth. Incubates #6775 (issue #5770).

jcsteh · 2017-07-10T07:00:36Z

This is now conflicting due to a recent merge to master. Would you mind resolving? Thanks.

dkager · 2017-07-10T09:00:41Z

Should be fixed.

Issues have been addressed and superseded by my subsequent review.

dkager added 7 commits December 4, 2016 13:40

Store whether a selection is anchored at the start in NVDAObject and …

8b19364

…TreeInterceptor. The property defaults to False. Also use this to have braille scroll the selection's end into view if the anchor is at the start. re nvaccess#5770

CursorManager: set the selection anchor position.

d0c28ed

Remove some code that is no longer needed:

e5a626f

- The copy of the braille cursor TextInfo in braille.TextInfoRegion.update(). - Duplicate call to detectPossibleSelectionChange(). This is now handled in EditableTextWithAutoSelectDetection.

More fiddling, this needs review.

abd8555

Merge branch 'master' into i5770

a45639a

Fixup.

28d57f1

Fix multi-line selection. The end of a TextInfo is exclusive, so comp…

762f64c

…ensate for that. As a consequence, braille will always show the last selected character, both for single and multi-line selections.

dkager commented Jan 21, 2017

View reviewed changes

dkager added 2 commits January 21, 2017 15:29

Simplification: getattr with default value.

a424d9d

Merge branch 'master' into i5770

49d7c28

dkager commented Jan 25, 2017

View reviewed changes

dkager added 2 commits May 16, 2017 16:18

Merge branch 'master' into i5770

00d05be

Merge branch 'master' into i5770

f4646a9

LeonarddeR mentioned this pull request Jun 8, 2017

review position isn't updated when moving through edit field with shift key pressed #250

Closed

LeonarddeR previously requested changes Jun 22, 2017

View reviewed changes

dkager added 2 commits June 22, 2017 09:40

Merge branch 'master' into i5770

b9f4449

Review actions.

ade7f20

jcsteh requested changes Jun 23, 2017

View reviewed changes

dkager commented Jun 23, 2017

View reviewed changes

dkager added 3 commits June 24, 2017 13:18

Review actions.

ebfdcba

Assume that a text selection is anchored at the start by default.

97ae45e

Unify CursorManager text selection anchor attributes and add unit tests.

8968496

dkager commented Jun 24, 2017

View reviewed changes

dkager commented Jun 25, 2017

View reviewed changes

jcsteh requested changes Jun 30, 2017

View reviewed changes

dkager added 2 commits June 30, 2017 10:56

Review actions.

1884308

Braille optimization: combine _getCursor() and _getSelection(). If th…

6813828

…ere is no selction, _getSelection() returns the cursor.

jcsteh requested changes Jul 3, 2017

View reviewed changes

Forgot to remove some commented-out code.

0e950d3

jcsteh added a commit that referenced this pull request Jul 4, 2017

Correctly follow selection in braille when extending beyond display w…

608518b

…idth. Incubates #6775 (issue #5770).

nvaccessAuto added the incubating label Jul 4, 2017

nvaccessAuto assigned jcsteh Jul 4, 2017

jcsteh approved these changes Jul 4, 2017

View reviewed changes

Merge branch 'master' into i5770

e7dc793

jcsteh added a commit that referenced this pull request Jul 11, 2017

Merge PR #6775 to resolve conflicts from recent merge to master.

77980a5

jcsteh merged commit a65d498 into nvaccess:master Aug 1, 2017

nvaccessAuto removed the incubating label Aug 1, 2017

nvaccessAuto added this to the 2017.3 milestone Aug 1, 2017

jcsteh mentioned this pull request Aug 1, 2017

Braille trace to update selected line message #4682

Closed

Uh oh!

Conversation

dkager commented Jan 21, 2017

What

How

Discussion

Example

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkager commented Jun 22, 2017

Uh oh!

jcsteh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsteh commented Jun 23, 2017

Uh oh!

dkager commented Jun 23, 2017

Uh oh!

LeonarddeR commented Jun 23, 2017

Uh oh!

dkager commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkager left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment