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

Issue212 selection highlights #218

Merged
merged 15 commits into from Dec 22, 2016

Conversation

Projects
None yet
2 participants
@tjguk
Copy link
Contributor

tjguk commented Dec 22, 2016

This PR is for the issues discussed in #212 but it builds on the (currently unmerged) work in #209

tjguk added some commits Dec 20, 2016

Use Python regex to highlight matches
The existing code used Scintilla's built-in findFirst method which changes the active selection as it goes. The attempt to replace the original selection was causing issues when selecting backwards with the keyboard. While it would probably be possible to get the right logic in place by getting the right combination of Scintilla's anchor and caret position, I've simply bypassed the problem by using Python's regex .finditer function to identify matching spans and converting those to Scintilla ranges.
Add a failing test for the issue where the cursor is moved by the edi…
…tor to the right of a selection.

Also rework some of the existing highlighter tests to exercise the editor functionality directly, relying less on our underlying implementation
For now, include the selected text because that's what the current im…
…plementation does; we can revisit the spec later and decide whether we want this functionality or not. (It doesn't make a ton of sense since, while it's selected, the highlight is hidden by the selected; and once it's unselected, the highlight will be cleared).
@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 22, 2016

Build still failing due to PEP8. :-(

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Dec 22, 2016

@tjguk

This comment has been minimized.

Copy link
Contributor Author

tjguk commented Dec 22, 2016

Perhaps I'm misunderstanding the output: it looks to me as though the overall merge is passing, but that individual commits are failing -- which they will, since they precede other commits which are exactly fixing up PEP8 errors. Should I have rebased before pushing the PR?

@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 22, 2016

Apologies... looks like it was Travis that had the collywobbles. I hit it with the proverbial spanner and it's gone green. ;-)

@ntoll

This comment has been minimized.

Copy link
Member

ntoll commented Dec 22, 2016

Looking at coverage, there's a couple of branches in the code that are not exercised. I'll add/massage the tests to get us back to 100%.

@ntoll ntoll merged commit 30240fa into mu-editor:master Dec 22, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tjguk tjguk deleted the tjguk:issue212-selection-highlights branch Dec 21, 2018

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