Conversation
breautek
left a comment
breautek
left a comment
There was a problem hiding this comment.
Thank you for your PR. I left some remarks and wonder what your thoughts will be.
Several JS files had some formatting changes which is breaking our lint rules. I suggest reverting these formatting changes. You can run npm test which should run the linter locally to confirm that lint passes.
For the README file, you've made a lot of drastic changes to the formatting, but I do like these changes. However, should we need to revert this PR in the future for whatever reason, it will cause the revert to your README improvements. I'd suggest considering pulling out the reformat and readability improvements as a separate PR, and keep the README changes here focused on the documentation additions for the downloadsDirectory. I don't consider this a blocker though.
| | `files` | dataDirectory | files | r/w | Yes | No | Yes | | ||
| | `Documents` | | documents | r/w | Yes | No | Yes | | ||
| | `<sdcard>/` | externalRootDirectory | sdcard | r/w\*\*\* | Yes | No | No | | ||
| | `<sdcard>/` | downloadDirectory | sdcard | r/w\*\*\* | Yes | No | No | |
There was a problem hiding this comment.
| | `<sdcard>/` | downloadDirectory | sdcard | r/w\*\*\* | Yes | No | No | | |
| | `<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, so these changes might be better to be applied manually rather then accepting the suggestion commit as is.
| 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))); |
There was a problem hiding this comment.
getExternalStoragePublicDirectory is deprecated as of API 29.
It appears Context.getExternalFilesDir is the replacement. I believe the Environment.DIRECTORY_DOWNLOADS can be still used with getExternalFilesDir but it would need to be confirmed.
2 participants
Platforms affected
Android
Motivation and Context
From Android 10 onwards it was very difficult to download due to permissions. The downloads folder has permission to access.
Description
added download path
Testing
Checklist
(platform)if this change only applies to one platform (e.g.(android))