ADD: [instantupload] setting to also upload existing files #2873
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2873 +/- ##
============================================
- Coverage 17.49% 16.93% -0.56%
Complexity 3 3
============================================
Files 376 365 -11
Lines 32385 32137 -248
Branches 4569 4529 -40
============================================
- Hits 5665 5444 -221
+ Misses 25799 25797 -2
+ Partials 921 896 -25
|
|
Looping in @tobiasKaminsky @mario @javaisbetterthanpython based on the feedback in #2812 Also looping in @nextcloud/designers and @jancborchardt in particular for the discussion on how this should work from the UX point of view as-in when and how should the upload existing images be triggered. From my point of view it should be a it-should-just-work meaning the user shouldn't have to expplicitely state that existing images should be uploaded. This is a bit more complex though since the question remains rather about when / in which cases this upload should be triggered. If you activate it for the very first time I'd say it should just upload the existing ones without asking while the question is what should happen when the auto upload has been activated/deactivated in the past and gets activated again because in this case it definitely should check against the server as-in a one-way sync. Since we don't have file hashes provided by the server we could only check for files with the same name and append a (1) at the end or maybe check for some other meta information present like timestamps :/ Looking forward to your feedback 😃 |
|
I'm not totally sure server-side duplicate checking falls into the scope of already existing files on a single device. |
That's true! |
@AndyScherzinger totally agree with that UX-wise! :) And @koying welcome, and thank you for your contribution! :) |
I agree that it should be automatic, but still think the user should be able to opt-out. Just having the setting enabled by default on addition of a new directory to autoUpload could be fine, right?
@AndyScherzinger I think the approach you suggested could work alright. Check for the same filename, and then compare timestamps. Although, I think instead of appending "(1)" to the filename on upload it might be better to append the timestamp of the upload, so you end up with something like "originalfilename-YYYYMMDD-HHMMSS.ext". This would account for multiple duplicates being merged into one directory and easily distinguish which version is which, rather than (1), (2), (3), etc. How does the app currently handle uploads when a filename conflict occurs? @koying Nice job man! This has been a much requested feature! |
Thanks. As I mentioned earlier, we might want to leave the issue of name conflicts out of this one, as this is already an issue when multiple devices are syncing to the same account. I actually think it's less likely to happen in this specific 1 device -> 1 instance case. OTOH, name conflicts, especially in the context of photos/videos, where android names them with a timestamp up to a second, should be very rare, and we have versioning on the server side (can't remember if it's optional/enabled by default), so we shouldn't loose anything, ever, so might not worth thinking too much about it. It might actually be counter-productive to over-engineer it, as 2 files named "IMG_20180810_163134.jpg" are quite likely to be the same, so we might end up creating unwanted duplicates on the server... |
|
Just to clarify on something. @AndyScherzinger said: " we could only check for files with the same name and append a (1)" We don't need to do this since there's already a way to do it. When creating an upload job, it should set overwrite to no and (1) and such would get appended automatically. |
|
I will go over the code today. |
Thanks a lot for finding the time to look into this matter.
Also a big thank you and kudos to your work on this feature, this highly appreciated, great job! 🥇
Like @mario said if you set override to "no" it'll take care of the renaming automatically. |
|
First of all, thank you for the contribution. Ok, so last parameter to uploadFileWithOverwrite could be changed to “false” if it’s an initial sync just to make sure we don’t step on anybodys’ toes in this “simple” scenario, even though in general auto upload would overwrite everything which is why I’ve always hesitated going with this approach. (This is in FilesSyncJob) The issue there is you don’t really know if it’s an initial upload while at that stage. Anyway… In order to accomplish an initial sync (on FIRST enable state of a particular folder) you don’t need an “existing” column. You would need to look into FilesSyncHelper, remove the “dryRun” condition, and instead do what the usual syncing would do (the “else”) part. Keep in mind if the syncedFolderInitiatedKey for this folder is null, you’d still need to initialize it. If a folder is disabled, you’d remove the syncedFolderInitiatedKey so the next time user runs you could once again ask if they want to upload all files. Hope I've helped at least a little bit! |
|
@mario Thanks for reviewing. Synthetically:
Did I get you right? The problem I see is how to determine it is an initial sync. OTOH, to follow the same logic of syncing "everything", I don't think removing syncedFolderInitiatedKey is desirable. |
|
Well, you could ask the user "Do you want to push existing files" once first enabled if you want as well. As for the syncedFolderInitiatedKey - you could indeed opt NOT to remove it. |
|
You just need to make sure you update it at the right time. :) |
|
Which NC15 there will be (hopefully) checksums on server side. |
Some user feeback on this: Using nextcloud together with your awesome android app, this PR would resolve the most crucial pain point within the app for me! Please make it work and merge asap.... :D |
|
Some additional user feedback For the last 1.5 day's I've been troubleshooting my fresh nextcloud setup, trying figure out whye the android auto uploader was not working - only to find this thread and discover it is a feature not to upload existing files. Pleeeease prioritize this one. |
|
This is a wonderful pull request. I too have spent hours debugging this. Is there any timeframe for a merge? Perhaps a forked app is required (I am baffled by anyone thinking that this is a 'feature') |
No, see #2873 (comment) - to be sure to have some kind of "sync" implementation to not create duplicates on the server or override differing files with the same file name we need to have checksums (server doesn't support this yet).
Uploading existing files to the server is something that hasn't been implemented yet so of course it is a feature. A functionality not present and not mandatory to be compliant with any kind of specification/standard is of course a feature and not a bug. Just becaouse you expect this to be happening doesn't make it a bug... While I get your frustration, this is something that needs to be more or less bullet proof since such a change in behavior will hit >250K installations. So possibly creating duplicates on the server (in case you activate/deactivate the auto upload for a single folder several times) will lead to the exact opposite of what you @tmolteno are trying to achieve. It won't make them happy - it'll make them angry and frustrated. Just my 2 cents. |
|
In the meantime, couldn't we implement the feature to only function on a target directory that is empty? That would probably be sufficient for most users in the interim while we wait for checksums |
@javaisbetterthanpython that would imho work fine and wouldn't lead to any trouble for the app users. what do you think @koying @tobiasKaminsky @mario @ardevd ? |
|
You mean only upload all the very first time instantupload is used? The typical use case (at least the one I created this PR for) is to add a local folder to the list to be synced, and naturally wanting the existing local files to be uploaded as well. As I said above, the conflict/overwrite issue already exists when syncing multiple devices, and must be resolved at FilesSyncJob level, globally, not at the level of choosing which local files to sync. Implementing this one (w/o the gui option, if so preferred) adds a case, but:
|
|
Maybe an option would be to make "forceOverwrite" a tri-state, and not allow overwrites nor renames on initial sync? |
|
@koying w/o having tested it what would happen in case of a "regular" media folder like the camera if I active/deactivate/activate? Would this simply create duplicates for all files from the camera folder? |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10936.apk |
AndyScherzinger
force-pushed
the
instantupload_all
branch
from
637f33a to
17c0096
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10998.apk |
Codacy277Lint
SpotBugs (new)
SpotBugs (master)
SpotBugs increased! |
AndyScherzinger
force-pushed
the
instantupload_all
branch
from
17c0096 to
e5421df
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11462.apk |
|
I added a PR to this one as koying seems to be inactive on this PR. This fixes the immediate issues that prevented the feature from working on my tests. |
Chris Koying Browet <cbro@semperpax.com> Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
AndyScherzinger
force-pushed
the
instantupload_all
branch
from
5ef9898 to
dc31c93
Compare
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11515.apk |
|
@ArisuOngaku thanks for the fixes, I cherry picked them to @koying's fork and to make things easier in the process pushed the branch to this repo (hope you are fine with this @koying), so I'll be closing this in favor of a PR from a branch within the repo which makes it easier to work on it. Successor PR is #4788 - so closing this one any further discussion/work should happen in #4788 |
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11516.apk |
|
@AndyScherzinger Sure, no prob. |
Codacy462Lint
SpotBugs (new)
SpotBugs (master)
SpotBugs increased! |
Codacy462Lint
SpotBugs (new)
SpotBugs (master)
SpotBugs increased! |
❤️
I took care of keeping it up-to-date with the app's master branch and @ArisuOngaku fixed some issues.
Yes and no. In general a solution for uploading existing files is to be achieved (one way or the other). As discussed here and in other places - yes, it is unclear how this should be achieved. I agreee that for a "proper" solution we need checksums from the server (which would be the 100% bulletproof way) but as also stated from @jospoortvliet to have "some"/80%-solution would be a good first step so I personally think thins can be the basis for such a change while it might need some more love. From my point of view we still do need an agreement within the project how a 80% sultion should look like (which is the issue that block any further progress on this at the moment) - while this is just my personal view on the matter. Still I try to keep this PR and your efforts alive since I don't want to let it go to waste! |
|
Hi all! thanks for the great contribution I've also spent a lot of time scratching my head trying to figure out what's going on, yes please! please merge this is in, this is one of the general functionalities a 'cloud' type sync app should perform that everyone assumes (it's what all major connectors do already), I believe the advantage of having this in the main branch far outway any issues that fall out from this. Quoting @AndyScherzinger in #412458129 yes absolutely - I just had to enable this option for about 26 various folders on my device, but not a show stopper, It certainly is what I believe to be "expected behaviour" of this nextcloud application on the whole. I tested the most recent APK in #549124650 and it does indeed work (Sidestory about checksums - other than wasted bandwidth, a lot of NAS devices - such as FreeNAS that i'm running this on - take care of de-dupe's automatically via ZFS de-dupe, other FS's may do the same) Please please please merge this in 💜 💜 |
|
Could you please link to that apk? Or is that the last compiled apk in this thread, i.e.: I just spent two days installing nextcloud only to find that this isn't possible atm and that you can't push folders but only single files (which would have been an acceptable one time workaround). |
|
@dekiesel best is to just install 3.10.0 RC1 via the GPlay beta programm or just see https://github.com/nextcloud/android/releases/tag/rc-3.11.0-01 |
|
Thanks for the tip! To give a little feedback: On Android 9 on a custom rom: uploading smaller folders 35 files, 9mb, everything worked fine. T app freezes on bigger folders (51 files, 50mb). I sent a feedback via mail too that should include a logfile. /edit: an hour later, without me doing anything, the files are uploading ❤️ |
|
@dekiesel glad to hear that 🎉 So for future issues regarding auto upload, best thing is to open an issue with the steps plus log and pinging @tobiasKaminsky and @ArisuOngaku |
Please be kind, first time I put my nose into the code.
This adds a setting to the instant uploads to also upload files already present on the device at the time instant upload is setup.
Ref: https://help.nextcloud.com/t/why-doesnt-android-client-upload-existing-pictures/10365
95% of the PR is for handling of the setting itself.
It might be worth wondering whether this deserves a setting at all, really, ie whether not uploading existing files is worthwhile, as it's a bit counter-intuitive, imo