From ee157f386e9f576d9a278cb4e36938148fe2f0d0 Mon Sep 17 00:00:00 2001 From: LongCat is Looong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Mon, 8 Feb 2021 11:46:31 -0800 Subject: [PATCH 1/4] remove samsung restart input workaround --- .../plugin/editing/TextInputPlugin.java | 36 ++----------------- .../plugin/editing/TextInputPluginTest.java | 18 +++++----- 2 files changed, 13 insertions(+), 41 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index 2a01930ab5154..5c2d49e6028b6 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -9,7 +9,6 @@ import android.graphics.Rect; import android.os.Build; import android.os.Bundle; -import android.provider.Settings; import android.text.Editable; import android.text.InputType; import android.util.SparseArray; @@ -22,7 +21,6 @@ import android.view.inputmethod.EditorInfo; import android.view.inputmethod.InputConnection; import android.view.inputmethod.InputMethodManager; -import android.view.inputmethod.InputMethodSubtype; import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; @@ -49,7 +47,6 @@ public class TextInputPlugin implements ListenableEditingState.EditingStateWatch @Nullable private InputConnection lastInputConnection; @NonNull private PlatformViewsController platformViewsController; @Nullable private Rect lastClientRect; - private final boolean restartAlwaysRequired; private ImeSyncDeferringInsetsCallback imeSyncCallback; private AndroidKeyProcessor keyProcessor; @@ -161,7 +158,6 @@ public void sendAppPrivateCommand(String action, Bundle data) { this.platformViewsController = platformViewsController; this.platformViewsController.attachTextInputPlugin(this); - restartAlwaysRequired = isRestartAlwaysRequired(); } @NonNull @@ -422,9 +418,9 @@ void setTextInputEditingState(View view, TextInputChannel.TextEditState state) { mLastKnownFrameworkTextEditingState = state; mEditable.setEditingState(state); - // Restart if there is a pending restart or the device requires a force restart - // (see isRestartAlwaysRequired). Restarting will also update the selection. - if (restartAlwaysRequired || mRestartInputPending) { + // Restart if there is a pending restart. Restarting will also update the + // selection. + if (mRestartInputPending) { mImm.restartInput(view); mRestartInputPending = false; } @@ -474,32 +470,6 @@ public void inspect(double x, double y) { (int) Math.ceil(minMax[3] * density)); } - // Samsung's Korean keyboard has a bug where it always attempts to combine characters based on - // its internal state, ignoring if and when the cursor is moved programmatically. The same bug - // also causes non-korean keyboards to occasionally duplicate text when tapping in the middle - // of existing text to edit it. - // - // Fully restarting the IMM works around this because it flushes the keyboard's internal state - // and stops it from trying to incorrectly combine characters. However this also has some - // negative performance implications, so we don't want to apply this workaround in every case. - @SuppressLint("NewApi") // New API guard is inline, the linter can't see it. - @SuppressWarnings("deprecation") - private boolean isRestartAlwaysRequired() { - InputMethodSubtype subtype = mImm.getCurrentInputMethodSubtype(); - // Impacted devices all shipped with Android Lollipop or newer. - if (subtype == null - || Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP - || !Build.MANUFACTURER.equals("samsung")) { - return false; - } - String keyboardName = - Settings.Secure.getString( - mView.getContext().getContentResolver(), Settings.Secure.DEFAULT_INPUT_METHOD); - // The Samsung keyboard is called "com.sec.android.inputmethod/.SamsungKeypad" but look - // for "Samsung" just in case Samsung changes the name of the keyboard. - return keyboardName.contains("Samsung"); - } - @VisibleForTesting void clearTextInputClient() { if (inputTarget.type == InputTarget.Type.PLATFORM_VIEW) { diff --git a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java index d936c13918d6c..4797286bad8f8 100644 --- a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java +++ b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java @@ -339,13 +339,15 @@ public void setTextInputEditingState_alwaysSetEditableWhenDifferent() { assertTrue(textInputPlugin.getEditable().toString().equals("Shibuyawoo")); } - // See https://github.com/flutter/flutter/issues/29341 and - // https://github.com/flutter/flutter/issues/31512 - // All modern Samsung keybords are affected including non-korean languages and thus - // need the restart. + // Regression test for https://github.com/flutter/flutter/issues/73433. + // See also: https://github.com/flutter/flutter/issues/29341 and + // https://github.com/flutter/flutter/issues/31512. + // All modern Samsung keybords were affected including non-korean languages + // and thus needed the restart. The restart workaround seems to have caused + // #73433 and it's no longer needed. @Test - public void setTextInputEditingState_alwaysRestartsOnAffectedDevices2() { - // Initialize a TextInputPlugin that needs to be always restarted. + public void setTextInputEditingState_DontForceRestartOnPreviouslyAffectedSamsungDevices() { + // Initialize a TextInputPlugin with a Samsung keypad. ShadowBuild.setManufacturer("samsung"); InputMethodSubtype inputMethodSubtype = new InputMethodSubtype(0, 0, /*locale=*/ "en", "", "", false, false); @@ -382,8 +384,8 @@ public void setTextInputEditingState_alwaysRestartsOnAffectedDevices2() { textInputPlugin.setTextInputEditingState( testView, new TextInputChannel.TextEditState("", 0, 0, -1, -1)); - // Verify that we've restarted the input. - assertEquals(2, testImm.getRestartCount(testView)); + // Verify that we've NOT restarted the input. + assertEquals(1, testImm.getRestartCount(testView)); } @Test From 33fdb7fdb6f11483ffd73932b34b5f5e936cba08 Mon Sep 17 00:00:00 2001 From: LongCat is Looong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Tue, 9 Feb 2021 23:30:01 -0800 Subject: [PATCH 2/4] Remove the Samsung compsoing region fix on newer Samsung keyboards --- .../plugin/editing/TextInputPlugin.java | 56 ++++++++++++++- .../plugin/editing/TextInputPluginTest.java | 72 +++++++++++++++++-- 2 files changed, 120 insertions(+), 8 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index 5c2d49e6028b6..57a527904f026 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -6,9 +6,11 @@ import android.annotation.SuppressLint; import android.content.Context; +import android.content.pm.PackageManager; import android.graphics.Rect; import android.os.Build; import android.os.Bundle; +import android.provider.Settings; import android.text.Editable; import android.text.InputType; import android.util.SparseArray; @@ -21,6 +23,7 @@ import android.view.inputmethod.EditorInfo; import android.view.inputmethod.InputConnection; import android.view.inputmethod.InputMethodManager; +import android.view.inputmethod.InputMethodSubtype; import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; @@ -47,6 +50,7 @@ public class TextInputPlugin implements ListenableEditingState.EditingStateWatch @Nullable private InputConnection lastInputConnection; @NonNull private PlatformViewsController platformViewsController; @Nullable private Rect lastClientRect; + private final boolean restartAlwaysRequired; private ImeSyncDeferringInsetsCallback imeSyncCallback; private AndroidKeyProcessor keyProcessor; @@ -158,6 +162,7 @@ public void sendAppPrivateCommand(String action, Bundle data) { this.platformViewsController = platformViewsController; this.platformViewsController.attachTextInputPlugin(this); + restartAlwaysRequired = isRestartAlwaysRequired(); } @NonNull @@ -418,9 +423,9 @@ void setTextInputEditingState(View view, TextInputChannel.TextEditState state) { mLastKnownFrameworkTextEditingState = state; mEditable.setEditingState(state); - // Restart if there is a pending restart. Restarting will also update the - // selection. - if (mRestartInputPending) { + // Restart if there is a pending restart or the device requires a force restart + // (see isRestartAlwaysRequired). Restarting will also update the selection. + if (restartAlwaysRequired || mRestartInputPending) { mImm.restartInput(view); mRestartInputPending = false; } @@ -470,6 +475,51 @@ public void inspect(double x, double y) { (int) Math.ceil(minMax[3] * density)); } + // Samsung's Korean keyboard has a bug where it always attempts to combine characters based on + // its internal state, ignoring if and when the cursor is moved programmatically. The same bug + // also causes non-korean keyboards to occasionally duplicate text when tapping in the middle + // of existing text to edit it. + // + // Fully restarting the IMM works around this because it flushes the keyboard's internal state + // and stops it from trying to incorrectly combine characters. However this also has some + // negative performance implications, so we don't want to apply this workaround in every case. + @SuppressLint("NewApi") // New API guard is inline, the linter can't see it. + @SuppressWarnings("deprecation") + private boolean isRestartAlwaysRequired() { + InputMethodSubtype subtype = mImm.getCurrentInputMethodSubtype(); + // Impacted devices all shipped with Android Lollipop or newer. + if (subtype == null + || Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP + || !Build.MANUFACTURER.equals("samsung")) { + return false; + } + String keyboardName = + Settings.Secure.getString( + mView.getContext().getContentResolver(), Settings.Secure.DEFAULT_INPUT_METHOD); + // The Samsung keyboard is called "com.sec.android.inputmethod/.SamsungKeypad" but look + // for "Samsung" just in case Samsung changes the name of the keyboard. + if (!keyboardName.contains("Samsung")) { + return false; + } + + final long versionCode; + try { + versionCode = + mView + .getContext() + .getPackageManager() + .getPackageInfo("com.sec.android.inputmethod", 0) + .getLongVersionCode(); + } catch (PackageManager.NameNotFoundException e) { + Log.w(TAG, "com.sec.android.inputmethod is not installed."); + return false; + } + + // 3.3.31.83 is a known version that's free of the aforementioned bug. + // TODO(LongCatIsLooong): Find the minimum version that has the fix. + return versionCode < 333183070; + } + @VisibleForTesting void clearTextInputClient() { if (inputTarget.type == InputTarget.Type.PLATFORM_VIEW) { diff --git a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java index 4797286bad8f8..c5a3035035f51 100644 --- a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java +++ b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java @@ -18,6 +18,7 @@ import android.annotation.TargetApi; import android.content.Context; +import android.content.pm.PackageInfo; import android.content.res.AssetManager; import android.graphics.Insets; import android.graphics.Rect; @@ -64,6 +65,7 @@ import org.mockito.Mock; import org.robolectric.RobolectricTestRunner; import org.robolectric.RuntimeEnvironment; +import org.robolectric.Shadows; import org.robolectric.annotation.Config; import org.robolectric.annotation.Implementation; import org.robolectric.annotation.Implements; @@ -71,6 +73,7 @@ import org.robolectric.shadows.ShadowAutofillManager; import org.robolectric.shadows.ShadowBuild; import org.robolectric.shadows.ShadowInputMethodManager; +import org.robolectric.shadows.ShadowPackageManager; @Config( manifest = Config.NONE, @@ -339,16 +342,75 @@ public void setTextInputEditingState_alwaysSetEditableWhenDifferent() { assertTrue(textInputPlugin.getEditable().toString().equals("Shibuyawoo")); } - // Regression test for https://github.com/flutter/flutter/issues/73433. // See also: https://github.com/flutter/flutter/issues/29341 and // https://github.com/flutter/flutter/issues/31512. - // All modern Samsung keybords were affected including non-korean languages - // and thus needed the restart. The restart workaround seems to have caused - // #73433 and it's no longer needed. + // Some recent versions of Samsung keybords were affected including non-korean + // languages and thus needed the restart. + @Test + public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() { + // Initialize a TextInputPlugin with a Samsung keypad. + ShadowBuild.setManufacturer("samsung"); + // final ShadowPackageManager packageManager = RuntimeEnvironment.getPackageManager(); + final ShadowPackageManager packageManager = + Shadows.shadowOf( + RuntimeEnvironment.application.getApplicationContext().getPackageManager()); + final PackageInfo info = new PackageInfo(); + info.packageName = "com.sec.android.inputmethod"; + info.versionCode = 200000000; + packageManager.addPackage(info); + InputMethodSubtype inputMethodSubtype = + new InputMethodSubtype(0, 0, /*locale=*/ "en", "", "", false, false); + Settings.Secure.putString( + RuntimeEnvironment.application.getContentResolver(), + Settings.Secure.DEFAULT_INPUT_METHOD, + "com.sec.android.inputmethod/.SamsungKeypad"); + TestImm testImm = + Shadow.extract( + RuntimeEnvironment.application.getSystemService(Context.INPUT_METHOD_SERVICE)); + testImm.setCurrentInputMethodSubtype(inputMethodSubtype); + View testView = new View(RuntimeEnvironment.application); + TextInputChannel textInputChannel = new TextInputChannel(mock(DartExecutor.class)); + TextInputPlugin textInputPlugin = + new TextInputPlugin(testView, textInputChannel, mock(PlatformViewsController.class)); + textInputPlugin.setTextInputClient( + 0, + new TextInputChannel.Configuration( + false, + false, + true, + TextInputChannel.TextCapitalization.NONE, + null, + null, + null, + null, + null)); + // There's a pending restart since we initialized the text input client. Flush that now. + textInputPlugin.setTextInputEditingState( + testView, new TextInputChannel.TextEditState("", 0, 0, -1, -1)); + + // Move the cursor. + assertEquals(1, testImm.getRestartCount(testView)); + textInputPlugin.setTextInputEditingState( + testView, new TextInputChannel.TextEditState("", 0, 0, -1, -1)); + + // Verify that we've NOT restarted the input. + assertEquals(2, testImm.getRestartCount(testView)); + } + + // Regression test for https://github.com/flutter/flutter/issues/73433. + // The restart workaround seems to have caused #73433 and it's no longer + // needed. @Test - public void setTextInputEditingState_DontForceRestartOnPreviouslyAffectedSamsungDevices() { + public void setTextInputEditingState_DontForceRestartOnNewSamsungKeyboard() { // Initialize a TextInputPlugin with a Samsung keypad. ShadowBuild.setManufacturer("samsung"); + final ShadowPackageManager packageManager = + Shadows.shadowOf( + RuntimeEnvironment.application.getApplicationContext().getPackageManager()); + final PackageInfo info = new PackageInfo(); + info.packageName = "com.sec.android.inputmethod"; + info.versionCode = 333183070; + packageManager.addPackage(info); InputMethodSubtype inputMethodSubtype = new InputMethodSubtype(0, 0, /*locale=*/ "en", "", "", false, false); Settings.Secure.putString( From 569de832b6e116e4590fd2521fcb49ba533cbff8 Mon Sep 17 00:00:00 2001 From: LongCat is Looong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Wed, 10 Feb 2021 15:43:45 -0800 Subject: [PATCH 3/4] review --- .../io/flutter/plugin/editing/TextInputPluginTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java index c5a3035035f51..38c557d147b4d 100644 --- a/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java +++ b/shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java @@ -344,13 +344,12 @@ public void setTextInputEditingState_alwaysSetEditableWhenDifferent() { // See also: https://github.com/flutter/flutter/issues/29341 and // https://github.com/flutter/flutter/issues/31512. - // Some recent versions of Samsung keybords were affected including non-korean + // Some recent versions of Samsung keybords are affected including non-korean // languages and thus needed the restart. @Test public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() { // Initialize a TextInputPlugin with a Samsung keypad. ShadowBuild.setManufacturer("samsung"); - // final ShadowPackageManager packageManager = RuntimeEnvironment.getPackageManager(); final ShadowPackageManager packageManager = Shadows.shadowOf( RuntimeEnvironment.application.getApplicationContext().getPackageManager()); @@ -393,13 +392,13 @@ public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() { textInputPlugin.setTextInputEditingState( testView, new TextInputChannel.TextEditState("", 0, 0, -1, -1)); - // Verify that we've NOT restarted the input. + // Verify that we've restarted the input. assertEquals(2, testImm.getRestartCount(testView)); } // Regression test for https://github.com/flutter/flutter/issues/73433. // The restart workaround seems to have caused #73433 and it's no longer - // needed. + // needed on newer versions of Samsung keyboard. @Test public void setTextInputEditingState_DontForceRestartOnNewSamsungKeyboard() { // Initialize a TextInputPlugin with a Samsung keypad. @@ -489,7 +488,7 @@ public void setTextInputEditingState_doesNotRestartOnUnaffectedDevices() { textInputPlugin.setTextInputEditingState( testView, new TextInputChannel.TextEditState("", 0, 0, -1, -1)); - // Verify that we've restarted the input. + // Verify that we've NOT restarted the input. assertEquals(1, testImm.getRestartCount(testView)); } From 4385f6b1e47a6b1282d88cea640ae087febcd87d Mon Sep 17 00:00:00 2001 From: LongCat is Looong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Wed, 10 Feb 2021 18:13:49 -0800 Subject: [PATCH 4/4] lower version --- .../android/io/flutter/plugin/editing/TextInputPlugin.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java index 57a527904f026..6f045eb0c79c1 100644 --- a/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java +++ b/shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java @@ -515,9 +515,10 @@ private boolean isRestartAlwaysRequired() { return false; } - // 3.3.31.83 is a known version that's free of the aforementioned bug. + // 3.3.23.33 is a known version that's free of the aforementioned bug. + // 3.0.24.96 still has this bug. // TODO(LongCatIsLooong): Find the minimum version that has the fix. - return versionCode < 333183070; + return versionCode < 332333999; } @VisibleForTesting