Skip to content
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

[image_picker] add getMedia method #3892

Merged
merged 5 commits into from
Jun 15, 2023
Merged

Conversation

tarrinneal
Copy link
Contributor

@tarrinneal tarrinneal commented May 3, 2023

Adds getMedia and getMultipleMedia methods to all platforms.

fixes flutter/flutter#89159

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@vashworth vashworth left a comment

Choose a reason for hiding this comment

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

Should there be a single vs multi media picker option? Seems to me that currently it's set up for multiple with no way to limit it to a single image/video selection

* Processes the video.
*/
- (void)processVideo API_AVAILABLE(ios(14)) {
NSString *typeIdentifier = self.result.itemProvider.registeredTypeIdentifiers.firstObject;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we confident the first type is always the one we want? Does using different types cause different outcomes? Like if the video was an MP4, I'd image it might have UTTypeAudiovisualContent, UTTypeMovie, UTTypeMPEG4Movie, etc. Does using UTTypeAudiovisualContent vs UTTypeMPEG4Movie make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first type is just the type that was registered first. I think it should be fine as is, but if you know of some reason this may cause issues, it can be updated.

@tarrinneal
Copy link
Contributor Author

Should there be a single vs multi media picker option? Seems to me that currently it's set up for multiple with no way to limit it to a single image/video selection

I actually had that written in initially and removed it since I didn't think it would make a lot of sense. I can add it back in if people disagree.

@stuartmorgan
Copy link
Contributor

I actually had that written in initially and removed it since I didn't think it would make a lot of sense. I can add it back in if people disagree.

Plugin clients will definitely want the ability to limit to a single item. (Ideally we'd want to limit to N items, but we can't actually implement that on all platforms, which is why we don't have the API.)

The way we should handle it structurally is at the platform interface layer have a single method with a bool for multiple selection, and at the app-facing level have two methods (so that they don't have to extract from a list in the common case of only allowing one item).

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

I just did quick high-level feedback for now.

@tarrinneal tarrinneal requested a review from stuartmorgan June 5, 2023 16:38
@tarrinneal tarrinneal requested a review from stuartmorgan June 8, 2023 02:18
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM!

auto-submit bot pushed a commit that referenced this pull request Jun 9, 2023
Adds `getMedia` and `getMultipleMedia` methods to  image_picker_platform_interface.

precursor to #3892

part of flutter/flutter#89159
@hectorAguero
Copy link

I actually had that written in initially and removed it since I didn't think it would make a lot of sense. I can add it back in if people disagree.

Plugin clients will definitely want the ability to limit to a single item. (Ideally we'd want to limit to N items, but we can't actually implement that on all platforms, which is why we don't have the API.)

The way we should handle it structurally is at the platform interface layer have a single method with a bool for multiple selection, and at the app-facing level have two methods (so that they don't have to extract from a list in the common case of only allowing one item).

Hey @stuartmorgan, not fully related to this PR, but what platforms doesn't support N item limit?

For what I know, In the past was Android, but know there is the new photopicker, also I saw you reopen this issue but the comments and likes are not enabled flutter/flutter#85772

@stuartmorgan
Copy link
Contributor

what platforms doesn't support N item limit?

Almost all of them, as far as I know.

I saw you reopen this issue but the comments and likes are not enabled flutter/flutter#85772

That was an oversight; fixed

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Still LGTM!

tarrinneal added a commit that referenced this pull request Jun 15, 2023
Adds `getMedia` and `getMultipleMedia` methods to all image_picker
platforms.

~~waiting on #4174

precursor to #3892

part of flutter/flutter#89159

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [relevant style guides] and ran the
auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages
repo does use `dart format`.)
- [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [x] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style].
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 15, 2023
@auto-submit auto-submit bot merged commit c0cba0b into flutter:main Jun 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 16, 2023
flutter/packages@f9314a3...59d93d6

2023-06-16 [email protected] [tool] Add command aliases (flutter/packages#4207)
2023-06-16 [email protected] [google_map_flutter] Fix map object regression due to async changes (flutter/packages#4171)
2023-06-16 [email protected] [url_launcher] Add ignores for deprecated member to test (flutter/packages#4220)
2023-06-15 [email protected] Roll Flutter from 95be76a to b0188cd (10 revisions) (flutter/packages#4221)
2023-06-15 [email protected] [camera_android] Upgrading roboelectric from 4.5 to 4.10.3 (flutter/packages#4018)
2023-06-15 [email protected] [go_router] Fixes bug that GoRouterState in top level redirect doesn'â�¦ (flutter/packages#4173)
2023-06-15 [email protected] [go_router]Updates documentations around GoRouter.of, GoRouter.maybeOf, and BuildContext extension. (flutter/packages#4176)
2023-06-15 [email protected] [tool] Support code excerpts for any .md file (flutter/packages#4212)
2023-06-15 [email protected] [webview_flutter] Add support for limitsNavigationsToAppBoundDomains (flutter/packages#4026)
2023-06-15 [email protected] [webview_flutter][webview_flutter_android] Add android support for handling geolocation permissions (flutter/packages#3795)
2023-06-15 [email protected] [image_picker] add getMedia method (flutter/packages#3892)
2023-06-15 [email protected] [image_picker] getMedia platform implementations (flutter/packages#4175)
2023-06-14 [email protected] Ignore `textScaleFactor` deprecation (flutter/packages#4209)
2023-06-14 [email protected] [pigeon] Enable Obj-C integration tests in CI (flutter/packages#4215)
2023-06-14 [email protected] [flutter_adaptive_scaffold] Support RTL (flutter/packages#4204)
2023-06-14 [email protected] Manual roll Flutter from 09b7e56 to 95be76a (14 revisions) (flutter/packages#4214)
2023-06-14 [email protected] Roll Flutter (stable) from 682aa38 to 796c8ef (5 revisions) (flutter/packages#4213)
2023-06-14 [email protected] Roll Flutter from 353b8bc to 09b7e56 (17 revisions) (flutter/packages#4206)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
nksteven pushed a commit to nksteven/ez_image_picker_ios that referenced this pull request Aug 29, 2023
Adds `getMedia` and `getMultipleMedia` methods to all image_picker
platforms.

~~waiting on flutter/packages#4174

precursor to flutter/packages#3892

part of flutter/flutter#89159

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [relevant style guides] and ran the
auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages
repo does use `dart format`.)
- [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded
by square brackets, e.g. `[shared_preferences]`
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated `pubspec.yaml` with an appropriate new version according
to the [pub versioning philosophy], or this PR is [exempt from version
changes].
- [x] I updated `CHANGELOG.md` to add a description of the change,
[following repository CHANGELOG style].
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[relevant style guides]:
https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
[pub versioning philosophy]: https://dart.dev/tools/pub/versioning
[exempt from version changes]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#version-and-changelog-updates
[following repository CHANGELOG style]:
https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: image_picker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[image_picker]: Proposal to Pick both image and video simultaneously
5 participants