[image_picker]Add argument maxDuration to video record#674
[image_picker]Add argument maxDuration to video record#674vagrantrobbie wants to merge 3 commits intoflutter:masterfrom
Conversation
Can pass a double of seconds to maxDuration to limit the duration of the video record
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
|
I signed it! |
|
CLAs look good, thanks! |
| 'pickVideo', | ||
| <String, dynamic>{ | ||
| 'source': source.index, | ||
| 'maxDuration':maxDuration |
| break; | ||
| } | ||
| } else if ([@"pickVideo" isEqualToString:call.method]) { | ||
|
|
There was a problem hiding this comment.
nit: unnecessary extra blank line.
| (NSString *)kUTTypeMPEG4 | ||
| ]; | ||
| _imagePickerController.videoQuality = UIImagePickerControllerQualityTypeHigh; | ||
|
|
There was a problem hiding this comment.
nit: unnecessary extra blank line.
|
|
||
| static Future<File> pickVideo({ | ||
| @required ImageSource source, | ||
| double maxDuration |
There was a problem hiding this comment.
Can you add a doc comment to pickVideo that documents this function (similarly to how it is done for pickImage) and also describe the new parameter maxDuration in there?
|
|
||
| static Future<File> pickVideo({ | ||
| @required ImageSource source, | ||
| double maxDuration |
There was a problem hiding this comment.
Also: would it be better if maxDuration is actually of type Duration? Currently it is unclear what the unit for durations is. Seconds? Minutes? Hours?
There was a problem hiding this comment.
I will add a note that the maxDuration is in Seconds.
| intent.putExtra(MediaStore.EXTRA_OUTPUT, videoUri); | ||
| grantUriPermissions(intent, videoUri); | ||
|
|
||
| // Set a maximum duration for the video record |
There was a problem hiding this comment.
nit: the comment is not really providing any value. Maybe remove?
| _result = result; | ||
| _arguments = call.arguments; | ||
|
|
||
| // Set a maximum duration for the video record |
|
You'll also have to address the formatting issues uncovered by our CI: https://cirrus-ci.com/task/5089761320501248 |
Remove extra white space and document ImagePicker.pickVideo method channel
|
Now uses Duration |
|
@vagrantrobbie looks like something bad happened with the dependencies here, not sure why. Can you try updating to tip of tree and resubmitting? Maybe try running the tests locally, see if they work there. |
|
@vagrantrobbie Could you try rebase and see if the CI issue is resolved? |
|
@vagrantrobbie I'm going to relentlessly close this PR since it has been inactive for a while. I encourage you to reopen it when you are ready to work on it. |
Can pass a double of seconds to maxDuration to limit the duration of the video record