Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stop file actions when conflict dialog skipped #46354

Conversation

grnd-alt
Copy link
Member

@grnd-alt grnd-alt commented Jul 8, 2024

Summary

relies on: nextcloud-libraries/nextcloud-upload#1270

Checklist

@grnd-alt grnd-alt requested a review from skjnldsv as a code owner July 8, 2024 09:19
@grnd-alt grnd-alt linked an issue Jul 8, 2024 that may be closed by this pull request
4 tasks
@grnd-alt grnd-alt force-pushed the fix/45548-bug-duplicate-filefolder-names-delete-file-when-moving branch from 7da9782 to b80d19e Compare July 8, 2024 09:27
susnux

This comment was marked as outdated.

@skjnldsv
Copy link
Member

skjnldsv commented Jul 9, 2024

I see the point, but looking a the PR that introduced that issue: #43667,

If the conflict dialog returns two empty arrays, selected and renamed, that means the user chose to keep the file already in the directory path, thereby choosing to delete the current selected file they want to move

I actually don't understand this anymore.
Is this really the outcome we want, if someone didn't pick the old NOR the new, that means we want to delete the source file? I'm far from certain 🤔

@grnd-alt
Copy link
Member Author

grnd-alt commented Jul 9, 2024

I see the point, but looking a the PR that introduced that issue: #43667,

If the conflict dialog returns two empty arrays, selected and renamed, that means the user chose to keep the file already in the directory path, thereby choosing to delete the current selected file they want to move

I actually don't understand this anymore. Is this really the outcome we want, if someone didn't pick the old NOR the new, that means we want to delete the source file? I'm far from certain 🤔

@skjnldsv
I've wondered about that behavior as well. Having both arrays (selected and renamed) empty actually does not mean no file has been selected but that only the existing file has been selected (the one on the right). Without this PR, skipping files caused the same result as selecting only existing files, now we have different cases for each. But i don't know if it's expected behavior to delete files when it's selected to keep only the existing, or if that should mean the same as skipping(keeping both)

@grnd-alt grnd-alt self-assigned this Jul 10, 2024
@susnux
Copy link
Contributor

susnux commented Jul 10, 2024

From my understanding

N = Selected new ones
O = Selected old ones

Renamed = (n e N) in O
Selected = (n e N) not in O

So if you have upload (files to upload) and existing (all files that are already existing) and resolve using the conflict picker, then you have:

  • upload
  • existing
  • renamed (from the submit event)
  • selected (from the submit event)

So now we have two simple tasks

  1. Handle selected
    1. delete from existing if it is in selected
    2. upload (or move) all selected
  2. upload all renamed with their new name (or move with new name)

But now we also have all files not in renamed and not in selected, meaning those files where we selected to keep the old one.

  • For upload this is basically a "skip uploading file"
  • For moving I think it should also just cancel (skip) that file, this is how the file managers on Linux / Windows work.

So what do two empty arrays mean?
All old files where selected -> I would say we should simply skip the moving in those cases.

@susnux
Copy link
Contributor

susnux commented Jul 10, 2024

TLDR:

Files not in renamed and not inselected are marked for "use the old one" -> File managers on Linux / Windows will just cancel the move and (my opinion) we should also just cancel the move operation for those files.

@grnd-alt grnd-alt force-pushed the fix/45548-bug-duplicate-filefolder-names-delete-file-when-moving branch 2 times, most recently from f53f43f to 6ad500f Compare July 11, 2024 10:38
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Was also the suggested solution by designers.

@grnd-alt grnd-alt force-pushed the fix/45548-bug-duplicate-filefolder-names-delete-file-when-moving branch from 6ad500f to 3f78a7c Compare July 16, 2024 07:30
Signed-off-by: grnd-alt <salimbelakkaf@outlook.de>
@grnd-alt grnd-alt force-pushed the fix/45548-bug-duplicate-filefolder-names-delete-file-when-moving branch from 3f78a7c to 3ebfe95 Compare July 16, 2024 07:42
@grnd-alt grnd-alt enabled auto-merge July 16, 2024 07:58
@grnd-alt grnd-alt merged commit 170849b into master Jul 16, 2024
111 checks passed
@grnd-alt grnd-alt deleted the fix/45548-bug-duplicate-filefolder-names-delete-file-when-moving branch July 16, 2024 08:14
Copy link

welcome bot commented Jul 16, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@grnd-alt
Copy link
Member Author

/backport to stable28

@grnd-alt
Copy link
Member Author

/backport to stable29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: Duplicate File/folder names delete file when moving
3 participants