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

[image_picker] Don't store null paths in lost cache #6678

Conversation

stuartmorgan
Copy link
Contributor

If the user cancels image selection on Android, store nothing in the lost image cache rather than storing an array with a null path.

While we could potentially keep this behavior and instead handle it differently on the Dart side, returning some new "cancelled" LostDataResponse, that would be semi-breaking; e.g., the current example's lost data handling would actually throw as written if we had a new non-isEmpty, non-exception, null-file response. Since nobody has requested the ability to specifically detect a "lost cancel" as being different from not having started the process of picking anything, this doesn't make that potentially-client-breaking change. If it turns out there's a use case for that in the future, we can revisit that (but should not do it by storing a null entry in a file array anyway).

Fixes flutter/flutter#114551

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.

If the user cancels image selection on Android, store nothing in the
lost image cache rather than storing an array with a null path.

While we could potentially keep this behavior and instead handle it
differently on the Dart side, returning some new "cancelled"
`LostDataResponse`, that would be semi-breaking; e.g., the current
example's lost data handling would actually throw as written if we had a
new non-`isEmpty`, non-exception, null-`file` response. Since nobody has
requested the ability to specifically detect a "lost cancel" as being
different from not having started the process of picking anything, this
doesn't make that potentially-client-breaking change. If it turns out
there's a use case for that in the future, we can revisit that (but
should not do it by storing a null entry in a file array anyway).

Fixes flutter/flutter#114551
Copy link
Contributor

@GaryQian GaryQian 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 Dec 16, 2022
@auto-submit auto-submit bot merged commit 4d11be4 into flutter:main Dec 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 19, 2022
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 19, 2022
* 5f62d21eb [local_auth] Fix failed biometric authentication not throwing error (flutter/plugins#6821)

* ca974ab0c [webview_flutter_web] Copies web implementation of webview_flutter from v4_webview (flutter/plugins#6854)

* 4d11be416 [image_picker] Don't store null paths in lost cache (flutter/plugins#6678)

* fd2841fd0 [webview_flutter_android] Fix timeouts in the integration tests (flutter/plugins#6857)

* abc9f9a9b [flutter_plugin_tools] If `clang-format` does not run, fall back to other executables in PATH (flutter/plugins#6853)

* 7efb5e89d [video_player] Add compatibility with the current platform interface (flutter/plugins#6855)

* 32dcbf3e3 [image_picker] Improve image_picker for iOS to handle more image types (flutter/plugins#6812)

* 840a04954 [webview_flutter] Copies app-facing implementation of webview_flutter from v4_webview (flutter/plugins#6856)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
…#117314)

* 5f62d21eb [local_auth] Fix failed biometric authentication not throwing error (flutter/plugins#6821)

* ca974ab0c [webview_flutter_web] Copies web implementation of webview_flutter from v4_webview (flutter/plugins#6854)

* 4d11be416 [image_picker] Don't store null paths in lost cache (flutter/plugins#6678)

* fd2841fd0 [webview_flutter_android] Fix timeouts in the integration tests (flutter/plugins#6857)

* abc9f9a9b [flutter_plugin_tools] If `clang-format` does not run, fall back to other executables in PATH (flutter/plugins#6853)

* 7efb5e89d [video_player] Add compatibility with the current platform interface (flutter/plugins#6855)

* 32dcbf3e3 [image_picker] Improve image_picker for iOS to handle more image types (flutter/plugins#6812)

* 840a04954 [webview_flutter] Copies app-facing implementation of webview_flutter from v4_webview (flutter/plugins#6856)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
If the user cancels image selection on Android, store nothing in the
lost image cache rather than storing an array with a null path.

While we could potentially keep this behavior and instead handle it
differently on the Dart side, returning some new "cancelled"
`LostDataResponse`, that would be semi-breaking; e.g., the current
example's lost data handling would actually throw as written if we had a
new non-`isEmpty`, non-exception, null-`file` response. Since nobody has
requested the ability to specifically detect a "lost cancel" as being
different from not having started the process of picking anything, this
doesn't make that potentially-client-breaking change. If it turns out
there's a use case for that in the future, we can revisit that (but
should not do it by storing a null entry in a file array anyway).

Fixes flutter/flutter#114551
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: image_picker platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[image_picker] [Android] retrieveLostData raises _CastError if user cancelled choosing a photo
2 participants