You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by GitBox <gi...@apache.org> on 2022/08/07 02:04:02 UTC

[GitHub] [cordova-plugin-file] breautek commented on a diff in pull request #532: added download path

breautek commented on code in PR #532:
URL: https://github.com/apache/cordova-plugin-file/pull/532#discussion_r939595126


##########
README.md:
##########
@@ -125,53 +128,53 @@ the `cordova.file.*` properties map to physical paths on a real device.
 ### iOS File System Layout
 
 | Device Path                                    | `cordova.file.*`            | `iosExtraFileSystems` | r/w? | persistent? | OS clears | sync | private |
-|:-----------------------------------------------|:----------------------------|:----------------------|:----:|:-----------:|:---------:|:----:|:-------:|
-| `/var/mobile/Applications/<UUID>/`             | applicationStorageDirectory | -                     | r    |     N/A     |     N/A   | N/A  |   Yes   |
-| &nbsp;&nbsp;&nbsp;`appname.app/`               | applicationDirectory        | bundle                | r    |     N/A     |     N/A   | N/A  |   Yes   |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`www/`     | -                           | -                     | r    |     N/A     |     N/A   | N/A  |   Yes   |
-| &nbsp;&nbsp;&nbsp;`Documents/`                 | documentsDirectory          | documents             | r/w  |     Yes     |     No    | Yes  |   Yes   |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`NoCloud/` | -                           | documents-nosync      | r/w  |     Yes     |     No    | No   |   Yes   |
-| &nbsp;&nbsp;&nbsp;`Library`                    | -                           | library               | r/w  |     Yes     |     No    | Yes? |   Yes   |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`NoCloud/` | dataDirectory               | library-nosync        | r/w  |     Yes     |     No    | No   |   Yes   |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`Cloud/`   | syncedDataDirectory         | -                     | r/w  |     Yes     |     No    | Yes  |   Yes   |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`Caches/`  | cacheDirectory              | cache                 | r/w  |     Yes*    |  Yes\*\*\*| No   |   Yes   |
-| &nbsp;&nbsp;&nbsp;`tmp/`                       | tempDirectory               | -                     | r/w  |     No\*\*  |  Yes\*\*\*| No   |   Yes   |
-
-
-  \* Files persist across app restarts and upgrades, but this directory can
-     be cleared whenever the OS desires. Your app should be able to recreate any
-     content that might be deleted.
+| :--------------------------------------------- | :-------------------------- | :-------------------- | :--: | :---------: | :-------: | :--: | :-----: |
+| `/var/mobile/Applications/<UUID>/`             | applicationStorageDirectory | -                     |  r   |     N/A     |    N/A    | N/A  |   Yes   |
+| &nbsp;&nbsp;&nbsp;`appname.app/`               | applicationDirectory        | bundle                |  r   |     N/A     |    N/A    | N/A  |   Yes   |
+| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`www/`     | -                           | -                     |  r   |     N/A     |    N/A    | N/A  |   Yes   |
+| &nbsp;&nbsp;&nbsp;`Documents/`                 | documentsDirectory          | documents             | r/w  |     Yes     |    No     | Yes  |   Yes   |
+| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`NoCloud/` | -                           | documents-nosync      | r/w  |     Yes     |    No     |  No  |   Yes   |
+| &nbsp;&nbsp;&nbsp;`Library`                    | -                           | library               | r/w  |     Yes     |    No     | Yes? |   Yes   |
+| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`NoCloud/` | dataDirectory               | library-nosync        | r/w  |     Yes     |    No     |  No  |   Yes   |
+| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`Cloud/`   | syncedDataDirectory         | -                     | r/w  |     Yes     |    No     | Yes  |   Yes   |
+| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`Caches/`  | cacheDirectory              | cache                 | r/w  |    Yes\*    | Yes\*\*\* |  No  |   Yes   |
+| &nbsp;&nbsp;&nbsp;`tmp/`                       | tempDirectory               | -                     | r/w  |   No\*\*    | Yes\*\*\* |  No  |   Yes   |
+
+\* Files persist across app restarts and upgrades, but this directory can
+be cleared whenever the OS desires. Your app should be able to recreate any
+content that might be deleted.
 
 \*\* Files may persist across app restarts, but do not rely on this behavior. Files
-     are not guaranteed to persist across updates. Your app should remove files from
-     this directory when it is applicable, as the OS does not guarantee when (or even
-     if) these files are removed.
+are not guaranteed to persist across updates. Your app should remove files from
+this directory when it is applicable, as the OS does not guarantee when (or even
+if) these files are removed.
 
 \*\*\* The OS may clear the contents of this directory whenever it feels it is
-     necessary, but do not rely on this. You should clear this directory as
-     appropriate for your application.
+necessary, but do not rely on this. You should clear this directory as
+appropriate for your application.
 
 ### Android File System Layout
 
-| Device Path                                     | `cordova.file.*`            | `AndroidExtraFileSystems` | r/w? | persistent? | OS clears | private |
-|:------------------------------------------------|:----------------------------|:--------------------------|:----:|:-----------:|:---------:|:-------:|
-| `file:///android_asset/`                        | applicationDirectory        | assets                    | r    |     N/A     |     N/A   |   Yes   |
-| `/data/data/<app-id>/`                          | applicationStorageDirectory | -                         | r/w  |     N/A     |     N/A   |   Yes   |
-| &nbsp;&nbsp;&nbsp;`cache`                       | cacheDirectory              | cache                     | r/w  |     Yes     |     Yes\* |   Yes   |
-| &nbsp;&nbsp;&nbsp;`files`                       | dataDirectory               | files                     | r/w  |     Yes     |     No    |   Yes   |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`Documents` |                             | documents                 | r/w  |     Yes     |     No    |   Yes   |
-| `<sdcard>/`                                     | externalRootDirectory       | sdcard                    | r/w\*\*\*  |     Yes     |     No    |   No    |
-| &nbsp;&nbsp;&nbsp;`Android/data/<app-id>/`      | externalApplicationStorageDirectory | -                 | r/w  |     Yes     |     No    |   No    |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`cache`     | externalCacheDirectory       | cache-external            | r/w  |     Yes     |     No\*\*|   No    |
-| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`files`     | externalDataDirectory       | files-external            | r/w  |     Yes     |     No    |   No    |
+| Device Path                                     | `cordova.file.*`                    | `AndroidExtraFileSystems` |   r/w?    | persistent? | OS clears | private |
+| :---------------------------------------------- | :---------------------------------- | :------------------------ | :-------: | :---------: | :-------: | :-----: |
+| `file:///android_asset/`                        | applicationDirectory                | assets                    |     r     |     N/A     |    N/A    |   Yes   |
+| `/data/data/<app-id>/`                          | applicationStorageDirectory         | -                         |    r/w    |     N/A     |    N/A    |   Yes   |
+| &nbsp;&nbsp;&nbsp;`cache`                       | cacheDirectory                      | cache                     |    r/w    |     Yes     |   Yes\*   |   Yes   |
+| &nbsp;&nbsp;&nbsp;`files`                       | dataDirectory                       | files                     |    r/w    |     Yes     |    No     |   Yes   |
+| &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;`Documents` |                                     | documents                 |    r/w    |     Yes     |    No     |   Yes   |
+| `<sdcard>/`                                     | externalRootDirectory               | sdcard                    | r/w\*\*\* |     Yes     |    No     |   No    |
+| `<sdcard>/`                                     | downloadDirectory                   | sdcard                    | r/w\*\*\* |     Yes     |    No     |   No    |

Review Comment:
   ```suggestion
   | `<sdcard>/Downloads`                                     | downloadDirectory                   | sdcard                    | r/w |     Yes     |    No     |   No    |
   ```
   
   I think the download directory is generally located at `<sdcard>/Download` but I think this should be confirmed. Additionally, the triple star indicator is for a note on API 30 changes where the external root directory is no longer a writable path, which I don't think applies to the Downloads folder.
   
   Suggestion is to show changes, but it will likely screw up the textual formatting of the table.



##########
src/android/FileUtils.java:
##########
@@ -1025,6 +1025,7 @@ private JSONObject requestAllPaths() throws JSONException {
             ret.put("externalDataDirectory", toDirUrl(context.getExternalFilesDir(null)));
             ret.put("externalCacheDirectory", toDirUrl(context.getExternalCacheDir()));
             ret.put("externalRootDirectory", toDirUrl(Environment.getExternalStorageDirectory()));
+            ret.put("downloadDirectory", toDirUrl(Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS)));

Review Comment:
   [getExternalStoragePublicDirectory](https://developer.android.com/reference/android/os/Environment#getExternalStoragePublicDirectory(java.lang.String)) is deprecated as of API 29.
   
   It appears [Context.getExternalFilesDir](https://developer.android.com/reference/android/content/Context#getExternalFilesDir(java.lang.String)) is the replacement. I believe the `Environment.DIRECTORY_DOWNLOADS` can be still used with `getExternalFilesDir` but it would need to be confirmed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org