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

Fix cursorManager selection bugs #7131

Merged
merged 6 commits into from May 8, 2017
Merged

Fix cursorManager selection bugs #7131

merged 6 commits into from May 8, 2017

Conversation

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented May 3, 2017

  • In browse mode, pressing shift+home after selecting forward now unselects to the beginning of the line as expected. (#5746)
  • In browse mode, select all (control+a) no longer fails to select all text if the caret is not at the start of the text. (#6909)
  • cursorManager: Fix selecting forward, then selecting backward without unselecting, then unselecting forward, and vice versa. This one is hard to explain; see the unit tests for details.

This should be merged normally, not squashed.

Fixes #5746. Fixes #6909.

@jcsteh jcsteh added the p1 label May 3, 2017
@jcsteh jcsteh requested a review from feerrenrut May 3, 2017
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

This comment has been minimized.

@feerrenrut

feerrenrut May 3, 2017
Member

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.

This comment has been minimized.

@jcsteh

jcsteh May 3, 2017
Author Contributor

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"

This comment has been minimized.

@feerrenrut

feerrenrut May 3, 2017
Member

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

This comment has been minimized.

@jcsteh

jcsteh May 3, 2017
Author Contributor

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)
@@ -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):

This comment has been minimized.

@feerrenrut

feerrenrut May 4, 2017
Member

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"

This comment has been minimized.

@feerrenrut

feerrenrut May 4, 2017
Member

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.

@@ -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):

This comment has been minimized.

@feerrenrut

feerrenrut May 4, 2017
Member

This depends on test_unselectNextChar

jcsteh added a commit that referenced this pull request May 4, 2017
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 8, 2017

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

@Adriani90
Copy link
Collaborator

@Adriani90 Adriani90 commented Jul 12, 2018

Why has this pull request not been merged in 2017.2?

@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented Jul 13, 2018

@Adriani90 commented on Jul 13, 2018, 5:04 AM GMT+10:

Why has this pull request not been merged in 2017.2?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants