Move batchedit related logics into InputConnection

chrome/android/java/src/org/chromium/chrome/browser/omnibox/AutocompleteEditText.java

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.omnibox;
 
 import android.content.Context;
 import android.graphics.Rect;
 import android.os.StrictMode;
 import android.text.Editable;
@@ skipping 25 matching lines @@
 
     private final AutocompleteSpan mAutocompleteSpan;
     private final AccessibilityManager mAccessibilityManager;
 
     /**
      * Whether default TextView scrolling should be disabled because autocomplete has been added.
      * This allows the user entered text to be shown instead of the end of the autocomplete.
      */
     private boolean mDisableTextScrollingFromAutocomplete;
 
-    private boolean mInBatchEditMode;
+    private int mBatchEditNestCount;

[Ted C, 2017/05/26 23:43:45]

The thing I struggle with is that this moves more state/shared variables between
the input connection and this class.  It seems like that will make the split
even harder.

Is it possible to move more of these into the connection?  Should we work
towards making that a static class that doesn't muck with the internals?

I guess I'm wondering where you think the split should end up happening as I
think that would be good to get an agreement on before we start moving a ton of
code.

[Changwan Ryu, 2017/06/05 19:07:39]

Hmm.. I didn’t really think much about how to split InputConnection and the
model so far. I’m planning to move most of the logic here into
AutocompleteEditTextModel and AutocompleteEditTextModelBase (interface), and add
a new model (SpannableAutocompleteEditTextModel) after that. And the only thing
I had in mind was to split between View and model where I just wanted to have as
few number of methods called between them as possible. I’m not planning to
invest refactoring efforts in the old model, but I’ll try to make sure that the
new model has static InputConnection. Is this ok?

     private int mBeforeBatchEditAutocompleteIndex = -1;
     private String mBeforeBatchEditFullText;
     private boolean mSelectionChangedInBatchMode;
     private boolean mTextDeletedInBatchMode;
 
     // Set to true when the text is modified programmatically. Initially set to true until the old
     // state has been loaded.
     private boolean mIgnoreTextChangeFromAutocomplete = true;
     private boolean mLastEditWasDelete;
 
@@ skipping 34 matching lines @@
         int expectedSelectionStart = getText().getSpanStart(mAutocompleteSpan);
         int expectedSelectionEnd = getText().length();
         if (expectedSelectionStart < 0) {
             expectedSelectionStart = expectedSelectionEnd;
         }
 
         return selectionStart == expectedSelectionStart && selectionEnd == expectedSelectionEnd;
     }
 
     /**
-     * @return Whether the URL is currently in batch edit mode triggered by an IME.  No external
-     *         text changes should be triggered while this is true.
-     */
-    // isInBatchEditMode is a package protected method on TextView, so we intentionally chose
-    // a different name.
-    private boolean isHandlingBatchInput() {
-        return mInBatchEditMode;
-    }
-
-    /**
      * @return The user text without the autocomplete text.
      */
     public String getTextWithoutAutocomplete() {
         int autoCompleteIndex = getText().getSpanStart(mAutocompleteSpan);
         if (autoCompleteIndex < 0) {
             return getTextWithAutocomplete();
         } else {
             return getTextWithAutocomplete().substring(0, autoCompleteIndex);
         }
     }
 
     /** @return Whether any autocomplete information is specified on the current text. */
     @VisibleForTesting
     public boolean hasAutocomplete() {
         return getText().getSpanStart(mAutocompleteSpan) >= 0
                 || mAutocompleteSpan.mAutocompleteText != null
                 || mAutocompleteSpan.mUserText != null;
     }
 
     /**
      * Whether we want to be showing inline autocomplete results. We don't want to show them as the
      * user deletes input. Also if there is a composition (e.g. while using the Japanese IME),
      * we must not autocomplete or we'll destroy the composition.
      * @return Whether we want to be showing inline autocomplete results.
      */
     public boolean shouldAutocomplete() {
         if (mLastEditWasDelete) return false;
         Editable text = getText();
 
-        return isCursorAtEndOfTypedText() && !isHandlingBatchInput()
+        return isCursorAtEndOfTypedText() && mBatchEditNestCount == 0
                 && BaseInputConnection.getComposingSpanEnd(text)
                 == BaseInputConnection.getComposingSpanStart(text);
     }
 
-    @Override
-    public void onBeginBatchEdit() {
-        if (DEBUG) Log.i(TAG, "onBeginBatchEdit");
-        mBeforeBatchEditAutocompleteIndex = getText().getSpanStart(mAutocompleteSpan);
-        mBeforeBatchEditFullText = getText().toString();
-
-        super.onBeginBatchEdit();
-        mInBatchEditMode = true;
-        mTextDeletedInBatchMode = false;
-    }
-
-    @Override
-    public void onEndBatchEdit() {
-        if (DEBUG) Log.i(TAG, "onEndBatchEdit");
-        super.onEndBatchEdit();
-        mInBatchEditMode = false;
+    private void onPostEndBatchEdit() {
         if (mSelectionChangedInBatchMode) {
             validateSelection(getSelectionStart(), getSelectionEnd());
             mSelectionChangedInBatchMode = false;
         }
 
         String newText = getText().toString();
         if (!TextUtils.equals(mBeforeBatchEditFullText, newText)
                 || getText().getSpanStart(mAutocompleteSpan) != mBeforeBatchEditAutocompleteIndex) {
             // If the text being typed is a single character that matches the next character in the
             // previously visible autocomplete text, we reapply the autocomplete text to prevent
             // a visual flickering when the autocomplete text is cleared and then quickly reapplied
             // when the next round of suggestions is received.
             if (shouldAutocomplete() && mBeforeBatchEditAutocompleteIndex != -1
                     && mBeforeBatchEditFullText != null
                     && mBeforeBatchEditFullText.startsWith(newText) && !mTextDeletedInBatchMode
                     && newText.length() - mBeforeBatchEditAutocompleteIndex == 1) {
                 setAutocompleteText(newText, mBeforeBatchEditFullText.substring(newText.length()));
             }
             notifyAutocompleteTextStateChanged(mTextDeletedInBatchMode, true);
         }
 
         mTextDeletedInBatchMode = false;
         mBeforeBatchEditAutocompleteIndex = -1;
         mBeforeBatchEditFullText = null;
     }
 
     @Override
     protected void onSelectionChanged(int selStart, int selEnd) {
         if (DEBUG) Log.i(TAG, "onSelectionChanged -- selStart: %d, selEnd: %d", selStart, selEnd);
-        if (!mInBatchEditMode) {
+        if (mBatchEditNestCount == 0) {
             int beforeTextLength = getText().length();
             if (validateSelection(selStart, selEnd)) {
                 boolean textDeleted = getText().length() < beforeTextLength;
                 notifyAutocompleteTextStateChanged(textDeleted, false);
             }
         } else {
             mSelectionChangedInBatchMode = true;
         }
         super.onSelectionChanged(selStart, selEnd);
     }
@@ skipping 59 matching lines @@
             // in the case of autocomplete we want the last typed character to be shown, which
             // is the start of selection.
             mDisableTextScrollingFromAutocomplete = false;
             bringPointIntoView(getSelectionStart());
             retVal = true;
         }
         return retVal;
     }
 
     /**
-     * Autocompletes the text on the url bar and selects the text that was not entered by the
-     * user. Using append() instead of setText() to preserve the soft-keyboard layout.
+     * Autocompletes the text and selects the text that was not entered by the user. Using append()
+     * instead of setText() to preserve the soft-keyboard layout.
      * @param userText user The text entered by the user.
      * @param inlineAutocompleteText The suggested autocompletion for the user's text.
      */
     public void setAutocompleteText(CharSequence userText, CharSequence inlineAutocompleteText) {
         if (DEBUG) {
             Log.i(TAG, "setAutocompleteText -- userText: %s, inlineAutocompleteText: %s", userText,
                     inlineAutocompleteText);
         }
         boolean emptyAutocomplete = TextUtils.isEmpty(inlineAutocompleteText);
 
@@ skipping 49 matching lines @@
 
     @Override
     protected void onTextChanged(CharSequence text, int start, int lengthBefore, int lengthAfter) {
         if (DEBUG) {
             Log.i(TAG, "onTextChanged -- text: %s, start: %d, lengthBefore: %d, lengthAfter: %d",
                     text, start, lengthBefore, lengthAfter);
         }
 
         super.onTextChanged(text, start, lengthBefore, lengthAfter);
         boolean textDeleted = lengthAfter == 0;
-        if (!mInBatchEditMode) {
+        if (mBatchEditNestCount == 0) {
             notifyAutocompleteTextStateChanged(textDeleted, true);
         } else {
             mTextDeletedInBatchMode = textDeleted;
         }
     }
 
     @Override
     public void setText(CharSequence text, BufferType type) {
         if (DEBUG) Log.i(TAG, "setText -- text: %s", text);
 
         mDisableTextScrollingFromAutocomplete = false;
 
-        // Avoid setting the same text to the URL bar as it will mess up the scroll/cursor
-        // position.
+        // Avoid setting the same text as it will mess up the scroll/cursor position.
         // Setting the text is also quite expensive, so only do it when the text has changed
-        // (since we apply spans when the URL is not focused, we only optimize this when the
-        // URL is being edited).
+        // (since we apply spans when the view is not focused, we only optimize this when the
+        // text is being edited).
         if (!TextUtils.equals(getEditableText(), text)) {
             // Certain OEM implementations of setText trigger disk reads. crbug.com/633298
             StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
             try {
                 super.setText(text, type);
             } finally {
                 StrictMode.setThreadPolicy(oldPolicy);
             }
         }
 
@@ skipping 54 matching lines @@
 
     @VisibleForTesting
     public void setIgnoreImeForTest(boolean ignore) {
         mIgnoreImeForTest = ignore;
     }
 
     private InputConnectionWrapper mInputConnection = new InputConnectionWrapper(null, true) {
         private final char[] mTempSelectionChar = new char[1];
 
         @Override
+        public boolean beginBatchEdit() {
+            ++mBatchEditNestCount;
+            if (mBatchEditNestCount == 1) {
+                if (DEBUG) Log.i(TAG, "beginBatchEdit");
+                mBeforeBatchEditAutocompleteIndex = getText().getSpanStart(mAutocompleteSpan);
+                mBeforeBatchEditFullText = getText().toString();
+
+                boolean retVal = super.beginBatchEdit();
+                mTextDeletedInBatchMode = false;
+                return retVal;
+            }
+            return super.beginBatchEdit();
+        }
+
+        @Override
+        public boolean endBatchEdit() {
+            mBatchEditNestCount = Math.max(mBatchEditNestCount - 1, 0);
+            if (mBatchEditNestCount == 0) {
+                if (DEBUG) Log.i(TAG, "endBatchEdit");
+                boolean retVal = super.endBatchEdit();
+                onPostEndBatchEdit();
+                return retVal;
+            }
+            return super.endBatchEdit();
+        }
+
+        @Override
         public boolean commitText(CharSequence text, int newCursorPosition) {
             if (DEBUG) Log.i(TAG, "commitText: [%s]", text);
             Editable currentText = getText();
             if (currentText == null) return super.commitText(text, newCursorPosition);
 
             int selectionStart = Selection.getSelectionStart(currentText);
             int selectionEnd = Selection.getSelectionEnd(currentText);
             int autocompleteIndex = currentText.getSpanStart(mAutocompleteSpan);
             // If the text being committed is a single character that matches the next character
             // in the selection (assumed to be the autocomplete text), we only move the text
@@ skipping 15 matching lines @@
                                 AccessibilityEvent.TYPE_VIEW_TEXT_CHANGED);
                         event.setFromIndex(selectionStart);
                         event.setRemovedCount(0);
                         event.setAddedCount(1);
                         event.setBeforeText(currentText.toString().substring(0, selectionStart));
                         sendAccessibilityEventUnchecked(event);
                     }
 
                     setAutocompleteText(currentText.subSequence(0, selectionStart + 1),
                             currentText.subSequence(selectionStart + 1, selectionEnd));
-                    if (!mInBatchEditMode) {
+                    if (mBatchEditNestCount == 0) {
                         notifyAutocompleteTextStateChanged(false, false);
                     }
                     return true;
                 }
             }
 
             boolean retVal = super.commitText(text, newCursorPosition);
 
             // Ensure the autocomplete span is removed if it is no longer valid after committing the
             // text.
@@ skipping 115 matching lines @@
         }
 
         /** Removes this span from the current text and clears the internal state. */
         public void clearSpan() {
             getText().removeSpan(this);
             mAutocompleteText = null;
             mUserText = null;
         }
     }
 }