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

request media_location #14247

Merged
merged 11 commits into from
Jan 14, 2025
Merged

request media_location #14247

merged 11 commits into from
Jan 14, 2025

Conversation

tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Dec 27, 2024

Part of #14135 (comment)

  • Tests written, or not not needed

@BluePixel4k
Copy link

#14135 (comment) - Quote with my answer/question:

Thus I created a draft PR, with an APK, that you can install in parallel: #14247 (comment)
@BluePixel4k can you test this?

Thanks a lot! Yes, it's working fine for me with this version 👍 But is it possible to enable the ACCESS_MEDIA_LOCATION already before the user grants the image and video permission? Because if a user doesn't grant the permission but shares a picture via his local gallery app to the Nextcloud app, the GPS information is missing.

Is this possible?

@hw-hass23
Copy link

#14135 (comment) - Quote with my answer/question:

Thus I created a draft PR, with an APK, that you can install in parallel: #14247 (comment)
@BluePixel4k can you test this?

Thanks a lot! Yes, it's working fine for me with this version 👍 But is it possible to enable the ACCESS_MEDIA_LOCATION already before the user grants the image and video permission? Because if a user doesn't grant the permission but shares a picture via his local gallery app to the Nextcloud app, the GPS information is missing.

Is this possible?

Some gallery apps themselves have settings for "safe sharing" which zeros the GPS information upon sharing. Xiaomi Poco gallery is one of them (needed to disable "share without location" for proper sharing to Nextcloud app), whereas Google Foto left GPS intact.

@tobiasKaminsky
Copy link
Member Author

/backport to stable-3.30

@tobiasKaminsky
Copy link
Member Author

If no custom upload folder is found:
2025-01-09-091542

If custom upload folder is found:
2025-01-09-091306

@Whisprin
Copy link

Whisprin commented Jan 9, 2025

thanks @tobiasKaminsky
Do you intend to send a notification to users after the upgrade? In order to satisfy #14349: Inform users who don't open the app regularly and rely on automatic background backups.

In case any (not just a custom) auto upload folder pair is found: Users should be informed explicitly that past Auto uploads didn't run since 3.30.6 (probably some time in mid December) and future Auto uploads won't run unless the new permission is granted.
It can also be made more explicit that Auto uploads of files other than photos and videos remain broken, even after granting the new permissions.
I'll post a revised draft of this message here later.

Is there way or a plan to ensure that those missed Auto uploads are carried out after the Photo and video permission is granted?
In my tests those past, missed uploads are not picked up automatically after the permission is granted.

@Whisprin
Copy link

Whisprin commented Jan 9, 2025

How about:

Re-enable Disrupted Auto Uploads

Please check manually for missing file uploads since mid-December.
To re-enable Auto Uploads for new photos and videos:

  • Grant "Photos and Videos" permission in the next dialog,
  • Grant "Location" permission to keep location data in uploaded photos.

To re-enable Auto Uploads for all other files:

  • Auto Uploads of other files are not possible anymore with the Play Store version of the Nextcloud app.

Auto Uploads stopped working because Google rejected the necessary file permission in a previous app update. Nextcloud is working with Google to rectify the situation and restore full functionality.

@alperozturk96
Copy link
Collaborator

alperozturk96 commented Jan 13, 2025

@tobiasKaminsky I am unable to view the storage and notification permissions. Only the Google Play warnings are appearing. Also, Codacy Static Code Analysis fails.

viewThemeUtils.androidx.themeNotificationCompatBuilder(context, notificationBuilder)

val notificationManager = context.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
notificationManager.notify(notificationId, notificationBuilder.build())

Check failure

Code scanning / CodeQL

Use of implicit PendingIntents High

An implicit Intent is created
and sent to an unspecified third party through a PendingIntent.
This autofix suggestion was applied.
Show autofix suggestion Hide autofix suggestion

Copilot Autofix AI 6 days ago

To fix the problem, we need to ensure that the Intent used to create the PendingIntent is explicit. This can be done by setting the component name explicitly in the Intent. Additionally, we should verify that all other PendingIntent creations in the code follow the same practice.

  • Modify the Intent on line 210 to explicitly set the component name.
  • Ensure that the PendingIntent is created with the FLAG_IMMUTABLE flag to prevent modification by other applications.
Suggested changeset 1
app/src/main/java/com/nextcloud/client/jobs/MediaFoldersDetectionWork.kt

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/src/main/java/com/nextcloud/client/jobs/MediaFoldersDetectionWork.kt b/app/src/main/java/com/nextcloud/client/jobs/MediaFoldersDetectionWork.kt
--- a/app/src/main/java/com/nextcloud/client/jobs/MediaFoldersDetectionWork.kt
+++ b/app/src/main/java/com/nextcloud/client/jobs/MediaFoldersDetectionWork.kt
@@ -209,3 +209,4 @@
         val notificationId = randomIdGenerator.nextInt()
-        val intent = Intent(context, FileDisplayActivity::class.java).apply {
+        val intent = Intent().apply {
+            setClassName(context, FileDisplayActivity::class.java.name)
             setAction(FileDisplayActivity.AUTO_UPLOAD_NOTIFICATION)
EOF
@@ -209,3 +209,4 @@
val notificationId = randomIdGenerator.nextInt()
val intent = Intent(context, FileDisplayActivity::class.java).apply {
val intent = Intent().apply {
setClassName(context, FileDisplayActivity::class.java.name)
setAction(FileDisplayActivity.AUTO_UPLOAD_NOTIFICATION)
Copilot is powered by AI and may make mistakes. Always verify output.
@tobiasKaminsky tobiasKaminsky committed this autofix suggestion 6 days ago.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@alperozturk96 alperozturk96 force-pushed the media_permission branch 2 times, most recently from 2957d11 to 15e5511 Compare January 14, 2025 07:35
@alperozturk96
Copy link
Collaborator

alperozturk96 commented Jan 14, 2025

@tobiasKaminsky How can you suppress this or maybe reduce the condition count?

Screenshot 2025-01-14 at 08 48 54

tobiasKaminsky and others added 11 commits January 14, 2025 09:20
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Signed-off-by: Tobias Kaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
…ationPermission when user pressed the close button

Signed-off-by: alperozturk <[email protected]>
Co-authored-by: Tom <[email protected]>
Signed-off-by: Tobias Kaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: tobiasKaminsky <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Copy link

Codacy

Lint

TypemasterPR
Warnings5655
Errors33

SpotBugs

CategoryBaseNew
Bad practice6565
Correctness5858
Dodgy code296294
Experimental11
Internationalization77
Malicious code vulnerability11
Multithreaded correctness77
Performance5353
Security1818
Total506504

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14247.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 0abff31 into master Jan 14, 2025
17 of 20 checks passed
@tobiasKaminsky tobiasKaminsky deleted the media_permission branch January 14, 2025 08:46
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.

7 participants