feat: New image cropper (pure Flutter) #1063
Conversation
github-actions
bot
added
Android
🌐 l10n
Product addition
There was a problem hiding this comment.
Hi @M123-dev!
That was a good idea to get rid of additional lines in AndroidManifest.xml thank to the new solution: wasn't there something related on the iOS side too?
My main concern here is an easy one: the code would be easier to read if you create a Crop StatelessWidget instead of hiding your widget into a method. I'm a Java boy, sorry about that, I like files and classes...
|
|
||
| final Directory tempDir = await getTemporaryDirectory(); | ||
| final String tempPath = tempDir.path; | ||
| final String filePath = '$tempPath/upload_img_file_01.tmp'; |
There was a problem hiding this comment.
Should probably be deleted after the upload, though I understand that it will overridden by the next image.
Btw it's always just one upload at a time?
There was a problem hiding this comment.
I just checked we cannot delete the file directly, otherwise it will fail uploading, becouse it can't finde the file. I think that with the file object we only pass some kind of pointer. But since it's in the temp directory it won't be a problem
There was a problem hiding this comment.
It's better, but I still don't like it at 100%. Please read my comments, and feel free to ignore them and merge if you are in a hurry.
About the file deletion, again I wouldn't want to prevent you from merging quickly important code, but I don't see why you cannot delete the file after the upload. If really you cannot, that may be the symptom of something else like a hidden bug.
| return FutureBuilder<Uint8List?>( | ||
| future: pickImage(), | ||
| builder: ( | ||
| BuildContext context, | ||
| AsyncSnapshot<Uint8List?> snap, | ||
| ) { |
There was a problem hiding this comment.
Honestly I have problems with this syntax, but I don't blame you in particular because it's not the only place in the project where it's coded that way - and maybe it's not your problem if I have a problem with that :)
In short, I don't think it makes sense to open a page/Scaffold on a Future when the Future is a dialog or a file picker. Besides, this class is called ImageCropPage and therefore should crop an image (in this case a Uint8List), and that's it.
Step 1 the caller calls a file picker, step 2 it calls ImageCropPage on the file picker result. ImageCropPage could be called from anywhere, not necessarily from a Future that necessarily opens a file picker.
There was a problem hiding this comment.
Thanks for your review @monsieurtanuki the Scaffold before the FutureBuilder makes sense, and for the File, from the comments on the file implementation:
/// A reference to a file on the file system.
///
/// AFileholds a [path] on which operations can be performed.
/// You can get the parent directory of the file using [parent],
/// a property inherited from [FileSystemEntity].
Se would have to remove it after sending, not in the cropper. But thats not in the scope of this PR,
There was a problem hiding this comment.
Actually no, just added it 😄
Fixes: #1038
CI failing because of: #1062