From 410cc501b2066bc2fddbe58a67260673df642aae Mon Sep 17 00:00:00 2001 From: Yohei Yukawa Date: Tue, 24 Jan 2017 00:29:07 -0800 Subject: [PATCH] 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 --- ...InputMethodSubtypeSwitchingController.java | 49 +++++------ ...tMethodSubtypeSwitchingControllerTest.java | 84 ++++++++++++++++++- 2 files changed, 104 insertions(+), 29 deletions(-) diff --git a/core/java/com/android/internal/inputmethod/InputMethodSubtypeSwitchingController.java b/core/java/com/android/internal/inputmethod/InputMethodSubtypeSwitchingController.java index 8d117837df834..a94b1612e8a3d 100644 --- 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; + int result = compareNullableCharSequences(mImeName, other.mImeName); + if (result != 0) { + return result; } - if (TextUtils.isEmpty(other.mImeName)) { - return -1; + result = compareNullableCharSequences(mSubtypeName, other.mSubtypeName); + if (result != 0) { + return result; } - if (!TextUtils.equals(mImeName, other.mImeName)) { - return mImeName.toString().compareTo(other.mImeName.toString()); + result = (mIsSystemLocale ? -1 : 0) - (other.mIsSystemLocale ? -1 : 0); + if (result != 0) { + return result; } - 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; - } - if (TextUtils.isEmpty(mSubtypeName)) { - return 1; - } - if (TextUtils.isEmpty(other.mSubtypeName)) { - return -1; - } - 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 34c34d7c07bb9..dc7541709dac1 100644 --- 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 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 createEnabledImeSubtypes() { final List 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 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)); + } + } }