Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restart all modern Samsung keyboard IMM #12780

Merged
merged 10 commits into from
Oct 4, 2019
Merged

Conversation

GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Oct 3, 2019

Turns out the restart workaround introduced in #12432 is applicable to all languages in the Samsung keyboard.

This PR enables the workaround on all Samsung devices using Samsung keyboard with Android Lollipop and newer as the oldest affected device shipped with Lollipop.

Fixes flutter/flutter#31512 (workaround)

? 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

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

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

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

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

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

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.

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

? 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

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.

// All modern Samsung keybords are affected including non-korean languages and thus
// need the restart.
@Test
public void setTextInputEditingState_alwaysRestartsOnAffectedDevices2() {
Copy link
Contributor

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.

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for following up and fixing this one too! Should make people very happy.

@justinmc
Copy link
Contributor

justinmc commented Oct 4, 2019

Do you think this issue be fixed by this as well? flutter/flutter#30656

@justinmc
Copy link
Contributor

justinmc commented Oct 4, 2019

One more that might be fixed by this... 🤞 flutter/flutter#40153

@GaryQian
Copy link
Contributor Author

GaryQian commented Oct 4, 2019

Well, the common thread seems to be Samsung keyboard, so hopefully these problems all go away...

@GaryQian GaryQian merged commit ecf4f46 into flutter:master Oct 4, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 4, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 7, 2019
git@github.com:flutter/engine.git/compare/7d90779bb66f...1d62160

git log 7d90779..1d62160 --no-merges --oneline
2019-10-04 iska.kaushik@gmail.com Prettify all CMX files (flutter/engine#12800)
2019-10-04 garyq@google.com Restart all modern Samsung keyboard IMM (flutter/engine#12780)
2019-10-04 dnfield@google.com Reland fuchsia build improvements (flutter/engine#12795)
2019-10-04 50856934+nturgut@users.noreply.github.com Fixing selection issues in Firefox (flutter/engine#12793)
2019-10-04 chinmaygarde@google.com Disable EmbedderTest::CanLaunchAndShutdownMultipleTimes. (flutter/engine#12799)
2019-10-04 iska.kaushik@gmail.com [flutter_runner] Update the cmx files to include TZ support (flutter/engine#12798)
2019-10-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia fbdf48ecb204..95edac1c9a4a (1 commits) (flutter/engine#12790)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC liyuqian@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
git@github.com:flutter/engine.git/compare/7d90779bb66f...1d62160

git log 7d90779..1d62160 --no-merges --oneline
2019-10-04 iska.kaushik@gmail.com Prettify all CMX files (flutter/engine#12800)
2019-10-04 garyq@google.com Restart all modern Samsung keyboard IMM (flutter/engine#12780)
2019-10-04 dnfield@google.com Reland fuchsia build improvements (flutter/engine#12795)
2019-10-04 50856934+nturgut@users.noreply.github.com Fixing selection issues in Firefox (flutter/engine#12793)
2019-10-04 chinmaygarde@google.com Disable EmbedderTest::CanLaunchAndShutdownMultipleTimes. (flutter/engine#12799)
2019-10-04 iska.kaushik@gmail.com [flutter_runner] Update the cmx files to include TZ support (flutter/engine#12798)
2019-10-04 skia-flutter-autoroll@skia.org Roll src/third_party/skia fbdf48ecb204..95edac1c9a4a (1 commits) (flutter/engine#12790)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC liyuqian@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
@thanhhai08sk
Copy link

Awesome! Thanks so much for fixing this!

@MXNXV-ERR
Copy link

MXNXV-ERR commented Jul 31, 2024

Locking caps lock and then typing resets the locked Caps lock after 1 character is typed on "Samsung keyboard" , in Text field
Can this be due to the restart of keyboard IMM resetting the keyboard state?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Samsung Keyboard duplicates text when restoring composing regions (restart text input, punctuation)
6 participants