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

Selection Fix #199

Merged
merged 3 commits into from
Nov 2, 2015
Merged

Selection Fix #199

merged 3 commits into from
Nov 2, 2015

Conversation

JordanMartinez
Copy link
Contributor

Turns out I wrote this on top of the branch I used for exposing the scroll API. Do you want me to create a separate branch without the exposure and create a PR ? Not sure what best practices are in this regard.

…ext by using the SHIFT+SHORTCUT+LEFT/RIGHT key combination, the selected text no longer includes the leading/trailing boundary character (such as a space)
@TomasMikula
Copy link
Member

It's good to base independent PRs off the master branch, so that they can be dealt with independently. But we can now first resolve the other PR and once that is merged, the other commit will be cleared from this PR automatically.

@JordanMartinez
Copy link
Contributor Author

Thought so.

@TomasMikula
Copy link
Member

Why does Shortcut+Left/Right skip 2 boundaries, while Shift+Shortcut+Left/Right only 1?

@JordanMartinez
Copy link
Contributor Author

When I wrote the initial code, I realized that Shift+Shortcut+Left/Right would also select trailing/leading word boundaries, so I reduced it to 1. I wasn't sure if Shortcut+Left/Right should do the same.

@TomasMikula
Copy link
Member

At least they should be consistent. The problem with skipping just one boundary is that if the cursor is at the beginning of a word and you press (Shift+)Shortcut+Left, it will only move to the end of the previous word, not to the beginning of the previous word. So probably some more sophisticated logic is needed to fix that. But I'm OK if this PR just fixes the double-click case and leaves the rest as it was (i.e. skip 2 word boundaries for the rest).

@JordanMartinez
Copy link
Contributor Author

That makes sense.

@JordanMartinez
Copy link
Contributor Author

I opened an issue for the above inconsistency so it can be dealt with separately.

@TomasMikula
Copy link
Member

Cool, thank you!

TomasMikula added a commit that referenced this pull request Nov 2, 2015
Double-click on word not to select the adjacent whitespace.
@TomasMikula TomasMikula merged commit fca5d95 into FXMisc:master Nov 2, 2015
@JordanMartinez JordanMartinez deleted the selectionFix branch November 2, 2015 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants