-
Notifications
You must be signed in to change notification settings - Fork 6k
Scribe (Android stylus handwriting text input) #52943
Changes from 6 commits
38f1ee1
f5c45e2
2c70875
2da6b2e
815e6cd
da21044
cdf7366
a298ad0
39249a5
14ff786
8217442
d5e0301
39a4c8e
27ca8db
521fb22
2a27c55
5046264
f9856a2
989eb2a
47bd6b7
948d387
c2f1425
5d385c5
0a54134
aa95c06
0962507
885f4bb
8ef8f49
49fa99b
585ac33
250adbc
e96d8bc
8056d17
016223e
cc69690
6f1a6d8
1ae66f1
f019841
d802eb2
3685c9d
0ceab93
bc4dafd
00621e2
53212fc
02c8f43
6d86717
44c22d5
06edcdb
df68cb8
37cab6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| import static io.flutter.Build.API_LEVELS; | ||
|
|
||
| import android.annotation.TargetApi; | ||
| import android.os.Build; | ||
| import android.view.View; | ||
| import android.view.inputmethod.InputMethodManager; | ||
| import androidx.annotation.NonNull; | ||
|
|
@@ -19,18 +20,20 @@ | |
| * | ||
| * <p>The plugin handles requests for scribe sent by the {@link | ||
| * io.flutter.embedding.engine.systemchannels.ScribeChannel}. | ||
| * | ||
| * <p>On API versions below 33, the plugin does nothing. | ||
| */ | ||
| public class ScribePlugin implements ScribeChannel.ScribeMethodHandler { | ||
|
|
||
| private final ScribeChannel mScribeChannel; | ||
| private final InputMethodManager mInputMethodManager; | ||
| @NonNull private final View mView; | ||
| @NonNull private final ScribeChannel mScribeChannel; | ||
| @NonNull private final InputMethodManager mInputMethodManager; | ||
| @NonNull public View mView; | ||
|
||
|
|
||
| @TargetApi(API_LEVELS.API_34) | ||
| @RequiresApi(API_LEVELS.API_34) | ||
| public ScribePlugin( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the view that is passed in here. Views in andoid IIRC do not have a long lifespan and this view is final. What happens when the app is backgrounded?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the FlutterView from when ScribePlugin is created in FlutterView.java: scribePlugin =
new ScribePlugin(
this, textInputPlugin.getInputMethodManager(), this.flutterEngine.getScribeChannel());I need it in order to call InputMethodManager.startStylusHandwriting. Is there a better way to get ahold of the View at that time that's called maybe?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not so confident as to block the pr but I suspect that you can't make that variable final. That there is not a 1 to 1 relationship between flutter view and plugin instantiation. I am specifically thinking of add to app and cached engine scenarios. None of our existing tests would catch this because they would not execute this code path.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (From another thread) I've made a view setter so it can be updated if needed. |
||
| @NonNull View view, @NonNull InputMethodManager imm, @NonNull ScribeChannel scribeChannel) { | ||
| view.setAutoHandwritingEnabled(false); | ||
| if (Build.VERSION.SDK_INT >= API_LEVELS.API_33) { | ||
| view.setAutoHandwritingEnabled(false); | ||
| } | ||
|
|
||
| mView = view; | ||
| mInputMethodManager = imm; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,14 @@ | |
|
|
||
| package io.flutter.embedding.engine.systemchannels; | ||
|
|
||
| import static io.flutter.Build.API_LEVELS; | ||
| import static org.mockito.Mockito.any; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.never; | ||
| import static org.mockito.Mockito.times; | ||
| import static org.mockito.Mockito.verify; | ||
|
|
||
| import android.annotation.TargetApi; | ||
| import androidx.test.ext.junit.runners.AndroidJUnit4; | ||
| import io.flutter.embedding.engine.dart.DartExecutor; | ||
| import io.flutter.plugin.common.BinaryMessenger; | ||
|
|
@@ -19,22 +22,22 @@ | |
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.robolectric.annotation.Config; | ||
|
|
||
| @RunWith(AndroidJUnit4.class) | ||
| public class ScribeChannelTest { | ||
| private static void sendToBinaryMessageHandler( | ||
| private static BinaryMessenger.BinaryReply sendToBinaryMessageHandler( | ||
| BinaryMessenger.BinaryMessageHandler binaryMessageHandler, String method) { | ||
| MethodCall methodCall = new MethodCall(method, null); | ||
| ByteBuffer encodedMethodCall = StandardMethodCodec.INSTANCE.encodeMethodCall(methodCall); | ||
| binaryMessageHandler.onMessage( | ||
| (ByteBuffer) encodedMethodCall.flip(), mock(BinaryMessenger.BinaryReply.class)); | ||
| BinaryMessenger.BinaryReply mockReply = mock(BinaryMessenger.BinaryReply.class); | ||
| binaryMessageHandler.onMessage((ByteBuffer) encodedMethodCall.flip(), mockReply); | ||
| return mockReply; | ||
| } | ||
|
|
||
| ScribeChannel.ScribeMethodHandler mockHandler; | ||
| BinaryMessenger.BinaryMessageHandler binaryMessageHandler; | ||
|
|
||
| @SuppressWarnings("deprecation") | ||
| // setMessageHandler is deprecated. | ||
| @Before | ||
| public void setUp() { | ||
| ArgumentCaptor<BinaryMessenger.BinaryMessageHandler> binaryMessageHandlerCaptor = | ||
|
|
@@ -45,24 +48,57 @@ public void setUp() { | |
|
|
||
| scribeChannel.setScribeMethodHandler(mockHandler); | ||
|
|
||
| verify(mockBinaryMessenger, times(1)) | ||
| verify((BinaryMessenger) mockBinaryMessenger, times(1)) | ||
| .setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture()); | ||
|
|
||
| binaryMessageHandler = binaryMessageHandlerCaptor.getValue(); | ||
| } | ||
|
|
||
| @Config(minSdk = API_LEVELS.API_34) | ||
| @TargetApi(API_LEVELS.API_34) | ||
| @Test | ||
| public void respondsToStartStylusHandwriting() { | ||
| sendToBinaryMessageHandler(binaryMessageHandler, ScribeChannel.METHOD_START_STYLUS_HANDWRITING); | ||
| BinaryMessenger.BinaryReply mockReply = | ||
| sendToBinaryMessageHandler( | ||
| binaryMessageHandler, ScribeChannel.METHOD_START_STYLUS_HANDWRITING); | ||
|
|
||
| verify(mockReply).reply(any(ByteBuffer.class)); | ||
| verify(mockHandler).startStylusHandwriting(); | ||
| } | ||
|
|
||
| @Config(minSdk = API_LEVELS.API_34) | ||
| @TargetApi(API_LEVELS.API_34) | ||
| @Test | ||
| public void respondsToIsStylusHandwritingAvailable() { | ||
| sendToBinaryMessageHandler( | ||
| binaryMessageHandler, ScribeChannel.METHOD_IS_STYLUS_HANDWRITING_AVAILABLE); | ||
| BinaryMessenger.BinaryReply mockReply = | ||
| sendToBinaryMessageHandler( | ||
| binaryMessageHandler, ScribeChannel.METHOD_IS_STYLUS_HANDWRITING_AVAILABLE); | ||
|
|
||
| verify(mockReply).reply(any(ByteBuffer.class)); | ||
| verify(mockHandler).isStylusHandwritingAvailable(); | ||
| } | ||
|
|
||
| @Config(sdk = API_LEVELS.API_32) | ||
| @TargetApi(API_LEVELS.API_32) | ||
| @Test | ||
| public void respondsToStartStylusHandwritingWhenAPILevelUnsupported() { | ||
| BinaryMessenger.BinaryReply mockReply = | ||
| sendToBinaryMessageHandler( | ||
| binaryMessageHandler, ScribeChannel.METHOD_START_STYLUS_HANDWRITING); | ||
|
|
||
| verify(mockReply).reply(any(ByteBuffer.class)); | ||
|
||
| verify(mockHandler, never()).startStylusHandwriting(); | ||
| } | ||
|
|
||
| @Config(sdk = API_LEVELS.API_33) | ||
| @TargetApi(API_LEVELS.API_33) | ||
| @Test | ||
| public void respondsToIsStylusHandwritingAvailableWhenAPILevelUnsupported() { | ||
| BinaryMessenger.BinaryReply mockReply = | ||
| sendToBinaryMessageHandler( | ||
| binaryMessageHandler, ScribeChannel.METHOD_IS_STYLUS_HANDWRITING_AVAILABLE); | ||
|
|
||
| verify(mockReply).reply(any(ByteBuffer.class)); | ||
| verify(mockHandler, never()).isStylusHandwritingAvailable(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,12 +6,15 @@ | |
|
|
||
| import static io.flutter.Build.API_LEVELS; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertThrows; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import android.annotation.TargetApi; | ||
| import android.content.Context; | ||
| import android.os.Build; | ||
| import android.view.View; | ||
| import android.view.inputmethod.InputMethodManager; | ||
| import androidx.test.core.app.ApplicationProvider; | ||
|
|
@@ -35,7 +38,9 @@ public void setUp() { | |
| ScribeChannel mockScribeChannel = mock(ScribeChannel.class); | ||
| testView = new View(ctx); | ||
| mockImm = mock(InputMethodManager.class); | ||
| when(mockImm.isStylusHandwritingAvailable()).thenReturn(true); | ||
| if (Build.VERSION.SDK_INT >= API_LEVELS.API_34) { | ||
| when(mockImm.isStylusHandwritingAvailable()).thenReturn(true); | ||
| } | ||
| scribePlugin = new ScribePlugin(testView, mockImm, mockScribeChannel); | ||
| } | ||
|
|
||
|
|
@@ -56,4 +61,30 @@ public void scribePluginStartStylusHandwriting() { | |
|
|
||
| verify(mockImm).startStylusHandwriting(testView); | ||
| } | ||
|
|
||
| @Config(sdk = API_LEVELS.API_33) | ||
| @TargetApi(API_LEVELS.API_33) | ||
| @Test | ||
| public void scribePluginStartStylusHandwritingWhenAPILevelUnsupported() { | ||
| assertNotNull(scribePlugin); | ||
|
|
||
| assertThrows( | ||
| NoSuchMethodError.class, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nonblocking: This feels like an odd error to expect.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just confirming that it fails when the API level is too low. I could remove these tests if that's not necessary?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should keep the test it just seems to me that the error would be something like "feature not available" or "api too low" or something. "No such method". That type of error feels like a programing mistake not an error that tells you what is wrong. |
||
| () -> { | ||
| scribePlugin.startStylusHandwriting(); | ||
| }); | ||
| } | ||
|
|
||
| @Config(sdk = API_LEVELS.API_32) | ||
| @TargetApi(API_LEVELS.API_32) | ||
| @Test | ||
| public void scribePluginIsStylusHandwritingAvailableWhenAPILevelUnsupported() { | ||
| assertNotNull(scribePlugin); | ||
|
|
||
| assertThrows( | ||
| NoSuchMethodError.class, | ||
| () -> { | ||
| scribePlugin.isStylusHandwritingAvailable(); | ||
| }); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the dart side of the plugin needs to check if it is running on android and check the api level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess in practice this probably means that you want to call this wrapped in a
try/catchin Dart. My thought process is that this method channel method is just a proxy for InputMethodManager.isStylusHandwritingAvailable. If I'm not even able to call that, I should error, rather than succeed withfalse, which the app developer might interpret to mean that InputMethodManager.isStylusHandwritingAvailable returned false.If that sounds reasonable then I'll at least make sure this is clearly documented in the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update here: #52943 (comment)