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

Feature: Show confirmation dialog when the undo operation is to delete items. #11185

Merged
merged 10 commits into from
Feb 7, 2023

Conversation

hishitetsu
Copy link
Member

@hishitetsu hishitetsu commented Feb 6, 2023

Resolved / Related Issues
Items resolved / related issues by this PR.

Details
A confirmation dialog will appear when the undo operation is to delete items to prevent unexpected deletion.

TODO

  • The setting should be separate from "Show confirmation dialog when deleting items". No new settings this time.
  • When cancelling the undo operation, the history is erased even though it is not actually undone. Is that right? keep the history.
  • On the confirmation dialog, we can choose to put it in the trash instead of deleting it. Is that right? It's ok.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@hishitetsu hishitetsu changed the title WIP Feature: Show confirmation dialog when the undo operation is to delete items. Feb 6, 2023
@yaira2
Copy link
Member

yaira2 commented Feb 6, 2023

I'm still a little confused as to why ctrl + z is deleting the folder, we should really match File Explorers behavior in that case and skip cutting the folder.

On the confirmation dialog, we can choose to put it in the trash instead of deleting it. Is that right?

Can we show the regular confirm delete dialog? This already has the logic and indicates that it'll be permanently deleted.

The setting should be separate from "Show confirmation dialog when deleting items".

Perhaps a new setting for "Show confirmation dialog when permanently deleting items". I think this was already requested so might be a good thing to do.

@hishitetsu
Copy link
Member Author

hishitetsu commented Feb 6, 2023

I'm still a little confused as to why ctrl + z is deleting the folder

In Files, creating a new folder like executing the following command.
MKDIR AnyFolderName
But in Explorer, like this.
MKDIR "New folder";RENAME "New folder" AnyFolderName
So ctrl+z behaves delete in Files and rename back in Explorer.
If we don't rename the new folder in Explorer, ctrl+z behaves delete like Files.

Can we show the regular confirm delete dialog? This already has the logic and indicates that it'll be permanently deleted.

Yes, this PR will do so.
But I am concerned that in this dialog I can choose to put it in the trash instead of permanently deleting it.

Perhaps a new setting for "Show confirmation dialog when permanently deleting items".

I think we should add a settings for "Show confirmation dialog when the undo operation is to delete items" as the title of this PR.
"Show confirmation dialog when permanently deleting items" might be a good idea, but I think there should be separate settings for normal delete and ctrl+z delete.

@yaira2
Copy link
Member

yaira2 commented Feb 7, 2023

In Files, creating a new folder like executing the following command.
MKDIR AnyFolderName
But in Explorer, like this.
MKDIR "New folder";RENAME "New folder" AnyFolderName
So ctrl+z behaves delete in Files and rename back in Explorer.
If we don't rename the new folder in Explorer, ctrl+z behaves delete like Files.

Would it be desirable to copy this behavior so that it's consistent?

Yes, this PR will do so.
But I am concerned that in this dialog I can choose to put it in the trash instead of permanently deleting it.

Unchecking the "permanently" delete will send to trash, does this work?

I think we should add a settings for "Show confirmation dialog when the undo operation is to delete items" as the title of this PR.
"Show confirmation dialog when permanently deleting items" might be a good idea, but I think there should be separate settings for normal delete and ctrl+z delete.

My concern is that it's confusing to explain what this does.

@hishitetsu
Copy link
Member Author

Would it be desirable to copy this behavior so that it's consistent?

I don't think so. I think the folder rather should be deleted, not rename to "New folder" when undoing. Instead, it would be better to show a confirmation dialog if necessary.

Unchecking the "permanently" delete will send to trash, does this work?

Yes.

My concern is that it's confusing to explain what this does.

How about adding the following sub-settings?

Show confirmation dialog when deleting items - On or Off
-> Also when the undo operation is to delete items - On or Off

@yaira2
Copy link
Member

yaira2 commented Feb 7, 2023

Show confirmation dialog when deleting items - On or Off
-> Also when the undo operation is to delete items - On or Off

I think it's still confusing, I think we should release the change without a setting and if users request an option to turn it off, we can reevaluate the behavior.

@hishitetsu
Copy link
Member Author

Ok, the rest is a matter of the operation history.
In this PR, the history is deleted regardless of whether or not the user canceled. If the user cancels, should the history remain?
If the history remains, it may be more consistent but the user can't undo further past operations.

@yaira2
Copy link
Member

yaira2 commented Feb 7, 2023

Which one is safer?

@hishitetsu
Copy link
Member Author

I think it is safer to keep a history since the user may think that the history remains if they cancel.

@hishitetsu hishitetsu marked this pull request as ready for review February 7, 2023 02:30
@hishitetsu
Copy link
Member Author

I committed a new code to keep the history. So I think it is ready to merge.

@hishitetsu
Copy link
Member Author

Sorry, this code may not be good. I will try to fix it immediately.

@hishitetsu
Copy link
Member Author

This time it's okay.

@yaira2
Copy link
Member

yaira2 commented Feb 7, 2023

It looks like the issue with ctrl + z was fixed but I'm not seeing a confirmation prompt

@hishitetsu
Copy link
Member Author

Really? When creating a folder and performing ctrl+z, I see the dialog.
image

@yaira2
Copy link
Member

yaira2 commented Feb 7, 2023

I rebuilt the branch and it's working now, LGTM

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Feb 7, 2023
@yaira2 yaira2 merged commit d9cc70c into files-community:main Feb 7, 2023
@hishitetsu hishitetsu deleted the WarnUndo branch February 7, 2023 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Cut and paste into the source folder causes source folder to be deleted
2 participants