Track selection end in RichInputConnection

Change-Id: Ie5cffe03b676dcde83896cda139b42f3829eb528

2 files changed, 78 insertions, 43 deletions

diff --git a/java/src/com/android/inputmethod/latin/LatinIME.java b/java/src/com/android/inputmethod/latin/LatinIME.java
index 326c53f0e..cae59d84b 100644
--- a/java/src/com/android/inputmethod/latin/LatinIME.java
+++ b/java/src/com/android/inputmethod/latin/LatinIME.java
@@ -906,7 +906,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
         // refresh later.
         final boolean canReachInputConnection;
         if (!mConnection.resetCachesUponCursorMoveAndReturnSuccess(editorInfo.initialSelStart,
-                false /* shouldFinishComposition */)) {
+                editorInfo.initialSelEnd, false /* shouldFinishComposition */)) {
             // We try resetting the caches up to 5 times before giving up.
             mHandler.postResetCaches(isDifferentTextField, 5 /* remainingTries */);
             // mLastSelection{Start,End} are reset later in this method, don't need to do it here
@@ -1108,7 +1108,8 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
         // TODO: revisit this when LatinIME supports hardware keyboards.
         // NOTE: the test harness subclasses LatinIME and overrides isInputViewShown().
         // TODO: find a better way to simulate actual execution.
-        if (isInputViewShown() && !mConnection.isBelatedExpectedUpdate(oldSelStart, newSelStart)) {
+        if (isInputViewShown() && !mConnection.isBelatedExpectedUpdate(oldSelStart, newSelStart,
+                    oldSelEnd, newSelEnd)) {
             // TODO: the following is probably better done in resetEntireInputState().
             // it should only happen when the cursor moved, and the very purpose of the
             // test below is to narrow down whether this happened or not. Likewise with
@@ -1134,13 +1135,13 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
                 // Another option would be to send suggestions each time we set the composing
                 // text, but that is probably too expensive to do, so we decided to leave things
                 // as is.
-                resetEntireInputState(newSelStart);
+                resetEntireInputState(newSelStart, newSelEnd);
             } else {
-                // resetEntireInputState calls resetCachesUponCursorMove, but with the second
-                // argument as true. But in all cases where we don't reset the entire input state,
-                // we still want to tell the rich input connection about the new cursor position so
-                // that it can update its caches.
-                mConnection.resetCachesUponCursorMoveAndReturnSuccess(newSelStart,
+                // resetEntireInputState calls resetCachesUponCursorMove, but forcing the
+                // composition to end.  But in all cases where we don't reset the entire input
+                // state, we still want to tell the rich input connection about the new cursor
+                // position so that it can update its caches.
+                mConnection.resetCachesUponCursorMoveAndReturnSuccess(newSelStart, newSelEnd,
                         false /* shouldFinishComposition */);
             }
 
@@ -1363,7 +1364,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
 
     // This will reset the whole input state to the starting state. It will clear
     // the composing word, reset the last composed word, tell the inputconnection about it.
-    private void resetEntireInputState(final int newCursorPosition) {
+    private void resetEntireInputState(final int newSelStart, final int newSelEnd) {
         final boolean shouldFinishComposition = mWordComposer.isComposingWord();
         resetComposingState(true /* alsoResetLastComposedWord */);
         final SettingsValues settingsValues = mSettings.getCurrent();
@@ -1372,7 +1373,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
         } else {
             setSuggestedWords(settingsValues.mSuggestPuncList, false);
         }
-        mConnection.resetCachesUponCursorMoveAndReturnSuccess(newCursorPosition,
+        mConnection.resetCachesUponCursorMoveAndReturnSuccess(newSelStart, newSelEnd,
                 shouldFinishComposition);
     }
 
@@ -1715,7 +1716,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
                 if (mWordComposer.isCursorFrontOrMiddleOfComposingWord()) {
                     // If we are in the middle of a recorrection, we need to commit the recorrection
                     // first so that we can insert the character at the current cursor position.
-                    resetEntireInputState(mLastSelectionStart);
+                    resetEntireInputState(mLastSelectionStart, mLastSelectionEnd);
                 } else {
                     commitTyped(LastComposedWord.NOT_A_SEPARATOR);
                 }
@@ -1783,7 +1784,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
             if (mWordComposer.isCursorFrontOrMiddleOfComposingWord()) {
                 // If we are in the middle of a recorrection, we need to commit the recorrection
                 // first so that we can insert the batch input at the current cursor position.
-                resetEntireInputState(mLastSelectionStart);
+                resetEntireInputState(mLastSelectionStart, mLastSelectionEnd);
             } else if (wordComposerSize <= 1) {
                 // We auto-correct the previous (typed, not gestured) string iff it's one character
                 // long. The reason for this is, even in the middle of gesture typing, you'll still
@@ -2071,7 +2072,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
         if (mWordComposer.isCursorFrontOrMiddleOfComposingWord()) {
             // If we are in the middle of a recorrection, we need to commit the recorrection
             // first so that we can remove the character at the current cursor position.
-            resetEntireInputState(mLastSelectionStart);
+            resetEntireInputState(mLastSelectionStart, mLastSelectionEnd);
             // When we exit this if-clause, mWordComposer.isComposingWord() will return false.
         }
         if (mWordComposer.isComposingWord()) {
@@ -2239,7 +2240,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
         if (mWordComposer.isCursorFrontOrMiddleOfComposingWord()) {
             // If we are in the middle of a recorrection, we need to commit the recorrection
             // first so that we can insert the character at the current cursor position.
-            resetEntireInputState(mLastSelectionStart);
+            resetEntireInputState(mLastSelectionStart, mLastSelectionEnd);
             isComposingWord = false;
         }
         // We want to find out whether to start composing a new word with this character. If so,
@@ -2351,7 +2352,7 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
         if (mWordComposer.isCursorFrontOrMiddleOfComposingWord()) {
             // If we are in the middle of a recorrection, we need to commit the recorrection
             // first so that we can insert the separator at the current cursor position.
-            resetEntireInputState(mLastSelectionStart);
+            resetEntireInputState(mLastSelectionStart, mLastSelectionEnd);
         }
         if (mWordComposer.isComposingWord()) { // May have changed since we stored wasComposing
             if (currentSettings.mCorrectionEnabled) {
@@ -2983,7 +2984,8 @@ public class LatinIME extends InputMethodService implements KeyboardActionListen
      * @param remainingTries How many times we may try again before giving up.
      */
     private void retryResetCaches(final boolean tryResumeSuggestions, final int remainingTries) {
-        if (!mConnection.resetCachesUponCursorMoveAndReturnSuccess(mLastSelectionStart, false)) {
+        if (!mConnection.resetCachesUponCursorMoveAndReturnSuccess(mLastSelectionStart,
+                    mLastSelectionEnd, false)) {
             if (0 < remainingTries) {
                 mHandler.postResetCaches(tryResumeSuggestions, remainingTries - 1);
                 return;
diff --git a/java/src/com/android/inputmethod/latin/RichInputConnection.java b/java/src/com/android/inputmethod/latin/RichInputConnection.java
index 80e137a34..4a7f530f9 100644
--- a/java/src/com/android/inputmethod/latin/RichInputConnection.java
+++ b/java/src/com/android/inputmethod/latin/RichInputConnection.java
@@ -57,15 +57,20 @@ public final class RichInputConnection {
     private static final int INVALID_CURSOR_POSITION = -1;
 
     /**
-     * This variable contains an expected value for the cursor position. This is where the
-     * cursor may end up after all the keyboard-triggered updates have passed. We keep this to
-     * compare it to the actual cursor position to guess whether the move was caused by a
-     * keyboard command or not.
-     * It's not really the cursor position: the cursor may not be there yet, and it's also expected
-     * there be cases where it never actually comes to be there.
+     * This variable contains an expected value for the selection start position. This is where the
+     * cursor or selection start may end up after all the keyboard-triggered updates have passed. We
+     * keep this to compare it to the actual selection start to guess whether the move was caused by
+     * a keyboard command or not.
+     * It's not really the selection start position: the selection start may not be there yet, and
+     * in some cases, it may never arrive there.
      */
     private int mExpectedSelStart = INVALID_CURSOR_POSITION; // in chars, not code points
     /**
+     * The expected selection end.  Only differs from mExpectedSelStart if a non-empty selection is
+     * expected.  The same caveats as mExpectedSelStart apply.
+     */
+    private int mExpectedSelEnd = INVALID_CURSOR_POSITION; // in chars, not code points
+    /**
      * This contains the committed text immediately preceding the cursor and the composing
      * text if any. It is refreshed when the cursor moves by calling upon the TextView.
      */
@@ -150,15 +155,17 @@ public final class RichInputConnection {
      * data, so we empty the cache and note that we don't know the new cursor position, and we
      * return false so that the caller knows about this and can retry later.
      *
-     * @param newSelStart The new position of the selection start, as received from the system.
-     * @param shouldFinishComposition Whether we should finish the composition in progress.
+     * @param newSelStart the new position of the selection start, as received from the system.
+     * @param newSelEnd the new position of the selection end, as received from the system.
+     * @param shouldFinishComposition whether we should finish the composition in progress.
      * @return true if we were able to connect to the editor successfully, false otherwise. When
      *   this method returns false, the caches could not be correctly refreshed so they were only
      *   reset: the caller should try again later to return to normal operation.
      */
     public boolean resetCachesUponCursorMoveAndReturnSuccess(final int newSelStart,
-            final boolean shouldFinishComposition) {
+            final int newSelEnd, final boolean shouldFinishComposition) {
         mExpectedSelStart = newSelStart;
+        mExpectedSelEnd = newSelEnd;
         mComposingText.setLength(0);
         final boolean didReloadTextSuccessfully = reloadTextCache();
         if (!didReloadTextSuccessfully) {
@@ -169,11 +176,13 @@ public final class RichInputConnection {
         if (lengthOfTextBeforeCursor > newSelStart
                 || (lengthOfTextBeforeCursor < Constants.EDITOR_CONTENTS_CACHE_SIZE
                         && newSelStart < Constants.EDITOR_CONTENTS_CACHE_SIZE)) {
-            // newSelStart may be lying -- when rotating the device (probably a framework bug). If
-            // we have less chars than we asked for, then we know how many chars we have, and if we
-            // got more than newSelStart says, then we know it was lying. In both cases the length
-            // is more reliable.
+            // newSelStart and newSelEnd may be lying -- when rotating the device (probably a
+            // framework bug). If we have less chars than we asked for, then we know how many chars
+            // we have, and if we got more than newSelStart says, then we know it was lying. In both
+            // cases the length is more reliable.  Note that we only have to check newSelStart (not
+            // newSelEnd) since if newSelEnd is wrong, the newSelStart will be wrong as well.
             mExpectedSelStart = lengthOfTextBeforeCursor;
+            mExpectedSelEnd = lengthOfTextBeforeCursor;
         }
         if (null != mIC && shouldFinishComposition) {
             mIC.finishComposingText();
@@ -200,6 +209,7 @@ public final class RichInputConnection {
             // For some reason the app thinks we are not connected to it. This looks like a
             // framework bug... Fall back to ground state and return false.
             mExpectedSelStart = INVALID_CURSOR_POSITION;
+            mExpectedSelEnd = INVALID_CURSOR_POSITION;
             Log.e(TAG, "Unable to connect to the editor to retrieve text.");
             return false;
         }
@@ -351,8 +361,12 @@ public final class RichInputConnection {
         }
         if (mExpectedSelStart > beforeLength) {
             mExpectedSelStart -= beforeLength;
+            mExpectedSelEnd -= beforeLength;
         } else {
+            // There are fewer characters before the cursor in the buffer than we are being asked to
+            // delete.  Only delete what is there.
             mExpectedSelStart = 0;
+            mExpectedSelEnd -= mExpectedSelStart;
         }
         if (null != mIC) {
             mIC.deleteSurroundingText(beforeLength, afterLength);
@@ -387,6 +401,7 @@ public final class RichInputConnection {
             case KeyEvent.KEYCODE_ENTER:
                 mCommittedTextBeforeComposingText.append("\n");
                 mExpectedSelStart += 1;
+                mExpectedSelEnd = mExpectedSelStart;
                 break;
             case KeyEvent.KEYCODE_DEL:
                 if (0 == mComposingText.length()) {
@@ -398,18 +413,24 @@ public final class RichInputConnection {
                 } else {
                     mComposingText.delete(mComposingText.length() - 1, mComposingText.length());
                 }
-                if (mExpectedSelStart > 0) mExpectedSelStart -= 1;
+                if (mExpectedSelStart > 0 && mExpectedSelStart == mExpectedSelEnd) {
+                    // TODO: Handle surrogate pairs.
+                    mExpectedSelStart -= 1;
+                }
+                mExpectedSelEnd = mExpectedSelStart;
                 break;
             case KeyEvent.KEYCODE_UNKNOWN:
                 if (null != keyEvent.getCharacters()) {
                     mCommittedTextBeforeComposingText.append(keyEvent.getCharacters());
                     mExpectedSelStart += keyEvent.getCharacters().length();
+                    mExpectedSelEnd = mExpectedSelStart;
                 }
                 break;
             default:
                 final String text = new String(new int[] { keyEvent.getUnicodeChar() }, 0, 1);
                 mCommittedTextBeforeComposingText.append(text);
                 mExpectedSelStart += text.length();
+                mExpectedSelEnd = mExpectedSelStart;
                 break;
             }
         }
@@ -444,9 +465,11 @@ public final class RichInputConnection {
         if (DEBUG_BATCH_NESTING) checkBatchEdit();
         if (DEBUG_PREVIOUS_TEXT) checkConsistencyForDebug();
         mExpectedSelStart += text.length() - mComposingText.length();
+        mExpectedSelEnd = mExpectedSelStart;
         mComposingText.setLength(0);
         mComposingText.append(text);
-        // TODO: support values of i != 1. At this time, this is never called with i != 1.
+        // TODO: support values of newCursorPosition != 1. At this time, this is never called with
+        // newCursorPosition != 1.
         if (null != mIC) {
             mIC.setComposingText(text, newCursorPosition);
             if (ProductionFlag.USES_DEVELOPMENT_ONLY_DIAGNOSTICS) {
@@ -782,20 +805,30 @@ public final class RichInputConnection {
      * this update and not the ones in-between. This is almost impossible to achieve even trying
      * very very hard.
      *
-     * @param oldSelStart The value of the old cursor position in the update.
-     * @param newSelStart The value of the new cursor position in the update.
+     * @param oldSelStart The value of the old selection in the update.
+     * @param newSelStart The value of the new selection in the update.
+     * @param oldSelEnd The value of the old selection end in the update.
+     * @param newSelEnd The value of the new selection end in the update.
      * @return whether this is a belated expected update or not.
      */
-    public boolean isBelatedExpectedUpdate(final int oldSelStart, final int newSelStart) {
-        // If this is an update that arrives at our expected position, it's a belated update.
-        if (newSelStart == mExpectedSelStart) return true;
-        // If this is an update that moves the cursor from our expected position, it must be
-        // an explicit move.
-        if (oldSelStart == mExpectedSelStart) return false;
-        // The following returns true if newSelStart is between oldSelStart and
-        // mCurrentCursorPosition. We assume that if the updated position is between the old
-        // position and the expected position, then it must be a belated update.
-        return (newSelStart - oldSelStart) * (mExpectedSelStart - newSelStart) >= 0;
+    public boolean isBelatedExpectedUpdate(final int oldSelStart, final int newSelStart,
+            final int oldSelEnd, final int newSelEnd) {
+        // This update is "belated" if we are expecting it.  That is, mExpectedSelStart and
+        // mExpectedSelEnd match the new values that the TextView is updating TO.
+        if (mExpectedSelStart == newSelStart && mExpectedSelEnd == newSelEnd) return true;
+        // This update is not belated if mExpectedSelStart and mExpeectedSelend match the old
+        // values, and one of newSelStart or newSelEnd is updated to a different value.  In this
+        // case, there is likely something other than the IME that has moved the selection endpoint
+        // to the new value.
+        if (mExpectedSelStart == oldSelStart && mExpectedSelEnd == oldSelEnd
+                && (oldSelStart != newSelStart || oldSelEnd != newSelEnd)) return false;
+        // If nether of the above two cases holds, then the system may be having trouble keeping up
+        // with updates.  If 1) the selection is a cursor, 2) newSelStart is between oldSelStart
+        // and mExpectedSelStart, and 3) newSelEnd is between oldSelEnd and mExpectedSelEnd, then
+        // assume a belated update.
+        return (newSelStart == newSelEnd)
+                && (newSelStart - oldSelStart) * (mExpectedSelStart - newSelStart) >= 0
+                && (newSelEnd - oldSelEnd) * (mExpectedSelEnd - newSelEnd) >= 0;
     }
 
     /**