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

[image_picker_for_web] Migrate image_picker to package:cross_file #4083

Merged
merged 26 commits into from
Jul 9, 2021

Conversation

BeMacized
Copy link
Contributor

Changed all usages of PickedFile to XFile, to follow the changes made to the platform interface (#4072)

This PR will remain in draft state until the platform interface changes have been merged and published (#4072), so that the pubspec can be updated properly.

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.

@google-cla

This comment has been minimized.

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

This comment has been minimized.

@google-cla

This comment has been minimized.

@ditman
Copy link
Member

ditman commented Jul 9, 2021

Let's go! The platform_interface has been published here:

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

@google-cla

This comment has been minimized.

@ditman

This comment has been minimized.

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

ditman commented Jul 9, 2021

@BeMacized, I've brought the branch up to date with the latest master, removed the "deprecated" methods (let's add those only in the core plugin package), and used this opportunity to migrate the tests to run with flutter drive, instead of flutter test --platform=chrome (part of flutter/flutter#81982).

There's documentation linked in the new README files, but TL;DR, this is how tests can be run now:

  1. Ensure chromedriver --port=4444 is running
  2. From the root of the repo, run: dart ./script/tool/lib/src/main.dart drive-examples --packages=image_picker/image_picker_for_web --web.

And this is the output:

dit@dit:~/github/plugins$ dart ./script/tool/lib/src/main.dart drive-examples --packages=image_picker/image_picker_for_web --web
script/tool/lib/src/main.dart: Warning: Interpreting this as package URI, 'package:flutter_plugin_tools/src/main.dart'.

============================================================
|| Running for image_picker_for_web
============================================================

Running command: "flutter drive -d web-server --web-port=7357 --browser-name=chrome --driver test_driver/integration_test.dart --target integration_test/image_picker_for_web_test.dart" in /usr/local/google/home/dit/github/plugins/packages/image_picker/image_picker_for_web/example
Running "flutter pub get" in example...                            614ms
Launching integration_test/image_picker_for_web_test.dart on Web Server in debug mode...
Waiting for connection from debug service on Web Server...         16.6s
integration_test/image_picker_for_web_test.dart is being served at http://localhost:7357
The web-server device requires the Dart Debug Chrome extension for debugging. Consider using the Chrome or Edge devices for an improved development workflow.
Application finished.
[WARNING]: http://localhost:7357/dart_sdk.js 48732:16 "registerExtension() from dart:developer is only supported in build/run/test environments where the developer event method hooks have been set."
[WARNING]: http://localhost:7357/dart_sdk.js 48714:16 "postEvent() from dart:developer is only supported in build/run/test environments where the developer event method hooks have been set."
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +0: Can select a file (Deprecated)"
[WARNING]: http://localhost:7357/packages/image_picker_for_web/image_picker_for_web.dart.js 204 File chooser dialog can only be shown with a user activation.
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +1: Can select a file"
[WARNING]: http://localhost:7357/packages/image_picker_for_web/image_picker_for_web.dart.js 204 File chooser dialog can only be shown with a user activation.
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +2: computeCaptureAttribute"
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +3: createInputElement accept: any, capture: null"
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +4: createInputElement accept: any, capture: something"
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +5: (tearDownAll)"
[INFO]: http://localhost:7357/dart_sdk.js 27220:14 "00:00 +6: All tests passed!"
All tests passed.


------------------------------------------------------------
Run overview:
  image_picker_for_web - ran

Ran for 1 package(s)


No issues found!

Let me know if you can run the tests (or not!) and if the code changes make sense to you! Then we can wrap this up and get it ready to be merged!

@ditman
Copy link
Member

ditman commented Jul 9, 2021

Just verified that tests are running in CI. See this run, for example.

@BeMacized BeMacized marked this pull request as ready for review July 9, 2021 07:24
@BeMacized BeMacized requested a review from cyanglaz as a code owner July 9, 2021 07:24
@BeMacized
Copy link
Contributor Author

@ditman Alright! I can confirm the tests succeed locally as well, so I've marked this PR as ready for review.

Comment on lines +219 to +222
final objectUrl = _handleOnChangeEvent(event);
if (!_completer.isCompleted && objectUrl != null) {
_completer.complete(XFile(objectUrl));
}
Copy link
Member

@ditman ditman Jul 9, 2021

Choose a reason for hiding this comment

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

Let's extract the name on disk of the file from this event so we can initialize the XFile with it! (to fix flutter/flutter#58764)

Copy link
Member

Choose a reason for hiding this comment

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

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.

We'll leave the name/length extraction for v2.1.1, so we can finish this migration without too much feature creep.

@ditman
Copy link
Member

ditman commented Jul 10, 2021

amantoux pushed a commit to amantoux/plugins that referenced this pull request Jul 10, 2021
…utter#4083)

* Migrate image picker platform interface to cross_file
* Port tests from --platform=chrome to integration_test

Co-authored-by: David Iglesias Teixeira <[email protected]>
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
…utter#4083)

* Migrate image picker platform interface to cross_file
* Port tests from --platform=chrome to integration_test

Co-authored-by: David Iglesias Teixeira <[email protected]>
@mvanbeusekom mvanbeusekom deleted the issue/70886-impl-web branch September 21, 2021 09:37
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