compose: Support images from keyboard for Android

[PIG208]

Preview error message

Fixes: #419
Fixes: #1173

[chrisbobbe]

Exciting! Small question from a quick skim; I know this is still a draft.

[chrisbobbe]

This comment is specifically about the case where data ends up being present as an empty array, right? In the code you linked to, I assume that specifically means readStreamFully came up empty in line 531:

https://github.com/flutter/flutter/blob/0ffc4ce00/engine/src/flutter/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java#L531

final byte[] data = this.readStreamFully(is, 64 * 1024);

(Right?)

How about a different case: what could cause data to be null as opposed to present-but-empty? I noticed KeyboardInsertedContent.data is optional. This would be helpful to document upstream there. 🙂

Also, looking around the whole commitContent function you linked to, it looks like there are various unhappy paths there, where the function can't do something desirable and returns false. Like if the Android version is too old. What's the user-facing behavior in those unhappy paths?

[PIG208]

Having swept through the Java code, I couldn't find other places where data is null and gets passed via a textInputChannel.commitContent call. The link points to the only current non-test call site. The Dart code constructor doesn't have code paths where data is explicitly null either.

data can be empty if (a) the file is empty or (b) there was an IOException during the read. So data should probably be null if there is an error reading it, so that we can handle it properly.

---

When returning false, no textInputChannel.commitContent call is made and our handler does not get called at all. But the return value is probably read by some part of the Android SDK code: https://developer.android.com/reference/android/view/inputmethod/BaseInputConnection

Default implementation which invokes View.performReceiveContent on the target view if the view allows content insertion; otherwise returns false without any side effects.

true if this request is accepted by the application, whether the request is already handled or still being handled in background, false otherwise.

I assume the behavior is akin to when we do not provide ContentInsertionConfiguration at all. That also results in this method returning false because the allowedMimeTypes default to an empty list. This gives you an OS-specific error message:

[chrisbobbe]

Thanks! The implementation and tests look reasonable; some nits below. Could you please post screen recordings, showing both #419 and #1173?

[chrisbobbe]

Suggested change

	await enterTopic(tester, narrow: narrow, topic: 'some topic');
	await tester.enterText(contentInputFinder, 'see image: ');
	await enterTopic(tester, narrow: narrow, topic: 'some topic');
	await enterContent(tester, 'see image: ');

[chrisbobbe]

nit: can shorten URL a bit by using just the first 9 characters of the commit ID

[chrisbobbe]

Suggested change

	showErrorDialog(
	context: context,
	title: zulipLocalizations.errorContentNotInsertedTitle,
	message: zulipLocalizations.errorContentToInsertIsEmpty);
	showErrorDialog(context: context,
	title: zulipLocalizations.errorContentNotInsertedTitle,
	message: zulipLocalizations.errorContentToInsertIsEmpty);

[PIG208]

Could you please post screen recordings, showing both #419 and #1173?

Attach from pasting

Screen_Recording_20250124_183348.mp4

Attach from keyboard

Screen_Recording_20250124_183406.mp4

[PIG208]

Thanks for the review! I have updated the PR.

[chrisbobbe]

Thanks! Oh, also: could you check if this feature is available on all Android versions we support, and if it isn't, put a comment somewhere? (I guess with a TODO(android-sdk-<n>) to remove the comment when it becomes unnecessary.)

Otherwise LGTM, marking for Greg's review.

[PIG208]

Looks like the support requires at least API level 25, which corresponds to Android 7.1:

https://github.com/flutter/flutter/blob/0ffc4ce00ea7bb912e379adf39354644eab2c17e/engine/src/flutter/shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java#L498-L500

(image keyboard was added at the same API level)

This is way lower than Android 9, so it should be available on all versions we support.

[chrisbobbe]

Cool; yeah, if 9 is our min-supported, then there's nothing to do here; a comment won't be needed.