From e264ac392a886788ebfd1069e1d366e2b1edef72 Mon Sep 17 00:00:00 2001 From: Mady Mellor Date: Mon, 22 Jun 2015 16:46:29 -0700 Subject: [PATCH] Text selection: Fix word boundaries for languages without spaces WordIterator's getEnd/getBeginning methods does not support the needs for word boundary detection in text selection. Consider the words AABB (where AA and BB are each words). If getEnd is given the offset on the boundary between AA and BB, it would return that offset since it is the end of AA. For text selection we always want the "next end" if available. This CL adds two methods to word iterator that return the next end and previous beginning. This CL also alters the code in Start/EndHandle to use the x value and current / prev line positions to determine if the user is expanding or contracting the selection. Bug: 21305292 Change-Id: I6e7a83e53e245d71e43d78f1957f844f2ed1cdfb --- .../android/text/method/WordIterator.java | 118 +++++++++++++++--- core/java/android/widget/Editor.java | 93 +++++++++++++- 2 files changed, 190 insertions(+), 21 deletions(-) diff --git a/core/java/android/text/method/WordIterator.java b/core/java/android/text/method/WordIterator.java index 5dda8a765eb37..3688cfa33d5d1 100644 --- a/core/java/android/text/method/WordIterator.java +++ b/core/java/android/text/method/WordIterator.java @@ -147,24 +147,13 @@ public class WordIterator implements Selection.PositionIterator { * @throws IllegalArgumentException is offset is not valid. */ public int getBeginning(int offset) { - final int shiftedOffset = offset - mOffsetShift; - checkOffsetIsValid(shiftedOffset); - - if (isOnLetterOrDigit(shiftedOffset)) { - if (mIterator.isBoundary(shiftedOffset)) { - return shiftedOffset + mOffsetShift; - } else { - return mIterator.preceding(shiftedOffset) + mOffsetShift; - } - } else { - if (isAfterLetterOrDigit(shiftedOffset)) { - return mIterator.preceding(shiftedOffset) + mOffsetShift; - } - } - return BreakIterator.DONE; + // TODO: Check if usage of this can be updated to getBeginning(offset, true) if + // so this method can be removed. + return getBeginning(offset, false); } - /** If offset is within a word, returns the index of the last character of that + /** + * If offset is within a word, returns the index of the last character of that * word plus one, otherwise returns BreakIterator.DONE. * * The offsets that are considered to be part of a word are the indexes of its characters, @@ -177,11 +166,106 @@ public class WordIterator implements Selection.PositionIterator { * @throws IllegalArgumentException is offset is not valid. */ public int getEnd(int offset) { + // TODO: Check if usage of this can be updated to getEnd(offset, true), if + // so this method can be removed. + return getEnd(offset, false); + } + + /** + * If the offset is within a word or on a word boundary that can only be + * considered the start of a word (e.g. _word where "_" is any character that would not + * be considered part of the word) then this returns the index of the first character of + * that word. + * + * If the offset is on a word boundary that can be considered the start and end of a + * word, e.g. AABB (where AA and BB are both words) and the offset is the boundary + * between AA and BB, this would return the start of the previous word, AA. + * + * Returns BreakIterator.DONE if there is no previous boundary. + * + * @throws IllegalArgumentException is offset is not valid. + */ + public int getPrevWordBeginningOnTwoWordsBoundary(int offset) { + return getBeginning(offset, true); + } + + /** + * If the offset is within a word or on a word boundary that can only be + * considered the end of a word (e.g. word_ where "_" is any character that would not + * be considered part of the word) then this returns the index of the last character + * plus one of that word. + * + * If the offset is on a word boundary that can be considered the start and end of a + * word, e.g. AABB (where AA and BB are both words) and the offset is the boundary + * between AA and BB, this would return the end of the next word, BB. + * + * Returns BreakIterator.DONE if there is no next boundary. + * + * @throws IllegalArgumentException is offset is not valid. + */ + public int getNextWordEndOnTwoWordBoundary(int offset) { + return getEnd(offset, true); + } + + /** + * If the offset is within a word or on a word boundary that can only be + * considered the start of a word (e.g. _word where "_" is any character that would not + * be considered part of the word) then this returns the index of the first character of + * that word. + * + * If the offset is on a word boundary that can be considered the start and end of a + * word, e.g. AABB (where AA and BB are both words) and the offset is the boundary + * between AA and BB, and getPrevWordBeginningOnTwoWordsBoundary is true then this would + * return the start of the previous word, AA. Otherwise it would return the current offset, + * the start of BB. + * + * Returns BreakIterator.DONE if there is no previous boundary. + * + * @throws IllegalArgumentException is offset is not valid. + */ + private int getBeginning(int offset, boolean getPrevWordBeginningOnTwoWordsBoundary) { + final int shiftedOffset = offset - mOffsetShift; + checkOffsetIsValid(shiftedOffset); + + if (isOnLetterOrDigit(shiftedOffset)) { + if (mIterator.isBoundary(shiftedOffset) + && (!isAfterLetterOrDigit(shiftedOffset) + || !getPrevWordBeginningOnTwoWordsBoundary)) { + return shiftedOffset + mOffsetShift; + } else { + return mIterator.preceding(shiftedOffset) + mOffsetShift; + } + } else { + if (isAfterLetterOrDigit(shiftedOffset)) { + return mIterator.preceding(shiftedOffset) + mOffsetShift; + } + } + return BreakIterator.DONE; + } + + /** + * If the offset is within a word or on a word boundary that can only be + * considered the end of a word (e.g. word_ where "_" is any character that would not be + * considered part of the word) then this returns the index of the last character plus one + * of that word. + * + * If the offset is on a word boundary that can be considered the start and end of a + * word, e.g. AABB (where AA and BB are both words) and the offset is the boundary + * between AA and BB, and getNextWordEndOnTwoWordBoundary is true then this would return + * the end of the next word, BB. Otherwise it would return the current offset, the end + * of AA. + * + * Returns BreakIterator.DONE if there is no next boundary. + * + * @throws IllegalArgumentException is offset is not valid. + */ + private int getEnd(int offset, boolean getNextWordEndOnTwoWordBoundary) { final int shiftedOffset = offset - mOffsetShift; checkOffsetIsValid(shiftedOffset); if (isAfterLetterOrDigit(shiftedOffset)) { - if (mIterator.isBoundary(shiftedOffset)) { + if (mIterator.isBoundary(shiftedOffset) + && (!isOnLetterOrDigit(shiftedOffset) || !getNextWordEndOnTwoWordBoundary)) { return shiftedOffset + mOffsetShift; } else { return mIterator.following(shiftedOffset) + mOffsetShift; diff --git a/core/java/android/widget/Editor.java b/core/java/android/widget/Editor.java index e050bdab63f67..fa7c8e649e70a 100644 --- a/core/java/android/widget/Editor.java +++ b/core/java/android/widget/Editor.java @@ -123,6 +123,7 @@ public class Editor { private static final float[] TEMP_POSITION = new float[2]; private static int DRAG_SHADOW_MAX_TEXT_LENGTH = 20; private static final float LINE_SLOP_MULTIPLIER_FOR_HANDLEVIEWS = 0.5f; + private static final int UNSET_X_VALUE = -1; // Tag used when the Editor maintains its own separate UndoManager. private static final String UNDO_OWNER_TAG = "Editor"; @@ -735,7 +736,7 @@ public class Editor { retOffset = getWordIteratorWithText().getPunctuationBeginning(offset); } else { // Not on a punctuation boundary, find the word start. - retOffset = getWordIteratorWithText().getBeginning(offset); + retOffset = getWordIteratorWithText().getPrevWordBeginningOnTwoWordsBoundary(offset); } if (retOffset == BreakIterator.DONE) { return offset; @@ -750,7 +751,7 @@ public class Editor { retOffset = getWordIteratorWithText().getPunctuationEnd(offset); } else { // Not on a punctuation boundary, find the word end. - retOffset = getWordIteratorWithText().getEnd(offset); + retOffset = getWordIteratorWithText().getNextWordEndOnTwoWordBoundary(offset); } if (retOffset == BreakIterator.DONE) { return offset; @@ -4058,6 +4059,10 @@ public class Editor { private boolean mInWord = false; // Difference between touch position and word boundary position. private float mTouchWordDelta; + // X value of the previous updatePosition call. + private float mPrevX; + // Indicates if the handle has moved a boundary between LTR and RTL text. + private boolean mLanguageDirectionChanged = false; public SelectionStartHandleView(Drawable drawableLtr, Drawable drawableRtl) { super(drawableLtr, drawableRtl); @@ -4118,7 +4123,43 @@ public class Editor { int end = getWordEnd(offset); int start = getWordStart(offset); - if (offset < mPreviousOffset) { + if (mPrevX == UNSET_X_VALUE) { + mPrevX = x; + } + + final int selectionStart = mTextView.getSelectionStart(); + final boolean selectionStartRtl = layout.isRtlCharAt(selectionStart); + final boolean atRtl = layout.isRtlCharAt(offset); + final boolean isLvlBoundary = layout.isLevelBoundary(offset); + boolean isExpanding; + + // We can't determine if the user is expanding or shrinking the selection if they're + // on a bi-di boundary, so until they've moved past the boundary we'll just place + // the cursor at the current position. + if (isLvlBoundary || (selectionStartRtl && !atRtl) || (!selectionStartRtl && atRtl)) { + // We're on a boundary or this is the first direction change -- just update + // to the current position. + mLanguageDirectionChanged = true; + mTouchWordDelta = 0.0f; + positionAtCursorOffset(offset, false); + return; + } else if (mLanguageDirectionChanged && !isLvlBoundary) { + // We've just moved past the boundary so update the position. After this we can + // figure out if the user is expanding or shrinking to go by word or character. + positionAtCursorOffset(offset, false); + mTouchWordDelta = 0.0f; + mLanguageDirectionChanged = false; + return; + } else { + final float xDiff = x - mPrevX; + if (atRtl) { + isExpanding = xDiff > 0 || currLine > mPrevLine; + } else { + isExpanding = xDiff < 0 || currLine < mPrevLine; + } + } + + if (isExpanding) { // User is increasing the selection. if (!mInWord || currLine < mPrevLine) { // We're not in a word, or we're on a different line so we'll expand by @@ -4173,6 +4214,7 @@ public class Editor { } positionAtCursorOffset(offset, false); } + mPrevX = x; } @Override @@ -4187,6 +4229,7 @@ public class Editor { if (event.getActionMasked() == MotionEvent.ACTION_UP) { // Reset the touch word offset when the user has lifted their finger. mTouchWordDelta = 0.0f; + mPrevX = UNSET_X_VALUE; } return superResult; } @@ -4197,6 +4240,10 @@ public class Editor { private boolean mInWord = false; // Difference between touch position and word boundary position. private float mTouchWordDelta; + // X value of the previous updatePosition call. + private float mPrevX; + // Indicates if the handle has moved a boundary between LTR and RTL text. + private boolean mLanguageDirectionChanged = false; public SelectionEndHandleView(Drawable drawableLtr, Drawable drawableRtl) { super(drawableLtr, drawableRtl); @@ -4257,7 +4304,43 @@ public class Editor { int end = getWordEnd(offset); int start = getWordStart(offset); - if (offset > mPreviousOffset) { + if (mPrevX == UNSET_X_VALUE) { + mPrevX = x; + } + + final int selectionEnd = mTextView.getSelectionEnd(); + final boolean selectionEndRtl = layout.isRtlCharAt(selectionEnd); + final boolean atRtl = layout.isRtlCharAt(offset); + final boolean isLvlBoundary = layout.isLevelBoundary(offset); + boolean isExpanding; + + // We can't determine if the user is expanding or shrinking the selection if they're + // on a bi-di boundary, so until they've moved past the boundary we'll just place + // the cursor at the current position. + if (isLvlBoundary || (selectionEndRtl && !atRtl) || (!selectionEndRtl && atRtl)) { + // We're on a boundary or this is the first direction change -- just update + // to the current position. + mLanguageDirectionChanged = true; + mTouchWordDelta = 0.0f; + positionAtCursorOffset(offset, false); + return; + } else if (mLanguageDirectionChanged && !isLvlBoundary) { + // We've just moved past the boundary so update the position. After this we can + // figure out if the user is expanding or shrinking to go by word or character. + positionAtCursorOffset(offset, false); + mTouchWordDelta = 0.0f; + mLanguageDirectionChanged = false; + return; + } else { + final float xDiff = x - mPrevX; + if (atRtl) { + isExpanding = xDiff < 0 || currLine < mPrevLine; + } else { + isExpanding = xDiff > 0 || currLine > mPrevLine; + } + } + + if (isExpanding) { // User is increasing the selection. if (!mInWord || currLine > mPrevLine) { // We're not in a word, or we're on a different line so we'll expand by @@ -4312,6 +4395,7 @@ public class Editor { } positionAtCursorOffset(offset, false); } + mPrevX = x; } @Override @@ -4326,6 +4410,7 @@ public class Editor { if (event.getActionMasked() == MotionEvent.ACTION_UP) { // Reset the touch word offset when the user has lifted their finger. mTouchWordDelta = 0.0f; + mPrevX = UNSET_X_VALUE; } return superResult; }