Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@derTuca
Copy link
Contributor

@derTuca derTuca commented Sep 9, 2019

Description

Adds a maxDuration property so a maximum recording duration can be set.

Related Issues

flutter/flutter#40035

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

This way, we can limit the length of recorded video to a certain number of seconds.
@qwerqwq
Copy link

qwerqwq commented Sep 12, 2019

image_picker有压缩图片的api吗

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. Looks good overall, left some comments.

@cyanglaz cyanglaz self-assigned this Sep 13, 2019
@klaszlo8207
Copy link

klaszlo8207 commented Sep 14, 2019

Finally, this is awesome :) When will be this feature available? I need this ASAP. Thanks

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took another round of review, left some more comments.

@klaszlo8207
Copy link

klaszlo8207 commented Oct 8, 2019

Hey, what's happening with this PR? Nothing? 15 days....

@klaszlo8207
Copy link

Any news on this? Please help me

@cyanglaz
Copy link
Contributor

@derTuca It is been a while since I reviewed this PR. Do you still want to continue to work on this?

@klaszlo8207
Copy link

@cyanglaz can you help me in this PR? It looks like @derTuca has no time for this. Can you fix it and merge to the base code?

Thanks!

@cyanglaz
Copy link
Contributor

cyanglaz commented Nov 7, 2019

@derTuca Do you think you still want to work on this?

If anyone is interested to open another PR to work on this, I am happy to review it.

@klaszlo8207
Copy link

klaszlo8207 commented Nov 14, 2019

Can you fix the problems in the conflicting files @cyanglaz? and then just simple attach to the code base? It is a must to have feature, and I need this before last month :)

@derTuca
Copy link
Contributor Author

derTuca commented Nov 15, 2019

Sorry for the huge delays, I am caught up with some stuff at this time. I have adopted the proposed changes, @cyanglaz if you could review it again it would be great.

@derTuca derTuca requested a review from cyanglaz November 16, 2019 15:06
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot for the good work! Please rebase master and fix any CI issues, I will merge this once the CI is green!

@ened
Copy link
Contributor

ened commented Dec 11, 2019

Hi @derTuca could you kindly rebase to latest master and have another look at the CHANGELOG.md changes? The formatting for many entries is still a bit off.
The plugins repository is now building nicely again, so we can merge this PR soon. Thank you!

@cyanglaz
Copy link
Contributor

@derTuca Please also add a jUnit test to test the max duration property in Android.

@derTuca
Copy link
Contributor Author

derTuca commented Dec 31, 2019

@cyanglaz I haven't done much testing with Java, so I need some help with how to mock the ImagePickerDelegate class properly; I don't know if I just need to use @mock or if I need to do a more...proper...initialization of the constructor's args.

@cyanglaz
Copy link
Contributor

@derTuca There are some existing junit test cases in this plugin. You can take a look and see how they are constructed.

@klaszlo8207
Copy link

klaszlo8207 commented Jan 28, 2020

Hello all, any way to add this PR to the source code now? Last time I wrote was 2019 nov 14. This is very slooooooooooooooow.

…_max_duration

# Conflicts:
#	packages/image_picker/CHANGELOG.md
#	packages/image_picker/pubspec.yaml
@derTuca
Copy link
Contributor Author

derTuca commented Feb 2, 2020

@cyanglaz Done, thanks for the info!

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the java tests. We can also add iOS unit testing. There should be some unit test cases in this plugin as well.

@derTuca
Copy link
Contributor Author

derTuca commented Feb 18, 2020

Thanks for adding the java tests. We can also add iOS unit testing. There should be some unit test cases in this plugin as well.

Do you have any tips on how I can start on this? I have seen the other tests, but there are none testing passing a parameter to the controller per se (as far as I can tell), and I don't really know what to test here.

@cyanglaz
Copy link
Contributor

@derTuca I would:

  1. Create a FLTImagePickerPlugin with some mocking parameters. (Feel free to use OCMock).
  2. Pass maxDuration to the method channel.
  3. Check if the maxDuration of the controller is the same as the one you passed in.

@derTuca
Copy link
Contributor Author

derTuca commented Mar 17, 2020

For the life of everything, I swear that (with all the free time everybody suddenly has) I've looked into this, but I can't figure out how to check the duration in the controller. I can do the Android approach, where we just check if it returns the file path with the argument passed, but with the controller being private and all (so I can't mock it), I don't know how to check its maxDuration.

@cyanglaz am I missing the magic sauce? Or is it just straight up not possible to check it without modifying the plugin code?

@mehmetf
Copy link
Contributor

mehmetf commented Mar 25, 2020

FWIW, this is something customer:money cares about. @cyanglaz it would be great if you can help @derTuca here.

@cyanglaz
Copy link
Contributor

FWIW, this is something customer:money cares about. @cyanglaz it would be great if you can help @derTuca here.

Sure, I'll move this up in my priority list.

@cyanglaz
Copy link
Contributor

@derTuca In my opinion, It is totally legit to modify the code in order to make them testable. Feel free to do so. Let me know if you still face troubles. :)

@cyanglaz
Copy link
Contributor

cyanglaz commented Apr 3, 2020

@derTuca Just checking in how you are doing :) Are you able to continue working on this PR?

@ngoctranfire
Copy link

ngoctranfire commented Apr 5, 2020

@derTuca, curious about your progress here too. This is something I'm looking into as well too. If there's something I can help with here, I can try to do so as well too.

@cyanglaz
Copy link
Contributor

cyanglaz commented Apr 8, 2020

@derTuca I added the tests and resolved the conflicts for this PR in derTuca#1
Do you mind taking a look and merge?

@derTuca
Copy link
Contributor Author

derTuca commented Apr 9, 2020

@derTuca I added the tests and resolved the conflicts for this PR in derTuca#1
Do you mind taking a look and merge?

Merged, sorry for not being able to look into it after all.

@cyanglaz
Copy link
Contributor

cyanglaz commented Apr 9, 2020

@derTuca It seems the pull request went to a wrong branch. Could you create another PR to the correct branch to fix that?

@derTuca
Copy link
Contributor Author

derTuca commented Apr 9, 2020

Done. #2643.

@derTuca derTuca closed this Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants