Skip to content

Sync downloaded#2886

Merged
AndyScherzinger merged 1 commit into
masterfrom
syncDownloaded
Dec 15, 2018
Merged

Sync downloaded#2886
AndyScherzinger merged 1 commit into
masterfrom
syncDownloaded

Conversation

@tobiasKaminsky

@tobiasKaminsky tobiasKaminsky commented Aug 14, 2018

Copy link
Copy Markdown
Member

Enhancement discussed in #2718

  • remove "keep available offline"
  • sync job
    • only on wifi
    • syncs all downloaded files
      • if conflict, just do nothing
      • deletes local files that are not on server any longer
  • when entering folder with outdated downloaded file, a red icon is shown on file

TODO

  • check handling after upgrade

@mario

mario commented Aug 16, 2018

Copy link
Copy Markdown
Contributor

Would this support folders too?

@tobiasKaminsky

tobiasKaminsky commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

Currently not as we do not have (yet) any possibility in UI to set "available offline" for folders.
It just enhances the current way.

We should discuss this properly and then implement it in a separate PR.
(keep in sync for folders is really close to two sync, or?)

ocFolder.setEtagOnServer(updatedEtag);
storageManager.saveFile(ocFolder);
} catch (Exception e) {
Log_OC.e(TAG, "Failed to update etag on " + folder.getAbsolutePath());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should also log the exception, else you just know it failed but not for which reason

@AndyScherzinger

AndyScherzinger commented Aug 28, 2018

Copy link
Copy Markdown
Member

👍 looks good code-wise and also worked for me for the browsing scenario. Didn't test the actual sync though.

I also agree that we should have the folder variant in a different PR and that would be close to a two way sync while for example deleting a file on the client asks you if you want to delete the file on the server too, so that is rather a one way sync server->client + syncing back changed files to the server not sure if we should handle created files since that would mean someone created a file within the clients directory structure.

Approved with PullApprove

@AndyScherzinger

Copy link
Copy Markdown
Member

Imho this needs another DB version and a change routine to remove the fields from the database

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

change routine to remove the fields from the database

There is no way in sqlite to remove an existing column.
We can only ignore it and on new installation just do not create it…

@mario

mario commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

Looks decent, but why are there so many Glide changes in there?

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

This extends #2871.
I will update #2871 today.

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

@sehucke this is a very good feedback, thank you.

However I vote for merging this right now, as the meaning of icons stay the same and open up a new discussion how to improve this.

@AndyScherzinger

Copy link
Copy Markdown
Member

However I vote for merging this right now, as the meaning of icons stay the same and open up a new discussion how to improve this.

I second that!

@sehucke

sehucke commented Dec 12, 2018

Copy link
Copy Markdown

👍

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

Rebased, should now compile again

@mario

mario commented Dec 12, 2018

Copy link
Copy Markdown
Contributor

@tobiasKaminsky Codacy issues at least + build failed and I can then take a look.

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

Strange, only this PR is failing on tests, while e.g. #3350 is building fine: https://drone.nextcloud.com/nextcloud/android/7129

So I suggest, we ignore it ;-)

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

Hm. Seems indeed to be a valid problem:
Job intervall is set too low --> changed

@AndyScherzinger

Copy link
Copy Markdown
Member

👍 works fine for me, I had

  1. master + downloaded file (no wifi)
  2. updated file on server
  3. installed branch
  4. file is marked red (since server changed, but no wifi)
  5. activated wifi and waited for ~20 minutes
  6. checked file -> makred green, up-to-date, no fetching server version, client already had the right file version

Thus merging

@AndyScherzinger

Copy link
Copy Markdown
Member

@mario what do you think? Works fine for me (at least what I tested of course)

@matsest

matsest commented Dec 12, 2018

Copy link
Copy Markdown

Would this allow for downloading/exporting a file from NC that is not affected by the sync? E.g. if I want to download/export a file from NC to my local Download folder and not Android/Media/com.nextcloud.android? At least for me, I don't have a Save to Device in the Share menu for a file in NC (as I have in Dropbox).

@AndyScherzinger

Copy link
Copy Markdown
Member

@matsest No but you probably don't need that. SImply hit the share icon and choose an installed app, which allows for that e.g. Amaze File Manager :) (didn't test if this works though)

@matsest

matsest commented Dec 12, 2018

Copy link
Copy Markdown

@AndyScherzinger Yes, that works with Amaze - have tested. However, it does not work with my default Files app (it's a crappy default Huawei app in EMUI), i.e. which is what "Save to Device" is in a range of other apps. I don't know much work there is to enable support for sharing to all these different CMFiles, Huawei Files app and other vendor specific default file apps. But it's a nice feature to be able to share across apps..

That's what I like about the sharing menu in Android - I probably don't need to be able to share a SMS to my Snapchat app (a crude example) and 15 other apps in the pop-up, but Android share menu allows for these interfaces between almost every app.

Specifically for a file sync app like NC I think it's a good idea that manual export/save to device should be possible without installing a third-party app for users - in addition to the offline/sync feature in this PR. That being said, I love Amaze and prefer it over default Files in EMUI, so I won't shed tears if this isn't taken into development ;)

Sorry if this was off topic for the PR (:

@nextcloud nextcloud deleted a comment Dec 13, 2018
@tobiasKaminsky

Copy link
Copy Markdown
Member Author

Specifically for a file sync app like NC I think it's a good idea that manual export/save to device

That is the problem: if you save a file to a folder outside NC it will never get updated. Therefore I am not a big fan of doing this.
If you have audio/video/images downloaded you can see them in gallery app / media player…
For further discussion please open up a new issue.

@mario

mario commented Dec 13, 2018

Copy link
Copy Markdown
Contributor

Please make sure to drop the columns that we don't use anymore in the newest DB migration/transaction. Otherwise good.

@tobiasKaminsky

Copy link
Copy Markdown
Member Author

#2886 (comment)

Andy: Imho this needs another DB version and a change routine to remove the fields from the database

#2886 (comment)

There is no way in sqlite to remove an existing column.
We can only ignore it and on new installation just do not create it…

:-D :-D

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

Copy link
Copy Markdown
Member Author

As this was previously based on glide branch, I forgot to remove the "update thumbnail" stuff.
So this is now back in, as this is required by the old thumbnail system.

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Lint

TypemasterPR
Warnings8383
Errors00

FindBugs (new)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings159
Internationalization Warnings15
Malicious code vulnerability Warnings11
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings66
Dodgy code Warnings134
Total548

FindBugs (master)

Warning TypeNumber
Bad practice Warnings35
Correctness Warnings159
Internationalization Warnings15
Malicious code vulnerability Warnings11
Multithreaded correctness Warnings9
Performance Warnings121
Security Warnings66
Dodgy code Warnings134
Total550

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