compose: Prototype upload-from-library and upload-from-camera #70

chrisbobbe · 2023-04-19T01:18:38Z

Marking as draft while we think through how to get the right photo/video-picker UI on Android (details at #56 (comment) ) but the code is reviewable in the meantime, and people can try out the new features. 🙂

Fixes: #61
Fixes: #56

chrisbobbe · 2023-04-19T18:10:30Z

lib/widgets/compose_box.dart

+    } catch (e) {
+      if (e is PlatformException && e.code == 'camera_access_denied') {


Could one imagine a nice language feature that lets you match on more than just the type of the thing being thrown? The following is already possible:

} on PlatformException catch (e) {

But I'm thinking like, I dunno,

} on PlatformException(code: var c) when c == 'camera_access_denied' catch (e) {

or something?

This may be relevant: dart-lang/language#112 (comment)

This may be relevant: dart-lang/language#112 (comment)

Yeah. Looks like the Dart folks have indeed imagined that feature 🙂 Perhaps it'll get added at some point.

gnprice

Thanks! This looks good. Small comments below.

gnprice · 2023-04-20T19:43:20Z

lib/widgets/compose_box.dart

+      assert(f.readStream != null);  // We passed `withReadStream: true` to pickFiles.
+      return _File(content: f.readStream!, length: f.size, filename: f.name);
+    });
+    _uploadFiles(context: context, contentController: contentController, contentFocusNode: contentFocusNode,


should await — it doesn't directly matter for the behavior, but if e.g. we added some more code after this call, it'd be buggy if we didn't then notice we needed to add an await. And it's a lot easier to notice the need for it now while we're factoring this out.

gnprice · 2023-04-20T19:48:34Z

lib/widgets/dialog.dart

+        onPressed: onActionButtonPress,
+        child: _dialogActionText(actionButtonText ?? 'Continue')),
+    ]));
+}


nit: last line should end with newline, like other lines

gnprice · 2023-04-20T19:57:39Z

lib/widgets/compose_box.dart

+    } catch (e) {
+      if (e is PlatformException && e.code == 'camera_access_denied') {


This may be relevant: dart-lang/language#112 (comment)

Yeah. Looks like the Dart folks have indeed imagined that feature 🙂 Perhaps it'll get added at some point.

gnprice · 2023-04-20T20:01:17Z

lib/widgets/compose_box.dart

+          message: 'To upload an image, please grant Zulip additional permissions in Settings.',
+          actionButtonText: 'Open settings',
+          onActionButtonPress: () {
+            AppSettings.openAppSettings();


Looks like this is from a third-party library: https://pub.dev/packages/app_settings

Have you tried this out on both iOS and Android and it seems to work?

If at any point we're dissatisfied with how it works, the implementation is pretty simple — especially on iOS, where all the AppSettings.openFoo methods do the same thing:
https://github.com/spencerccf/app_settings/blob/ed65816d72933c9110dff9f4a74ee550e2af20b6/ios/Classes/SwiftAppSettingsPlugin.swift#L7-L10
but the Android version isn't complicated either:
https://github.com/spencerccf/app_settings/blob/ed65816d72933c9110dff9f4a74ee550e2af20b6/android/src/main/kotlin/com/example/appsettings/AppSettingsPlugin.kt#L45-L51

AppSettings.openAppSettings() worked great on my iPhone 13 Pro running iOS 16.

Android office device (Samsung Galaxy S9, Android 9) seemed OK, but I wonder: have you ever had an app link directly into the screen for managing an app's permissions? This one brought me here:

and then I scrolled down and found a link to permissions:

which I tapped and found what I needed:

This one brought me here:

Same result when I try that function on my Pixel 5 running Android 13. Except that the subtitle on "Permissions" is "No permissions requested", because we're successfully using the no-privileges-needed API for this on newer Android versions. (To even call this function, I hacked up the code a bit.)

I wonder: have you ever had an app link directly into the screen for managing an app's permissions?

Not sure. It's pretty uncommon for me to encounter an app linking me to any part of its system settings.

(Which doesn't mean we shouldn't do it; for one thing, this is an uncommon case, so it's quite possible that lots of apps offer the same thing if you do get into this case.)

Looking at docs, here's the intent type that the library is using for us:
https://developer.android.com/reference/android/provider/Settings#ACTION_APPLICATION_DETAILS_SETTINGS

Browsing around that page, here's a related intent type:
https://developer.android.com/reference/android/provider/Settings#ACTION_STORAGE_VOLUME_ACCESS_SETTINGS
which was deprecated, saying that "to manage storage permissions for a specific application" we should instead use… ACTION_APPLICATION_DETAILS_SETTINGS, just as app_settings is using for us.

There are various other intent types for particular permissions, including some storage-related:
https://developer.android.com/reference/android/provider/Settings#ACTION_MANAGE_APP_ALL_FILES_ACCESS_PERMISSION
https://developer.android.com/reference/android/provider/Settings#ACTION_REQUEST_MANAGE_MEDIA

I don't think either of those correspond to what we'd want here, though. In particular both of those were introduced only in recent API versions, and I think they correspond to the newer more-explicitly-powerful permissions that were introduced at the same time as making normal applications stop needing such permissions.

That reasoning makes sense.

gnprice · 2023-04-20T20:05:47Z

lib/widgets/compose_box.dart

+  final String filename;
+}
+
+_uploadFiles({


Suggested change

_uploadFiles({

void _uploadFiles({

Otherwise its return type is dynamic.

(There's probably a lint for this, which we should enable. If we ever do want a function returning dynamic, or a variable of type dynamic, etc., I'd want to say so explicitly.)

Future<void>, I think, since it's async?

If we're going to await the result, then definitely Future<void> instead. To explicitly say it's not the caller's business whether this function completes synchronously or does some async work, one can say void.

The docs on this are very terse: https://dart.dev/language/built-in-types

Better description here, perhaps: https://www.reddit.com/r/dartlang/comments/oo65yd/does_void_mean_return_null_in_dart_or_simply_just/

Ah OK, that makes sense. Since you pointed out that we should indeed await the result, in #70 (comment), I plan to go with Future<void>.

gnprice · 2023-04-20T20:06:27Z

lib/widgets/compose_box.dart

+  final ContentTextEditingController contentController;
+  final FocusNode contentFocusNode;
+
+  _handlePress(BuildContext context) async {


Similarly

Suggested change

_handlePress(BuildContext context) async {

dynamic _handlePress(BuildContext context) async {

and I guess I missed that previously on _AttachFileButton.

Hmm, I don't understand this one yet, and what are you referring to with "similarly"? Do you mean Future<void>, or should it really be dynamic?

By "similarly" I mean to not have an implicit dynamic.

But probably we don't actually want dynamic, and void is better.

gnprice · 2023-04-20T20:46:14Z

lib/widgets/compose_box.dart

+class _AttachFromCameraButton extends StatelessWidget {
+  _AttachFromCameraButton({required this.contentController, required this.contentFocusNode});
+
+  final _picker = ImagePicker();


It makes me a bit nervous to be constructing this in an initializer on a widget.

I think the right fix is that we should just construct it in _handlePress. If there were some way that could go wrong on account of constructing it multiple times, then we'd be exposed to the same problem from constructing it here — because we'll be constructing new _AttachFromCameraButton widgets each time StreamComposeBox rebuilds.

In fact, from the implementation, it looks like ImagePicker has no non-static properties at all — its methods (except for == and hashCode inherited from Object) all behave exactly the same on any instance, as if they were static methods. That means that where and when we construct it really doesn't matter.

So probably the methods should be static methods; or the class should have a const constructor, which would accomplish much the same thing (and be a smoother migration from the status quo). But because they're not and it doesn't, the API isn't promising that we have that flexibility. I'd therefore like to avoid the pattern that looks like a bug and makes me nervous 🙂

gnprice · 2023-04-20T20:53:05Z

lib/widgets/compose_box.dart

+    try {
+      result = await FilePicker.platform.pickFiles(type: FileType.media, allowMultiple: true, withReadStream: true);
+    } catch (e) {
+      // TODO(i18n)
+      showErrorDialog(context: context, title: 'Error', message: e.toString());
+      return;
+    }
+    if (result == null) {
+      return; // User cancelled; do nothing
+    }
+
+    if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007
+    else {
+      return;
+    }
+
+    final Iterable<_File> files = result.files.map((f) {
+      assert(f.readStream != null);  // We passed `withReadStream: true` to pickFiles.
+      return _File(content: f.readStream!, length: f.size, filename: f.name);
+    });


Kind of unsatisfying how much of this code gets duplicated between the three buttons, and especially the two that are using FilePicker.

I think it might improve things to factor much of this into something like

abstract class _AttachUploadsButton extends StatelessWidget { // … Icon get icon; Future<Iterable<_File>?> getFiles(BuildContext context); // … }

and then _AttachMediaButton and the others would be subclasses just implementing those two items. Those implementations would take care of error dialogs and settings requests.

The other upload buttons will extend this new base class too. Prompted by Greg's review: zulip#70 (comment)

chrisbobbe · 2023-04-22T01:06:24Z

Thanks for the review! Revision pushed, with a TODO about the better image-library upload experience on Android, and so marking as not-draft.

In this revision I added a commit to improve the existing upload-file UI:

compose: Handle a permissions-denial case in attach-file UI

and applied that same improvement to image-library uploads (by reusing code) since that also uses file_picker for now.

chrisbobbe · 2023-04-22T01:12:20Z

And please see my reply about AppSettings.openAppSettings() above: #70 (comment)

chrisbobbe · 2023-04-25T22:47:20Z

and so marking as not-draft.

(Er, now I've done that.)

gnprice

Thanks! Generally looks good; one substantive question, about the attach-image behavior.

gnprice · 2023-04-28T05:33:37Z

lib/widgets/compose_box.dart

+  Future<Iterable<_File>?> getFiles(BuildContext context) async {
+    // TODO: This doesn't give quite the right UI on Android.
+    //   Perhaps try `image_picker`: https://github.com/zulip/zulip-flutter/issues/56#issuecomment-1514001281
+    return _getFilePickerFiles(context);


It looks like this ends up identical to the attach-file button.

In the previous draft, this passed type: FileType.media to FilePicker.platform.pickFiles. Was it intentional to drop that?

Eep, thanks for the catch; I seem to have dropped that difference when refactoring to pull common logic out.

gnprice · 2023-04-28T05:34:17Z

lib/widgets/compose_box.dart

+        showSuggestedActionDialog(context: context, // TODO(i18n)
+            title: 'Permissions needed',


nit: indentation

gnprice · 2023-04-28T18:07:05Z

lib/widgets/compose_box.dart

+  final FocusNode contentFocusNode;
+
+  IconData get icon;
+  Future<Iterable<_File>?> getFiles(BuildContext context);


It'd be good to have a bit of dartdoc, particularly to say what null means.

… Hmm, actually, perhaps better: make it non-nullable, and just return an empty list in that case. Then at the call site, skip doing anything if the list is empty (rather than if null).

That will even behave differently from this version if there's some way that pickFiles or whatever can return an empty list: it'll do nothing in that case, whereas this version would go and focus the content input. And I think doing nothing is appropriate — if it does return an empty list, that probably means the user has effectively tried to abort the operation.

gnprice · 2023-04-28T18:40:14Z

lib/widgets/compose_box.dart

+          message: 'To upload an image, please grant Zulip additional permissions in Settings.',
+          actionButtonText: 'Open settings',
+          onActionButtonPress: () {
+            AppSettings.openAppSettings();


This one brought me here:

Same result when I try that function on my Pixel 5 running Android 13. Except that the subtitle on "Permissions" is "No permissions requested", because we're successfully using the no-privileges-needed API for this on newer Android versions. (To even call this function, I hacked up the code a bit.)

I wonder: have you ever had an app link directly into the screen for managing an app's permissions?

Not sure. It's pretty uncommon for me to encounter an app linking me to any part of its system settings.

(Which doesn't mean we shouldn't do it; for one thing, this is an uncommon case, so it's quite possible that lots of apps offer the same thing if you do get into this case.)

Looking at docs, here's the intent type that the library is using for us:
https://developer.android.com/reference/android/provider/Settings#ACTION_APPLICATION_DETAILS_SETTINGS

Browsing around that page, here's a related intent type:
https://developer.android.com/reference/android/provider/Settings#ACTION_STORAGE_VOLUME_ACCESS_SETTINGS
which was deprecated, saying that "to manage storage permissions for a specific application" we should instead use… ACTION_APPLICATION_DETAILS_SETTINGS, just as app_settings is using for us.

There are various other intent types for particular permissions, including some storage-related:
https://developer.android.com/reference/android/provider/Settings#ACTION_MANAGE_APP_ALL_FILES_ACCESS_PERMISSION
https://developer.android.com/reference/android/provider/Settings#ACTION_REQUEST_MANAGE_MEDIA

I don't think either of those correspond to what we'd want here, though. In particular both of those were introduced only in recent API versions, and I think they correspond to the newer more-explicitly-powerful permissions that were introduced at the same time as making normal applications stop needing such permissions.

…ress

So far we've used this code for the generic upload-file UI (zulip#57). Soon we'll use it for media-library uploads (zulip#56) and camera uploads (zulip#61).

The other upload buttons will extend this new base class too. Prompted by Greg's review: zulip#70 (comment)

And pull out a helper from _AttachFileButton, since it also uses `file_picker` and it's convenient to reuse that code here. Fixes: zulip#56

Toward zulip#61, "Open camera to take and share a photo".

Fixes: zulip#61

chrisbobbe · 2023-04-28T19:49:16Z

Thanks for the review! Revision pushed.

gnprice · 2023-04-28T22:48:34Z

Thanks! Looks good; merging.

chrisbobbe requested a review from gnprice April 19, 2023 01:18

chrisbobbe marked this pull request as draft April 19, 2023 01:19

chrisbobbe commented Apr 19, 2023

View reviewed changes

gnprice reviewed Apr 20, 2023

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Apr 22, 2023

compose [nfc]: Split abstract base class out from _AttachFileButton

3ec8c09

The other upload buttons will extend this new base class too. Prompted by Greg's review: zulip#70 (comment)

chrisbobbe force-pushed the pr-more-upload-methods branch from 340fb2c to a3a889a Compare April 22, 2023 01:04

chrisbobbe marked this pull request as ready for review April 25, 2023 22:47

gnprice reviewed Apr 28, 2023

View reviewed changes

chrisbobbe added 11 commits April 28, 2023 14:51

compose types: Avoid implicit dynamic in _AttachFileButton._handleP…

53cedd1

…ress

compose [nfc]: Factor files into a form we can get from camera/gallery

114bca4

compose [nfc]: Pull out _uploadFiles private helper

83e4bba

So far we've used this code for the generic upload-file UI (zulip#57). Soon we'll use it for media-library uploads (zulip#56) and camera uploads (zulip#61).

compose [nfc]: Split abstract base class out from _AttachFileButton

26ed6cb

The other upload buttons will extend this new base class too. Prompted by Greg's review: zulip#70 (comment)

deps: Add app_settings, to help user grant permissions

6671f74

dialog: Add showSuggestedActionDialog

0ecc515

compose: Handle a permissions-denial case in attach-file UI

34c9223

compose [nfc]: Restate default type: FileType.any in _AttachFileButton

ed409dc

compose: Prototype image/video-from-library upload UI

cb0fc46

And pull out a helper from _AttachFileButton, since it also uses `file_picker` and it's convenient to reuse that code here. Fixes: zulip#56

deps: Add image_picker at latest

d8b07ae

Toward zulip#61, "Open camera to take and share a photo".

compose: Prototype upload-from-camera UI

f99a3b3

Fixes: zulip#61

chrisbobbe force-pushed the pr-more-upload-methods branch from a3a889a to f99a3b3 Compare April 28, 2023 19:49

gnprice merged commit f99a3b3 into zulip:main Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Prototype upload-from-library and upload-from-camera #70

compose: Prototype upload-from-library and upload-from-camera #70

chrisbobbe commented Apr 19, 2023 •

edited

Loading

chrisbobbe Apr 19, 2023

gnprice Apr 20, 2023

gnprice left a comment

gnprice Apr 20, 2023

gnprice Apr 20, 2023

gnprice Apr 20, 2023

gnprice Apr 20, 2023

chrisbobbe Apr 22, 2023

gnprice Apr 28, 2023

chrisbobbe Apr 28, 2023

gnprice Apr 20, 2023

chrisbobbe Apr 21, 2023

gnprice Apr 21, 2023

chrisbobbe Apr 21, 2023

gnprice Apr 20, 2023

chrisbobbe Apr 21, 2023

gnprice Apr 21, 2023

gnprice Apr 20, 2023

gnprice Apr 20, 2023

chrisbobbe commented Apr 22, 2023

chrisbobbe commented Apr 22, 2023

chrisbobbe commented Apr 25, 2023

gnprice left a comment

gnprice Apr 28, 2023

chrisbobbe Apr 28, 2023

gnprice Apr 28, 2023

gnprice Apr 28, 2023

gnprice Apr 28, 2023

chrisbobbe commented Apr 28, 2023

gnprice commented Apr 28, 2023

		} catch (e) {
		if (e is PlatformException && e.code == 'camera_access_denied') {

	_handlePress(BuildContext context) async {
	dynamic _handlePress(BuildContext context) async {

		showSuggestedActionDialog(context: context, // TODO(i18n)
		title: 'Permissions needed',

compose: Prototype upload-from-library and upload-from-camera #70

compose: Prototype upload-from-library and upload-from-camera #70

Conversation

chrisbobbe commented Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 22, 2023

chrisbobbe commented Apr 22, 2023

chrisbobbe commented Apr 25, 2023

gnprice left a 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

chrisbobbe commented Apr 28, 2023

gnprice commented Apr 28, 2023

chrisbobbe commented Apr 19, 2023 •

edited

Loading