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

Problems selecting text with [shift]+[left arrow] #209

Closed
jrmhaig opened this Issue Dec 19, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@jrmhaig
Copy link

jrmhaig commented Dec 19, 2016

When I attempt to select text using shift + left arrow I cannot select more than one character.

  • The first left arrow does not move the cursor but selects the previous character
  • The second left arrow moved the cursor left and cancels the selection

This appears only to happen when the first character to the left of the cursor is alphanumeric. If the cursor is placed after a non-alphanumeric character (full stop, bracket, quote mark, etc) the selection works as expected, even continuing through other alphanumeric characters.

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 19, 2016

@jrmhaig

This comment has been minimized.

Copy link
Author

jrmhaig commented Dec 19, 2016

Oh, I meant to say that this is on Mac 10.9.5 (Mavericks). Thanks for confirming that it is not platform specific.

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 19, 2016

@carlosperate

This comment has been minimized.

Copy link
Member

carlosperate commented Dec 19, 2016

This is likely a bug introduced by https://github.com/mu-editor/mu/pull/164/files
It listens to the text selection to highlight any matched items in the rest of the text.
There is also a bug I need to solve where the scroll position is not saved/restored after the search.

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 19, 2016

@carlosperate Agreed; I'll try to make time to work on this, unless you prefer to pick it up yourself...?

@carlosperate

This comment has been minimized.

Copy link
Member

carlosperate commented Dec 19, 2016

Today I wanted to finish up some work I did last week on an exception hook for logging, if I don't wrap it up soon I'll ended up forgetting about it. The rest of the week I am not completely sure how much free time I might have, so if you could pick this up it would fantastic, thanks Tim!

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 19, 2016

A quick update in case I run out of time to carry on: the problem seems to be that setSelection moves the text cursor to the end of the selection. When you select, the highlight code takes over and attempts to mark all matching selections, using the built-in find/search which always highlights its matches. To restore things afterwards, it uses setSelection to restore the original selection. But that code appears to set the text cursor to the right-hand end of the selection

I'm just about to dive into the various levels of source code to see if I can confirm this. Unfortunately, the obvious workaround of simply setting the text cursor position afterwards seems to undo the current selection so they've got you coming and going!

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 20, 2016

@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 20, 2016

@tjguk you're a star!

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 21, 2016

I've been working and reworking some tests. But I can't find anything written down around approaches to testing so I'll ask here and we'll see what we get...

We're using mocking a lot in the interface tests but, at least in the areas I'm testing, I can't see why we don't just invoke the editor's functionality and let it do its thing. It's not destructive; I don't expect it to take a particularly long time; and it seems to me to make it easier to reason about the test.

As things stand, in some cases we're at risk of testing the implementation rather than the functionality as we try to use MagicMock to count function calls etc.

In the case of this particular bug, I don't think it's possible to test it without actually exercising the editor itself since we're looking at an unexpected interaction between our logic and the editor's functionality.

Clearly mocking is a useful tool, and let's use it, but I'd like to know what people think about defaulting to the editor itself? /cc @ntoll @carlosperate

@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 21, 2016

Morning @tjguk. The copious mocking is my fault - essentially my aim was to allow us to test things in as "headless" (i.e. non-GUI) a way as possible. There are, of course, risks to mocking away a bunch of things and finding the right balance is hard.

So, I hear you, agree with you and trust your judgement. Since this problem is related to unexpected interactions between the our code and the editor widget we obvious shouldn't mock away the editor.

Upon reflection (now that the test suite has grown substantially) I wonder about de-mocking some of the things I have mocked away since you correctly point out that we're in danger of testing implementation rather than function.

In any case, please move forward sans-mock! We can review and / or perform a mockectomy in the new year. ;-) 👍

@tjguk

This comment has been minimized.

Copy link
Contributor

tjguk commented Dec 21, 2016

My branch now includes passing tests (one of which was added as failing to test this situation). Per the discussion above, I've switched some of them to use the editor directly as they were previously relying on knowledge of the implementation which has changed as part of this fix.

In the course of testing, I've been unsure about some of the intentions behind the functionality; I've asked some questions over in issue #212 to try to establish a baseline spec to test against.

@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 22, 2016

Fixed by #218

@ntoll ntoll closed this Dec 22, 2016

@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 22, 2016

Thanks @tjguk for your efforts on this. Great stuff! :-) 🎅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment