Skip to content

Make external storage permission optional#9873

Merged
tobiasKaminsky merged 6 commits into
masterfrom
optional-files-permission
Mar 15, 2022
Merged

Make external storage permission optional#9873
tobiasKaminsky merged 6 commits into
masterfrom
optional-files-permission

Conversation

@AlvaroBrey
Copy link
Copy Markdown
Member

@AlvaroBrey AlvaroBrey commented Feb 22, 2022

Fixes #9343
Fixes #9845

  • Don't force permissions on app launch
  • Force permissions for upload activity
  • Force permissions for synced folders activity
  • Allow choosing between All Files and just READ_EXTERNAL_STORAGE in SDK 30+
    • Allow disabling All Files completely (through setup.xml? Just remove the permission from the manifest, code detects that)
    • Handle cases where there's no way to obtain All Files (No system activity for All Files permission #9845 ). Probably just assume that All Files is not available...
    • Properly designed choice dialog
  • Tests written, or not not needed

@AlvaroBrey AlvaroBrey force-pushed the optional-files-permission branch 3 times, most recently from 02d05da to 6ab94eb Compare February 23, 2022 17:09
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 23, 2022

Codecov Report

Merging #9873 (85433c5) into master (55c9811) will increase coverage by 0.66%.
The diff coverage is 6.71%.

❗ Current head 85433c5 differs from pull request most recent head 1a9014e. Consider uploading reports for the commit 1a9014e to get more accurate results

@@            Coverage Diff             @@
##           master    #9873      +/-   ##
==========================================
+ Coverage   30.50%   31.16%   +0.66%     
==========================================
  Files         513      514       +1     
  Lines       38890    38919      +29     
  Branches     5473     5477       +4     
==========================================
+ Hits        11864    12131     +267     
+ Misses      25234    24953     -281     
- Partials     1792     1835      +43     
Impacted Files Coverage Δ
...m/nextcloud/client/preferences/AppPreferences.java 0.00% <ø> (ø)
...xtcloud/client/preferences/AppPreferencesImpl.java 57.89% <0.00%> (ø)
.../com/owncloud/android/datamodel/MediaProvider.java 25.53% <0.00%> (ø)
...cloud/android/ui/activity/SyncedFoldersActivity.kt 18.05% <0.00%> (ø)
...droid/ui/dialog/StoragePermissionDialogFragment.kt 0.00% <0.00%> (ø)
.../java/com/owncloud/android/utils/PermissionUtil.kt 9.09% <5.76%> (ø)
...cloud/android/ui/activity/UploadFilesActivity.java 31.22% <26.31%> (ø)
...cloud/android/ui/activity/FileDisplayActivity.java 24.82% <33.33%> (ø)
.../java/com/nextcloud/client/mixins/MixinRegistry.kt
...m/nextcloud/client/migrations/MigrationsManager.kt
... and 1024 more

@AlvaroBrey AlvaroBrey force-pushed the optional-files-permission branch 4 times, most recently from aff25bc to cbe3afb Compare March 2, 2022 11:59
@AlvaroBrey
Copy link
Copy Markdown
Member Author

AlvaroBrey commented Mar 2, 2022

@nextcloud/designers I'd like to request feedback on the dialog design. If you have a better idea of how to present these choices, please let me know.

#9343 for context

This is the dialog shown at app startup, at which point the permission is optional. The text will change in cases where permissions are absolutely required (file upload, autosync setup) to reflect that ("Permissions needed" or something like that).
Of course the texts are also up for suggestions.

Click to view screenshot

screen

Extra food for thought: we may want to skip this optional, first-time permission request; after all it will be requested when actually needed. This makes app usage a bit more seamless first time, BUT autoupload folder detection won't work unless users grant the permissions first.

@AlvaroBrey AlvaroBrey force-pushed the optional-files-permission branch 3 times, most recently from d67b1ff to 06370da Compare March 3, 2022 11:07
@AlvaroBrey AlvaroBrey marked this pull request as ready for review March 3, 2022 12:44
Comment thread src/main/java/com/owncloud/android/ui/activity/SyncedFoldersActivity.java Outdated
Comment thread src/main/java/com/owncloud/android/ui/activity/UploadFilesActivity.java Outdated
Comment thread src/main/java/com/owncloud/android/utils/PermissionUtil.kt Outdated
@AlvaroBrey AlvaroBrey force-pushed the optional-files-permission branch 2 times, most recently from c83283c to c08930b Compare March 9, 2022 07:56
@jancborchardt
Copy link
Copy Markdown
Member

@AlvaroBrey text looks good, just 2 questions:

  • It says "permissions to access external storage" – is it enough to just say "permissions to access storage", or is that permission there automatically?
  • The dialog has no border radius, as per guidelines there’s a bit of border-radius to soften it (would also look better with our general design): https://material.io/components/dialogs#usage

@AlvaroBrey
Copy link
Copy Markdown
Member Author

AlvaroBrey commented Mar 10, 2022

* It says "permissions to access **external** storage" – is it enough to just say "permissions to access storage", or is that permission there automatically?
* The dialog has no border radius, as per guidelines there’s a bit of border-radius to soften it (would also look better with our general design): https://material.io/components/dialogs#usage

Done in 979a939

@AlvaroBrey AlvaroBrey force-pushed the optional-files-permission branch 3 times, most recently from 85433c5 to 5175ebc Compare March 11, 2022 12:25
@github-actions
Copy link
Copy Markdown

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/9873.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@AlvaroBrey AlvaroBrey linked an issue Mar 14, 2022 that may be closed by this pull request
Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
- Allow users to choose between MANAGE_EXTERNAL_STORAGE (full access) and READ_EXTERNAL_STORAGE (media read-only) in sdk >=30, with a dialog
- If All Files is not available (activity to manage not present, or permission not in manifest), request READ_EXTERNAL_STORAGE instead
- Misc improvements to permission request in UploadFilesActivity and SyncedFoldersActivity

Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
(ensure only one instance of it is ever shown at the same time)

Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
This makes it so that the folder loads even without granting storage permission, which is the expected behaviour now

Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
@AlvaroBrey AlvaroBrey force-pushed the optional-files-permission branch from 5175ebc to 1a9014e Compare March 14, 2022 16:40
@AlvaroBrey
Copy link
Copy Markdown
Member Author

@tobiasKaminsky ready for re-review

@nextcloud-android-bot
Copy link
Copy Markdown
Collaborator

Codacy

Lint

TypemasterPR
Warnings9393
Errors00

SpotBugs (new)

Warning Type Number
Bad practice Warnings 28
Correctness Warnings 78
Experimental Warnings 1
Internationalization Warnings 9
Malicious code vulnerability Warnings 57
Multithreaded correctness Warnings 9
Performance Warnings 67
Security Warnings 29
Dodgy code Warnings 350
Total 628

SpotBugs (master)

Warning Type Number
Bad practice Warnings 28
Correctness Warnings 78
Experimental Warnings 1
Internationalization Warnings 9
Malicious code vulnerability Warnings 57
Multithreaded correctness Warnings 9
Performance Warnings 67
Security Warnings 29
Dodgy code Warnings 350
Total 628

@github-actions
Copy link
Copy Markdown

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/9873.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@tobiasKaminsky tobiasKaminsky merged commit 9152097 into master Mar 15, 2022
@delete-merged-branch delete-merged-branch Bot deleted the optional-files-permission branch March 15, 2022 15:50
@joshtrichards joshtrichards added the hotspot: device storage Storage (on-device) related. Permissions, paths, inconsistencies, etc. label Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review hotspot: device storage Storage (on-device) related. Permissions, paths, inconsistencies, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error setting auto upload No system activity for All Files permission Allow app to function partially without All Files access on Android 10+

5 participants