nextcloud / desktop

💻 Desktop sync client for Nextcloud
https://nextcloud.com/install/#install-clients
GNU General Public License v2.0
3.01k stars 791 forks source link

Unsynced folders are lost, if you uncheck #1238

Closed aronpapp closed 1 year ago

aronpapp commented 5 years ago

Expected behaviour

Actual behaviour

Erases (unsynced) data silently.

Steps to reproduce

  1. Move or copy a folder to your sync folder using a filemamanger
  2. Open nextCloud client
  3. Do not wait to sync the new folder to the server
  4. Uncheck checkbox for the new folder
  5. Click Apply

Client configuration

Client version: Version 2.5.1final (build 20181204)

Operating system: Debian 9.8

OS language: en

Qt version used by client package (Linux only, see also Settings dialog): I guess it uses bundled Qt.

Client package (From Nextcloud or distro) (Linux only): Nextcloud-2.5.1-x86_64.AppImage

Installation path of client: No need to install

Server configuration

Issue is not related to the server.

Germano0 commented 5 years ago

What does it mean 4. Remove checkbox before new folder ?

aronpapp commented 5 years ago

What does it mean 4. Remove checkbox before new folder ?

Sorry, I mean uncheck the checkbox (also fixed in the initial post)

camilasan commented 5 years ago

You mean you uncheck the folder so it doesn't get synced, correct?

aronpapp commented 5 years ago

You mean you uncheck the folder so it doesn't get synced, correct?

Yes, before it gets get synced, gets deleted from the filesystem, so data is completly flushed.

freechelmi commented 4 years ago

That sounds like a very important problem.

b12f commented 4 years ago

There's a large discussion about this behaviour in the owncloud repo: https://github.com/owncloud/client/issues/2502

Includes more failure cases and a couple of proposed UI fixes.

salagula commented 3 years ago

That is really critical - we have exactly that problem. It dramatically lowers down the tool acceptance and. As the desktop app is available as source code: has someone tried to disable the method that deletes the local files? Any indication on the source code file where we can turn it off?

er-vin commented 3 years ago

I guess there's something I don't get regarding that one, but here unchecking something leads to this warning: Screenshot_20201022_125320

And it only removes if you confirm it. To me, it's pretty clear it's going to kill data in there.

b12f commented 3 years ago

The problem is that it also removes files that aren't synced yet. So let's say you added a big directory to nextcloud, but want to not sync it (for now, or not after all), it removes the data completely. When would this happen? Mostly during installation; you select a folder in which the data is that you want to sync, but then find out there's a couple of large files in there that you do not want to sync after all. Poof, data gone.

Edit: Alternatively, you have to wait for the complete sync to finish before you can unsync, or you have to reinstall nextcloud. In any case, the banner is not enough, and having this behaviour as default is bad. Both the discussion here and in the owncloud repository show that there are enough people who - even though they are technically versed - still get caught by surprise and lose data.

From a UX perspective, the warning is not enough imo. This behaviour can lead to unsynchronized data being lost forever, so this should be a big fat red banner if there are unsynchronized files to be deleted. Even better, it should be possible to unsynchronize without deletion.

salagula commented 3 years ago

I agree to b12f - it is not about the message, it is about the fact that it deletes files before they are synced. Again my question: can someone point me to the code segment where this is triggered? I would like to comment the section out (if possible)

er-vin commented 3 years ago

I agree to b12f - it is not about the message, it is about the fact that it deletes files before they are synced. Again my question: can someone point me to the code segment where this is triggered? I would like to comment the section out (if possible)

It's not that simple, it's really just about putting those folders in the selective sync exclude list and then the sync engine "doing its thing". It's not like there's a function where those folders are removed, this is deeply buried in the sync engine behavior AFAICT.

irisv commented 3 years ago

I guess there's something I don't get regarding that one, but here unchecking something leads to this warning: Screenshot_20201022_125320

And it only removes if you confirm it. To me, it's pretty clear it's going to kill data in there.

I'd like to vote for a dialogue that rather than cancel / apply gives three options:

  1. remove folder from sync and delete local data (= the current behaviour)
  2. remove folder from sync but keep local data (= my preferred behaviour in most cases)
  3. cancel
andreaslink-de commented 3 years ago
  1. remove folder from sync and delete local data (= the current behaviour)
  2. remove folder from sync but keep local data (= my preferred behaviour in most cases)
  3. cancel

Honestly, there is a big risk of doing no. 2 based on my own experience. I had cases where I removed a folder like this (might have be in older owncloud times) and I kept the folder in there but it was not synced anymore. After around x days I forgot about it and changed some important data on one machine in that folder and I changed files on the other machine some days later as well. It took me some further days to figure out, I was working on different instances and need to do manual sync work now.

So the rule should be, if it is in your locally synced nextcloud folder, then it's synced and if it is not synced it is not in there - store your data somewhere else, but don't confuse yourself with folders being in sync and some not.

So no. 1 and no. 3 are the only realistic options for not messing up with your personal data controlling, so I disagree and would be surprised you describe a usecase where no. 2 is your "most preferred behaviour".

Where I agree is that there could be an additional warning, if data is going to be deleted, which was not synced yet. Use case could be you move a bigger folder from anywhere to your local nextcloud folder to get it synced and expect the sync to be finished after a while, but due to low internet speed the folder was only half synced yet. If you then just try to remove the hook to get the folder deleted (maybe to save some local space) you have to be warned. There it makes sense for not loosing unsynced data.

sojer commented 3 years ago

Will a fix be included in the next nextcloud release? Or is there something planned for a future release?

irisv commented 3 years ago

So no. 1 and no. 3 are the only realistic options for not messing up with your personal data controlling, so I disagree and would be surprised you describe a usecase where no. 2 is your "most preferred behaviour".

Ok, then let me explain my usage to you. I have a set of core folders that need to be synced and kept up to date continuously. Those will never be deleted from the client, so those don't come into this discussion. When I delete a folder from the client, it is -by definition- a folder I've only had use for temporary and now no longer need. So your scenario where "around x days [you] forgot about it and changed some important data on one machine (...)" will never apply to these folders, because these are not files that are going to be changed. These are folders that I only needed to use for a limited period of time, that might have been handy to sync while they're useful but don't need to be synced any more once I'm done with whatever task they were created for. Still, sometimes I want to hang on to them locally, if only for a couple of minutes, because, well, deleting a whole folder in one go can be scary. What I do now is rename/move the folder, then remove it from the client and then figure out myself what want to delete and what needs to be archived & where. You might say that's the 'wrong' way of using Nextcloud, but I'm a belt-and-braces kind of person.

But from this discussion (and the post that lead me here today) I conclude there are people out there who are not taking that extra precaution and are then surprised that files have 'disappeared'. Disappearing files - even if in reality its merely the impression to some users that files have disappeared (even though they should have know that their actions would have this result), is just about the worst thing that can happen with a data storage solution. So something to be avoided at all cost, I'd think.

So more importantly than my specific way of using the client of my data management routines in general, is that forcing users to choose between deleting the local files or keeping them, is a way of hammering home the point that local files will be deleted. And if they're not yet on the server, that means they're gone for good. That's basically why I thought it'd be a good idea to introduce that additional option. Sometimes it's useful to force people to make a choice. Yes, people should know that already, but when you have people asking whether this will be 'fixed' in future releases, you can conclude that this isn't behaviour that is intuitively understood by all users. So you can be a purist about it and say that people should have read the warnings and read the manual and if they don't, whatever happens is their own fault, or you can nudge them in the right direction. So my suggestion was a way of giving those people that extra nudge they need.

Plus, in my usage case, it'd save me that extra precautionary step.

dirdi commented 3 years ago

Will a fix be included in the next nextcloud release? Or is there something planned for a future release?

Since there has not been reached a consensus about how the current behavior should be altered, this is very unlikely.


That said, I would like to suggest an alternative to the proposals that have been made so far: Add a third option to move the folder out of the sync folder.

If the users choose this option, the client would fire up a "choose directory" dialog and then move the not-yet-synced-folder out of the synced folder to the folder chosen by the client.

irisv commented 3 years ago

That said, I would like to suggest an alternative to the proposals that have been made so far: Add a third option to move the folder out of the sync folder.

If the users choose this option, the client would fire up a "choose directory" dialog and then move the not-yet-synced-folder out of the synced folder to the folder chosen by the client.

That'd also work for me. That's basically what I do now manually.

robballan commented 3 years ago

I’d add a 4th option that seems more intuitive to me: sync anything put in a Nextcloud folder BEFORE deleting the local copy that is unchecked for syncing. That way the cloud copy exists but the local copy does not.

For those who wish to keep a static local copy of some or all of the files — they shouldn’t be in Nextcloud folder to start with.

sadr0b0t commented 3 years ago

Maybe it should just forbid unchecking folders that are not yet synced to server with some kind of explanation why this checkbox is grayed out in disabled state and suggestion to move this folder out of sync folder manually in file manager.

The warning about removing synced folder from local file system is NOT the same as removing non-synced folder from local file system. In the 1-st case deleting data is safe. And in the 2-nd case deleting data is NOT safe. I think that removing local unsynced data is not a valid option, application should not do it in any case and should not suggest it. If user really wants to lose his data, he can just remove it in file manager manually.

robballan commented 3 years ago

You should NOT be able to uncheck a folder for syncing UNTIL it has been synced upstream. That would solve it.

On Nov 25, 2020, at 9:13 AM, Anton notifications@github.com wrote:

Maybe it should just forbid unchecking folders that are not yet synced to server with some kind of explanation why this checkbox is grayed out in disabled state and suggestion to move this folder out of sync folder manually in file manager.

The warning about removing synced folder from local file system is NOT the same as removing non-synced folder from local file system. In the 1-st case deleting data is safe. And in the 2-nd case deleting data is NOT safe. I think that removing local unsynced data is not a valid option, application should not do it in any case and should not suggest it. If user really wants to lose his data, he can just remove it in file manager manually.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.


raa

wsw70 commented 3 years ago

You should NOT be able to uncheck a folder for syncing UNTIL it has been synced upstream. That would solve it.

@b12f gives a very good case where this would not work: the initial setup of Nextcloud. The sync starts, you realize that there is a 200 GB subfolder you do not want to synchronize, uncheck it and the data is gone.

Not removing unsynced data is a solution that is safer.

sadr0b0t commented 3 years ago

The sync starts, you realize that there is a 200 GB subfolder you do not want to synchronize, uncheck it and the data is gone.

You can put sync on pause, go to file manager, move this folder somewhere else, continue to sync.

b12f commented 3 years ago

You can put sync on pause, go to file manager, move this folder somewhere else, continue to sync.

That's like telling someone they should reboot their pc if nextcloud crashes instead of fixing the cause of the crash. This is at its core a UX problem, and the solution you're offering is still hugely bad UX, especially because it would still mean people lose valuable data if they think they can change syncing options in the nextcloud UI, and then don't understand the current warning (which a lot of people don't)

sadr0b0t commented 3 years ago

especially because it would still mean people lose valuable data if they think they can change syncing options in the nextcloud UI, and then don't understand the current warning (which a lot of people don't)

My suggestion is to forbid removing unsynced data from nextcloud ui. The user should not be able to uncheck folder which not synced.

wsw70 commented 3 years ago

You can put sync on pause, go to file manager, move this folder somewhere else, continue to sync.

I install Nextcloud, start the sync, realize that what is being synced is not what I want to sync, uncheck - read the warning that states that the files in the unchecked folder will be removed and not synced anymore - but hey, they are not synced yet, because I just started the first sync after installation.

Crap, they managed to get synced finally... Good thing I have a backup /s

What I am trying to say is that once the sync is done then maybe the warning is fine (I could live with that, the idea being that I d not want anymore these files to be in Nextcloud) - but not at the install stage while the sync is going on.

As for the prompt itself, it should leave the choice for the files to be removed or stay as they are (not synced anymore, with some consequences down the road such as diffs between files if they are re-synced.)

wsw70 commented 3 years ago

My suggestion is to forbid removing unsynced data from nextcloud ui. The user should not be able to uncheck folder which not synced.

This is an interesting suggestion - fair point. I just wonder how it could work for selective sync of files within a folder

irisv commented 3 years ago

You can put sync on pause, go to file manager, move this folder somewhere else, continue to sync.

You're imagining a user with perfect knowledge like yourself. But this usage case involves people who have just installed Nextcloud and added a folder they didn't mean to add. The UI should also be safe to use for them.

My suggestion is to forbid removing unsynced data from nextcloud ui. The user should not be able to uncheck folder which not synced.

That would solve the issue of lost data, but if if I understand you correctly then in this scenario where a user unwittingly added a 200 GB subfolder they do not want to synchronize, that would involve uploading that 200 GB subfolder first, then the user would uncheck the box, not understanding the full implications of the warning, resulting in that 200 GB of local data being deleted. Then I imagine that user has a minor panic attack, until they realise the data is safe on the server. But then they have to download that 200 GB again and remove it from the server, just to get back to where they started. So that's 400 GB of unnecessary and unwanted traffic and one very irritated new user.

sadr0b0t commented 3 years ago

I just wonder how it could work for selective sync of files within a folder

NextCloud just does have partial syncing feature as far as I know. Unchecked folder in NextCloud UI just means that this folder exists on server but the client does not want to download it as local copy. It is not about partial syncing. Current UI allows to lose data by accident and I also think this is a bug.

For having partial sync for some data in enabled folders - I think this could be discussed as a separate feature (for example, this can be implemented as a king of .gitignore file in git).

sadr0b0t commented 3 years ago

then the user would uncheck the box, not understanding the full implications of the warning, resulting in that 200 GB of local data being deleted

No, the user should not be able to uncheck the box for the folder which is not synced - it should be drawn in disabled state with some kind of (optional) explanation why it is disabled.

sojer commented 3 years ago

image

Isn't it possible to change this dialog in such a way?

irisv commented 3 years ago

then the user would uncheck the box, not understanding the full implications of the warning, resulting in that 200 GB of local data being deleted

No, the user should not be able to uncheck the box for the folder which is not synced - it should be drawn in disabled state with some kind of (optional) explanation why it is disabled.

But after the folder has synced, the box would be enabled again, right? You're proposing unsynced folders have the checkbox disabled? So you have to sync first, before you can remove it? That's what I meant by uploading that 200 GB first:

in this scenario where a user unwittingly added a 200 GB subfolder they do not want to synchronize, that would involve uploading that 200 GB subfolder first [ie, completing the sync, then the disabled checkbox would become active again once completed] then the user would uncheck the box, not understanding the full implications of the warning, resulting in that 200 GB of local data being deleted. Then I imagine that user has a minor panic attack, until they realise the data is safe on the server...

So in this usage scenario, the user is unticking the box AFTER completing the sync. Sorry if that wasn't clear.

irisv commented 3 years ago

image

Isn't it possible to change this dialog in such a way?

* "Disable sync but untouch locally"
  Bevor the action starts, this folder/file is automatically added to the ignored files

* "Disable sync and remove locally"
  Proceed as currently

* "Cancel"
  Just stop

That's exactly what both b12f and I have suggested above. I think that for the new user, that's the most easy to understand and thus most foolproof.

sadr0b0t commented 3 years ago

So you have to sync first, before you can remove it? That's what I meant by uploading that 200 GB first:

You can either upload 200GB first or put upload on pause and move unsynced 200Gb folder in file manager somewhere else from sync dir location.

robballan commented 3 years ago

A. At some point the user has to be responsible for erroneous decisions. Being unable to stop a large sync does not lose data, it is merely highly inconvenient. Losing a local folder that is unsynced is potentionally far worse.

On Nov 26, 2020, at 5:08 AM, WoJ notifications@github.com wrote:

You should NOT be able to uncheck a folder for syncing UNTIL it has been synced upstream. That would solve it.

@b12f gives a very good case where this would not work: the initial setup of Nextcloud. The sync starts, you realize that there is a 200 GB subfolder you do not want to synchronize, uncheck it and the data is gone.

Not removing unsynced data is a solution that is safer.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.


raa

Forza-tng commented 3 years ago

image

Isn't it possible to change this dialog in such a way?

  • "Disable sync but untouch locally" Bevor the action starts, this folder/file is automatically added to the ignored files
  • "Disable sync and remove locally" Proceed as currently
  • "Cancel" Just stop

I think one of the main - perhaps the major - point is that if the user had put data in the local folder to be synced (uploaded) to the server and that upload is not yet finished. It is very easy to assume that "fine, I don't need my local data as it is already on the server", when in fact it isn't true.

The disable sync dialog should show that it is currently uploading local changes (and that there are un-synced local changes) and warn accordingly.

vegatron commented 3 years ago

WHY!!! So I just lost my data due to this terrible implementation! My folder was unsynced and I lost my data! I wanted to sync it later, I was moving new folders to it, it had many many files, so I unchecked it to sync it later as not to slow down my connection, and now I can't recover my file!!

who thought this was a good idea!! I'm really considering moving back to google drive, you just don't delete local data!

b12f commented 3 years ago

And this "feature" takes another victim.

At some point the user has to be responsible for erroneous decisions.

I just don't think this is ever true. Bad design exists, and this is a perfect example of it. The huge discussions both in the owncloud repo and here report multiple failure cases with permanent dataloss and it still feels to me like it is not being taken seriously. Even the "Delete this file" menu items on most OS's put the file in a recycle bin. You don't just permanently delete data after the user presses a grey button on a grey background that says "Apply" with a grey warning text next to it that "they should've just read".

Unfortunately, I've zero experience writing c++ code, otherwise I'd love to chip in and help. Not trying to be hostile, but I do want to stress the severity of this bug.

robballan commented 3 years ago

We are agreeing. I made that statement in the context that there is no system that can ever be perfectly designed to prevent users from making mistakes. However, there should be no system in which the design leads the user to make a mistake. And this is one where that happens.

It seems that what nextcloud’s developers are assuming is that you will only uncheck the need for a local copy after the data has already been synced up into the cloud server. But they don’t actually enforce this in the implementation, which catches the user by surprise. It appears that you can uncheck this requirement and have the local data erased before it is copied to the cloud server, which makes no sense from anyone’s perspective.

The fix should certainly be that no local data is ever erased until it has been synced upstream first. It should be obvious that when a user deselects the need for a local copy, it’s precisely because he assumes the remote copy exists. It’s clear the developers assume this is true because they leave the checkbox available to be rechecked later, which would then require the remote data to be copied back to the local store. The error is that the sync engine fails to verify the existence of the remote copy before deleting the local copy.

— raa

On Dec 24, 2020, at 9:46 PM, Benjamin Bädorf notifications@github.com wrote:

 And this "feature" takes another victim.

At some point the user has to be responsible for erroneous decisions.

I just don't think this is ever true. Bad design exists, and this is a perfect example of it. The huge discussions both in the owncloud repo and here report multiple failure cases with permanent dataloss and it still feels to me like it is not being taken seriously. Even the "Delete this file" menu items on most OS's put the file in a recycle bin. You don't just permanently delete data after the user presses a grey button on a grey background that says "Apply" with a grey warning text next to it that "they should've just read".

Unfortunately, I've zero experience writing c++ code, otherwise I'd love to chip in and help. Not trying to be hostile, but I do want to stress the severity of this bug.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

vegatron commented 3 years ago

It doesn't make sense for NextCloud app to delete local unsynched files. I can understand that it might deleted synched files, which could be an inconvience to get them back, but to permenantly delete local unsynched files is a disaster.

rasos commented 3 years ago

A client lost a local folder, which was in the sync tree and has been unchecked. The folder was not deleted immediately, but 4 months later after a total re-scan that was triggered after a server migration. So the client assumed, that the folder inside the sync tree does not get deleted and he could manually switch on and off server syncing.

The only reason that the unattended could happen according to the discussion here would be that a folder is being moved into the sync folder and then stopped from syncing, before it has been uploaded yet. But why did it remain in the sync folder for so long "undiscovered" as a deletion candidate?

Petermhen commented 2 years ago

WHY!!! So I just lost my data due to this terrible implementation! My folder was unsynced and I lost my data! I wanted to sync it later, I was moving new folders to it, it had many many files, so I unchecked it to sync it later as not to slow down my connection, and now I can't recover my file!!

who thought this was a good idea!! I'm really considering moving back to google drive, you just don't delete local data!

It is honestly unbelievable that there are people attempting to rationalise how this is anything but an absolute deal breaking flaw.

The bare minimum expectation of any program is for it to not unexpectedly permanently delete your possibly invaluable data. A program that breaks this rule can within justification be classed as malware by the affected user. The mind boggles.

github-actions[bot] commented 2 years ago

This bug report did not receive an update in the last 4 weeks. Please take a look again and update the issue with new details, otherwise the issue will be automatically closed in 2 weeks. Thank you!

b12f commented 2 years ago

Bump

vegatron commented 2 years ago

Bump

irisv commented 2 years ago

I've got another complaint about the client that is somewhat related, in that the client deletes files I didn't want deleted (that's why I'm posting it here rather than starting a new topic): After using an outdated version of the Nextcloud database, the client deleted all the local files that weren't yet in that database. The behaviour that I would have wanted, and that I think most users would want, is that if the clients finds a local file that isn't yet in Nextcloud, it should add the file to the database rather than remove the file from the local client folder. Or at the very least, ask!

Here's what happened: I'm running NextcloudPi on a Raspberry Pi, with the data folder on a USB harddrive attached to the Pi. It also runs some other software, and as I was mucking around with some of that, I made a image of the SD card (which includes all of Nextcloud, except the data folder), to copy on an alternative SD card so I didn't run the risk of messing with my live SD. After a first failed attempt of messing with that other, unrelated software, I had another bright spark and wanted to try something new. So I again made a fresh copy of the SD using that backup image (now about a week old), and booted up my Raspberry Pi with it. However, I forgot I still had the Nextcloud client running on my laptop. The client quietly and diligently set to work deleting all the local files that I had created since making that backup image. This wasn't at all what I was expecting: I was expecting to have to run the occ files:scan command to force it to find the new files. Luckily, it didn't delete the files from the Nextcloud data folder (after all, it didn't know it had those), but boy, did this give me a fright!

Now of course it was my own stupid fault for not exiting the client before starting Nextcloud with an outdated database, and in this case, no damage was done (besides raising my heartrate for a bit). But in a scenario where someone is restoring Nextcloud after some catastrophic event, when they more likely than not are restoring with an outdated database, the last thing you want the client to do, is delete local files!

Conclusion: an "always ask before deleting local files" option in the client would be a great step forward, for this case as well as the problem this thread is devoted to.

github-actions[bot] commented 2 years ago

This bug report did not receive an update in the last 4 weeks. Please take a look again and update the issue with new details, otherwise the issue will be automatically closed in 2 weeks. Thank you!

b12f commented 2 years ago

Bumping again

github-actions[bot] commented 2 years ago

This bug report did not receive an update in the last 4 weeks. Please take a look again and update the issue with new details, otherwise the issue will be automatically closed in 2 weeks. Thank you!

vegatron commented 2 years ago

I'm still struggling with PTSD over the loss of my data

dawnmy commented 2 years ago

I am also a victim of this bug