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 desktop support #3882

Merged
merged 35 commits into from
Jun 10, 2023

Conversation

stuartmorgan
Copy link
Contributor

@stuartmorgan stuartmorgan commented May 2, 2023

(This is the combination PR for overall review.)

This adds initial desktop support across all three desktop platforms:

  • Adds the concept of a camera delegation as discussed in [image_picker_windows] The Windows implementation of the image_picker should support ImageSource.camera. flutter#102115, and adds a helper base class at the platform interface level for implementations that want to use that approach. (Sharing it simplifies both the platform implementations and the client usage.)
  • Adds a new app-facing and platform-interface API to check at runtime for support for a given source, to make writing code using image_picker in a world of dynamic camera support viable. (In particular this is critical for published packages that use image_picker, as they won't know in advance if a client app will have a delegate set up).
  • Updates the Windows implementation to:
    • use the new delegation base class
    • implement the newer getImageFromSource API as an opportunistic fix while changing the code (if not implemented, it would fall back to getImage)
    • use new APIs in its example.
  • Made macOS and Linux implementations that are clones of the Windows approach. They are both slightly simpler however, as MIME type (for Linux) and UTI (for macOS) allow creating generic "image" and "video" filters instead of having to hard-code a file extension list.

Since in my opinion this level of support is sufficient to allow basic real-world use of the desktop implementations, due to having a structure in place for handling camera support, this includes endorsing the desktop implementations. It is still the case that desktop does not support image resizing, which isn't ideal, but I don't think we should block on that. It is clearly documented in the README.

The desktop implementations are separate packages even though they are mostly the same right now because:

  • the amount of (non-example) code being duplicated is small, and
  • it makes it much easier for us to change platform implementations over time; e.g., recent versions of macOS support PHPicker, meaning we can later add native code for that to the macOS implementation, with just a fallback to the current file_selector wrapper. It's plausible that better native options for Windows and/or Linux may become available in the future as well.

Fixes flutter/flutter#102115
Fixes flutter/flutter#102320
Fixes flutter/flutter#85100

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.

@stuartmorgan
Copy link
Contributor Author

As usual for adding new platforms, this looks like a ton more code to review than it actually is; most of it by volume is example boilerplate:

  • The Dart code in the implementation package examples is copied directly from the (slightly modified in this PR) Windows code
  • All of the non-Dart code in the examples is just flutter create boilerplate.

@stuartmorgan
Copy link
Contributor Author

I did this in a single PR because I wanted to approach it from the standpoint of "what do we need to get to something end-to-end workable to endorse desktop. It was useful for discovering things like, it works much better for everyone if the camera delegation goes into the platform interface as an option, rather than duplicated in each platform.

Now that I have that though, I'm happy to carve it up into a series of PRs if you would prefer smaller reviews:

  • Update the platform interface with the delegation concept.
  • Update Windows to use delegation.
  • Add an unendorsed macOS implementation.
  • Add an unendorsed Linux implementation.
  • Add the new app-facing API and endorsement.

auto-submit bot pushed a commit that referenced this pull request Jun 8, 2023
Platform interface portion of #3882

Adds CameraDelegatingImagePickerPlatform and ImagePickerCameraDelegate, and supportsImageSource

Part of flutter/flutter#102115
Part of flutter/flutter#102320
Part of flutter/flutter#85100
stuartmorgan added a commit to stuartmorgan/packages that referenced this pull request Jun 9, 2023
Platform implementation portion of
flutter#3882

Updates the Windows implementation to use the new base class for camera
delegation, and creates new macOS and Linux implementations that are
near-duplicates.

These are separate packages, rather than a single shared package,
because it's likely that they will diverge over time (e.g., the TODO for
macOS to use a system image picker control on newer versions of macOS),
and the amount of code that could be shared is minimal anyway.

Part of flutter/flutter#102115
Part of flutter/flutter#102320
Part of flutter/flutter#85100
stuartmorgan added a commit to stuartmorgan/packages that referenced this pull request Jun 9, 2023
Platform implementation portion of
flutter#3882

Updates the Windows implementation to use the new base class for camera
delegation, and creates new macOS and Linux implementations that are
near-duplicates.

These are separate packages, rather than a single shared package,
because it's likely that they will diverge over time (e.g., the TODO for
macOS to use a system image picker control on newer versions of macOS),
and the amount of code that could be shared is minimal anyway.

Part of flutter/flutter#102115
Part of flutter/flutter#102320
Part of flutter/flutter#85100
auto-submit bot pushed a commit that referenced this pull request Jun 9, 2023
Platform implementation portion of
#3882

Updates the Windows implementation to use the new base class for camera delegation, and creates new macOS and Linux implementations that are near-duplicates.

These are separate packages, rather than a single shared package, because it's likely that they will diverge over time (e.g., the TODO for macOS to use a system image picker control on newer versions of macOS), and the amount of code that could be shared is minimal anyway.

Part of flutter/flutter#102115 Part of flutter/flutter#102320 Part of flutter/flutter#85100
@stuartmorgan stuartmorgan added the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label Jun 10, 2023
@stuartmorgan
Copy link
Contributor Author

Overriding changelog: The remaining macOS changes in this PR are just the binary files from the example app that I accidentally didn't transfer to the other branch because I created it using a patch file.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 10, 2023
@auto-submit auto-submit bot merged commit cd7b935 into flutter:main Jun 10, 2023
@stuartmorgan stuartmorgan deleted the image-picker-macos-linux branch June 11, 2023 13:28
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 12, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 12, 2023
flutter/packages@afe2f05...c9865e8

2023-06-11 engine-flutter-autoroll@skia.org Roll Flutter from da127f1 to 3df163f (25 revisions) (flutter/packages#4183)
2023-06-10 stuartmorgan@google.com [image_picker] Add desktop support (flutter/packages#3882)
2023-06-09 tarrinneal@gmail.com [image_picker] getMedia platform changes (flutter/packages#4174)
2023-06-09 stuartmorgan@google.com [image_picker] Add desktop support - implementations (flutter/packages#4172)
2023-06-09 engine-flutter-autoroll@skia.org Roll Flutter from 6e254a3 to da127f1 (28 revisions) (flutter/packages#4170)
2023-06-09 32538273+ValentinVignal@users.noreply.github.com [go_router_builder] Include required and positional query parameters in the location (flutter/packages#4163)

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 flutter-ecosystem@google.com,rmistry@google.com 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
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 override: no changelog needed Override the check requiring CHANGELOG updates for most changes p: image_picker platform-linux platform-macos platform-windows
Projects
None yet
3 participants