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

Replace the ownCloud file picker with the Android one #2899

Closed
24 tasks done
abelgardep opened this issue Jun 11, 2020 · 11 comments · Fixed by #3293
Closed
24 tasks done

Replace the ownCloud file picker with the Android one #2899

abelgardep opened this issue Jun 11, 2020 · 11 comments · Fixed by #3293

Comments

@abelgardep
Copy link
Contributor

abelgardep commented Jun 11, 2020

As a continuation of #2436, where we replaced the ownCloud file picker with the Android one when uploading files, oC app should replace completely the file picker with the android one.

This oC file picker is still used to select the camera folder available for Camera Uploads in Preferences. Steps:
1- Open settings
2- Enable picture uploads
3- Choose camera folder

oC Custom file picker Storage Access Framework
custom-file-picker saf

With this replacement, we will perform a big cleanup and it will be ready for Scoped Storage.

TASKS

  • Research (if needed)
  • Create branch new_arch/camera_uploads
  • Development tasks
    • Use SAF file picker
    • decide weather to use SAF for accessing photos
    • if yes: move accessing local storage to SAF.
    • Create a new model in domain module for camera uploads config
    • Add a new worker to perform camera uploads
    • Add a new usecase to retrieve camera uploads configuration
    • Add a new worker to perform single uploads with a content uri
    • Add a constraint to perform uploads only when wifi is enabled
    • Remove local file if behavior for camera uploads is move
    • Show the uploads in the uploads list view
    • Cancel uploads when removing the account
    • Cancel uploads individually
    • Notifications: Syncing camera uploads and uploading file individually (Check if needed)
    • Improve error handling: Add new errors cases
    • Check problems when upgrading: Previous uri is not valid
    • Clean legacy code
    • Fix unit tests
  • Code review and apply changes requested
  • Design test plan
  • QA
  • Merge branch new_arch/camera_uploads into master

PR

QA

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

Bugs / improvements

@jesmrec jesmrec mentioned this issue Jun 11, 2020
5 tasks
@jesmrec jesmrec added this to the 2.16-current milestone Jun 12, 2020
@jesmrec
Copy link
Collaborator

jesmrec commented Jun 12, 2020

Add another case: share with ownCloud

Case 1:

  1. Open "Photos".
  2. Select one or more pictures.
  3. Select "Share with" -> ownCloud

Case 2:

  1. Open browser, any web
  2. Select some text
  3. Select "Share" -> ownCloud

@abelgardep
Copy link
Contributor Author

Add another case: share with ownCloud

Case 1:

1. Open "Photos".

2. Select one or more pictures.

3. Select "Share with"  -> ownCloud

Case 2:

1. Open browser, any web

2. Select some text

3. Select "Share" -> ownCloud

That's not the same case. Those cases uses the oC file picker to select where you want to store those files, but in the oC hierarchy. This issue is to select which folder you want to allow autouploads and uses system hierarchy, so we can use android one.

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 12, 2020

ok you meant the folder from the camera app (external from oC). I mistaked it with the camera upload folder

@jesmrec jesmrec modified the milestones: 2.16-current, backlog Jun 19, 2020
@jesmrec jesmrec modified the milestones: backlog, 2.18-next Feb 24, 2021
@theScrabi
Copy link
Contributor

theScrabi commented Feb 24, 2021

It is possible to use this file picker however it will return SAF URIs, which means we should then move the storage operations over to SAF as well. Another more ugly solution would be to translate SAF URIs to native Unix file paths.
This code part may need to be changed as well:

private void syncFiles() {
//Get local images and videos
String localCameraPath = mCameraUploadsSourcePath;
File[] localFiles = new File[0];
if (localCameraPath != null) {
File cameraFolder = new File(localCameraPath);
localFiles = cameraFolder.listFiles();
}
if (localFiles != null) {
localFiles = orderFilesByCreationTimestamp(localFiles);
for (File localFile : localFiles) {
handleFile(localFile);
}
}
Timber.d("All files synced, finishing job");
}

A related issue to this: #2877

@theScrabi
Copy link
Contributor

theScrabi commented Feb 25, 2021

It was proposed to make this work the same way as we made single file upload work. What we do there is copy the file from a ContentProvider to a tmp directory. This file in tmp is then uploaded:
in some way we are already using ContentResolvers here. Which is the right way to go:
https://github.com/owncloud/android/blob/e1c7b7028d0e6adedc771f420aef0fbf9c75af58/owncloudApp/src/main/java/com/owncloud/android/ui/asynctasks/CopyAndUploadContentUrisTask.java#L145-146
If we do it the same way for image/video upload however this would required to copy every image/video once before uploading. I'm concerned about the performance. I'd propose moving the dialog once we move to Scoped storrage.

@jesmrec jesmrec modified the milestones: 2.18-current, 2.19-next May 6, 2021
@abelgardep
Copy link
Contributor Author

We'll need to replace the ownCloud file picker with the SAF one in order to make the camera uploads work with ScopedStorage

@abelgardep abelgardep self-assigned this Jun 8, 2021
@jesmrec jesmrec mentioned this issue Jun 17, 2021
24 tasks
@jesmrec jesmrec modified the milestones: 2.19-current, 2.18.1 Jun 17, 2021
@abelgardep
Copy link
Contributor Author

abelgardep commented Jun 28, 2021

The old camera uploads job has been replaced with a new worker
The old ownCloud picker has been replaced with the native picker to select the camera folder.

A notification with how many pictures and videos will be synced is shown now.
If there are any errors with the source path, a notification is shown now.

Updated several files to kotlin and new architecture.
Camera uploads configuration moved to room instead of preferences.

Pros:

  • It will work before and after moving to Scoped Storage
  • It is easier to add constraints like: trigger only when charging, trigger only when the battery is not low, etc

Cons:

  • Users will need to select the camera source path again
  • Uploads triggered by camera uploads won't show progress (at least for the moment), but it should not be a big problem since they normally will be uploaded in the background.

To discuss:

  • Camera uploads repeat intervals. At the moment, it is triggered every 15 minutes. Maybe it is not needed to trigger them as often
  • Camera uploads conditions: We can add some conditions like: only when charging, only when the battery is not low with no much effort if needed

Differences

Flow

Previous flow:

  1. The external file was copied to a temporal file inside oC folder
  2. Temporal file was uploaded to the server
  3. Temporal file was moved to the final location inside oC folder
  4. If the behavior was 'MOVE', the external file was deleted

New flow:

  1. The external file is uploaded to the server
  2. If the behavior is 'MOVE', the external file is deleted

With this change, we won't use temporal files, and we won't store the file on the oC folder.
PRO: Save space in storage
CON: If the user wants to see the image, we need to download it.

Disabling camera uploads

Previous behavior when disabling camera uploads:

  • Reset only important fields: behavior, wifi-only etc didn't reset

New behavior when disabling camera uploads:

  • Reset full fields

PRO: More consistent way. If you disable the feature, it is completely reset.
CON: If the user enables it again, he needs to configure it again.

Connection

Previous behavior when the device is not connected:

  • Upload fails, notification is shown

New behavior when the device is not connected:

  • Upload enqueued, will be uploaded when the device is connected

PRO: It won't try to upload the files till the device is connected.
CON: Not sure.

Known issues

  • After replacement, users will need to update the source path. It means that the timestamp of the last scan will be updated and photos that were taken between upgrading to 2.18.1 and the new source path configuration won't be uploaded.

@jesmrec @michaelstingl

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 28, 2021

To discuss:

  • Camera uploads repeat intervals. At the moment, it is triggered every 15 minutes. Maybe it is not needed to trigger them as often

for me it's ok for the moment to keep 15 mins. Why should we set a longer scan frequency?

  • Camera uploads conditions: We can add some conditions like: only when charging, only when the battery is not low with no much effort if needed

interesting new features... for the future.

  • At the moment, camera uploads last sync timestamps are stored in the old database, and configuration(source path, behavior, account, etc) is stored in preferences. It could be an option to move both of them to a common place: new database. It would be easier to maintain, and we could take advantage of the needed reset in the next release.

if that makes the code easier to mantain, no problem from my side. But the initial reset would be only the camera path. With this improvement, will we need a complete reset of the feature (like the 2.18 one)?

@abelgardep
Copy link
Contributor Author

for me it's ok for the moment to keep 15 mins. Why should we set a longer scan frequency?

Well, I think that maybe it is not needed to trigger camera uploads almost 100 times per day, but it is fine for me to keep the previous behavior.

if that makes the code easier to maintain, no problem from my side. But the initial reset would be only the camera path. With this improvement, will we need a complete reset of the feature (like the 2.18 one)?

Yes, I think that camera path reset will be enough.

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 29, 2021

Well, I think that maybe it is not needed to trigger camera uploads almost 100 times per day, but it is fine for me to keep the previous behavior.

Could this parameter be setup-able, or brandable? that means... giving users the choice to select the scan frequency (with the minimum of 15 mins, for sure)

@abelgardep
Copy link
Contributor Author

Could this parameter be setup-able, or brandable? that means... giving users the choice to select the scan frequency (with the minimum of 15 mins, for sure)

Yes, it could be brandable, or customizable. Probably in another issue 👍

@abelgardep abelgardep linked a pull request Jul 2, 2021 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants