Fix cursorManager selection bugs #7131
Conversation
…he end of the text. (#6909)
… unselecting, then unselecting forward, and vice versa. The selection direction wasn't being changed appropriately in the second movement.
jcsteh
added
the
p2
| 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 | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -79,3 +154,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" | |||
There was a problem hiding this comment.
What kind of error message do you get when this fails?
There was a problem hiding this comment.
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)
tests/unit/test_cursorManager.py
Outdated
| @@ -47,6 +49,32 @@ def test_unselectPrevChar(self): | |||
| cm.script_selectCharacter_back(None) # "a" unselected | |||
| self.assertEqual(cm.selectionOffsets, (0, 0)) # Caret at "a", no selection | |||
|
|
|||
| def test_unselectNextChar(self): | |||
There was a problem hiding this comment.
I don't really understand the names "unselectPrevChar" and "unselectNextChar" somehow they seem the wrong way around? They might be clearer if called "selectPreviousUnsel" and "selectNextUnsel"
There was a problem hiding this comment.
In fact, the later tests use the term "forward" and "back" rather than "next" and "prev". It would be nice if we could be consistent here, I think forward and back make more sense since they are used in the name of the script_ functions.
tests/unit/test_cursorManager.py
Outdated
| @@ -57,6 +85,16 @@ def test_selForwardThenUnselThenSelBackward(self): | |||
| cm.script_selectCharacter_back(None) | |||
| self.assertEqual(cm.selectionOffsets, (0, 1)) # "a" selected | |||
|
|
|||
| def test_selBackwardThenUnselThenSelForward(self): | |||
There was a problem hiding this comment.
This depends on test_unselectNextChar
|
@michaelDCurran, @feerrenrut and I all agreed this should be merged into master before the translation freeze despite it not being fully incubated, as it it is a p1 fix which needs to be included in the release. |
|
Why has this pull request not been merged in 2017.2? |
|
@Adriani90 commented on Jul 13, 2018, 5:04 AM GMT+10:
It was. The PR is marked as merged, has its milestone set to 2017.2 and has entries in the What's New for the issues it fixes. |
This should be merged normally, not squashed.
Fixes #5746. Fixes #6909.