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

[image_picker_platform_interface] Migrate image_picker to package:cross_file #4072

Merged
merged 9 commits into from
Jul 8, 2021

Conversation

BeMacized
Copy link
Contributor

@BeMacized BeMacized commented Jun 21, 2021

Removed the PickedFile implementations and migrated the platform interface to make use of the cross_file library.
Changes have been based on #3388.

Web implementation can be found in: #4083.

Android/iOS implementation can be found in #4073.

Relevant issue:

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. (Note that 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the 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.

@ditman
Copy link
Member

ditman commented Jun 29, 2021

@BeMacized I've been going back and forth with this one for a couple of days, but we have some internal customers that would benefit greatly from this being a non-breaking change.

Instead of changing the signature of the methods to return an XFile, could we add a new set of methods that return the new type, while keeping the old methods available (and mark them as deprecated?)

That way our customers are going to be able to see the incoming change before forcing them to update their apps.

At a later date, after a few months, or when the next inescapable major version comes, we can delete the deprecated methods and the PickedFile class :/

What do you think?

@BeMacized
Copy link
Contributor Author

@ditman If you believe this is something important enough that we should take that route, I'd say we do it.

The only thing I'm a bit vague on atm is what the naming should be for the new alternatives to pickImage, pickVideo and LostData. Just putting a "XFile" suffix on them feels a bit odd, especially considering they will be the only ones left after their deprecated counterparts have been removed in a later version.

Do you happen to have any suggestions?

@ditman
Copy link
Member

ditman commented Jun 29, 2021

@BeMacized I think we're at a point where we can reuse the names from v0.6, that have already been removed from everywhere :)

I'd say we replace pick by get, maybe? getImage, getVideo, getMultipleImages... It's not great because we can trigger the camera with this, but at least it doesn't need to use XFile anywhere in the name.

As for the LostData, we might reuse the names from the earlier version? LostData class -> LostDataResponse, and retrieveLostData -> getLostData?

There's some notes about the old method names here: https://pub.dev/documentation/image_picker/latest/#deprecation-warnings-in-pickimage-pickvideo-and-lostdataresponse

@BeMacized
Copy link
Contributor Author

@ditman Ah, that indeed sounds like a good idea! I'll see if I can update the PRs for this tomorrow.

@ditman
Copy link
Member

ditman commented Jun 29, 2021

Thanks for the patience and extra work @BeMacized!

@BeMacized
Copy link
Contributor Author

@ditman Would it be possible for you to take another look? :)

@ayushin
Copy link

ayushin commented Jul 6, 2021

Really looking forward to this merged

@ditman
Copy link
Member

ditman commented Jul 7, 2021

Apologies, I was working in fixing a framework P2, I'll get on this today.

@ditman
Copy link
Member

ditman commented Jul 8, 2021

I just realized that making things @Deprecated here will break other packages when this gets published, see for example the image_picker with this patch:

dit@dit:~/github/plugins/packages/image_picker/image_picker$ flutter analyze
Warning: You are using these overridden dependencies:
! image_picker_platform_interface 2.2.0 from path ../image_picker_platform_interface
Running "flutter pub get" in image_picker...                       807ms
Analyzing image_picker...                                               

   info • 'LostData' is deprecated and shouldn't be used. Switch to using LostDataResponse instead • test/image_picker_test.dart:313:17 •
          deprecated_member_use
   info • 'LostData' is deprecated and shouldn't be used. Switch to using LostDataResponse instead • test/image_picker_test.dart:326:17 •
          deprecated_member_use

For other users the breakage might be different. I'll update this PR removing the @Deprecated annotations. We'll add the deprecation warnings to the image_picker package directly, so when people attempt to use the old methods that return PickedFile, instead of the new ones that return XFile they'll get the message (plugin users, not us!).

That way people will get the deprecation warnings only once everything is in place for them to switch to the new code.

Once the methods that are actually used by users are marked as deprecated, we can revisit this package and mark these as deprecated (even though it shouldn't be needed; they can be directly removed after the deprecated methods are removed from the core plugin).

My bad, I'll fix this PR ASAP!

@google-cla

This comment has been minimized.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jul 8, 2021
@ditman

This comment has been minimized.

@google-cla google-cla bot added cla: yes and removed cla: no labels Jul 8, 2021
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

This looks good to me, I'll take an eye to the tests and see what's going on there (the branch needed a refresh from master to pick some changes from earlier today).

@BeMacized, can you verify that you can still work on the other packages with my changes applied? Once you verify that you can still keep working, we can merge this change.

@BeMacized
Copy link
Contributor Author

@ditman I can confirm the changes have no effect on the implementation PRs, they have been updated accordingly.

@ditman
Copy link
Member

ditman commented Jul 9, 2021

Finally published: https://pub.dev/packages/image_picker_platform_interface/changelog#220

amantoux pushed a commit to amantoux/plugins that referenced this pull request Jul 10, 2021
Ralph-Li added a commit to Insight-Timer/plugins that referenced this pull request Jul 15, 2021
* upstream_master: (40 commits)
  [image_picker] Image picker fix camera device (flutter#3898)
  [flutter_plugin_tools] Improve license-check output (flutter#4154)
  [webview_flutter] Fix broken keyboard issue link (flutter#3266)
  [flutter_plugin_tools] Support format on Windows (flutter#4150)
  [flutter_plugin_tools] Make unit tests pass on Windows (flutter#4149)
  [image_picker_for_web] Migrate image_picker to package:cross_file (flutter#4083)
  [various] Prepare plugin repo for binding API improvements (flutter#4148)
  [quick_actions] Add const constructor (flutter#4131)
  [in_app_purchase] Add iOS currency symbol to ProductDetails (flutter#4144)
  [in_app_purchase] Added priceCurrencySymbol to SkuDetailsWrapper (flutter#4114)
  [image_picker_platform_interface] Add methods that return package:cross_file (flutter#4072)
  [flutter_plugin_tools] Improve and test 'format' (flutter#4145)
  [flutter_plugin_tools] Only check target packages in analyze (flutter#4146)
  [in_app_purchase] Fix crash when retrieveReceiptWithError gives an error. (flutter#4138)
  [video_player] Pause video when it completes (flutter#3727)
  [in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115)
  [in_app_purchase] Add documentation for price change confirmations (flutter#4092)
  [camera] android-rework part 8: Supporting modules for final implementation (flutter#4054)
  [plugin_platform_interface] Fix README broken link (flutter#4143)
  [various] Prepare plugin repo for binding API improvements (flutter#4137)
  ...
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
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