Fix sync conflict when user chooses "Keep already existing file" (Quick)

[wknds]

One of the goal of merge #7949 was to omit notifications on successful downloads. Unfortunately, this introduced a bug for handling file conflicts. When the user tries to resolve the conflict by keeping the server version, the expected behavior is to download the file (handled by FileDownloader). When the file is downloaded successfully, FileDownloader.notifyDownloadResult leaves the method (line 637) before actually removing the file from the UploadsStorageManager. There is another pull request #8923 from @BurningKarl, tackling this issue by providing a new concept for keeping the server version. This pull request tries to fix the issue by using the present concept in the nextcloud app.

Testing

Is a test relevant here?

• Tests written, or not not needed

Application tests. Invoke the conflict manager and then solve the conflict by keeping already existing file:

1. Sync files on nextcloud. Before sync, both the server and local version are modified each.
   1.1 Result: Ok
2. Share a file to a nextcloud folder. The shared file has the same filename as an existing file on the destination folder.
   2.1 Result: Ok
3. Add a file to a media scan folder (setting: original file will be moved to app folder). The name of the added file already exists on the corresponding nextcloud folder.
   3.1 Result: Failed.

[BurningKarl]

Hi @wknds, this would already be an improvement over the currently buggy state of conflict notifications, but I'd like to make a point that this is not enough and that the sync conflict dialog needs further improvements: This implementation does not allow the user to just cancel the upload operation.

After your PR is merged the four options are to keep the "New file", keep the "Already existing file", keep both (by ticking both boxes) or "Cancel". Using the first option the server version is overwritten, using the second option the local version is overwritten, using the third option the local version is uploaded using a different name and using the fourth option the sync conflict dialog is closed. Crucially, clicking on "Cancel" does not dismiss the sync conflict, just the dialog. The app will try to upload again, will fail and show a new sync conflict notification.

I think the sync conflict dialog needs an overhaul to allow the user to just dismiss the sync conflict and from my perspective my PR #8923 is the easiest and most user-friendly way to do so. Ultimately, its up to the main developers to decide this, but I hope they consider this problem when reviewing this PR.

[wknds]

Hello @BurningKarl. I replayed your usecase which you have described in #8923 and I agree it is confusing. I am going to upload your changes on my phone in the next days to test your implementation, so I can give a feedback on user experience.

What confuses me most is the naming of the options in the current conflict manager. There are two scenarios:

1. I upload a file to nextcloud which has never been on nextcloud before but accidentally has the same filename with a file on the nextcloud server.
   1.1 -> The options are not confusing to me. "Already existing file" is meant to be the synced file known by nextcloud. "New file" is meant to be the file I am trying to (auto-)upload to nextcloud.
2. I modify an already synced file on my nextcloud app (e.g. using a password manager), while the same file on the server is also modified in the meanwhile.
   2.1 -> The options are confusing to me. Is "new file" meant to be the local file or the one on the server. Which of the modified version is newer?

[BurningKarl]

I agree that the naming is confusing in the second case. It would be more clear if one option was "server version" and the other one "local version". Unfortunately, it is then unclear which one is renamed if the user decides to keep both. Currently, the local one is renamed when uploading by adding "(2)" at the end of the filename. Maybe adding "from my " at the end of the newly uploaded file would be better.

In any case, this naming problem probably deserves its own pull request.

[nextcloud-android-bot]

stable-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/2315-IT-stable-15-38

[AlvaroBrey]

Thanks @wknds and sorry for the late review

[AlvaroBrey]

This will break the behaviour when "delete file from source folder" is chosen as an upload option, as it will download the file regardless of that selection.

As I commented in #8923, I think the main problem here is that the conflicts on autoupload and manual upload shouldn't be treated in the same way (in manual upload, keeping the local file untouched makes sense most of the time, but not in autoupload).

I would instead take a page from #8923's book and add an else block that just dismisses the upload when shouldDeleteLocal is true. This is, I believe, the best way to improve the behaviour we have right now without changing it too much, until we do a proper overhaul of the conflicts system.