Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Restart all modern Samsung keyboard IMM #12780

Merged
GaryQian merged 10 commits into from
Oct 4, 2019
Merged

Restart all modern Samsung keyboard IMM #12780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
665d969
Restart all modern samsung keyboard IMM
GaryQian Oct 3, 2019
fc1e164
Check default keyboard to determine if fix should be applied.
GaryQian Oct 3, 2019
8f9fc09
Change test name
GaryQian Oct 4, 2019
a8008d6
VTOL
GaryQian Oct 4, 2019
5a695e1
Fix tests
GaryQian Oct 4, 2019
f186de1
Imports
GaryQian Oct 4, 2019
582bfc9
putString
GaryQian Oct 4, 2019
2ce3a69
Short circuit to skip keyboard lookup on non-samsung devices
GaryQian Oct 4, 2019
ff6ba89
Remove redundant test, add sdk=27
GaryQian Oct 4, 2019
bf11a2a
Fix test
GaryQian Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import android.annotation.SuppressLint;
import android.content.Context;
import android.os.Build;
import android.provider.Settings;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.VisibleForTesting;
Expand Down Expand Up @@ -315,7 +316,9 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) {
}

// 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.
// 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
Expand All @@ -324,13 +327,14 @@ private void applyStateToSelection(TextInputChannel.TextEditState state) {
@SuppressWarnings("deprecation")
private boolean isRestartAlwaysRequired() {
InputMethodSubtype subtype = mImm.getCurrentInputMethodSubtype();
if (subtype == null) {
// 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 language = (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N)
? subtype.getLanguageTag()
: subtype.getLocale();
return Build.MANUFACTURER.equals("samsung") && language.equals("ko");
String keyboardName = Settings.Secure.getString(mView.getContext().getContentResolver(), Settings.Secure.DEFAULT_INPUT_METHOD);
Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this API call read from disk? I thought this API was slow because it ended up doing I/O, but I could be mistaken here.

Copy link
Contributor Author

@GaryQian GaryQian Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me look it up

Copy link
Contributor Author

@GaryQian GaryQian Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be no mention of it storing it on disk, which I would imagine if it were stored on disk, would at least be mentioned a few times.

Copy link
Contributor Author

@GaryQian GaryQian Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reads from something called sNameValueCache, which I am assuming reads from disk at some point but the values should be cached when we use it here.

Copy link
Contributor Author

@GaryQian GaryQian Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, I added short circuiting to prevent non-samsung devices from performing the lookup.

Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into it, this LGTM.

// 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");
}

private void clearTextInputClient() {
Expand Down
34 changes: 30 additions & 4 deletions shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.flutter.plugin.editing;

import android.content.Context;
import android.provider.Settings;
import android.util.SparseIntArray;
import android.view.View;
import android.view.inputmethod.InputMethodManager;
Expand All @@ -24,7 +25,7 @@
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;

@Config(manifest = Config.NONE, shadows = TextInputPluginTest.TestImm.class)
@Config(manifest = Config.NONE, shadows = TextInputPluginTest.TestImm.class, sdk = 27)
@RunWith(RobolectricTestRunner.class)
public class TextInputPluginTest {
@Test
Expand All @@ -47,12 +48,15 @@ public void setTextInputEditingState_doesNotRestartWhenTextIsIdentical() {
assertEquals(1, testImm.getRestartCount(testView));
}

// See https://github.com/flutter/flutter/issues/29341
// 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.
@Test
public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() {
public void setTextInputEditingState_alwaysRestartsOnAffectedDevices2() {
Copy link
Contributor

@mklim mklim Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are failing because they're running on sdk=16, so the LOLLIPOP check is blocking them. You can change them by adding sdk=27 in the @Config annotation at the top.

On a second look, I'd also just replace the existing setTextInputEditingState_alwaysRestartsOnAffectedDevices with this one since we don't care about special casing the ko locale anymore. I think leaving that test around is a little confusing now.

// Initialize a TextInputPlugin that needs to be always restarted.
ShadowBuild.setManufacturer("samsung");
InputMethodSubtype inputMethodSubtype = new InputMethodSubtype(0, 0, /*locale=*/"ko", "", "", false, false);
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);
Expand All @@ -69,6 +73,28 @@ public void setTextInputEditingState_alwaysRestartsOnAffectedDevices() {
assertEquals(2, testImm.getRestartCount(testView));
}

@Test
public void setTextInputEditingState_doesNotRestartOnUnaffectedDevices() {
// Initialize a TextInputPlugin that needs to be always restarted.
ShadowBuild.setManufacturer("samsung");
InputMethodSubtype inputMethodSubtype = new InputMethodSubtype(0, 0, /*locale=*/"en", "", "", false, false);
Settings.Secure.putString(RuntimeEnvironment.application.getContentResolver(), Settings.Secure.DEFAULT_INPUT_METHOD, "com.fake.test.blah/.NotTheRightKeyboard");
TestImm testImm = Shadow.extract(RuntimeEnvironment.application.getSystemService(Context.INPUT_METHOD_SERVICE));
testImm.setCurrentInputMethodSubtype(inputMethodSubtype);
View testView = new View(RuntimeEnvironment.application);
TextInputPlugin textInputPlugin = new TextInputPlugin(testView, mock(DartExecutor.class), mock(PlatformViewsController.class));
textInputPlugin.setTextInputClient(0, new TextInputChannel.Configuration(false, false, TextInputChannel.TextCapitalization.NONE, 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));

// Move the cursor.
assertEquals(1, testImm.getRestartCount(testView));
textInputPlugin.setTextInputEditingState(testView, new TextInputChannel.TextEditState("", 0, 0));

// Verify that we've restarted the input.
assertEquals(1, testImm.getRestartCount(testView));
}

@Test
public void setTextInputEditingState_nullInputMethodSubtype() {
TestImm testImm = Shadow.extract(RuntimeEnvironment.application.getSystemService(Context.INPUT_METHOD_SERVICE));
Expand Down