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

[image_picker] add requestFullMetadata for iOS (optional permissions) - platform interface changes for multi image picking #5914

Merged

Conversation

PiotrMitkowski
Copy link
Contributor

@PiotrMitkowski PiotrMitkowski commented Jun 6, 2022

Description

Platform interface changes for #5915 - adding possibility to disable full metadata when picking multiple images

Related issues

flutter/flutter#65995

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/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin 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.

@PiotrMitkowski PiotrMitkowski marked this pull request as ready for review June 7, 2022 15:30
@PiotrMitkowski
Copy link
Contributor Author

@stuartmorgan do you mind taking a look at the PR?

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.

I remember this feature should also support picking a single image, did you move that to a different PR?

Comment on lines 1 to 2
## 2.6.0
* Deprecates `getMultiImage` in favor of a new method `getMultiImageWithOptions`.
Copy link
Contributor

Choose a reason for hiding this comment

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

add a space inbetween

@PiotrMitkowski
Copy link
Contributor Author

I remember this feature should also support picking a single image, did you move that to a different PR?

It has been added in this PR: #5603. Lately we had a discussion with Stuart about adding the same feature to multi image picking and this PR addresses it.

/// The `options` argument controls additional settings that can be used when
/// picking an image. See [MultiImagePickerOptions] for more details.
///
/// If no images were picked, the return value is null.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make the return value non-nullable, and return empty for no images, so that we don't have two ways of expressing no files.

/// compression is not supported for the image that is picked, a warning
/// message will be logged.
///
/// If null, the image will be returned with the original quality.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is stale.

// found in the LICENSE file.

/// Specifies options for picking multiple images from the device's gallery.
class MultiImagePickerOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we would share this with ImagePickerOptions, and extract a shared class that has the elements that are image-specific (i.e., everything here), and then an outer class with anything else. We don't need to do another change to the singe-image version now, but we should prep for it by structuring this that way: all of the fields here should move to a new ImageOptions, and then MultiImagePickerOptions should have one ImageOptions field. Eventually we can switch the single-image version to use ImageOptions instead of these four fields too.

@PiotrMitkowski
Copy link
Contributor Author

@cyanglaz @stuartmorgan could you do another review, please? 🙂

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.

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

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@stuartmorgan
Copy link
Contributor

Looks like submit-queue status is stale.

@stuartmorgan stuartmorgan merged commit 278bc66 into flutter:main Jul 26, 2022
@stuartmorgan stuartmorgan removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2022
@PiotrMitkowski PiotrMitkowski deleted the ios-optional-permissions-interface branch July 26, 2022 14:24
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 26, 2022
…rmissions) - platform interface changes for multi image picking (flutter/plugins#5914)
yutaaraki-toydium pushed a commit to yutaaraki-toydium/plugins that referenced this pull request Aug 12, 2022
… - platform interface changes for multi image picking (flutter#5914)

Platform interface changes for flutter#5915 - adding possibility to disable full metadata when picking multiple images
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
… - platform interface changes for multi image picking (flutter#5914)

Platform interface changes for flutter#5915 - adding possibility to disable full metadata when picking multiple images
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants