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

Tweak counsel-find-file-delete #1779

Closed
wants to merge 4 commits into from

Conversation

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Oct 10, 2018

Please review each commit in turn.

Re: #1774
Cc: @ChoppinBlockParty

basil-conto added 4 commits Oct 10, 2018
Add trailing space to y-or-n-p prompt as per the function's
docstring.
(counsel-find-file-delete): Don't prompt for directory deletion,
which dired-delete-file already handles in a customizable way.
(counsel--yes-or-no-p): New function.
(counsel-find-file-delete): Use it instead of hard-coding short
y-or-n-p prompt for destructive operations like file deletions.
(counsel-find-file-delete):
Maybe delete buffers visiting recently deleted file.

Re: abo-abo#1774
@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Oct 11, 2018

I ran into the following dired-delete-file quirk:

  1. make plain
  2. C-xd//tmpRET
  3. +fooRET
  4. C-xC-ffooC-jbarRET
  5. C-oC-xC-s
  6. C-xC-fDELfoo
  7. M-ok
    • Expected Dired prompt: "Recursively delete /tmp/foo/? (yes, no, all, quit, help)"
    • Actual Dired prompt: "Recursively delete ? (yes, no, all, quit, help)"

This is due to the implementation of dired-make-relative.

One option is to hot-swap it with file-relative-name:

diff --git a/counsel.el b/counsel.el
index bfa63ac..2fd7089 100644
--- a/counsel.el
+++ b/counsel.el
@@ -1742,7 +1742,8 @@ counsel-find-file-delete
             ;; `dired-delete-file', which see, already prompts for directories
             (eq t (car (file-attributes x)))
             (counsel--yes-or-no-p "Delete %s? " x))
-    (dired-delete-file x dired-recursive-deletes delete-by-moving-to-trash)
+    (cl-letf (((symbol-function 'dired-make-relative) #'file-relative-name))
+      (dired-delete-file x dired-recursive-deletes delete-by-moving-to-trash))
     (dired-clean-up-after-deletion x)
     (ivy--reset-state ivy-last)))

So long as you don't mind /tmp/foo/ (the default-directory) being abbreviated as ./ and /tmp/ (the parent directory) being abbreviated as ../.

Another option is to pass the directory-file-name of the file/directory being deleted to dired-delete-file:

diff --git a/counsel.el b/counsel.el
index bfa63ac..9c55f8a 100644
--- a/counsel.el
+++ b/counsel.el
@@ -1742,7 +1742,9 @@ counsel-find-file-delete
             ;; `dired-delete-file', which see, already prompts for directories
             (eq t (car (file-attributes x)))
             (counsel--yes-or-no-p "Delete %s? " x))
-    (dired-delete-file x dired-recursive-deletes delete-by-moving-to-trash)
+    (dired-delete-file (directory-file-name x)
+                       dired-recursive-deletes
+                       delete-by-moving-to-trash)
     (dired-clean-up-after-deletion x)
     (ivy--reset-state ivy-last)))

So long as you don't mind the missing trailing slash of directory names.

Yet another option is to leave things as they are, and hope the empty file name will make the user think twice about deleting the directory containing the file being visited by the current buffer.

Thoughts, @abo-abo, @ChoppinBlockParty, @DamienCassou?

@ChoppinBlockParty
Copy link

@ChoppinBlockParty ChoppinBlockParty commented Oct 11, 2018

Good catch. I am wondering will (expand-file-name x) change the prompt?

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Oct 11, 2018

will (expand-file-name x) change the prompt?

No, dired-make-relative already receives an absolute file name.

@ChoppinBlockParty
Copy link

@ChoppinBlockParty ChoppinBlockParty commented Oct 11, 2018

Yes, sure, sorry for that dumb question. Thant looks like weird behavior, may be dired functions do not fit for ivy? Because I do not think that I can reproduce this with dired. May be we should use default functions like delete-file delete-directory and if for some reason use does not like this he can write his own more elaborated delete action?

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Oct 11, 2018

Thant looks like weird behavior

Yes, but don't forget that it's an unlikely edge-case, not the usual behaviour, and that there are workarounds, as I mentioned earlier.

may be dired functions do not fit for ivy?

@DamienCassou argued in #1671 that dired-delete-file saves us having to reimplement directory deletion, which is a fair point. The source comments before dired-delete-file further encourage use of dired-delete-file outside of Dired. Besides, The quirk I described earlier can always be reported and fixed upstream, so that Counsel does not have to care about it forever.

May be we should use default functions like delete-file delete-directory

Feel free to implement this if you're interested. But I don't think it's possible to do so as flexibly as dired-delete-file without duplicating the whole thing, which I would not agree with.

if for some reason use does not like this he can write his own more elaborated delete action?

Yes, this is universally true in Emacs.

@ChoppinBlockParty
Copy link

@ChoppinBlockParty ChoppinBlockParty commented Oct 11, 2018

I actually do not understand the value what it is doing, except calling delete-file on file and delete-directory on directory. From #1671 "features such as rename of file-visiting buffers" did not find such feature in dired-delete-file; recursive deletion prompt - does it make sense to have it at all, sound to me like a duplicate confirmation of "Do you want to delete this directory?", because if you says "yes, I want to delete directory", and then you says "no, I do not want to delete recursively", why did you say "yes" at the first point?

@ChoppinBlockParty
Copy link

@ChoppinBlockParty ChoppinBlockParty commented Oct 16, 2018

@basil-conto I do not want to be disturbing, it was just an opinion, please, feel free to merge your work, I find it very helpful and looking forward to try it out.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Oct 16, 2018

@ChoppinBlockParty No, I think there's truth to what you're saying, I just haven't had the time to look at this issue again; sorry for the delay.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Oct 26, 2018

Thanks, all.

@basil-conto basil-conto deleted the blc/delete-file branch Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants