Add support for image insertion on Android #35619

tneotia · 2022-08-23T01:16:40Z

This PR adds support for image keyboard support on Android, utilizing the commitContent function.

Partially addresses flutter/flutter#20796. Associated Flutter PR: flutter/flutter#110052.

This is a re-PR of #27763 to clean up the commits.

cc @justinmc and @zlshames

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

tneotia · 2022-08-23T16:02:15Z

I'm also lost on how I can make this test pass. The error isn't helping me much to be honest:

io.flutter.plugin.editing.InputConnectionAdaptorTest > testCommitContent FAILED
    Wanted but not invoked:
    dartExecutor.send(
        <Capturing argument>,
        <Capturing argument>,
        isNull()
    );
    -> at io.flutter.embedding.engine.dart.DartExecutor.send(DartExecutor.java:223)

    However, there was exactly 1 interaction with this mock:
    dartExecutor.setMessageHandler(
        "flutter/textinput",
        io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler@6030d1b0
    );
    -> at io.flutter.plugin.common.MethodChannel.setMethodCallHandler(MethodChannel.java:149)
        at io.flutter.embedding.engine.dart.DartExecutor.send(DartExecutor.java:223)
        at io.flutter.plugin.editing.InputConnectionAdaptorTest.testCommitContent(InputConnectionAdaptorTest.java:203)

chinmaygarde · 2022-08-25T20:20:26Z

cc @stuartmorgan for finding a reviewer for this.

stuartmorgan-g · 2022-08-26T15:52:56Z

@GaryQian Could you take a look at this?

@gspencergoog or @dkwingsmt should probably look as well given recent changes to the keyboard system.

justinmc

Just a few small comments here. I just reviewed the framework PR and this engine one seems fine, though I'm still trying to get it running locally so I can try it out.

Bump @GaryQian @gspencergoog @dkwingsmt for a secondary review.

shell/platform/android/io/flutter/embedding/engine/systemchannels/TextInputChannel.java

justinmc · 2022-09-14T18:39:07Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+        return false;
+      }
+    } else {
+      return false;


What will this end up looking like in the framework if this isn't supported by the device for some reason? Ideally an app developer would be able to gracefully handle this situation.

There's a toast message displayed that "<app> doesn't support image insertion here". It will display this toast any time this function returns false. To my knowledge, it cannot be customized and is an Android framework thing.

Ok sounds like that's the same as native so that should be fine 👍

justinmc · 2022-09-14T18:41:06Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+          final Map<String, Object> obj = new HashMap<>();
+          obj.put("mimeType", mimeType);
+          obj.put("data", data);
+          obj.put("uri", uri.toString());


Are all 3 of these parameters guaranteed to be valid at this point, or should there be any checking and error handling here?

Not sure to be honest. This is why we made the CommittedContent class parameters nullable, just in case something doesn't work correctly.

So I guess it will be up to the framework to handle an invalid CommittedContent.

Correct, or the developer themselves. They can choose what they want to do if a parameter they need happens to be null.

GaryQian · 2022-09-14T19:53:40Z

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

+
+          // Commit the content to the text input channel and release the permission.
+          textInputChannel.commitContent(mClient, obj);
+          inputContentInfo.releasePermission();


Do you have to release permission when is is null? You request it in line 508 but only release when successful here.

Fixed. Good catch :)

chinmaygarde · 2022-09-15T20:15:57Z

And JIT release builds are failing on what look to be related errors.

tneotia · 2022-09-15T22:52:24Z

Think I fixed the JIT builds by merging upstream

justinmc · 2022-09-16T16:48:37Z

shell/platform/android/test/io/flutter/plugin/editing/InputConnectionAdaptorTest.java

@@ -171,6 +174,44 @@ public void testPerformContextMenuAction_paste() {
    assertTrue(editable.toString().startsWith(textToBePasted));
  }

+  @Test
+  public void testCommitContent() throws JSONException {


Heads up that this test appears to be failing

io.flutter.plugin.editing.InputConnectionAdaptorTest > testCommitContent FAILED Wanted but not invoked: dartExecutor.send( <Capturing argument>, <Capturing argument>, isNull() ); -> at io.flutter.embedding.engine.dart.DartExecutor.send(DartExecutor.java:223) However, there was exactly 1 interaction with this mock: dartExecutor.setMessageHandler( "flutter/textinput", io.flutter.plugin.common.MethodChannel$IncomingMethodCallHandler@18698c6a ); -> at io.flutter.plugin.common.MethodChannel.setMethodCallHandler(MethodChannel.java:146) at io.flutter.embedding.engine.dart.DartExecutor.send(DartExecutor.java:223) at io.flutter.plugin.editing.InputConnectionAdaptorTest.testCommitContent(InputConnectionAdaptorTest.java:203)

I'm aware of this, the error hasn't really helped me to figure out why its failing. No idea what needs to change to get it to work.

chinmaygarde · 2022-09-22T20:11:44Z

@justinmc @GaryQian Are you able to troubleshoot @tneotia s test issue? Perhaps we need to rope in others?

justinmc · 2022-09-29T18:53:20Z

I tried to look into this but locally I'm always getting a bunch of failures in the java tests, even on main. Maybe something with my setup...

zanderso · 2022-09-29T20:28:51Z

From PR review triage: It looks like the next step on this PR is for @tneotia to fix the presubmit test failures, which do appear related to this change.

chinmaygarde · 2022-10-06T20:17:02Z

Closing this as stale. Please re-open once progress can be made to patch the presubmit failures.

tneotia · 2022-10-06T20:39:20Z

I really wish this wouldn't be closed ... I just need a little help getting this test to run. I have no experience writing tests and only have a windows system so I don't even have a great way to run these.

So close to getting this merged yet so far away

justinmc · 2022-10-07T18:32:37Z

I really want to see this merged too. I'm going to be out for a week but when I get back I'll try to take a look at the test again if I have time. Not sure what's going on with my engine setup.

mrorabau · 2022-10-09T15:45:30Z

@chinmaygarde @gspencergoog @dkwingsmt @justinmc @tneotia

I'm sure this will come across as a rant, but it is meant more of a general observation. There are several flutter projects (including the engine itself) that are backed by google where PRs for fixes or enhancements are at least encouraged in the documentation. When suggestions or questions are asked in advance, maintainers (especially google staff) tend to remain somewhat silent or request that a PR just be submitted. When in fact one or more outside contributors attempt to create and submit a PR, if any issues come up (such as with unit tests), there is little-to-no assistance to be found from google maintainers. While I know myself, and I suspect most others appreciate the hard work that the flutter team (including google) puts in to everything, I think providing some minimal dedicated time to assisting free contributions would go a long way to both helping improve the platform and the general flutter community. I would bet a little bit of help would encourage those contributing to learn the specifics of why a PR failed to past tests, and/or how code could better align with the coding style of a particular project and most importantly, encourage contributors to make additional submissions.

Instead, even basic functionality included in other native apps are missing, often for months/years, such as this PR. When the public community attempts to fill in the gaps they are met with a (unintended?) wall of resistance both technically and from the maintainers. I can state that this sentiment is shared with several other OS contributors I've communicated with.

If there is a better way for contributors to request assistance, or if in reality the maintainers do not feel these contributions to be beneficial, I think it would at least be fair to mention this honestly in the documentation so that contributors can reduce wasting time on these efforts, and instead provide submissions to other open source projects.

Thanks for your time, and I hope also that this PR gets re-opened.

zlshames · 2022-10-09T16:15:55Z

@chinmaygarde @gspencergoog @dkwingsmt @justinmc @tneotia

I'm sure this will come across as a rant, but it is meant more of a general observation. There are several flutter projects (including the engine itself) that are backed by google where PRs for fixes or enhancements are at least encouraged in the documentation. When suggestions or questions are asked in advance, maintainers (especially google staff) tend to remain somewhat silent or request that a PR just be submitted. When in fact one or more outside contributors attempt to create and submit a PR, if any issues come up (such as with unit tests), there is little-to-no assistance to be found from google maintainers. While I know myself, and I suspect most others appreciate the hard work that the flutter team (including google) puts in to everything, I think providing some minimal dedicated time to assisting free contributions would go a long way to both helping improve the platform and the general flutter community. I would bet a little bit of help would encourage those contributing to learn the specifics of why a PR failed to past tests, and/or how code could better align with the coding style of a particular project and most importantly, encourage contributors to make additional submissions.

Instead, even basic functionality included in other native apps are missing, often for months/years, such as this PR. When the public community attempts to fill in the gaps they are met with a (unintended?) wall of resistance both technically and from the maintainers. I can state that this sentiment is shared with several other OS contributors I've communicated with.

If there is a better way for contributors to request assistance, or if in reality the maintainers do not feel these contributions to be beneficial, I think it would at least be fair to mention this honestly in the documentation so that contributors can reduce wasting time on these efforts, and instead provide submissions to other open source projects.

Thanks for your time, and I hope also that this PR gets re-opened.

I second this 100%. I know Tanay has worked really hard to get this PR into a state where it can move forward. But he's stuck and any assistance would be greatly appreciated; for a feature that a lot of people are asking for.

This isn't a framework that we created or are super well versed in. So it's already an intimidating codebase and an uphill battle to get it all working between the framework & engine.

ANY guidance on ways to implement things or fix unit tests would go a long way... With just a little effort from the maintainers side

stuartmorgan-g · 2022-10-09T17:23:01Z

@mrorabau The #hackers channel on the Flutter Discord would be a much better place for a general discussion about code review practice than a specific PR.

(I'd be happy to weigh in there, but not here, as I don't want to drive the discussion here further off of the topic of the PR itself. And others who would certainly have relevant things to say aren't even on this PR.)

justinmc · 2022-10-18T23:24:17Z

I'm going to try to push a new commit that fixes the test.

justinmc · 2022-10-18T23:35:05Z

@tneotia I don't have permission to push a commit to your branch, but my fix is here: e5ef030

Feel free to make the changes yourself. Or maybe you can allow me to push my commit by checking "Allow edits and access to secrets by maintainers" at the bottom of the right bar of this PR? I'm not sure how that works exactly.

I also merged main in locally, I'm not sure if you'll need to do that or not.

I tested this locally with its corresponding framework PR (flutter/flutter#110052) too, and everything worked great. Super excited for this feature.

tneotia · 2022-10-18T23:45:54Z

I don't see that button unfortunately, but I will apply your changes in an hour or so. Thank you so, so, SO much for your help on this. Can't wait to get this in a stable flutter release :)

justinmc · 2022-10-18T23:47:24Z

Sounds good, thank you for your work and persistence!

tneotia · 2022-10-19T00:40:57Z

~~Not sure why ci.yaml validation is failing (I merged master),~~ but the test fix is now included here.

tneotia · 2022-10-19T14:07:29Z

@chinmaygarde @zanderso test failures are fixed and everything seems ready to go here :)

justinmc

LGTM 👍. I'm glad the test fix worked!

As mentioned elsewhere, I was able to get this code running with the associated framework PR and its example, and it worked great.

This needs one more approval and then we can merge.

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

GaryQian

LGTM, with a small comment ^^^.

…35619)

Add support for image insertion on Android

4a68d3d

flutter-dashboard bot added the platform-android label Aug 23, 2022

tneotia added 3 commits August 22, 2022 22:01

Fix formatting and compile errors

ebbf8d5

Catch IOException

6b3793b

Fix formatting and unrecognized symbol

eee16ef

tneotia mentioned this pull request Aug 24, 2022

Add support for image insertion on Android flutter/flutter#110052

Merged

8 tasks

zanderso requested review from GaryQian, gspencergoog and dkwingsmt September 8, 2022 20:28

justinmc reviewed Sep 14, 2022

View reviewed changes

GaryQian reviewed Sep 14, 2022

View reviewed changes

Fix nits and release permission in every case

0fa0edd

Merge remote-tracking branch 'upstream/master' into gif-insertion

6313ccb

justinmc reviewed Sep 16, 2022

View reviewed changes

chinmaygarde closed this Oct 6, 2022

justinmc reopened this Oct 18, 2022

justinmc and others added 2 commits October 18, 2022 20:39

Fix test

d6d7031

Merge remote-tracking branch 'upstream/master' into gif-insertion

db0cfee

Merge remote-tracking branch 'upstream/master' into gif-insertion

76e8c2a

tneotia requested review from justinmc and removed request for dkwingsmt and gspencergoog October 19, 2022 01:40

justinmc approved these changes Oct 19, 2022

View reviewed changes

GaryQian reviewed Oct 19, 2022

View reviewed changes

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java Outdated Show resolved Hide resolved

GaryQian approved these changes Oct 19, 2022

View reviewed changes

Make method private

97a2d01

GaryQian added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 20, 2022

auto-submit bot merged commit 4e3a5e3 into flutter:main Oct 20, 2022

engine-flutter-autoroll mentioned this pull request Oct 20, 2022

Roll Flutter Engine from bcd83df4ecbd to 3a7acb3dbe9b (6 revisions) flutter/flutter#113771

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2022

4e3a5e3c4 Add support for image insertion on Android (flutter/engine#…

097fcf6

…35619)

zanderso mentioned this pull request Nov 10, 2022

Image keyboard support flutter/flutter#20796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for image insertion on Android #35619

Add support for image insertion on Android #35619

tneotia commented Aug 23, 2022 •

edited

Loading

tneotia commented Aug 23, 2022

chinmaygarde commented Aug 25, 2022

stuartmorgan-g commented Aug 26, 2022

justinmc left a comment

justinmc Sep 14, 2022

tneotia Sep 14, 2022 •

edited

Loading

justinmc Sep 16, 2022

justinmc Sep 14, 2022

tneotia Sep 14, 2022

justinmc Sep 16, 2022

tneotia Sep 16, 2022

GaryQian Sep 14, 2022

tneotia Sep 14, 2022

chinmaygarde commented Sep 15, 2022

tneotia commented Sep 15, 2022

justinmc Sep 16, 2022

tneotia Sep 16, 2022

chinmaygarde commented Sep 22, 2022

justinmc commented Sep 29, 2022

zanderso commented Sep 29, 2022

chinmaygarde commented Oct 6, 2022

tneotia commented Oct 6, 2022

justinmc commented Oct 7, 2022

mrorabau commented Oct 9, 2022

zlshames commented Oct 9, 2022

stuartmorgan-g commented Oct 9, 2022

justinmc commented Oct 18, 2022

justinmc commented Oct 18, 2022

tneotia commented Oct 18, 2022

justinmc commented Oct 18, 2022

tneotia commented Oct 19, 2022 •

edited

Loading

tneotia commented Oct 19, 2022

justinmc left a comment

GaryQian left a comment

Add support for image insertion on Android #35619

Add support for image insertion on Android #35619

Conversation

tneotia commented Aug 23, 2022 • edited Loading

Pre-launch Checklist

tneotia commented Aug 23, 2022

chinmaygarde commented Aug 25, 2022

stuartmorgan-g commented Aug 26, 2022

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tneotia Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented Sep 15, 2022

tneotia commented Sep 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde commented Sep 22, 2022

justinmc commented Sep 29, 2022

zanderso commented Sep 29, 2022

chinmaygarde commented Oct 6, 2022

tneotia commented Oct 6, 2022

justinmc commented Oct 7, 2022

mrorabau commented Oct 9, 2022

zlshames commented Oct 9, 2022

stuartmorgan-g commented Oct 9, 2022

justinmc commented Oct 18, 2022

justinmc commented Oct 18, 2022

tneotia commented Oct 18, 2022

justinmc commented Oct 18, 2022

tneotia commented Oct 19, 2022 • edited Loading

tneotia commented Oct 19, 2022

justinmc left a comment

Choose a reason for hiding this comment

GaryQian left a comment

Choose a reason for hiding this comment

tneotia commented Aug 23, 2022 •

edited

Loading

tneotia Sep 14, 2022 •

edited

Loading

tneotia commented Oct 19, 2022 •

edited

Loading