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

Double-clicking a word selects the word plus the leading or trailing space #197

Closed
benedictleejh opened this issue Oct 30, 2015 · 23 comments

Comments

@benedictleejh
Copy link

In the InlineStyleTextArea, when I double-click on a word, it selects the word and its leading or trailing space. I'd like for the double-click to only select the word itself, without the leading or trailing space (similar to the JavaFX text area). Is this possible, and if so, how should I do so?

@JordanMartinez
Copy link
Contributor

Here's a quick workaround:

area.addEventHandler(MouseEvent.MOUSE_CLICKED, new EventHandler<MouseEvent>() {
    @Override
    public void handle(MouseEvent event) {
        if (event.getClickCount() == 2) {
            IndexRange range = area.getSelection();
            String selectedText = area.getSelectedText();
            if (selectedText.startsWith(" ")) {
                area.selectRange(range.getStart() + 1, range.getEnd());
            } else if (selectedText.endsWith(" ")) {
                area.selectRange(range.getStart(), range.getEnd() - 1);
            }
        }
    }
});

@TomasMikula
Copy link
Member

Thanks for reporting and thanks for the work-around. We should fix this at the core, though.

@JordanMartinez
Copy link
Contributor

Although I don't fully understand BreakIterator, I found a way to make it work.

  • Remove NavigationActions.previousWord()'s line: "wordBreakIterator.previous()"
  • Remove NavigationActions.nextWord()'s line: "wordBreakIterator.next()"

@TomasMikula
Copy link
Member

That will fix this issue, but previousWord and nextWord are used elsewhere, so it will likely break other usages.

@JordanMartinez
Copy link
Contributor

Something similar to this issue is when the user presses the combination SHIFT+Shortcut+LEFT/RIGHT. The selection will also include an extra space.

For example, let's say "|" represents the caret position in the following sentence
"This sim|ple sentence gives an illustration,"
Pressing the above combination to the right will select "ple " instead of just "ple" (shown by angle brackets)
"This sim<ple >sentence gives an illustration"
Every additional selection, if doing the same key press, will still include the extra space char.

"This sim<ple sentence >gives an illustration"
"This sim<ple sentence gives >an illustration"

If going to the left, the selection will be " sim" instead of "sim"
"This< sim>ple sentence gives an illustration."

@JordanMartinez
Copy link
Contributor

How about changing previousWord() and nextWord() to the following (not sure how the javadoc would need to be updated):

default void previousWord(SelectionPolicy selectionPolicy, boolean applyPrevious) {
    if(getLength() == 0) {
        return;
    }

    BreakIterator wordBreakIterator = BreakIterator.getWordInstance();
    wordBreakIterator.setText(getText());
    wordBreakIterator.preceding(getCaretPosition());
    if (applyPrevious) { wordBreakIterator.previous(); }

    moveTo(wordBreakIterator.current(), selectionPolicy);
}

@TomasMikula
Copy link
Member

I was actually thinking of something similar, but instead of a boolean, use an int n and then call wordBreakIterator.previous() n-1 times. A behavior-preserving refactoring would be to change all previousWord() calls to previousWord(2). Then to address this issue would be to change it to previousWord(1) somewhere. Maybe changing the method name would also make sense.

@JordanMartinez
Copy link
Contributor

That makes sense, so something like this:

/**
* Skips n number of word boundaries backwards.
* Based on the given selection policy, anchor either moves with
* the caret, stays put, or moves to the former caret position.
*/
default void skipToPreviousWordBoundary(int n, SelectionPolicy selectionPolicy) {
    if(getLength() == 0) {
        return;
    }

    BreakIterator wordBreakIterator = BreakIterator.getWordInstance();
    wordBreakIterator.setText(getText());
    wordBreakIterator.preceding(getCaretPosition());
    if (n < 1) n = 1;
    // Do I need a maximum limit based on getLength() here ?
    for (int i = 0; i < n - 1; i++) {
        wordBreakIterator.previous();
    }

    moveTo(wordBreakIterator.current(), selectionPolicy);
}

Renaming it would make sense, but I'm not sure if my above suggestion is the best naming scheme.

@JordanMartinez
Copy link
Contributor

If there's no need for a maximum limit, I guess a better way to write that for-loop code would be:

if (n >= 2) {
    for (int i = 0; i < n-1; i++) {
        wordBreakIterator.previous();
    }
}

@TomasMikula
Copy link
Member

I don't really see a difference. What about just

    wordBreakIterator.preceding(getCaretPosition());
    for (int i = 1; i < n; i++) {
        wordBreakIterator.previous();
    }

@JordanMartinez
Copy link
Contributor

Yeah, that's simpler.

What about the method's name?

@TomasMikula
Copy link
Member

What about wordBreaksBackwards(int n), wordBreaksForwards(int n)?

@JordanMartinez
Copy link
Contributor

I think that's good.

@TomasMikula
Copy link
Member

OK, if you want to go ahead and submit a PR, go ahead ;)

@effad
Copy link

effad commented Nov 16, 2015

FYI: Just stumbled across this problem too and looking forward for a fix.

@JordanMartinez
Copy link
Contributor

This was fixed in fca5d95. Tomas just hasn't made a new release with the bug fix included yet.

@Crystark
Copy link

Crystark commented Apr 7, 2016

👍

Would be great if this could be released.
Will this also fix the fact double clicking the first word of a new line also selects the previous line separator ?

Thanks

@JordanMartinez
Copy link
Contributor

@Crystark (I know it's not exactly ideal but...) you could always use the snapshot version. Also, does that issue arise in the snapshot version? Or only in the 0.6.10 release?

@Crystark
Copy link

Crystark commented May 1, 2016

@JordanMartinez The thing is i would have to build the snapshot version myself and make sure it's in my local m2 repository each time i want to build my project. i'm not working on only one computer and i don't see a way to automate that through a maven dependency. Any ETA on a new version (even a beta) being pushed to maven central ?

@Crystark
Copy link

Crystark commented May 1, 2016

Forget what i said. looks like the snapshots are being deployed. i'll test that

@Crystark
Copy link

Crystark commented May 1, 2016

@JordanMartinez I can confirm everything works fine in the snapshot version. Thanks.

It would be nice if you could release some milestones so that one doesn't have to depend on a moving version.

@JordanMartinez
Copy link
Contributor

@Crystark Good to know! Thanks for checking. AFAIK, Tomas hasn't made a new release because when I implemented #152, it broke the undo handling by making it more complex. So, once issue #233 is fixed (you can follow the progress via FXMisc/UndoFX#8), he should make a new stable release. As for an ETA, I'm not sure. I'll be working on the issue during this week, but it is a complicated issue.

Also, the current snapshot release should be reliable because there's no other work planned besides the undo issue (and fixing any bugs that may be discovered until then). For reference, you could look at #259.

@TomasMikula
Copy link
Member

Fixed in 0.7-M1.

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

No branches or pull requests

5 participants