Upload folder support #4859

kilves · 2019-11-19T21:51:08Z

#4505 develeper stopped updating his PR, so here's an alternate solution that should implement the requirements mentioned in old PR.

Fixes #184

codecov · 2019-11-20T17:40:07Z

Codecov Report

Merging #4859 into master will decrease coverage by 0.13%.
The diff coverage is 1.34%.

@@             Coverage Diff              @@
##             master    #4859      +/-   ##
============================================
- Coverage     17.71%   17.58%   -0.14%     
  Complexity        3        3              
============================================
  Files           384      383       -1     
  Lines         32650    32624      -26     
  Branches       4597     4611      +14     
============================================
- Hits           5785     5736      -49     
- Misses        25942    25974      +32     
+ Partials        923      914       -9

Impacted Files	Coverage Δ	Complexity Δ
...oud/android/ui/fragment/LocalFileListFragment.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...cloud/android/ui/activity/UploadFilesActivity.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...android/ui/asynctasks/CheckAvailableSpaceTask.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...cloud/android/ui/adapter/LocalFileListAdapter.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...cloud/android/ui/activity/FileDisplayActivity.java	`19.11% <2.17%> (-0.98%)`	`0 <0> (ø)`
...ava/com/nextcloud/client/account/RegisteredUser.kt	`60% <0%> (-40%)`	`0% <0%> (ø)`
...rc/main/java/com/nextcloud/java/util/Optional.java	`0% <0%> (-17.31%)`	`0% <0%> (ø)`
...oud/android/ui/activity/SyncedFoldersActivity.java	`23.39% <0%> (-2.93%)`	`0% <0%> (ø)`
...java/com/nextcloud/client/account/AnonymousUser.kt	`75% <0%> (-2.78%)`	`0% <0%> (ø)`
...xtcloud/client/account/UserAccountManagerImpl.java	`46.76% <0%> (-2.16%)`	`0% <0%> (ø)`
... and 43 more

nextcloud-android-bot · 2019-11-21T12:20:14Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11695

nextcloud-android-bot · 2019-11-21T21:50:19Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11701

kilves · 2019-11-22T18:39:58Z

IT test fail might be unrelated, the exact same error has happened before on a completely different PR: https://kaminsky.me/nc-dev/android-integrationTests/10775/index.html

AndyScherzinger · 2019-11-26T07:53:04Z

hi @kilves,

first of all welcome to the Nextcloud Android client project and thank for taking an interest in it and for picking up this task ❤️

Tests are actually a bit flaky, so no worries. Can you do a rebase against the latest master please. See #4859 (comment) which shows an increase in linbt warnings (breaking the build) which might be due to the not-up-to-date master branch of yours (or needs fixing, hard to tell right now). The rebase would also remove all changes not made within this PR itself 😃

cc @tobiasKaminsky for review

AndyScherzinger · 2019-11-26T07:53:17Z

Also cc @ezaquarii for review :)

ezaquarii

Sorry for dry tone, but I'm finishing my train journey and wanted to wrap it up fast, fast, fast. :)

Anyway, we have quite some things to improve.

Also, would be it be possible to extract all of this logic from Activity into a separate class and put under unit test? We're suffering from logic mothballing in UI components and that puts a lot of strain on us already so we try to improve the quality rather aggressively.

src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

src/main/java/com/owncloud/android/ui/activity/UploadFilesActivity.java

src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

src/main/java/com/owncloud/android/ui/activity/UploadFilesActivity.java

src/main/java/com/owncloud/android/ui/adapter/LocalFileListAdapter.java

src/main/java/com/owncloud/android/ui/asynctasks/CheckAvailableSpaceTask.java

Changes in upload menu: * User is now able to select files on different folders at the same time * Made local folders selectable (long-press to select) * When local folder is selected, all files and subfolders are also selected * When user presses upload, app replicates directory structure to upload target (from the point of view of currently open local folder, or common ancestor folder if impossible) * Before upload starts, app automatically creates necessary folders to remote Signed-off-by: Petri <petri.hetta@dicode.fi>

Signed-off-by: Petri <petri.hetta@dicode.fi>

nextcloud-android-bot · 2019-11-28T19:05:10Z

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

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

nextcloud-android-bot · 2019-11-28T19:14:24Z

Codacy

333

Lint

Type	master	PR
Warnings	59	59
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	72
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	105
Security Warnings	44
Dodgy code Warnings	138
Total	412

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	70
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	104
Security Warnings	44
Dodgy code Warnings	136
Total	407

SpotBugs increased!

kilves · 2019-11-28T19:16:38Z

Ok, so I did some refactoring and the code should be more readable now. I also moved the logic to separate classes. I'm not sure about unit tests in this case. The code is still largely dependent on the existence of a context, because you still need intents to create folders before uploading files. So it would really be an integration test. I can write some though if they are really necessary. I'm also open to suggestions about FilesUploader class if there's not enough decoupling for example.

ezaquarii · 2019-11-28T20:05:48Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+
+        requester.uploadNewFile(
+            fileActivity,
+            account,


Let's not create Account variables.

We're running a continuous refactoring effort where all calls to toPlatformAccount() are being chased. Because we have tons of it, it's much easier to browse the analysis report if it's called in point of use rather than calls being coalesced via temporary variables, as IDE can tell us how many items we have in each class.

For the record, toPlaformAccount() is being retired as we speak/write, it just takes time.

requester.uploadNewFile(..., user.toPlaformAccount(), ...) // makes our refactoring easier

ezaquarii · 2019-11-28T20:11:37Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+
+public class FilesUploader {
+
+    private FileActivity fileActivity;


I don't see any Activity APIs being used, so we shall not depend on it.

ezaquarii · 2019-11-28T20:14:41Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+        intent.putExtra(OperationsService.EXTRA_ACCOUNT, user.toPlatformAccount());
+        intent.putExtra(OperationsService.EXTRA_REMOTE_PATH, folder);
+        intent.putExtra(OperationsService.EXTRA_CREATE_FULL_PATH, true);
+        fileActivity.getOperationsServiceBinder().queueNewOperation(intent);


Few issues here:

we inject FileActivity only to call a getter.

2 statements per line

We shall inject the OperationsServiceBinder instead.

ezaquarii · 2019-11-28T20:15:52Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+        String[] remotePaths = getRemoteTargetPaths(remoteTargets).toArray(new String[]{});
+
+        requester.uploadNewFile(
+            fileActivity,


Context is needed here.

ezaquarii · 2019-11-28T20:17:38Z

src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

@@ -286,6 +288,7 @@ protected void onCreate(Bundle savedInstanceState) {
        }

        mPlayerConnection = new PlayerServiceConnection(this);
+        filesUploader = new FilesUploader(this);


Let's constructor-inject all dependencies:

Context from getApplicationContext()

OperationsServiceBinder

ezaquarii · 2019-11-28T20:22:45Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+     * @param user      current user
+     * @return          true if this operation was consumed, false otherwise
+     */
+    public boolean onCreateFolderOperationFinish(CreateFolderOperation operation,


This name (and I know it's a delegate from legacy code, so no blames) suggests that we're creating some mysterious folder operation finish (a finish event?).

JavaDoc tells that it's onFinish<what/noun> callback.

Let's rename both methods.

ezaquarii · 2019-11-28T20:32:20Z

src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

-                false
-            );
+    private void uploadFromFileSystem(List<File> files, int resultCode) {
+        filesUploader.uploadFromFileSystem(files, resultCode, null, getCurrentDir(),


When args list is too long, we make it vertical to boost readability.

ezaquarii · 2019-11-28T20:32:26Z

src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java

-                    break;
-            }
+        String replicationStartDir = data.getStringExtra(UploadFilesActivity.EXTRA_REPLICATION_START_DIR);
+        filesUploader.uploadFromFileSystem(files, resultCode, replicationStartDir, getCurrentDir(),


When args list is too long, we make it vertical to boost readability.

ezaquarii · 2019-11-28T20:35:40Z

src/main/java/com/owncloud/android/utils/FileStorageUtils.java

+        String targetPath = targetDirectory.getAbsolutePath();
+        String currentPath = currentDirectory.getAbsolutePath();
+        if (targetPath.startsWith(currentPath)) {
+            return new File(targetPath.replace(targetPath.replace(currentPath, ""), ""));


It's generally bad practice to have more than 1 statement per line.
We have 3 here.

ezaquarii · 2019-11-28T20:37:09Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+        return false;
+    }
+
+    private List<UploadTarget> getRemoteTargets(@NonNull List<File> localFiles,


Javadoc pls

ezaquarii · 2019-11-28T20:52:08Z

src/main/java/com/owncloud/android/ui/asynctasks/CheckAvailableSpaceTask.java

@@ -41,6 +43,15 @@ public CheckAvailableSpaceTask(CheckAvailableSpaceListener callback, String... p
        this.callback = callback;
    }

+    public CheckAvailableSpaceTask(CheckAvailableSpaceListener callback, Collection<File> files) {


It would be beneficial to kill the legacy constructor that uses String[]. Is it feasible?

ezaquarii · 2019-11-28T20:56:14Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+    private List<File> localFiles = new ArrayList<>();
+    private int resultCode;
+
+    public static class UploadTarget {


In Kotlin, this would be 1 line data class. :)

ezaquarii · 2019-11-28T20:57:21Z

src/main/java/com/owncloud/android/files/FilesUploader.java

+     * @param currentDir            the directory user currently is in
+     * @param user                  current user
+     */
+    public void uploadFromFileSystem(List<File> localFiles,


JavaDoc tells me it's places a request.
Name tells me it uploads.

Shall we bring the function name on par with the description?

kilves · 2019-11-28T22:45:12Z

@ezaquarii did you delete the long comment chain about javadocs and refactoring?

ezaquarii

@kilves, first of all, I'd like to thank you for the effort taken to bring this feature forward, taking stale PR, and for the refactoring you've done. Before I move forward, please know that you have choosen to work on the most challenging part of the application.

For the time being, let's ignore my inline comments that I put in the code. While I was going through the code line by line over past hours, I realized that we have some fundamental issues to be addressed before we sink more time into marginal code improvements. I'll leave those comments in case we find it useful at some point, but please ignore them for now.

So far (and please correct me if I'm wrong), this is how I see the situation:

upload is done in the background by a FileUploader service (subclass of android.app.Service)
the service has a sneaky backdoor in form of the FileUploader.Requester (that is in fact an intent Builder cross-bred with some sort of ServiceConnection concept)
FilesUploader is a stateful wrapper around this backdoor into FileUploader
FileDisplayActivity.requestUploadOfFilesFromFileSystem() is a fire-and-forget enqueue method that uses that builder to enqueue uploads into service in UI-independent way

Here are my initial concerns:

We introduce new stateful FilesUploader concept that is a wrapper around a stateless Requester backdoor into the FileUploader service
This FilesUploader genuinely just builds a request that is then sent to the service, so it's more of a Builder kind of thing again...
...except that this time the request is partial and continuation is driven by callbacks and we changed queue push operation (requestUploadOfFilesFromFileSystem) into a state machine living in the UI, tying it to an unpredictable Activity lifecycle
for a large folder, partial traversal is still done on UI thread, which means blocking I/O (although it's split into smallers operations, so this might event work for humanely sized folders).

Overall, it looks to me like this design is not going to work for us. The upload driving logicsimply does not belong here (UI) and I'm not surprised that the old author abandoned it. Your refactoring made the code readable to a point where we can actually understand how difficult the situation is, so good work done here. Having said that, I still believe this PR is something we should rethink and get back to the whiteboard.

Before we get too depressed, I'd like to say that the FilesUploader logic is probably salvageable and I see 3 avenues:

Variant 1a - in ui, stateless

Remove the callbacks and make it stateless - construct list of files to upload by recursively traversing the folder, fire-and-forget gigantic request, respecting old design rules. In this variant we probably want to extend FileUpload.Requester with another method that builds fancy Intent with large list of files.

Pros:

simple(r)

Cons:

probably won't work with big folders as input size is unbounded
traversal on UI thread is not feasible due to blocking I/O and ANRs

Variant 1b - in service, stateless

Same as 1a, except that FileUpload (Service) will receive a folder path and perform the traversal to construct the list of files to upload in background. I'm not sure if it does not clash with the funky FileDisplayActivity.onActivityResult(...) -> requestUploadOfFilesFromFileSystem(...) logic, as there is a lot of if-ology there that I don't fully understand, but at first glance it looks like it just handles externally fired Intent

Pros:

traversal can be probably done in background easily

Cons:

not sure, but it looks like a refactoring rabbit hole
still in UI, which is wrong

Variang 2 - in service, stateful

If we can't have a some sort of quasi-functional solution (because of backend round-trips for example, or we need to check things between uploads), FilesUploader-the-wrapper should live inside FileUploader-the-service (with a different name of course), possibly as a properly encoded state machine.

Pros:

service is the place where this should live actually

Cons:

basically a rewrite
service is already suffering from high complexity - this avenue probably induces refactoring rabbit hole around this component

@kilves @tobiasKaminsky @AndyScherzinger IMO this is really tough problem and we should get back to the drawing board.

ezaquarii · 2019-11-28T23:00:30Z

@ezaquarii did you delete the long comment chain about javadocs and refactoring?

Sorry, it looks like my review was published before I finished it. This one is final. I moved this comment from attached-to-source-line to a standalone comment, as it mothballed into a summary. :)

tobiasKaminsky · 2019-12-02T11:29:25Z

@ezaquarii i totally agree with your analysis, but fear that there is (at least from my side) no time in nearer future to refactor this.

So I would go for 1b as a quick win (then we also do not have Android warning if UI takes too long) and we can also add this enhancement.

Meanwhile we still can & should discuss how to refactor this (in a new issue) with lots of smaller steps, which can be easily done and tested.

AndyScherzinger · 2019-12-04T09:42:58Z

I'd also say for now 1b seems to be feasable given the circumstances.

tobiasKaminsky · 2019-12-10T09:59:48Z

I switched label back to "1. developing", please change it once it is ready for review

AndyScherzinger · 2021-03-18T08:28:12Z

superseeded by #7877

kilves force-pushed the upload-folder-support branch from cc2a9f4 to a5803c2 Compare November 20, 2019 17:40

kilves force-pushed the upload-folder-support branch from 7eefd80 to fc718c8 Compare November 20, 2019 18:09

kilves force-pushed the upload-folder-support branch from 91a2213 to 88ecc91 Compare November 21, 2019 21:30

AndyScherzinger added the 3. to review label Nov 26, 2019

kilves force-pushed the upload-folder-support branch from 88ecc91 to 805a236 Compare November 26, 2019 08:43

ezaquarii requested changes Nov 26, 2019

View reviewed changes

phetta added 6 commits November 28, 2019 20:57

small changes to uploading a folder

1494653

Signed-off-by: Petri <petri.hetta@dicode.fi>

small fixes

74b7c8f

Signed-off-by: Petri <petri.hetta@dicode.fi>

small fixes

888d362

Signed-off-by: Petri <petri.hetta@dicode.fi>

remove redundant listener

6263c14

Signed-off-by: Petri <petri.hetta@dicode.fi>

fix codestyle

995369b

Signed-off-by: Petri <petri.hetta@dicode.fi>

kilves force-pushed the upload-folder-support branch from 805a236 to 6c3088a Compare November 28, 2019 19:00

refactor to classes, improve readability

4f50574

Signed-off-by: Petri <petri.hetta@dicode.fi>

kilves force-pushed the upload-folder-support branch from 6c3088a to 4f50574 Compare November 28, 2019 19:00

nextcloud deleted a comment from ezaquarii Nov 28, 2019

ezaquarii reviewed Nov 28, 2019

View reviewed changes

nextcloud deleted a comment from kilves Nov 28, 2019

ezaquarii requested changes Nov 28, 2019

View reviewed changes

tobiasKaminsky added 2. developing and removed 3. to review labels Dec 10, 2019

AndyScherzinger mentioned this pull request Aug 3, 2020

Uploading folders? #6629

Closed

AndyScherzinger mentioned this pull request Sep 30, 2020

Upload folder #7032

Closed

AndyScherzinger closed this Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload folder support #4859

Upload folder support #4859

kilves commented Nov 19, 2019 •

edited

Loading

codecov bot commented Nov 20, 2019 •

edited

Loading

nextcloud-android-bot commented Nov 21, 2019

nextcloud-android-bot commented Nov 21, 2019

kilves commented Nov 22, 2019 •

edited

Loading

AndyScherzinger commented Nov 26, 2019

AndyScherzinger commented Nov 26, 2019

ezaquarii left a comment •

edited

Loading

nextcloud-android-bot commented Nov 28, 2019

nextcloud-android-bot commented Nov 28, 2019

kilves commented Nov 28, 2019

ezaquarii Nov 28, 2019 •

edited

Loading

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019 •

edited

Loading

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

ezaquarii Nov 28, 2019

kilves commented Nov 28, 2019

ezaquarii left a comment •

edited

Loading

ezaquarii commented Nov 28, 2019 •

edited

Loading

tobiasKaminsky commented Dec 2, 2019

AndyScherzinger commented Dec 4, 2019

tobiasKaminsky commented Dec 10, 2019

AndyScherzinger commented Mar 18, 2021


		public class FilesUploader {

		private FileActivity fileActivity;

Upload folder support #4859

Upload folder support #4859

Conversation

kilves commented Nov 19, 2019 • edited Loading

codecov bot commented Nov 20, 2019 • edited Loading

Codecov Report

nextcloud-android-bot commented Nov 21, 2019

nextcloud-android-bot commented Nov 21, 2019

kilves commented Nov 22, 2019 • edited Loading

AndyScherzinger commented Nov 26, 2019

AndyScherzinger commented Nov 26, 2019

ezaquarii left a comment • edited Loading

Choose a reason for hiding this comment

nextcloud-android-bot commented Nov 28, 2019

nextcloud-android-bot commented Nov 28, 2019

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

SpotBugs increased!

kilves commented Nov 28, 2019

ezaquarii Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezaquarii Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kilves commented Nov 28, 2019

ezaquarii left a comment • edited Loading

Choose a reason for hiding this comment

Variant 1a - in ui, stateless

Variant 1b - in service, stateless

Variang 2 - in service, stateful

ezaquarii commented Nov 28, 2019 • edited Loading

tobiasKaminsky commented Dec 2, 2019

AndyScherzinger commented Dec 4, 2019

tobiasKaminsky commented Dec 10, 2019

AndyScherzinger commented Mar 18, 2021

kilves commented Nov 19, 2019 •

edited

Loading

codecov bot commented Nov 20, 2019 •

edited

Loading

kilves commented Nov 22, 2019 •

edited

Loading

ezaquarii left a comment •

edited

Loading

ezaquarii Nov 28, 2019 •

edited

Loading

ezaquarii Nov 28, 2019 •

edited

Loading

ezaquarii left a comment •

edited

Loading

ezaquarii commented Nov 28, 2019 •

edited

Loading