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.

Sign up for GitHub

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

Jump to bottom

Fix cursorManager selection bugs #7131

Merged
merged 6 commits into from May 8, 2017
Merged

Fix cursorManager selection bugs #7131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4d91259
Add tests for other selection behavior in cursorManager.
jcsteh May 3, 2017
21c18e0
cursorManager: Fix select all when the caret is in the middle or at t…
jcsteh May 3, 2017
11b3136
cursorManager: Fix selecting forward, then selecting backward without…
jcsteh May 3, 2017
28fe5b8
cursorManager: Fix unselecting to the beginning of the line. (#5746)
jcsteh May 3, 2017
8b662c7
Add additional tests.
jcsteh May 3, 2017
0eda1cb
Rename tests for consistency.
jcsteh May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 16 additions & 8 deletions source/cursorManager.py
View file
Open in desktop
@@ -1,6 +1,6 @@
#cursorManager.py
#A part of NonVisual Desktop Access (NVDA)
#Copyright (C) 2006-2016 NV Access Limited, Joseph Lee, Derek Riemer
#Copyright (C) 2006-2017 NV Access Limited, Joseph Lee, Derek Riemer
#This file is covered by the GNU General Public License.
#See the file COPYING for more details.

Expand Down Expand Up @@ -256,28 +256,34 @@ def _selectionMovementScriptHelper(self,unit=None,direction=None,toPosition=None
newInfo.move(unit, direction, endPoint="start" if direction < 0 else "end")
# Collapse this so we don't have to worry about which endpoint we used here.
newInfo.collapse(end=direction > 0)
if self._lastSelectionMovedStart:
# If we're selecting all, we're moving both endpoints.
# Otherwise, newInfo is the collapsed new active endpoint
# and we need to set the anchor endpoint.
movingSingleEndpoint = toPosition != textInfos.POSITION_ALL
if movingSingleEndpoint and self._lastSelectionMovedStart:
if newInfo.compareEndPoints(oldInfo, "startToEnd") > 0:
# We were selecting backwards, but now we're selecting forwards.
# For example:
# 1. Caret at 1
# 2. Shift+leftArrow: selection (0, 1)
# 3. Shift+control+rightArrow: next word at 3, so selection (1, 3)
newInfo.setEndPoint(oldInfo, "startToEnd")
self._lastSelectionMovedStart = False
else:
# We're selecting backwards.
# For example:
# 1. Caret at 1; selection (1, 1)
# 2. Shift+leftArrow: selection (0, 1)
newInfo.setEndPoint(oldInfo, "endToEnd")
else:
elif movingSingleEndpoint:
if newInfo.compareEndPoints(oldInfo, "startToStart") < 0:
# We were selecting forwards, but now we're selecting backwards.
# For example:
# 1. Caret at 1
# 2. Shift+rightArrow: selection (1, 2)
# 3. Shift+control+leftArrow: previous word at 0, so selection (0, 1)
newInfo.setEndPoint(oldInfo, "endToStart")
self._lastSelectionMovedStart = True
else:
# We're selecting forwards.
# For example:
Expand Down Expand Up @@ -318,11 +324,13 @@ def script_selectParagraph_back(self, gesture):
self._selectionMovementScriptHelper(unit=textInfos.UNIT_PARAGRAPH, direction=-1)

def script_selectToBeginningOfLine(self,gesture):
curInfo=self.makeTextInfo(textInfos.POSITION_SELECTION)
curInfo.collapse()
tempInfo=curInfo.copy()
tempInfo.expand(textInfos.UNIT_LINE)
if curInfo.compareEndPoints(tempInfo,"startToStart")>0:
# Make sure the active endpoint of the selection is after the start of the line.
sel=self.makeTextInfo(textInfos.POSITION_SELECTION)
line=sel.copy()
line.collapse()
line.expand(textInfos.UNIT_LINE)
compOp="startToStart" if self._lastSelectionMovedStart else "endToStart"
if sel.compareEndPoints(line,compOp)>0:
self._selectionMovementScriptHelper(unit=textInfos.UNIT_LINE,direction=-1)

def script_selectToEndOfLine(self,gesture):
Expand Down
134 changes: 127 additions & 7 deletions tests/unit/test_cursorManager.py
View file
Open in desktop
Expand Up @@ -29,43 +29,157 @@ def test_prevChar(self):

class TestSelection(unittest.TestCase):

def test_selectNextChar(self):
def test_selForward(self):
cm = CursorManager(text="abc") # Caret at "a"
cm.script_selectCharacter_forward(None)
self.assertEqual(cm.selectionOffsets, (0, 1)) # "a" selected

def test_selectPrevChar(self):
def test_selBackward(self):
"""Same as test_selForward, but with reversed direction.
"""
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectCharacter_back(None)
self.assertEqual(cm.selectionOffsets, (0, 1)) # "a" selected

def test_unselectPrevChar(self):
"""Depends on behavior tested by test_selectNextChar.
def test_selForwardThenUnsel(self):
"""Depends on behavior tested by test_selForward.
"""
cm = CursorManager(text="abc") # Caret at "a"
cm.script_selectCharacter_forward(None) # "a" selected
cm.script_selectCharacter_back(None) # "a" unselected
self.assertEqual(cm.selectionOffsets, (0, 0)) # Caret at "a", no selection

def test_selBackwardThenUnsel(self):
"""Depends on behavior tested by test_selBackward.
Same as test_selForwardThenUnsel, but with reversed directions.
"""
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectCharacter_back(None) # "a" selected
cm.script_selectCharacter_forward(None) # "a" unselected
self.assertEqual(cm.selectionOffsets, (1, 1)) # Caret at "b", no selection

def test_selForwardTwice(self):
"""Depends on behavior tested in test_selForward.
"""
cm = CursorManager(text="abc") # Caret at "a"
cm.script_selectCharacter_forward(None) # "a" selected
cm.script_selectCharacter_forward(None) # "b" selected
self.assertEqual(cm.selectionOffsets, (0, 2)) # "ab" selected

def test_selBackwardTwice(self):
"""Depends on behavior tested in test_selBackward.
Same as test_selForwardTwice, but with reversed directions.
"""
cm = CursorManager(text="abc", selection=(2, 2)) # Caret at "c"
cm.script_selectCharacter_back(None) # "b" selected
cm.script_selectCharacter_back(None) # "a" selected
self.assertEqual(cm.selectionOffsets, (0, 2)) # "ab" selected

def test_selForwardThenUnselThenSelBackward(self):
"""Test selecting forward, then unselecting and selecting backward.
Depends on behavior tested by test_unselectPrevChar.
Depends on behavior tested by test_selForwardThenUnsel.
"""
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectCharacter_forward(None) # "b" selected
cm.script_selectCharacter_back(None) # "b" unselected, caret at "b"
cm.script_selectCharacter_back(None)
self.assertEqual(cm.selectionOffsets, (0, 1)) # "a" selected

def test_selectForwardThenSelBackward(self):
def test_selBackwardThenUnselThenSelForward(self):
"""Test selecting backward, then unselecting and selecting forward.
Depends on behavior tested by test_selBackwardThenUnsel.
Same as test_selForwardThenUnselThenSelBackward, but with reversed directions.
"""
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectCharacter_back(None) # "a" selected
cm.script_selectCharacter_forward(None) # "a" unselected, caret at "b"
cm.script_selectCharacter_forward(None)
self.assertEqual(cm.selectionOffsets, (1, 2)) # "b" selected

def test_selForwardThenSelBackward(self):
"""Test selecting forward, then selecting backward without unselecting.
Depends on behavior tested by test_selectNextChar.
Depends on behavior tested by test_selForward.
"""
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectCharacter_forward(None) # "b" selected
cm.script_selectWord_back(None) # "b" unselected, "a" selected
self.assertEqual(cm.selectionOffsets, (0, 1)) # "a" selected

def test_selBackwardThenSelForward(self):
"""Test selecting backward, then selecting forward without unselecting.
Same as test_selForwardThenSelBackward, but with reversed directions.
"""
cm = CursorManager(text="abc", selection=(2, 2)) # Caret at "c"
cm.script_selectCharacter_back(None) # "b" selected
cm.script_selectWord_forward(None) # "b" unselected, "c" selected
self.assertEqual(cm.selectionOffsets, (2, 3)) # "c" selected

def test_selForwardThenSelBackwardThenUnsel(self):
"""Test selecting forward, then selecting backward without unselecting, then unselecting forward.
Depends on behavior tested by test_selForwardThenSelBackward.
"""
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectCharacter_forward(None) # "b" selected
cm.script_selectWord_back(None) # "b" unselected, "a" selected
cm.script_selectCharacter_forward(None) # "a" unselected
self.assertEqual(cm.selectionOffsets, (1, 1)) # Caret at "b", no selection

def test_selBackwardThenSelForwardThenUnsel(self):
"""Test selecting backward, then selecting forward without unselecting, then unselecting backward.
Same as test_selForwardThenSelBackwardThenUnsel, but with reversed directions.
Depends on behavior tested by test_selBackwardThenSelForward.
"""
cm = CursorManager(text="abc", selection=(2, 2)) # Caret at "c"
cm.script_selectCharacter_back(None) # "b" selected
cm.script_selectWord_forward(None) # "b" unselected, "c" selected
cm.script_selectCharacter_back(None) # "c" unselected
self.assertEqual(cm.selectionOffsets, (2, 2)) # Caret at "c", no selection

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no coverage for selectBackwardThenUnselThenSelForward but there is for selectForwardThenUnselThenSelBackward?

Can these tests be broken down? Is there state that can only be achieved by using the script methods? For instance, instead of testing as in test_selectBackwardThenSelForwardThenUnsel, instead have three tests (using a naming scheme of given_do_expect:

test_caretAtC_SelectCharBack_bSelected

cm = CursorManager(text="abc", selection=(2, 2)) # Caret at "c"
cm.script_selectCharacter_back(None) # "b" selected
self.assertEqual(cm.selectionOffsets, (1, 2)) # 'b' selected

test_bSelected_selectWordForward_cSelected

cm = CursorManager(text="abc", selection=(1, 2)) #'b' selected
cm.script_selectWord_forward(None) # "b" unselected, "c" selected
self.assertEqual(cm.selectionOffsets, (2, 3)) # 'c' selected

test_cSelected_selectCharacterBack_NoSelection

cm = CursorManager(text="abc", selection=(2, 3)) #'c' selected
cm.script_selectCharacter_back(None) # "c" unselected
self.assertEqual(cm.selectionOffsets, (2, 2)) # Caret at "c", no selection

Perhaps there is a reason we cant do this, like some state on CursorManager that we can not set through the constructor. But if we can I think this makes it easier to verify good test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no coverage for selectBackwardThenUnselThenSelForward but there is for selectForwardThenUnselThenSelBackward ?

This was an oversight when I wrote the original batch of tests. I shall fix.

Can these tests be broken down? Is there state that can only be achieved by using the script methods?

Yes; there is state here that is only set by the methods. I could set it manually for testing; it's just a boolean, though it's not something that should normally be tweaked by a caller. However, a big part of what we're testing here is that the state gets set correctly throughout these various movements. I'm concerned we might miss something if we initialise state directly, especially since it's already somewhat hard to grasp how one might get into various states.

def test_selToBottom(self):
cm = CursorManager(text="abc", selection=(1, 1)) # Caret at "b"
cm.script_selectToBottomOfDocument(None)
self.assertEqual(cm.selectionOffsets, (1, 3)) # "bc" selected

def test_selToTop(self):
cm = CursorManager(text="abc", selection=(2, 2)) # Caret at "c"
cm.script_selectToTopOfDocument(None)
self.assertEqual(cm.selectionOffsets, (0, 2)) # "ab" selected

def test_selToEndOfLine(self):
cm = CursorManager(text="ab\ncd", selection=(1, 1)) # Caret at "b"
cm.script_selectToEndOfLine(None)
self.assertEqual(cm.selectionOffsets, (1, 3)) # "b\n" selected

def test_selToBeginningOfLine(self):
cm = CursorManager(text="ab\ncd", selection=(4, 4)) # Caret at "d"
cm.script_selectToBeginningOfLine(None)
self.assertEqual(cm.selectionOffsets, (3, 4)) # "c" selected

def test_selToBeginningOfLineAtBeginning(self):
"""Test selecting to the beginning of the line when the caret is already at the beginning of the line.
In this case, nothing should happen.
"""
cm = CursorManager(text="ab\ncd", selection=(3, 3)) # Caret at "c"
cm.script_selectToBeginningOfLine(None)
self.assertEqual(cm.selectionOffsets, (3, 3)) # No selection

def test_selForwardThenSelToBeginningOfLine(self):
"""Depends on behavior tested by test_selForward.
"""
cm = CursorManager(text="ab\ncd", selection=(3, 3)) # Caret at "c"
cm.script_selectCharacter_forward(None) # "c" selected
cm.script_selectToBeginningOfLine(None) # "c" unselected
self.assertEqual(cm.selectionOffsets, (3, 3)) # Caret at "c", no selection

def test_selToEndThenBeginningOfLine(self):
"""Test for #5746.
Depends on behavior tested in test_selToEndOfLine and test_selToBeginningOfLine.
"""
cm = CursorManager(text="ab") # Caret at "a"
cm.script_selectToEndOfLine(None)
cm.script_selectToBeginningOfLine(None)
self.assertEqual(cm.selectionOffsets, (0, 0)) # Caret at "a", no selection

class TestSelectAll(unittest.TestCase):
"""Tests the select all command starting from different caret positions.
"""
Expand All @@ -79,3 +193,9 @@ def _selectAllTest(self, caret):

def test_selectAllFromStart(self):
self._selectAllTest(0) # Caret at "a"

def test_selectAllFromMiddle(self):
self._selectAllTest(1) # Caret at "b"
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of error message do you get when this fails?

Copy link
Contributor Author

@jcsteh jcsteh May 3, 2017 •
edited

Choose a reason for hiding this comment

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

Reverting the fix, we get this:

FAIL: test_selectAllFromMiddle (tests.unit.test_cursorManager.TestSelectAll)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\jamie\src\nvda\tests\unit\test_cursorManager.py", line 169, in test_selectAllFromMiddle
    self._selectAllTest(1) # Caret at "b"
  File "C:\Users\jamie\src\nvda\tests\unit\test_cursorManager.py", line 163, in _selectAllTest
    self.assertEqual(cm.selectionOffsets, (0, 3)) # "abc" selected
AssertionError: Tuples differ: (0, 1) != (0, 3)


def test_selectAllFromEnd(self):
self._selectAllTest(2) # Caret at "c"