The Wayback Machine - https://web.archive.org/web/20210113101328/https://github.com/apache/cordova-plugin-file/pull/259
Skip to content

/ cordova-plugin-file

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

Jump to bottom

feat(windows): known folders support #259

Open
wants to merge 5 commits into
base: master
from
Open

feat(windows): known folders support #259

wants to merge 5 commits into from

Conversation

@convey-gilbert
Copy link

@convey-gilbert convey-gilbert commented Oct 3, 2018 •
edited by janpio

Access to known user folders:
documentsDirectory: Documents,
picturesDirectory: Pictures,
musicDirectory: Music,
videoDirectory: Videos

Platforms affected

Windows

What does this PR do?

Add support for known folders. Essential feature that is needed to access any file outside of private app sandbox

Limitation:

There is stil a problem, that working on known folders / file libraries like this will only work on Windows 10, not on 8.x
(copied from comment below)

What testing has been done on this change?

Tested on Windows 10 Pro 1803 Build 17134.320

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.
@convey-gilbert
Windows known folders support
Loading status checks…
2999522 
Access to known user folders:
documentsDirectory: Documents, picturesDirectory: Pictures,
musicDirectory: Music,
videoDirectory: Videos
@janpio
Copy link
Member

@janpio janpio commented Oct 3, 2018

Nice!

eslint is not happy though:

/home/travis/build/apache/cordova-plugin-file/www/resolveLocalFileSystemURI.js
  58:18  error  Expected exception block, space or tab after '/*' in comment  spaced-comment
  58:18  error  Expected space or tab before '*/' in comment                  spaced-comment
@convey-gilbert
Comment deleted
Loading status checks…
8a845c3 
Has no effect
@janpio
Copy link
Member

@janpio janpio commented Oct 3, 2018 •
edited

Ouch, doesn't like your editor settings:

/home/travis/build/apache/cordova-plugin-file/src/windows/FileProxy.js
  186:2   error  Unexpected tab character                           no-tabs
  186:2   error  Expected indentation of 4 spaces but found 1 tab   indent
  186:58  error  Strings must use singlequote                       quotes
  187:2   error  Unexpected tab character                           no-tabs
  187:3   error  Expected indentation of 4 spaces but found 2 tabs  indent
  188:2   error  Unexpected tab character                           no-tabs
  188:2   error  Expected indentation of 0 spaces but found 1 tab   indent
  189:2   error  Unexpected tab character                           no-tabs
  189:2   error  Expected indentation of 4 spaces but found 1 tab   indent
  189:76  error  Strings must use singlequote                       quotes
  190:2   error  Unexpected tab character                           no-tabs
  190:3   error  Expected indentation of 4 spaces but found 2 tabs  indent
  191:2   error  Expected indentation of 0 spaces but found 1 tab   indent
  191:2   error  Unexpected tab character                           no-tabs
  192:2   error  Unexpected tab character                           no-tabs
  192:2   error  Expected indentation of 4 spaces but found 1 tab   indent
  193:2   error  Unexpected tab character                           no-tabs
  193:3   error  Expected indentation of 4 spaces but found 2 tabs  indent
  194:2   error  Mixed spaces and tabs                              no-mixed-spaces-and-tabs
  194:6   error  Unexpected tab character                           no-tabs
  195:2   error  Unexpected tab character                           no-tabs
  195:3   error  Expected indentation of 0 spaces but found 2 tabs  indent
  196:2   error  Unexpected tab character                           no-tabs
  196:3   error  Expected indentation of 4 spaces but found 2 tabs  indent
  197:2   error  Unexpected tab character                           no-tabs
  197:2   error  Mixed spaces and tabs                              no-mixed-spaces-and-tabs
  198:2   error  Unexpected tab character                           no-tabs
  198:3   error  Expected indentation of 0 spaces but found 2 tabs  indent
  199:2   error  Unexpected tab character                           no-tabs
  199:3   error  Expected indentation of 4 spaces but found 2 tabs  indent
  200:2   error  Mixed spaces and tabs                              no-mixed-spaces-and-tabs
  200:2   error  Unexpected tab character                           no-tabs
  201:2   error  Unexpected tab character                           no-tabs
  201:3   error  Expected indentation of 0 spaces but found 2 tabs  indent
  202:2   error  Unexpected tab character                           no-tabs
  202:3   error  Expected indentation of 4 spaces but found 2 tabs  indent
  203:2   error  Mixed spaces and tabs                              no-mixed-spaces-and-tabs
  203:2   error  Unexpected tab character                           no-tabs
  204:2   error  Unexpected tab character                           no-tabs
  204:3   error  Expected indentation of 0 spaces but found 2 tabs  indent
  205:2   error  Expected indentation of 0 spaces but found 1 tab   indent
  205:2   error  Unexpected tab character                           no-tabs

By the way: There is no real testing going on for Windows itself, we are currently working on fixing that.

PS: You can probably use eslint --fix (or a similar command) to fix that automatically.
@janpio janpio added this to 🐣 New PR / Untriaged in Apache Cordova: Plugin Pull Requests Oct 3, 2018
@convey-gilbert
Copy link
Author

@convey-gilbert convey-gilbert commented Oct 4, 2018

I can fix this irrelevant formalia you criticize. But beside that, it'll be more important to be concerned of real functionality and usefulness of the module.
There is stil a problem, that working on known folders / file libraries like this will only work on Windows 10, not on 8.x
I don't need to mention the mess with Android and memory cards :/
It might be a much better approach to use a syntax for symbolic path placeholders. At least Documents, Pictures, Music and Videos. And at least for current-user, user: and maybe public/default for systems without multi-user management. Eventual use of memory card should be transparent to the caller.
The pseudo-path will always access the root folder of the file library, sub-folders maybe added with path delimiter /
@janpio
Copy link
Member

@janpio janpio commented Oct 4, 2018 •
edited

I can fix this irrelevant formalia you criticize.

It is not me criticizing, but the eslint tool that we use on all/most Apache Cordova software. I just copied its output from CI over. As this enables us to keep a common code standard in Apache Cordova projects, it is very much relevant though.

It might be a much better approach to use a syntax ...

That sounds pretty sensible. Best open an issue in this repo and describe the changes you propose and the reasoning behind them. Sounds like a bigger change though, and would most probably require a new major release.
@convey-gilbert
Copy link
Author

@convey-gilbert convey-gilbert commented Oct 4, 2018

@janpio: I've now installed VisualLinter and will try to check for the tab/space inconsistencies again.
I've seen a typo, too: Property videoDirectory should be called videosDirectory. I'll change that, too.
I can work on the concept for virtual known folders maybe later. For the moment the extension enables at least on current Windows 10 devices automatic inter-store app and desktop app <--> store app file exchange.
I'll be back at this plugin after other more urgent work on cordova-plugin-clippingcamera, cordova-plugin-media-capture and creating a new plugin for useful interaction with usb/virtual-com-port devices on pc devices first, sorry… ;)
@janpio
Copy link
Member

@janpio janpio commented Oct 4, 2018

No worry, as long as you don't forget to fix this up we will be happy ;)
convey-gilbert and others added 3 commits Oct 4, 2018
@convey-gilbert
Typo videosDirectory
Loading status checks…
2f97eae 
and replaced tab with spaces
@convey-gilbert
Android: Decreased version requirements
Loading status checks…
ed1f86c
@janpio
Fix "Strings must use singlequote" eslint
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
9e51810
@aximobile
Copy link

@aximobile aximobile commented Nov 9, 2018

Hello,

I've been testing this pull request on the following device: Windows 10 pro 1511 build 10586.1176

It seems like Windows.Storage.UserDataPaths is always undefined.
I did some research and it looks like you need the Windows 10 Fall Creators Update (introduced v10.0.16299.0) to use the UserDataPaths.

Only specific Windows 10 devices can download this update which means that not all Windows 10 devices support this extension(like mine).

Is there any other way to get access to the known folders ?

I'm not a pro in native Windows development so any tips are welcome.
@janpio janpio moved this from 🐣 New PR / Untriaged to ⛔ Blocked: Tests failing in Apache Cordova: Plugin Pull Requests Jul 2, 2019
@janpio
janpio reviewed Jul 4, 2019
View changes
plugin.xml
@@ -30,7 +30,7 @@ xmlns:android="http://schemas.android.com/apk/res/android"
<issue>https://issues.apache.org/jira/browse/CB/component/12320651</issue>

<engines>
<engine name="cordova-android" version=">=6.3.0" />
<engine name="cordova-android" version=">=6.0.0" />

This comment has been minimized.

Sign in to view
@janpio

janpio Jul 4, 2019
Member

Why this change?

This comment has been minimized.

Sign in to view
@convey-gilbert

convey-gilbert Jul 5, 2019
Author

In fact we have still build configurations with cordova-android: 6.0.0
We haven't recognized an incompatibility with cordova-plugin-file. Was there a serious reason behind to restrict a higher cordova-android version?

@janpio janpio changed the title Windows known folders support feat(windows): known folders support Jul 4, 2019
@janpio
janpio requested changes Jul 4, 2019
View changes
Copy link
Member

@janpio janpio left a comment

This PR also needs changes to the README so this is actually discoverable to users: https://github.com/apache/cordova-plugin-file#where-to-store-files
Apache Cordova: Plugin Pull Requests automation moved this from ⛔ Blocked: Tests failing to 🙅 Reviewed, but not approved Jul 4, 2019
@janpio janpio closed this Jul 4, 2019
Apache Cordova: Plugin Pull Requests automation moved this from 🙅 Reviewed, but not approved to ☠️ Closed/Abandoned Jul 4, 2019
@janpio
Copy link
Member

@janpio janpio commented Jul 4, 2019

Restart tests.
@janpio janpio reopened this Jul 4, 2019
Apache Cordova: Plugin Pull Requests automation moved this from ☠️ Closed/Abandoned to 🐣 New PR / Untriaged Jul 4, 2019
@janpio
Copy link
Member

@janpio janpio commented Jul 4, 2019

CI: Once more please.
@janpio janpio closed this Jul 4, 2019
Apache Cordova: Plugin Pull Requests automation moved this from 🐣 New PR / Untriaged to ☠️ Closed/Abandoned Jul 4, 2019
@janpio janpio reopened this Jul 4, 2019
Apache Cordova: Plugin Pull Requests automation moved this from ☠️ Closed/Abandoned to 🐣 New PR / Untriaged Jul 4, 2019
@janpio janpio moved this from 🐣 New PR / Untriaged to 🙅 Reviewed, but not approved in Apache Cordova: Plugin Pull Requests Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janpio janpio

Assignees
No one assigned
Labels
enhancement platform: windows
Projects
Apache Cordova: Plugin Pull Requests
🙅 Reviewed, but not approved
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@convey-gilbert @janpio @aximobile
You can’t perform that action at this time.