-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[image_picker] Clarify maxDuration applies only to recording #10707
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Code Review
This pull request clarifies the documentation for the maxDuration parameter, noting that it only applies to video recording and not to videos selected from the gallery. The changes are accurate and improve clarity. I have provided a couple of suggestions to re-incorporate the information about the default behavior when maxDuration is not provided, as this was present in the previous documentation and is helpful for developers.
| /// The [maxDuration] argument specifies the maximum duration of the recorded video. | ||
| /// | ||
| /// This parameter only applies when recording a video using the camera. | ||
| /// It does not restrict the duration of videos selected from the gallery, | ||
| /// since the underlying platform gallery APIs do not support filtering | ||
| /// by video duration. |
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.
The clarification is great! However, the previous documentation mentioned what happens when maxDuration is not specified (it's infinite). This is useful information that has been removed. Consider re-adding it for completeness.
| /// The [maxDuration] argument specifies the maximum duration of the recorded video. | |
| /// | |
| /// This parameter only applies when recording a video using the camera. | |
| /// It does not restrict the duration of videos selected from the gallery, | |
| /// since the underlying platform gallery APIs do not support filtering | |
| /// by video duration. | |
| /// The [maxDuration] argument specifies the maximum duration of the recorded video. | |
| /// If no [maxDuration] is specified, the maximum duration will be infinite. | |
| /// | |
| /// This parameter only applies when recording a video using the camera. | |
| /// It does not restrict the duration of videos selected from the gallery, | |
| /// since the underlying platform gallery APIs do not support filtering | |
| /// by video duration. |
| /// The [maxDuration] argument specifies the maximum duration of the recorded | ||
| /// video. | ||
| /// | ||
| /// This parameter only applies when recording a video using the camera. | ||
| /// It does not restrict the duration of videos selected from the gallery, | ||
| /// since the underlying platform gallery APIs do not support filtering | ||
| /// by video duration. |
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.
Similar to my other comment, the information about the default behavior when maxDuration is null has been removed. It would be good to keep this information for developers for consistency and clarity.
| /// The [maxDuration] argument specifies the maximum duration of the recorded | |
| /// video. | |
| /// | |
| /// This parameter only applies when recording a video using the camera. | |
| /// It does not restrict the duration of videos selected from the gallery, | |
| /// since the underlying platform gallery APIs do not support filtering | |
| /// by video duration. | |
| /// The [maxDuration] argument specifies the maximum duration of the recorded | |
| /// video. If no [maxDuration] is specified, the maximum duration will be | |
| /// infinite. | |
| /// | |
| /// This parameter only applies when recording a video using the camera. | |
| /// It does not restrict the duration of videos selected from the gallery, | |
| /// since the underlying platform gallery APIs do not support filtering | |
| /// by video duration. |
|
Thanks for the submission! In the future, please do not delete portions of the checklist that is in the PR template; everything in it is there for a reason. This PR is missing required elements described in the checklist (I’ve restored it to the PR description), which need to be addressed before it moves forward with review. I am marking the PR as a Draft. Please review the checklist, updating the PR as appropriate, and when that’s complete please feel free to mark the PR as ready for review.
Please see the list of exemption reasons listed in the linked repository guides, and also the FAQ about versioning. |
|
Thanks for the guidance! I’ve restored the full PR template, addressed the |
|
@stuartmorgan-g Sir One more Doubt which part of PR TEMPLATE should not be changed which part we need to change so it will help me in furthur PR REQUEST Kindly help me to Make my First successful Contribution thank you |
You need to complete the entire checklist. Also, I'm not sure why you undid the fix I made to link to the issue this PR is about, rather than a different PR about the issue. Issues and PRs aren't the same thing, and I made the change for a reason.
It does not appear that you read the FAQ that I linked you to. |
Apologies for the confusion earlier, and thank you for your patience. This is my first contribution to flutter/packages, and I misunderstood how PR branches and the checklist requirements work. I’ve now added the version bump and CHANGELOG update to the same branch as the PR and completed the full checklist as requested. Thank you for the guidance and for taking the time to point me in the right direction. |
This PR clarifies the documentation for the
maxDurationparameter inimage_pickerto explicitly note that it only applies to video recordingusing the camera, and does not restrict the duration of videos selected
from the gallery.
This aligns the API documentation with current platform behavior and
limitations on iOS and Android, preventing confusion for developers who
expect gallery selection to be filtered by duration.
Fixes flutter/flutter#83630
This change updates only documentation comments and related metadata.
The package version and CHANGELOG have been updated accordingly.
No behavioral changes are included.
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.