[image_picker] Fix path of returned File objects when picking videos #1505

LinusU · 2019-04-17T15:30:55Z

Description

Returning .description on a NSURL gave back an url that starts with file:///, when passing this url to the File object it constructed a path like /file:///foo/bar/baz.ext.

This fixes this logic to use .path instead, which returns the path to the file.

Here is some more background to how I found this: https://github.com/firebase/firebase-ios-sdk/pull/2458/files#r276273826

Related Issues

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.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

Not sure how to add tests since the tests since to mock out the Objective-C part of the plugins, any help would be appreciated!

collinjackson · 2019-04-21T02:59:29Z

I tried manually testing this in the iOS simulator and unfortunately I'm getting "Error Loading Video" (both before and after the change)

cyanglaz · 2019-04-22T21:20:42Z

I tried manually testing this in the iOS simulator and unfortunately I'm getting "Error Loading Video" (both before and after the change)

Just curious, how did you test camera with simulator?

collinjackson · 2019-04-22T21:33:32Z

I tried manually testing this in the iOS simulator and unfortunately I'm getting "Error Loading Video" (both before and after the change)

Just curious, how did you test camera with simulator?

I didn't, I just dragged and dropped a MOV file onto the simulator to get it into the videos library.

cyanglaz · 2019-04-23T00:15:18Z

@LinusU Thank you for providing this. the .description returns the absolute path to the source. However .path seems to be more relevant to iOS file system and Apple seems suggesting to use .path.

Could you provide the use case that the absolute path failed? It seems that dart doesn't care which way the file path was expressed and works for both paths.

Also, it is probably a breaking change since the format of the returning value is changed, could you update the minor version? to 0.6.0?

LinusU · 2019-04-23T07:51:12Z

@cyanglaz

Thank you for providing this. the .description returns the absolute path to the source. However .path seems to be more relevant to iOS file system and Apple seems suggesting to use .path.

Since this is an NSURL instance, it returns an absolute url, not path. So the problem is that it returned file:///foo/bar/baz, instead of /foo/bar/baz.

When creating a dart File with the string file:///foo/bar/baz, it will not create a file that points to /foo/bar/baz, but rather a file that points to /file:///foo/bar/baz, which won't work.

Could you provide the use case that the absolute path failed? It seems that dart doesn't care which way the file path was expressed and works for both paths.

I'm getting an error that Dart cannot read the file /file:///foo/bar/baz, I don't see how this can have ever worked, unless iOS 12.2 changed the behaviour of .description on NSURL.

Also, it is probably a breaking change since the format of the returning value is changed, could you update the minor version? to 0.6.0?

I don't think it's a breaking change since this works around an error. Even if description previously returned the file path without prepending file:/// in front of it, it shouldn't change now...

Hope this clears it up ☺️

I'll rebase on master to fix the conflicts! 🚀

cyanglaz · 2019-04-23T16:37:10Z

I'm getting an error that Dart cannot read the file /file:///foo/bar/baz, I don't see how this can have ever worked, unless iOS 12.2 changed the behaviour of .description on NSURL.

I just checked with the the current flutter, it is able to successfully read the file at file:///...., if you run the example app on image picker, does it pick up the file?

wreppun · 2019-04-23T16:49:37Z

I can confirm this happens for us as well. We have

File _stripUriScheme(File file) {
  if (file.path.startsWith("file:///")) {
    final filePath = file.path.substring(7);
    debugPrint('File URL pattern dropped... path is now: $filePath');
    return File(filePath);
  }
  // N.B. file://<some-host>/<some-path> is not handled
  // ...but I don't think that happens in practice
  return file;
}

as a workaround until this gets merged.

IIRC, it was as simple as running

File f = await ImagePicker.pickVideo(source: ImageSource.gallery);
file.lengthSync();

cyanglaz · 2019-04-23T16:54:56Z

I can confirm this happens for us as well. We have

File _stripUriScheme(File file) {
  if (file.path.startsWith("file:///")) {
    final filePath = file.path.substring(7);
    debugPrint('File URL pattern dropped... path is now: $filePath');
    return File(filePath);
  }
  // N.B. file://<some-host>/<some-path> is not handled
  // ...but I don't think that happens in practice
  return file;
}

as a workaround until this gets merged.

IIRC, it was as simple as running

File f = await ImagePicker.pickVideo(source: ImageSource.gallery);
file.lengthSync();

@LinusU As commented here by @wreppun, people might be doing some surgery on the return value. So it is probably a breaking change for them. I still think we should bump the minor version and make it 0.6.0.

collinjackson · 2019-04-23T17:59:31Z

I agree, let's update the CHANGELOG with a description of new behavior and call it 0.6.0

LinusU · 2019-04-24T12:29:18Z

Rebased and changed to breaking change 👌

cyanglaz · 2019-04-24T16:40:59Z

packages/image_picker/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.6.0
+
+* Breaking change: Path of returned File objects when picking videos does no longer start with `file:///`, but are instead a normal absolute path.


please specify it is on iOS platform. Also the path starts with file:/// is the absolute path, you might want to update the wording a little bit?

Is this better?

Breaking change iOS: Returned File objects when picking videos now always holds an absolute path. Before this change, a File object with a path beginning with /file:/// could be returned.

Maybe
Breaking change iOS: Returned `File` objects when picking videos now always holds the correct path. Before this change, the absolute path (Begin with file:///) was returned.

(Begin with file:///)

just to clarify, the path returned on the File object (e.g. print(file.path)) starts with a single forward slash, and then file:, followed by three forward slashes. At least on my iPhone 8, iOS 12.2.

This is the code that we are currently using in our app to work around it:

if (file.path.startsWith('/file:///')) { file = File(file.path.substring(8)); }

Changed to

Breaking change iOS: Returned File objects when picking videos now always holds the correct path. Before this change, the path returned could have file:// prepended to it.

What do you think?

(Begin with file:///)

just to clarify, the path returned on the File object (e.g. print(file.path)) starts with a single forward slash, and then file:, followed by three forward slashes. At least on my iPhone 8, iOS 12.2.

This is the code that we are currently using in our app to work around it:

if (file.path.startsWith('/file:///')) { file = File(file.path.substring(8)); }

yeah that was a typo of mine, you can change it to /file:/// in the changelog.

LinusU requested a review from cyanglaz as a code owner April 17, 2019 15:30

googlebot added the cla: yes label Apr 17, 2019

LinusU mentioned this pull request Apr 17, 2019

FIRStorageUploadTask: fail on invalid file URL firebase/firebase-ios-sdk#2458

Merged

collinjackson self-requested a review April 21, 2019 02:26

collinjackson approved these changes Apr 21, 2019

View reviewed changes

cyanglaz mentioned this pull request Apr 22, 2019

[image_picker] example app video load error fix. #1531

Merged

LinusU force-pushed the image-picker-video-url branch from 2f36ef8 to 5840688 Compare April 23, 2019 07:57

bparrishMines added the bugfix label Apr 23, 2019

LinusU force-pushed the image-picker-video-url branch from 5840688 to fbc1a91 Compare April 24, 2019 12:29

LinusU force-pushed the image-picker-video-url branch from fbc1a91 to 6d082d8 Compare April 24, 2019 12:30

cyanglaz suggested changes Apr 24, 2019

View reviewed changes

LinusU force-pushed the image-picker-video-url branch 2 times, most recently from a30d144 to fb086c1 Compare April 24, 2019 17:04

[image_picker] Fix path of returned File objects when picking videos

650d217

LinusU force-pushed the image-picker-video-url branch from fb086c1 to 650d217 Compare April 24, 2019 22:07

cyanglaz merged commit 055c2ee into flutter:master Apr 25, 2019

LinusU deleted the image-picker-video-url branch April 26, 2019 08:31

		@@ -1,3 +1,7 @@
		## 0.6.0

		* Breaking change: Path of returned File objects when picking videos does no longer start with `file:///`, but are instead a normal absolute path.

[image_picker] Fix path of returned File objects when picking videos #1505

[image_picker] Fix path of returned File objects when picking videos #1505

Uh oh!

Conversation

LinusU commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

collinjackson commented Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

collinjackson commented Apr 22, 2019

Uh oh!

cyanglaz commented Apr 23, 2019

Uh oh!

LinusU commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz commented Apr 23, 2019

Uh oh!

wreppun commented Apr 23, 2019

Uh oh!

cyanglaz commented Apr 23, 2019

Uh oh!

collinjackson commented Apr 23, 2019

Uh oh!

LinusU commented Apr 24, 2019

Uh oh!

cyanglaz Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

LinusU Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

LinusU Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

LinusU Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

cyanglaz Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LinusU commented Apr 17, 2019 •

edited

Loading

collinjackson commented Apr 21, 2019 •

edited

Loading

cyanglaz commented Apr 22, 2019 •

edited

Loading

LinusU commented Apr 23, 2019 •

edited

Loading