Fix ImeSubtypeListItem#compareTo()

It turns out that ImeSubtypeListItem#compareTo() does not satisfy the
contract of Comparable#compareTo(), which can trigger
IllegalArgumentException from Collections.sort() depending on the
runtime condition.

This CL makes it clear that two instances of ImeSubtypeListItem will be
compared with with those fileds in the following order.

  1. ImeSubtypeListItem#mImeName
  2. ImeSubtypeListItem#mSubtypeName
  3. ImeSubtypeListItem#mIsSystemLocale
  4. ImeSubtypeListItem#mIsSystemLanguage

Bug: 34255739
Test: adb shell am instrument -w -e class com.android.internal.inputmethod.InputMethodSubtypeSwitchingControllerTest com.android.frameworks.coretests/android.support.test.runner.AndroidJUnitRunner
Change-Id: I47f902cc8f5873926d238c30e462d08d7dbebcf7

diff --git a/core/java/com/android/internal/inputmethod/InputMethodSubtypeSwitchingController.java b/core/java/com/android/internal/inputmethod/InputMethodSubtypeSwitchingController.java
index 8d11783..a94b161 100644 (file)
--- a/core/java/com/android/internal/inputmethod/InputMethodSubtypeSwitchingController.java
+++ b/core/java/com/android/internal/inputmethod/InputMethodSubtypeSwitchingController.java
@@ -95,39 +95,32 @@ public class InputMethodSubtypeSwitchingController {
             }
         }
 
+        private static int compareNullableCharSequences(@Nullable CharSequence c1,
+                @Nullable CharSequence c2) {
+            // For historical reasons, an empty text needs to put at the last.
+            final boolean empty1 = TextUtils.isEmpty(c1);
+            final boolean empty2 = TextUtils.isEmpty(c2);
+            if (empty1 || empty2) {
+                return (empty1 ? 1 : 0) - (empty2 ? 1 : 0);
+            }
+            return c1.toString().compareTo(c2.toString());
+        }
+
         @Override
         public int compareTo(ImeSubtypeListItem other) {
-            if (TextUtils.isEmpty(mImeName)) {
-                return 1;
-            }
-            if (TextUtils.isEmpty(other.mImeName)) {
-                return -1;
-            }
-            if (!TextUtils.equals(mImeName, other.mImeName)) {
-                return mImeName.toString().compareTo(other.mImeName.toString());
-            }
-            if (TextUtils.equals(mSubtypeName, other.mSubtypeName)) {
-                return 0;
-            }
-            if (mIsSystemLocale) {
-                return -1;
-            }
-            if (other.mIsSystemLocale) {
-                return 1;
-            }
-            if (mIsSystemLanguage) {
-                return -1;
-            }
-            if (other.mIsSystemLanguage) {
-                return 1;
+            int result = compareNullableCharSequences(mImeName, other.mImeName);
+            if (result != 0) {
+                return result;
             }
-            if (TextUtils.isEmpty(mSubtypeName)) {
-                return 1;
+            result = compareNullableCharSequences(mSubtypeName, other.mSubtypeName);
+            if (result != 0) {
+                return result;
             }
-            if (TextUtils.isEmpty(other.mSubtypeName)) {
-                return -1;
+            result = (mIsSystemLocale ? -1 : 0) - (other.mIsSystemLocale ? -1 : 0);
+            if (result != 0) {
+                return result;
             }
-            return mSubtypeName.toString().compareTo(other.mSubtypeName.toString());
+            return (mIsSystemLanguage ? -1 : 0) - (other.mIsSystemLanguage ? -1 : 0);
         }
 
         @Override

diff --git a/core/tests/coretests/src/com/android/internal/inputmethod/InputMethodSubtypeSwitchingControllerTest.java b/core/tests/coretests/src/com/android/internal/inputmethod/InputMethodSubtypeSwitchingControllerTest.java
index 34c34d7..dc75417 100644 (file)
--- a/core/tests/coretests/src/com/android/internal/inputmethod/InputMethodSubtypeSwitchingControllerTest.java
+++ b/core/tests/coretests/src/com/android/internal/inputmethod/InputMethodSubtypeSwitchingControllerTest.java
@@ -33,7 +33,8 @@ import java.util.Arrays;
 import java.util.List;
 
 public class InputMethodSubtypeSwitchingControllerTest extends InstrumentationTestCase {
-    private static final String DUMMY_PACKAGE_NAME = "dymmy package name";
+    private static final String DUMMY_PACKAGE_NAME = "dummy package name";
+    private static final String DUMMY_IME_LABEL = "dummy ime label";
     private static final String DUMMY_SETTING_ACTIVITY_NAME = "";
     private static final boolean DUMMY_IS_AUX_IME = false;
     private static final boolean DUMMY_FORCE_DEFAULT = false;
@@ -88,6 +89,35 @@ public class InputMethodSubtypeSwitchingControllerTest extends InstrumentationTe
         }
     }
 
+    private static ImeSubtypeListItem createDummyItem(String imeName,
+            String subtypeName, String subtypeLocale, int subtypeIndex, String systemLocale) {
+        final ResolveInfo ri = new ResolveInfo();
+        final ServiceInfo si = new ServiceInfo();
+        final ApplicationInfo ai = new ApplicationInfo();
+        ai.packageName = DUMMY_PACKAGE_NAME;
+        ai.enabled = true;
+        si.applicationInfo = ai;
+        si.enabled = true;
+        si.packageName = DUMMY_PACKAGE_NAME;
+        si.name = imeName;
+        si.exported = true;
+        si.nonLocalizedLabel = DUMMY_IME_LABEL;
+        ri.serviceInfo = si;
+        ArrayList<InputMethodSubtype> subtypes = new ArrayList<>();
+        subtypes.add(new InputMethodSubtypeBuilder()
+                .setSubtypeNameResId(0)
+                .setSubtypeIconResId(0)
+                .setSubtypeLocale(subtypeLocale)
+                .setIsAsciiCapable(true)
+                .build());
+        final InputMethodInfo imi = new InputMethodInfo(ri, DUMMY_IS_AUX_IME,
+                DUMMY_SETTING_ACTIVITY_NAME, subtypes, DUMMY_IS_DEFAULT_RES_ID,
+                DUMMY_FORCE_DEFAULT, true /* supportsSwitchingToNextInputMethod */,
+                false /* supportsDismissingWindow */);
+        return new ImeSubtypeListItem(imeName, subtypeName, imi, subtypeIndex, subtypeLocale,
+                systemLocale);
+    }
+
     private static List<ImeSubtypeListItem> createEnabledImeSubtypes() {
         final List<ImeSubtypeListItem> items = new ArrayList<>();
         addDummyImeSubtypeListItems(items, "LatinIme", "LatinIme", Arrays.asList("en_US", "fr"),
@@ -329,4 +359,56 @@ public class InputMethodSubtypeSwitchingControllerTest extends InstrumentationTe
         assertFalse(item_e.mIsSystemLocale);
         assertFalse(item_EN_US.mIsSystemLocale);
     }
+
+    @SmallTest
+    public void testImeSubtypeListComparator() throws Exception {
+        {
+            final List<ImeSubtypeListItem> items = Arrays.asList(
+                    createDummyItem("X", "A", "en_US", 0, "en_US"),
+                    createDummyItem("X", "A", "en", 1, "en_US"),
+                    createDummyItem("X", "A", "ja", 2, "en_US"),
+                    createDummyItem("X", "Z", "en_US", 3, "en_US"),
+                    createDummyItem("X", "Z", "en", 4, "en_US"),
+                    createDummyItem("X", "Z", "ja", 5, "en_US"),
+                    createDummyItem("X", "", "en_US", 6, "en_US"),
+                    createDummyItem("X", "", "en", 7, "en_US"),
+                    createDummyItem("X", "", "ja", 8, "en_US"),
+                    createDummyItem("Y", "A", "en_US", 9, "en_US"),
+                    createDummyItem("Y", "A", "en", 10, "en_US"),
+                    createDummyItem("Y", "A", "ja", 11, "en_US"),
+                    createDummyItem("Y", "Z", "en_US", 12, "en_US"),
+                    createDummyItem("Y", "Z", "en", 13, "en_US"),
+                    createDummyItem("Y", "Z", "ja", 14, "en_US"),
+                    createDummyItem("Y", "", "en_US", 15, "en_US"),
+                    createDummyItem("Y", "", "en", 16, "en_US"),
+                    createDummyItem("Y", "", "ja", 17, "en_US"),
+                    createDummyItem("", "A", "en_US", 18, "en_US"),
+                    createDummyItem("", "A", "en", 19, "en_US"),
+                    createDummyItem("", "A", "ja", 20, "en_US"),
+                    createDummyItem("", "Z", "en_US", 21, "en_US"),
+                    createDummyItem("", "Z", "en", 22, "en_US"),
+                    createDummyItem("", "Z", "ja", 23, "en_US"),
+                    createDummyItem("", "", "en_US", 24, "en_US"),
+                    createDummyItem("", "", "en", 25, "en_US"),
+                    createDummyItem("", "", "ja", 26, "en_US"));
+
+            for (int i = 0; i < items.size(); ++i) {
+                assertEquals(0, items.get(i).compareTo(items.get(i)));
+                for (int j = i + 1; j < items.size(); ++j) {
+                    assertTrue(items.get(i).compareTo(items.get(j)) < 0);
+                    assertTrue(items.get(j).compareTo(items.get(i)) > 0);
+                }
+            }
+        }
+
+        {
+            // Following two items have the same priority.
+            final ImeSubtypeListItem nonSystemLocale1 =
+                    createDummyItem("X", "A", "ja_JP", 0, "en_us");
+            final ImeSubtypeListItem nonSystemLocale2 =
+                    createDummyItem("X", "A", "hi_IN", 1, "en_us");
+            assertEquals(0, nonSystemLocale1.compareTo(nonSystemLocale2));
+            assertEquals(0, nonSystemLocale2.compareTo(nonSystemLocale1));
+        }
+    }
 }