compose: Prototype upload-from-library and upload-from-camera #70
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| import 'package:app_settings/app_settings.dart'; | ||
| import 'package:file_picker/file_picker.dart'; | ||
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter/services.dart'; | ||
| import 'package:image_picker/image_picker.dart'; | ||
| import 'dialog.dart'; | ||
|
|
||
| import '../api/route/messages.dart'; | ||
|
|
@@ -210,85 +213,221 @@ class _StreamContentInputState extends State<_StreamContentInput> { | |
| } | ||
| } | ||
|
|
||
| /// Data on a file to be uploaded, from any source. | ||
| /// | ||
| /// A convenience class to represent data from the generic file picker, | ||
| /// the media library, and the camera, in a single form. | ||
| class _File { | ||
| _File({required this.content, required this.length, required this.filename}); | ||
|
|
||
| final Stream<List<int>> content; | ||
| final int length; | ||
| final String filename; | ||
| } | ||
|
|
||
| Future<void> _uploadFiles({ | ||
| required BuildContext context, | ||
| required ContentTextEditingController contentController, | ||
| required FocusNode contentFocusNode, | ||
| required Iterable<_File> files, | ||
| }) async { | ||
| assert(context.mounted); | ||
| final store = PerAccountStoreWidget.of(context); | ||
|
|
||
| final List<_File> tooLargeFiles = []; | ||
| final List<_File> rightSizeFiles = []; | ||
| for (final file in files) { | ||
| if ((file.length / (1 << 20)) > store.maxFileUploadSizeMib) { | ||
| tooLargeFiles.add(file); | ||
| } else { | ||
| rightSizeFiles.add(file); | ||
| } | ||
| } | ||
|
|
||
| if (tooLargeFiles.isNotEmpty) { | ||
| final listMessage = tooLargeFiles | ||
| .map((file) => '${file.filename}: ${(file.length / (1 << 20)).toStringAsFixed(1)} MiB') | ||
| .join('\n'); | ||
| showErrorDialog( // TODO(i18n) | ||
| context: context, | ||
| title: 'File(s) too large', | ||
| message: | ||
| '${tooLargeFiles.length} file(s) are larger than the server\'s limit of ${store.maxFileUploadSizeMib} MiB and will not be uploaded:\n\n$listMessage'); | ||
| } | ||
|
|
||
| final List<(int, _File)> uploadsInProgress = []; | ||
| for (final file in rightSizeFiles) { | ||
| final tag = contentController.registerUploadStart(file.filename); | ||
| uploadsInProgress.add((tag, file)); | ||
| } | ||
| if (!contentFocusNode.hasFocus) { | ||
| contentFocusNode.requestFocus(); | ||
| } | ||
|
|
||
| for (final (tag, file) in uploadsInProgress) { | ||
| final _File(:content, :length, :filename) = file; | ||
| Uri? url; | ||
| try { | ||
| final result = await uploadFile(store.connection, | ||
| content: content, length: length, filename: filename); | ||
| url = Uri.parse(result.uri); | ||
| } catch (e) { | ||
| if (!context.mounted) return; | ||
| // TODO(#37): Specifically handle `413 Payload Too Large` | ||
| // TODO(#37): On API errors, quote `msg` from server, with "The server said:" | ||
| showErrorDialog(context: context, | ||
| title: 'Failed to upload file: $filename', message: e.toString()); | ||
| } finally { | ||
| contentController.registerUploadEnd(tag, url); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| abstract class _AttachUploadsButton extends StatelessWidget { | ||
| const _AttachUploadsButton({required this.contentController, required this.contentFocusNode}); | ||
|
|
||
| final ContentTextEditingController contentController; | ||
| final FocusNode contentFocusNode; | ||
|
|
||
| IconData get icon; | ||
|
|
||
| /// Request files from the user, in the way specific to this upload type. | ||
| /// | ||
| /// Subclasses should manage the interaction completely, e.g., by catching and | ||
| /// handling any permissions-related exceptions. | ||
| /// | ||
| /// To signal exiting the interaction with no files chosen, | ||
| /// return an empty [Iterable] after showing user feedback as appropriate. | ||
| Future<Iterable<_File>> getFiles(BuildContext context); | ||
|
|
||
| void _handlePress(BuildContext context) async { | ||
| final files = await getFiles(context); | ||
| if (files.isEmpty) { | ||
| return; // Nothing to do (getFiles handles user feedback) | ||
| } | ||
|
|
||
| if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007 | ||
| else { | ||
| return; | ||
| } | ||
|
|
||
| await _uploadFiles( | ||
| context: context, | ||
| contentController: contentController, | ||
| contentFocusNode: contentFocusNode, | ||
| files: files); | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return IconButton(icon: Icon(icon), onPressed: () => _handlePress(context)); | ||
| } | ||
| } | ||
|
|
||
| Future<Iterable<_File>> _getFilePickerFiles(BuildContext context, FileType type) async { | ||
| FilePickerResult? result; | ||
| try { | ||
| result = await FilePicker.platform | ||
| .pickFiles(allowMultiple: true, withReadStream: true, type: type); | ||
| } catch (e) { | ||
| if (e is PlatformException && e.code == 'read_external_storage_denied') { | ||
| // Observed on Android. If Android's error message tells us whether the | ||
| // user has checked "Don't ask again", it seems the library doesn't pass | ||
| // that on to us. So just always prompt to check permissions in settings. | ||
| // If the user hasn't checked "Don't ask again", they can always dismiss | ||
| // our prompt and retry, and the permissions request will reappear, | ||
| // letting them grant permissions and complete the upload. | ||
| showSuggestedActionDialog(context: context, // TODO(i18n) | ||
| title: 'Permissions needed', | ||
| message: 'To upload files, please grant Zulip additional permissions in Settings.', | ||
| actionButtonText: 'Open settings', | ||
| onActionButtonPress: () { | ||
| AppSettings.openAppSettings(); | ||
| }); | ||
| } else { | ||
| // TODO(i18n) | ||
| showErrorDialog(context: context, title: 'Error', message: e.toString()); | ||
| } | ||
| return []; | ||
| } | ||
| if (result == null) { | ||
| return []; // User cancelled; do nothing | ||
| } | ||
|
|
||
| return 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); | ||
| }); | ||
| } | ||
|
|
||
| class _AttachFileButton extends _AttachUploadsButton { | ||
| const _AttachFileButton({required super.contentController, required super.contentFocusNode}); | ||
|
|
||
| @override | ||
| IconData get icon => Icons.attach_file; | ||
|
|
||
| @override | ||
| Future<Iterable<_File>> getFiles(BuildContext context) async { | ||
| return _getFilePickerFiles(context, FileType.any); | ||
| } | ||
| } | ||
|
|
||
| class _AttachMediaButton extends _AttachUploadsButton { | ||
| const _AttachMediaButton({required super.contentController, required super.contentFocusNode}); | ||
|
|
||
| @override | ||
| IconData get icon => Icons.image; | ||
|
|
||
| @override | ||
| 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, FileType.media); | ||
| } | ||
| } | ||
|
|
||
| class _AttachFromCameraButton extends _AttachUploadsButton { | ||
| const _AttachFromCameraButton({required super.contentController, required super.contentFocusNode}); | ||
|
|
||
| @override | ||
| IconData get icon => Icons.camera_alt; | ||
|
|
||
| @override | ||
| Future<Iterable<_File>> getFiles(BuildContext context) async { | ||
| final picker = ImagePicker(); | ||
| final XFile? result; | ||
| try { | ||
| // Ideally we'd open a platform interface that lets you choose between | ||
| // taking a photo and a video. `image_picker` doesn't yet have that | ||
| // option: https://github.com/flutter/flutter/issues/89159 | ||
| // so just stick with images for now. We could add another button for | ||
| // videos, but we don't want too many buttons. | ||
| result = await picker.pickImage(source: ImageSource.camera, requestFullMetadata: false); | ||
| } catch (e) { | ||
| if (e is PlatformException && e.code == 'camera_access_denied') { | ||
| // iOS has a quirk where it will only request the native | ||
| // permission-request alert once, the first time the app wants to | ||
| // use a protected resource. After that, the only way the user can | ||
| // grant it is in Settings. | ||
| showSuggestedActionDialog(context: context, // TODO(i18n) | ||
| title: 'Permissions needed', | ||
| message: 'To upload an image, please grant Zulip additional permissions in Settings.', | ||
| actionButtonText: 'Open settings', | ||
| onActionButtonPress: () { | ||
| AppSettings.openAppSettings(); | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.)
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: Browsing around that page, here's a related intent type: There are various other intent types for particular permissions, including some storage-related: 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. There was a problem hiding this comment. That reasoning makes sense. |
||
| }); | ||
| } else { | ||
| // TODO(i18n) | ||
| showErrorDialog(context: context, title: 'Error', message: e.toString()); | ||
| } | ||
| return []; | ||
| } | ||
| if (result == null) { | ||
| return []; // User cancelled; do nothing | ||
| } | ||
| final length = await result.length(); | ||
|
|
||
| return [_File(content: result.openRead(), length: length, filename: result.name)]; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -490,6 +629,8 @@ class _StreamComposeBoxState extends State<StreamComposeBox> { | |
| child: Row( | ||
| children: [ | ||
| _AttachFileButton(contentController: _contentController, contentFocusNode: _contentFocusNode), | ||
| _AttachMediaButton(contentController: _contentController, contentFocusNode: _contentFocusNode), | ||
| _AttachFromCameraButton(contentController: _contentController, contentFocusNode: _contentFocusNode), | ||
| ])), | ||
| ])))); | ||
| } | ||
|
|
||
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.
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:
But I'm thinking like, I dunno,
or something?
This may be relevant: dart-lang/language#112 (comment)
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.
Yeah. Looks like the Dart folks have indeed imagined that feature 🙂 Perhaps it'll get added at some point.