-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
rodydavis
commented
May 18, 2020
- Adding pickMedia()
- Updated Example
- Fixing some typos
Borrowed from Rody Davis' file_access package. Co-authored-by: Rody Davis <rody.davis.jr@gmail.com>
- Adding pickMedia() - Updated Example - Fixing some typos
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.
Rody, this looks to me like a new feature for the release after enabling web with the "new" API. Are you in the future already? :P :P Can we release this after the current big PR goes through?
If you have specific changes about the current PR, I'd be glad to merge them, but in this case, the new pickMedia
API looks a bit premature, doesn't it?
@@ -40,7 +40,7 @@ class MyHomePage extends StatefulWidget { | |||
class _MyHomePageState extends State<MyHomePage> { | |||
PickedFile _imageFile; | |||
dynamic _pickImageError; | |||
bool isVideo = false; | |||
RetrieveType mediaSource = RetrieveType.video; |
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.
I'd call this mediaType
to not confuse it with the current source
(gallery vs camera)
}); | ||
} | ||
}); | ||
switch (mediaSource) { |
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.
I am not a fan of this global controlling the behavior of this method.
Do you think it'd be possible to split _onImageButtonPressed
in two methods: _onImageButtonPressed
and _onVideoButtonPressed
? (Also I've seen other parts of the code using this global-but-not-quite-part-of-the-state variable)
(Anyway, that might be a refactor for another time)
@required ImageSource source, | ||
@required RetrieveType type, | ||
double imageMaxWidth, | ||
double imageMaxHeight, | ||
int imageQuality, | ||
Duration videoMaxDuration, | ||
CameraDevice preferredCameraDevice = CameraDevice.rear, |
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.
I like this change, but I'd prefer to add this after the web version is in, to not add a new platform + new functionality. Can we do this in a follow up to the current change?
About the functionality itself, this seems to be a big fusion of all the pick* methods that are currently implemented. If "pickAudio" or "pickFile" get added, this method will grow in complexity (it'll probably need more parameters).
Maybe the best way to go is to have a completely agnostic "pickFile" method that lets you select anything from the disk.
That API can have the RetrieveType parameter if we want to limit to some types, but it wouldn't concern itself with "imageQuality" or "videoDuration".
That would be a much simpler API, if you needed to pick from multiple types IMO!
Duration videoMaxDuration, | ||
CameraDevice preferredCameraDevice = CameraDevice.rear, | ||
}) { | ||
switch (type) { |
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.
We should avoid these switches
on Enum IMO, because when you add a new value to the Enum, you break whoever was switching on it, if they don't have a default
case.
There should always be a default
case (or possibly better, not switch on Enums :P)
Oh for sure! Yeah I was just making it to show you what I was thinking. PR is fine as it is 👍🏼 |
@rodydavis let's revisit this, after the web version gets merged. What do you think about having a less specific I'm sure this was inspired by your own use cases of the picker in the past, I'm very curious! (We can discuss via email if you prefer) |
Yeah I like the idea of it being less specific for sure. For many apps though I need to rebuild the logic for picking a photo, image or file and they all usually are under a onpressed event and return a file. Having a pickFile would be great to add. And then if it is filtered to just photos or videos it would choose appropriately. Side note, are there plans to support saving image, photos and videos too? That’s a bother thing I’m heavily replicating, but could also be another plugin |
I think saving needs to be a separate plugin. It's super transparent for the mobile use case, but it's not so clear in the Browser. (I'm concerned with initializing the file in a way that makes sense with the mobile code :/) |