Skip to content
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

potential fix for #6248 #11161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

potential fix for #6248 #11161

wants to merge 1 commit into from

Conversation

tobiasKaminsky
Copy link
Member

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

  • Tests written, or not not needed

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #11161 (ecbea61) into master (0b3707c) will decrease coverage by 0.86%.
The diff coverage is n/a.

❗ Current head ecbea61 differs from pull request most recent head b8791e2. Consider uploading reports for the commit b8791e2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #11161      +/-   ##
============================================
- Coverage     31.58%   30.72%   -0.86%     
+ Complexity     3400     3264     -136     
============================================
  Files           566      564       -2     
  Lines         41601    41611      +10     
  Branches       5628     5640      +12     
============================================
- Hits          13138    12784     -354     
- Misses        26508    26895     +387     
+ Partials       1955     1932      -23     
Impacted Files Coverage Δ
...com/nextcloud/client/database/entity/FileEntity.kt 0.00% <0.00%> (-96.60%) ⬇️
.../com/owncloud/android/ui/dialog/LoadingDialog.java 0.00% <0.00%> (-95.24%) ⬇️
...owncloud/android/authentication/PassCodeManager.kt 33.89% <0.00%> (-38.17%) ⬇️
...main/java/com/owncloud/android/utils/FileUtil.java 60.00% <0.00%> (-26.67%) ⬇️
...owncloud/android/ui/activity/PassCodeActivity.java 0.51% <0.00%> (-25.65%) ⬇️
.../owncloud/android/datamodel/MediaFoldersModel.java 40.00% <0.00%> (-20.00%) ⬇️
...owncloud/android/ui/adapter/OCFileListAdapter.java 39.76% <0.00%> (-6.20%) ⬇️
...nextcloud/client/jobs/MediaFoldersDetectionWork.kt 22.87% <0.00%> (-5.23%) ⬇️
...ncloud/android/ui/fragment/OCFileListFragment.java 35.61% <0.00%> (-4.48%) ⬇️
...d/android/operations/SynchronizeFileOperation.java 20.21% <0.00%> (-4.26%) ⬇️
... and 34 more

Copy link
Member

@AlvaroBrey AlvaroBrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK this permission has to be requested at runtime, it's not automatically granted.

@tobiasKaminsky
Copy link
Member Author

AFAIK this permission has to be requested at runtime, it's not automatically granted.

Seems to work, according to users in #6248

But here it is said that it "requires explicit user consent in order to gain access to this information":
https://developer.android.com/training/data-storage/shared/media#media-location-permission

@github-actions
Copy link

@nextcloud-android-bot
Copy link
Collaborator

@joshtrichards
Copy link
Member

I'm wondering if the need for this is only arising as a side effect of permitting users to choose to permit All Files Access (MANAGE_EXTERNAL_STORAGE / ACTION_MANAGE_ALL_FILES_ACCESS_PERMISSION)... or not.

What if we just don't let them choose and make it "You must say yes" to this intent action when you install if you want Nextcloud to function properly.

Storage permissions are a PITA to sort out with all the different Android versions. 😢 Worse if we support different combinations of permissions by default.

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Copy link

Codacy

Lint

TypemasterPR
Warnings7474
Errors00

SpotBugs

CategoryBaseNew
Bad practice2626
Correctness7070
Dodgy code363363
Experimental22
Internationalization99
Malicious code vulnerability22
Multithreaded correctness99
Performance5858
Security1818
Total557557

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11161.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.

Copy link

blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants