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

[New arch] Camera uploads #3293

Merged
merged 56 commits into from
Jul 14, 2021
Merged

[New arch] Camera uploads #3293

merged 56 commits into from
Jul 14, 2021

Conversation

abelgardep
Copy link
Contributor

@abelgardep abelgardep commented Jul 2, 2021

Related Issues

Implements #2899

Library PR (if needed): owncloud/android-library#414

Screenshots

Timestamp field added Native picker to select camera folder
new_field camera_uploads_picker
Camera upload succeeded notification Camera upload failed notification
notification_success notification_fail
  • Added changelog files for the fixed issues in folder changelog/unreleased. More info here

QA

Test plan: https://github.com/owncloud/QA/blob/master/Mobile/Android/Release_2.18.1/Camera%20uploads%20%2B%20picker.md

@abelgardep abelgardep force-pushed the new_arch/camera_uploads branch 2 times, most recently from b1596fb to eea4d51 Compare July 6, 2021 12:00
Copy link
Contributor

@jabarros jabarros left a comment

Choose a reason for hiding this comment

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

LGTM

@abelgardep abelgardep force-pushed the new_arch/camera_uploads branch from eea4d51 to 5bea766 Compare July 6, 2021 12:09
@jesmrec
Copy link
Collaborator

jesmrec commented Jul 6, 2021

(1) [FIXED]

  1. Install from scratch
  2. Enable instant uploads

Current: notification with Picture uploads source path is not valid anymore. By clicking on the notification, user is redirected to the Uploads view

Expected: Notification should point the user to the proper section in Settings. Also (as another idea), i'd add it in the notification message with something like: Click to set up correctly

Pixel 2
Android 11
5bea766f

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 6, 2021

(2) [FIXED]

Every time the camera folder is scanned, a new notification arises with the number of uploads to be done (replacement of the progress bar).

But, notification is also shown when there is no uploads to do. Would it be posible not to show it when the number is 0? if no pictures/videos were taken nothing new has happened to notify.

Pixel 2
Android 11
5bea766f

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 6, 2021

(3) [WONT DO]

This is more a question than report

The camera folder is shown as /tree/primary:DCIM/xxxxx

is this string handleable? in case it is, i'd get rid of the first chunk (/tree/primary) and let the "real" path for the user (DCIM/...).

@abelgardep
Copy link
Contributor Author

(1)

Expected: Notification should point the user to the proper section in Settings. Also (as another idea), I'd add it in the notification message with something like: Click to set up correctly

I agree that the notification should point to the proper section in 'Settings'. Related to the message, you mean to add that text to the current one? In that case, yes, we could add it, but I would not replace the current message.

(2)

But, notification is also shown when there are no uploads to do. Would it be possible not to show it when the number is 0? if no pictures/videos were taken nothing new has happened to notify.

Of course!! We can remove the notification when nothing new is found. Up to you, I think that previously the "Syncing camera uploads" were shown independently of the files found. That's why I kept it this way.

(3)

is this string handleable? in case it is, I'd get rid of the first chunk (/tree/primary) and let the "real" path for the user (DCIM/...).

Yes, it is completely handleable. Removing the/tree/primary path could be confusing for SD card paths. But, it is completely fine to modify the String.

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 6, 2021

I agree that the notification should point to the proper section in 'Settings'. Related to the message, you mean to add that text to the current one? In that case, yes, we could add it, but I would not replace the current message.

yes, adding the message to the current one. So, users will know that they have to do something, apart of being notified.

Of course!! We can remove the notification when nothing new is found. Up to you, I think that previously the "Syncing camera uploads" were shown independently of the files found. That's why I kept it this way.

probably, in testing stage i enter/leave Settings many times, so, i get a bunch of notifications with 0 uploads, that could be annoying. In real cases won't happen this... but i'd prefer to use notifications for cases in which new uploads are incoming.

Yes, it is completely handleable. Removing the/tree/primary path could be confusing for SD card paths. But, it is completely fine to modify the String.

I did not check with SD card yet... let me check before making a final decision about (3)

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 7, 2021

(4) [FIXED]

I am experiencing repetitions in uploads in different devices. That means:

  1. Take 2 pictures
  2. Go to Settings and go back (the way to force triggering uploads)
  3. After a couple of minutes, every pic is uploaded twice

I can see in the logs:

2021-07-07 09:33:01.143 8957-11393/com.owncloud.android.debug V/(UploadsStorageManager.java:102) .storeUpload(): Inserting content://com.android.externalstorage.documents/tree/primary%3ADCIM%2FCamera/document/primary%3ADCIM%2FCamera%2FPXL_20210707_073145894.jpg with status=UPLOAD_IN_PROGRESS
...
2021-07-07 09:33:02.633 8957-11394/com.owncloud.android.debug V/(UploadsStorageManager.java:138) .updateUpload(): Updating content://com.android.externalstorage.documents/tree/primary%3ADCIM%2FCamera/document/primary%3ADCIM%2FCamera%2FPXL_20210707_073145894.jpg with status=UPLOAD_SUCCEEDED

and some seconds later:

2021-07-07 09:33:20.282 8957-11851/com.owncloud.android.debug V/(UploadsStorageManager.java:102) .storeUpload(): Inserting content://com.android.externalstorage.documents/tree/primary%3ADCIM%2FCamera/document/primary%3ADCIM%2FCamera%2FPXL_20210707_073145894.jpg with status=UPLOAD_IN_PROGRESS
...
2021-07-07 09:33:21.432 8957-8994/com.owncloud.android.debug D/(UploadsStorageManager.java:155) .updateUpload(): updateCameraUploadSync returns with: 1 for file: content://com.android.externalstorage.documents/tree/primary%3ADCIM%2FCamera/document/primary%3ADCIM%2FCamera%2FPXL_20210707_073145894.jpg

same picture is uploaded again

could it be a matter of a non-updated last sync date?

EDIT: repetitions only happen with the app in foreground. If the app triggers uploads in backgrounds, no repetitions happen. Strange.

Pixel 2
Android 11
5bea766f

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 7, 2021

about (3), i'd keep the whole string, indicating whether SDcard or internal storage is used.

catching files from SD card seems to work fine.

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 7, 2021

(5) [FIXED]

  1. Install the app from scratch
  2. Enable Camera and Video uploads, both with same set up (same account, folder and camera folder)
  3. Wait till WorkManager passes at least once for photos and once for videos (here, the notification asked to be removed in (2) helps a lot to know when happens)
  4. Open Settings -> Video uploads
  5. Change Video upload path for a different location
  6. Wait again for WorkManager for the videos
  7. Open Settings -> Video uploads

Current: Location is the one selected in step 2
Expected: Location is the one selected in step 5

Pixel 2 Android 11
Nexus 6P Android 7
5bea766f

@abelgardep
Copy link
Contributor Author

I see that there are some problems that could be fixed with a little tweak.

At the moment, each time we leave the picture uploads section or video uploads section, we enqueue the work to perform the camera uploads replacing the current one. (More info about it)

This means that we cancel the current one and enqueue a new one, so it could lead to some errors when replacing the worker if the timestamp is not updated yet. The new worker could get the same timestamp as the previous one.

This behavior is pretty useful during development, so we don't need to wait till the next auto-trigger, at the moment 15 minutes. But it can lead to errors, so I will change to KEEP policy. I think that this tweak could fix (4) and (5)

probably, in testing stage I enter/leave Settings many times, so, I get a bunch of notifications with 0 uploads, that could be annoying. In real cases won't happen this... but I'd prefer to use notifications for cases in which new uploads are incoming.

With this change, the notification with 0 uploads won't be shown every time we enter settings sections. But we can hide them anyway.

@jesmrec

@abelgardep abelgardep force-pushed the new_arch/camera_uploads branch from 5bea766 to 6df01e3 Compare July 7, 2021 16:06
@abelgardep
Copy link
Contributor Author

(1) Ready to test. I didn't change the message since it is too long and it won't be readable on most devices.
(2) Done, No notification when there are no files to upload.
(4) and (5) According to this comment I have changed the policy from REPLACE to KEEP, so they should be ready to test again

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 8, 2021

(6) [FIXED]

  1. Enable camera uploads for pictures or videos and select correct camera folder
  2. In Original file will be, select move
  3. Take pictures and wait till they are uploaded

Current: Pictures are uploaded to oC, but the original file keeps in the device

Expected: Pictures are uploaded to oC and the original file is removed from device

Pixel 2 Android 11
090df6e

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 12, 2021

Everything passed. Nice new feature that will help a lot in the future!

Approved

…ra uploads are triggered via workmanager and it checks wifi connection automatically.
@abelgardep abelgardep force-pushed the new_arch/camera_uploads branch from 3413045 to 16c392f Compare July 14, 2021 06:35
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@abelgardep abelgardep merged commit 1d22cb2 into master Jul 14, 2021
@abelgardep abelgardep deleted the new_arch/camera_uploads branch July 14, 2021 07:04
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.

Replace the ownCloud file picker with the Android one
3 participants