Add image keyboard support on Android - #87203

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot I fixed it

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot I consent.

@tneotia You'll need to do the same rebase or squash here as in the engine PR.

@tneotia You'll need to do the same rebase or squash here as in the engine PR.

Hey, so when I originally checked-out the repo to make the changes, this is what I did (for the engine):

For the framework, I did a similar thing, but instead of checking out a hash, i checked-out a tag. The engine repo doesn't have those tags. The reason I did the reset was because I wanted to make sure that the existing commits were only ones that existed in the 2.2.3 tag, and drop anything newer. And the reason we only wanted the 2.2.3 changes (instead of master) was because we wanted only stable/officially released code that was compatible with the flutter framework v 2.2.3

git checkout <sha256 hash of the cherry picks for 2.2.3>
git reset --hard <sha256 hash>
git branch ......

# made my changes

git commit ......
git push ......

Should this have been done a different way? I am also no git expert, but i generally know the basic actions

Somewhere in there you need to either squash the commits down to one commit, or rebase them so that the only changes between the master branch and your branch are your changes. The CLA bot doesn't really like any PR where more than one author appears in the list of changes.

The workflow I use is:

# Make a new branch
git fetch origin
git fetch upstream
git checkout master
git checkout -b git-insertion-pr
git push -u origin git-insertion-pr
# Make changes and push
git commit -m "message"
git push
# More changes
git commit -m "message"
# Incorporate changes made to master
git fetch upstream
git rebase upstream/master
# Fix conflicts
git push --force origin git-insertion-pr

With git, there are a number of ways you can accomplish the same thing (sadly), but this one works for me.

I think the problem here is that you need to rebase on top of master, so the relevant commands would be:

git fetch upstream
git rebase upstream/master
# Fix conflicts
git push --force origin git-insertion-pr

This all assumes that the output of git remote -v looks like:

origin	[email protected]:BlueBubblesApp/flutter.git (fetch)
origin	[email protected]:BlueBubblesApp/flutter.git (push)
upstream	[email protected]:flutter/flutter.git (fetch)
upstream	[email protected]:flutter/flutter.git (push)

If that's not the case, then you'll probably have to at least set up the upstream that way and try rebasing.

@gspencergoog this one should be fixed now as well. Again thanks for your help, learned something new today!

@gspencergoog this one should be fixed now as well. Again thanks for your help, learned something new today!

Beautiful, thank you for doing that

I've left a review (flutter-team-archive/engine#27763 (review)) on the engine PR in order to find consensus on this approach before I get too detailed with a review here.

I've left a review (flutter/engine#27763 (review)) on the engine PR in order to find consensus on this approach before I get too detailed with a review here.

Hopefully this isn't a dumb question, but is there a way to use the engine in "dev" mode? So I don't need to rebuild the engine each time I make a change and need to test it? That's how I've been doing it because I never found a way to use the engine without it being compiled first.

In the meantime... I will be recompiling the engine with some changes to test how changing the mimeType support list will work.

Edit 1: So, I just rebuilt the engine after removing all the mimeTypes from the setContentMimeType list. For whatever reason, GIF insertion still worked. This leads me to believe that calling setContentMimeType doesn't do anything... I actually pulled it from Anton here: #20796 (comment).

Now, I am rebuilding the engine with only 1 mime type (png) to see if i can still insert GIFs... if that still works, I'm going to try and remove the setContentMimeType call entirely and see what happens...

@zlshames Sorry I missed your question before, but no, unfortunately you must rebuild the engine with ninja as in the compiling the engine wiki page each time you make a change, the run your Flutter app with --local-engine=.... If you've already built it once, it should be much faster than the first time.

@zlshames Sorry I missed your question before, but no, unfortunately you must rebuild the engine with ninja as in the compiling the engine wiki page each time you make a change, the run your Flutter app with --local-engine=.... If you've already built it once, it should be much faster than the first time.

That's what I figured, and have been doing. No worries! Been busy working on our latest app release, but will get back to working on this PR (hopefully) shortly

Sounds good, happy to take a look whenever you have time to update it 👍

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@zlshames No pressure, just curious if you've had a chance to look at this?

@zlshames No pressure, just curious if you've had a chance to look at this?

Apologies! I have not forgotten about it. Just am super busy with the holiday season right now. And previously, with working on updates to a different part of the app that uses this custom code. I know @tneotia updated our code, I just need to test setting custom MIME Types so users can select which they want to handle (vs. just automatically handling every type). I would like to do it by the end of the year.

If no one sees an update by me nearing Christmas, give me a ping and I'll try to get on it :)

@zlshames I hope you had a nice holiday. Just checking in, I know lots of people are looking forward to this PR, and it seems that you were fairly close with only a few failing checks. Any update or ETA?

@justinmc I made a bunch of changes and comments to your comments. Let me know if there is anything else.

Alright, someone might have to help me out here. Admittedly, I'm not a full-time Dart developer. I can't for the life of me figure out what's wrong in our template comment's sample code. It's saying that it's receiving an unexpected character, which I would have assumed would have been due to the unescaped single-quote, but it still fails when the quote is escaped.

@zlshames @tneotia Are you all still available to help get this PR merged? Would you be able to merge in master (or rebase) to update the branch and get the checks to run again? It looks like the analyzer failure is an infrastructure thing.

@justinmc I could have sworn we discussed needing to refactor it and decouple it from the TextInputClient code. Our other PR was closed due to inactivity as well as that they weren't sure if it was how they would want to architect the solution. @tneotia correct me if I'm misremembering. We can always reopen the PR, but I just don't think this is the official solution they wanted.

If anyone from Google wants to chime in and offer their opinion, we are open! Obviously, we would love to get this native in Flutter. But at the same time, I'm not sure where to refactor it so that it's decoupled

After catching back up on the code here and in the engine PR, I think the current approach with TextInputClient is already the best option 👍 . @zlshames I think if you re-open the engine PR and get both PRs to pass CI, then I can re-review and get this moving again without any major changes.

There was a strong argument made for decoupling this from TextInputClient here, but I think that this feature is fundamentally a part of TextInputClient. Sending rich image content from the platform keyboard requires TextInputClient's input connection. Very custom behavior is still possible (say for someone writing a custom text editor) by overriding commitContent.

In general we should work to decouple our platform channel methods as discussed in my design doc, but in this case the logic belongs in TextInputClient.

CC @LongCatIsLooong, I believe you and I discussed this a few weeks ago and came to the same conclusion.

I am going back to school shortly, but I will definitely have some time next week to mess with this and bring it back up to date. I may re-PR this just because there are way, way, way too many commits here.

Building our app with a custom flutter engine has been.... well... not great. We'd like to get this into a stable Flutter release ASAP! :)

Closing this in favor of #110052. Will leave a review over there soon. Thanks!