Faster and simpler replace in SSB, take two

This is a new version of CL 179343 which had to be reverted.

This problem of the previous CL is that the ComposingSpan that
was part of the replacement text was correctly added during the
replace but was immediately removed because it had a zero-length
size.

Swapping the add and remove blocks solves the problem.

The new non-zero length enforcement also revealed a bug in the
spell checker where we were creating useless range spans.

Change-Id: I59cebd4708af3becc7ab625ae41bc36837f1a1cf

diff --git a/core/java/android/text/SpannableStringBuilder.java b/core/java/android/text/SpannableStringBuilder.java
index ae9042c..0f8efd1 100644 (file)
--- a/core/java/android/text/SpannableStringBuilder.java
+++ b/core/java/android/text/SpannableStringBuilder.java
@@ -50,6 +50,8 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
     public SpannableStringBuilder(CharSequence text, int start, int end) {
         int srclen = end - start;
 
+        if (srclen < 0) throw new StringIndexOutOfBoundsException();
+
         int len = ArrayUtils.idealCharArraySize(srclen + 1);
         mText = new char[len];
         mGapStart = srclen;
@@ -87,7 +89,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
                 if (en > end - start)
                     en = end - start;
 
-                setSpan(spans[i], st, en, fl);
+                setSpan(false, spans[i], st, en, fl);
             }
         }
     }
@@ -149,7 +151,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
         if (where == mGapStart)
             return;
 
-        boolean atend = (where == length());
+        boolean atEnd = (where == length());
 
         if (where < mGapStart) {
             int overlap = mGapStart - where;
@@ -171,7 +173,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
             else if (start == where) {
                 int flag = (mSpanFlags[i] & START_MASK) >> START_SHIFT;
 
-                if (flag == POINT || (atend && flag == PARAGRAPH))
+                if (flag == POINT || (atEnd && flag == PARAGRAPH))
                     start += mGapLength;
             }
 
@@ -182,7 +184,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
             else if (end == where) {
                 int flag = (mSpanFlags[i] & END_MASK);
 
-                if (flag == POINT || (atend && flag == PARAGRAPH))
+                if (flag == POINT || (atEnd && flag == PARAGRAPH))
                     end += mGapLength;
             }
 
@@ -284,7 +286,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
                 }
 
                 if (st != ost || en != oen)
-                    setSpan(mSpans[i], st, en, mSpanFlags[i]);
+                    setSpan(false, mSpans[i], st, en, mSpanFlags[i]);
             }
         }
 
@@ -305,28 +307,6 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
 
         TextUtils.getChars(tb, tbstart, tbend, mText, start);
 
-        if (tb instanceof Spanned) {
-            Spanned sp = (Spanned) tb;
-            Object[] spans = sp.getSpans(tbstart, tbend, Object.class);
-
-            for (int i = 0; i < spans.length; i++) {
-                int st = sp.getSpanStart(spans[i]);
-                int en = sp.getSpanEnd(spans[i]);
-
-                if (st < tbstart)
-                    st = tbstart;
-                if (en > tbend)
-                    en = tbend;
-
-                if (getSpanStart(spans[i]) < 0) {
-                    setSpan(false, spans[i],
-                            st - tbstart + start,
-                            en - tbstart + start,
-                            sp.getSpanFlags(spans[i]));
-                }
-            }
-        }
-
         if (end > start) {
             // no need for span fixup on pure insertion
             boolean atEnd = (mGapStart + mGapLength == mText.length);
@@ -358,6 +338,25 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
                 }
             }
         }
+
+        if (tb instanceof Spanned) {
+            Spanned sp = (Spanned) tb;
+            Object[] spans = sp.getSpans(tbstart, tbend, Object.class);
+
+            for (int i = 0; i < spans.length; i++) {
+                int st = sp.getSpanStart(spans[i]);
+                int en = sp.getSpanEnd(spans[i]);
+
+                if (st < tbstart) st = tbstart;
+                if (en > tbend) en = tbend;
+
+                // Add span only if this object is not yet used as a span in this string
+                if (getSpanStart(spans[i]) < 0) {
+                        setSpan(false, spans[i], st - tbstart + start, en - tbstart + start,
+                                sp.getSpanFlags(spans[i]));
+                }
+            }
+        }
     }
 
     private void removeSpan(int i) {
@@ -389,7 +388,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
 
     // Documentation from interface
     public SpannableStringBuilder replace(final int start, final int end,
-                        CharSequence tb, int tbstart, int tbend) {
+            CharSequence tb, int tbstart, int tbend) {
         int filtercount = mFilters.length;
         for (int i = 0; i < filtercount; i++) {
             CharSequence repl = mFilters[i].filter(tb, tbstart, tbend, this, start, end);
@@ -411,53 +410,26 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
         TextWatcher[] textWatchers = getSpans(start, start + origLen, TextWatcher.class);
         sendBeforeTextChanged(textWatchers, start, origLen, newLen);
 
-        if (origLen == 0 || newLen == 0) {
-            change(start, end, tb, tbstart, tbend);
-        } else {
-            int selstart = Selection.getSelectionStart(this);
-            int selend = Selection.getSelectionEnd(this);
-
-            // XXX just make the span fixups in change() do the right thing
-            // instead of this madness!
-
-            checkRange("replace", start, end);
-            moveGapTo(end);
-
-            if (mGapLength < 2)
-                resizeFor(length() + 1);
-
-            for (int i = mSpanCount - 1; i >= 0; i--) {
-                if (mSpanStarts[i] == mGapStart)
-                    mSpanStarts[i]++;
+        // Try to keep the cursor / selection at the same relative position during
+        // a text replacement. If replaced or replacement text length is zero, this
+        // is already taken care of.
+        boolean adjustSelection = origLen != 0 && newLen != 0;
+        int selstart = 0;
+        int selend = 0;
+        if (adjustSelection) {
+            selstart = Selection.getSelectionStart(this);
+            selend = Selection.getSelectionEnd(this);
+        }
 
-                if (mSpanEnds[i] == mGapStart)
-                    mSpanEnds[i]++;
-            }
+        checkRange("replace", start, end);
 
-            mText[mGapStart] = ' ';
-            mGapStart++;
-            mGapLength--;
+        change(start, end, tb, tbstart, tbend);
 
-            if (mGapLength < 1) {
-                new Exception("mGapLength < 1").printStackTrace();
-            }
-
-            change(start + 1, start + 1, tb, tbstart, tbend);
-            change(start, start + 1, "", 0, 0);
-            change(start + newLen, start + newLen + origLen, "", 0, 0);
-
-            /*
-             * Special case to keep the cursor in the same position
-             * if it was somewhere in the middle of the replaced region.
-             * If it was at the start or the end or crossing the whole
-             * replacement, it should already be where it belongs.
-             * TODO: Is there some more general mechanism that could
-             * accomplish this?
-             */
+        if (adjustSelection) {
             if (selstart > start && selstart < end) {
                 long off = selstart - start;
 
-                off = off * newLen / (end - start);
+                off = off * newLen / origLen;
                 selstart = (int) off + start;
 
                 setSpan(false, Selection.SELECTION_START, selstart, selstart,
@@ -466,7 +438,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
             if (selend > start && selend < end) {
                 long off = selend - start;
 
-                off = off * newLen / (end - start);
+                off = off * newLen / origLen;
                 selend = (int) off + start;
 
                 setSpan(false, Selection.SELECTION_END, selend, selend, Spanned.SPAN_POINT_POINT);
@@ -489,12 +461,10 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
     }
 
     private void setSpan(boolean send, Object what, int start, int end, int flags) {
-        int nstart = start;
-        int nend = end;
-
         checkRange("setSpan", start, end);
 
-        if ((flags & START_MASK) == (PARAGRAPH << START_SHIFT)) {
+        int flagsStart = (flags & START_MASK) >> START_SHIFT;
+        if (flagsStart == PARAGRAPH) {
             if (start != 0 && start != length()) {
                 char c = charAt(start - 1);
 
@@ -503,7 +473,8 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
             }
         }
 
-        if ((flags & END_MASK) == PARAGRAPH) {
+        int flagsEnd = flags & END_MASK;
+        if (flagsEnd == PARAGRAPH) {
             if (end != 0 && end != length()) {
                 char c = charAt(end - 1);
 
@@ -512,26 +483,33 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
             }
         }
 
-        if (flags == Spanned.SPAN_EXCLUSIVE_EXCLUSIVE && start == end) {
-            throw new IllegalArgumentException(
-                    "SPAN_EXCLUSIVE_EXCLUSIVE spans cannot have a zero length");
+        // 0-length Spanned.SPAN_EXCLUSIVE_EXCLUSIVE
+        if (flagsStart == POINT && flagsEnd == MARK && start == end) {
+            if (send) {
+                throw new IllegalArgumentException(
+                        "SPAN_EXCLUSIVE_EXCLUSIVE spans cannot have a zero length");
+            } else {
+                // Silently ignore invalid spans when they are created from this class.
+                // This avoids the duplication of the above test code before all the
+                // calls to setSpan that are done in this class
+                return;
+            }
         }
 
+        int nstart = start;
+        int nend = end;
+
         if (start > mGapStart) {
             start += mGapLength;
         } else if (start == mGapStart) {
-            int flag = (flags & START_MASK) >> START_SHIFT;
-
-            if (flag == POINT || (flag == PARAGRAPH && start == length()))
+            if (flagsStart == POINT || (flagsStart == PARAGRAPH && start == length()))
                 start += mGapLength;
         }
 
         if (end > mGapStart) {
             end += mGapLength;
         } else if (end == mGapStart) {
-            int flag = (flags & END_MASK);
-
-            if (flag == POINT || (flag == PARAGRAPH && end == length()))
+            if (flagsEnd == POINT || (flagsEnd == PARAGRAPH && end == length()))
                 end += mGapLength;
         }
 
@@ -1231,6 +1209,7 @@ public class SpannableStringBuilder implements CharSequence, GetChars, Spannable
     private int mSpanCount;
 
     // TODO These value are tightly related to the public SPAN_MARK/POINT values in {@link Spanned}
+    private static final int MARK = 1;
     private static final int POINT = 2;
     private static final int PARAGRAPH = 3;
 

diff --git a/core/java/android/widget/SpellChecker.java b/core/java/android/widget/SpellChecker.java
index 9afaee3..c725b64 100644 (file)
--- a/core/java/android/widget/SpellChecker.java
+++ b/core/java/android/widget/SpellChecker.java
@@ -227,8 +227,7 @@ public class SpellChecker implements SpellCheckerSessionListener {
         for (int i = 0; i < length; i++) {
             final SpellParser spellParser = mSpellParsers[i];
             if (spellParser.isFinished()) {
-                spellParser.init(start, end);
-                spellParser.parse();
+                spellParser.parse(start, end);
                 return;
             }
         }
@@ -240,8 +239,7 @@ public class SpellChecker implements SpellCheckerSessionListener {
 
         SpellParser spellParser = new SpellParser();
         mSpellParsers[length] = spellParser;
-        spellParser.init(start, end);
-        spellParser.parse();
+        spellParser.parse(start, end);
     }
 
     private void spellCheck() {
@@ -421,8 +419,11 @@ public class SpellChecker implements SpellCheckerSessionListener {
     private class SpellParser {
         private Object mRange = new Object();
 
-        public void init(int start, int end) {
-            setRangeSpan((Editable) mTextView.getText(), start, end);
+        public void parse(int start, int end) {
+            if (end > start) {
+                setRangeSpan((Editable) mTextView.getText(), start, end);
+                parse();
+            }
         }
 
         public boolean isFinished() {