apache / cordova-plugin-file

Apache Cordova File Plugin
https://cordova.apache.org/
Apache License 2.0
741 stars 756 forks source link

CB-9887 cordova.file.* paths for windows platform #143

Closed ghenry22 closed 8 years ago

ghenry22 commented 8 years ago

Update the plugin to define cordova.file.* paths for windows platform as appropriate.

Documentation updated to show where to store things.

Have tested this in my own app and the paths are detected as file plugin works as expected now.

Means you can use cordova.file.dataDirectory in your app and then run that app on ios, android, windows with consistent behaviour without requiring platform specific code.

alexangas commented 8 years ago

I've tested (on Windows Phone 8.1) and examined this and it looks great, thanks! Is there any reason not to define applicationStorageDirectory and set it to the same value as dataDirectory?

ghenry22 commented 8 years ago

my understanding is that applicationStorageDirectory is a bit redundant. It is the same as the applicationDirectory but has a note that you can change to the specific subdirectories that might have write access (ie /localState is the same as where dataDirectory already points).

I can add it in case it's useful to someone purely for compatibility though.

ghenry22 commented 8 years ago

added applicationStorageDirectory to defined paths and updated readme with relevant comments for this path.

alexangas commented 8 years ago

I see, I wasn't aware of that! I've tested this and looks good, cheers. It would be great to get this merged.

purplecabbage commented 8 years ago

Hi @ghenry22 ! This looks good, as do your other prs. Have you signed an iCLA? [1] It is hard to tell because you do not have your name on your github profile, so I have nothing the verify against [2]

[1] http://cordova.apache.org/contribute/ [2] https://people.apache.org/committer-index.html

ghenry22 commented 8 years ago

@purplecabbage yep I have signed an ICLA and have already had a PR merged into the file plugin.

I'll send you a direct message on slack with full name etc to make it easier to verify

jasongin commented 8 years ago

It looks like CB-9090 and CB-8257 are about exactly the same problem as this CB-9887.

ghenry22 commented 8 years ago

CB-9090 is for the whitelist plugin CB-8257 is the same issue but for the wp8 platform. This fix is for the windows platform, I don't have native code skills to update the wp8 plugin.

jasongin commented 8 years ago

Sorry I mistyped... I meant to refer to CB-9020 not CB-9090. I was thinking CB-8257 was referring to WP 8.1 (which uses the 'windows' platform for plugins), but it might actually be just WP 8.0, which doesn't support the cordova.file.* paths either.

ghenry22 commented 8 years ago

CB-9020 does say they are targeting WP 8.1 which uses the new windows platform so this PR would also resolve CB-9020.

CB-8257 seems to refer specifically to the wp8 platform which uses different native code. Although I thought that wp8 support was deprecated so perhaps that could be closed with a will not fix deprecated and advice to update to target WP8.1 to resolve the issue?

Regarding the path changes you mentioned you're probably right, or maybe it should be ms-appx-web? I'll do a bit of reading and update the PR if necessary.

ghenry22 commented 8 years ago

MS docs confirm the same, should be ms-appx:///

Committing an update to fix now.

reference: https://msdn.microsoft.com/en-us/library/windows.applicationmodel.package.installedlocation.aspx

ghenry22 commented 8 years ago

once merged you could also close out PR #121 as duplicate as it attempts to solve the same issue but generates absolute paths from c:... which are not usable in the app and result in encoding errors when used with file or file transfer plugin.

purplecabbage commented 8 years ago

Done! Thanks ghenry22 !