[image_picker] add forceFullMetadata to interface #4288
Conversation
| @@ -1,3 +1,11 @@ | |||
| ## 2.3.1 | |||
There was a problem hiding this comment.
should be 2.4.0 as there are non-breaking changes to the public API, same thing in pubspec.yaml
There was a problem hiding this comment.
I've updated this as well as the version & changelog for web
There was a problem hiding this comment.
I think the web change needs to be in the other PR
There was a problem hiding this comment.
Since web directly extends ImagePickerPlatform, it's required to match the function signatures for ImagePickerPlatform
As a result, the automated web tests fail if it's not also changed to match. Granted, there's no changes to web beyond adding bool forceFullMetadata = true, to the function signatures.
There was a problem hiding this comment.
hmm. This is weird. I would think it is the other way around. Since the platform_interface change hasn't been published, the web shouldn't pick up the newest API, thus the override should report a compile error.
There was a problem hiding this comment.
@cpboyd Do you mind double checking again. The test for web should not fail because the web package is depending on an old version of platform_interface, where the method signature hasn't been changed.
There was a problem hiding this comment.
@cyanglaz I just pushed a clean commit without the web changes
There was a problem hiding this comment.
Since web directly
extends ImagePickerPlatform, it's required to match the function signatures forImagePickerPlatformAs a result, the automated web tests fail if it's not also changed to match. Granted, there's no changes to web beyond adding
bool forceFullMetadata = true,to the function signatures.
This is a description of why this is a breaking change, so I'm not clear on why this was landed. You cannot add parameters to methods to an interface without breaking all implementations.
You also cannot just ignore that something is a breaking change by also pushing a change in another package. I don't understand how this was approved with unpublished code changes to a separate package.
There was a problem hiding this comment.
@cyanglaz I just pushed a clean commit without the web changes
Part of the problem here is that this did not actually happen :(
cyanglaz
added
the
waiting for tree to go green
| @@ -73,6 +78,7 @@ abstract class ImagePickerPlatform extends PlatformInterface { | |||
| double? maxWidth, | |||
| double? maxHeight, | |||
| int? imageQuality, | |||
| bool forceFullMetadata = true, | |||
There was a problem hiding this comment.
This is a breaking change. This needs to be reverted, as this version is broken (and we need to figure out how to make sure this kind of thing doesn't pass CI in the future).
Reverts #4288 which published a breaking change as if it were a non-breaking change. The discussion in that PR prior to it landing was incorrect, because adding a new parameter with a default value is non-breaking *only for clients*. It is breaking for subclasses that override it, and the purpose of the platform interface is for implementations to subclass it and override everything.
|
Now that the fire is hopefully out due to the revert: please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-platform-interface-method-parameters for the right way to re-implement this. |
) Reverts flutter#4288 which published a breaking change as if it were a non-breaking change. The discussion in that PR prior to it landing was incorrect, because adding a new parameter with a default value is non-breaking *only for clients*. It is breaking for subclasses that override it, and the purpose of the platform interface is for implementations to subclass it and override everything.
) Reverts flutter#4288 which published a breaking change as if it were a non-breaking change. The discussion in that PR prior to it landing was incorrect, because adding a new parameter with a default value is non-breaking *only for clients*. It is breaking for subclasses that override it, and the purpose of the platform interface is for implementations to subclass it and override everything.
) Reverts flutter#4288 which published a breaking change as if it were a non-breaking change. The discussion in that PR prior to it landing was incorrect, because adding a new parameter with a default value is non-breaking *only for clients*. It is breaking for subclasses that override it, and the purpose of the platform interface is for implementations to subclass it and override everything.
Splitting interface portion from #3264 per #3264 (comment)
Description
Make iOS 11+ permissions requests optional per https://developer.apple.com/forums/thread/653414.
PHAssets aren't actually required and there was already some fallback code in place.
To keep this as a non-breaking change, I added an optional parameter of
forceFullMetadatawith a default oftrue.Setting this to false will disable any permissions checking on iOS and simply display the image picker.
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.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?