[Bug] A gap between `dirvish` and `dired-do-redisplay`.

jcguu95 commented 1 year ago

Thank you for the bug report

[X] I am using the latest version of dirvish related packages.
[X] I checked CUSTOMIZING|EXTENSIONS.
[ ] You may also try reproduce the issue using clean environment and minimal configurations with the command emacs -Q.

Bug description

I have a customize function to delete marked files at point. After that's done successfully, most of the time the entries do not get removed from dired immediately. The correct function to reload/redisplay the dired buffer seems to be dired-do-redisplay. However, when run that function right after deleting something (in this example, /tmp/virtual-machine.pdf), I get the error as follows.

Debugger entered--Lisp error: (file-missing "Reading directory" "No such file or directory" "virtual-machine.pdf")
  access-file("virtual-machine.pdf" "Reading directory")
  #f(compiled-function (file switches &optional wildcard full-directory-p) "Insert directory listing for FILE, formatted according to SWITCHES.\nLeaves point after the inserted text.\nSWITCHES may be a string of options, or a list of strings\nrepresenting individual options.\nOptional third arg WILDCARD means treat FILE as shell wildcard.\nOptional fourth arg FULL-DIRECTORY-P means file is a directory and\nswitches do not contain `d', so that a full listing is expected.\n\nThis works by running a directory listing program\nwhose name is in the variable `insert-directory-program'.\nIf WILDCARD, it also runs the shell specified by `shell-file-name'.\n\nWhen SWITCHES contains the long `--dired' option, this function\ntreats it specially, for the sake of dired.  However, the\nnormally equivalent short `-D' option is just passed on to\n`insert-directory-program', as any other option." #<bytecode -0x2a95334c96e6b59>)("virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil)
  ls-lisp--insert-directory(#f(compiled-function (file switches &optional wildcard full-directory-p) "Insert directory listing for FILE, formatted according to SWITCHES.\nLeaves point after the inserted text.\nSWITCHES may be a string of options, or a list of strings\nrepresenting individual options.\nOptional third arg WILDCARD means treat FILE as shell wildcard.\nOptional fourth arg FULL-DIRECTORY-P means file is a directory and\nswitches do not contain `d', so that a full listing is expected.\n\nThis works by running a directory listing program\nwhose name is in the variable `insert-directory-program'.\nIf WILDCARD, it also runs the shell specified by `shell-file-name'.\n\nWhen SWITCHES contains the long `--dired' option, this function\ntreats it specially, for the sake of dired.  However, the\nnormally equivalent short `-D' option is just passed on to\n`insert-directory-program', as any other option." #<bytecode -0x2a95334c96e6b59>) "virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil)
  apply(ls-lisp--insert-directory #f(compiled-function (file switches &optional wildcard full-directory-p) "Insert directory listing for FILE, formatted according to SWITCHES.\nLeaves point after the inserted text.\nSWITCHES may be a string of options, or a list of strings\nrepresenting individual options.\nOptional third arg WILDCARD means treat FILE as shell wildcard.\nOptional fourth arg FULL-DIRECTORY-P means file is a directory and\nswitches do not contain `d', so that a full listing is expected.\n\nThis works by running a directory listing program\nwhose name is in the variable `insert-directory-program'.\nIf WILDCARD, it also runs the shell specified by `shell-file-name'.\n\nWhen SWITCHES contains the long `--dired' option, this function\ntreats it specially, for the sake of dired.  However, the\nnormally equivalent short `-D' option is just passed on to\n`insert-directory-program', as any other option." #<bytecode -0x2a95334c96e6b59>) ("virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil))
  insert-directory("virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil)
  dired-insert-directory("/tmp/" "-Ghlv --group-directories-first --time-style=long-..." ("virtual-machine.pdf"))
  dired-add-entry("/tmp/virtual-machine.pdf" nil t)
  dired-update-file-line("/tmp/virtual-machine.pdf")
  dired-do-redisplay(nil 1)
  funcall-interactively(dired-do-redisplay nil 1)
  call-interactively(dired-do-redisplay nil nil)
  command-execute(dired-do-redisplay)

Steps to reproduce

Define my own file trashing function.

  (defun dired/async/trash-marked-files ()
    (interactive)
    (cl-flet
        ((async/trash-files
          (files)
          (let ((cmd (concat "trash-put"
                             " "
                             (mapconcat (lambda (x)
                                          (format "'%s'" x))
                                        files
                                        " "))))
            (async-shell-command cmd))))
      (dired/funcall-on-marked-files #'async/trash-files)))

In dired, mark a file, and run the customized command above dired/async-trash-marked-files.
Right after the command succeed, run dired-do-redisplay.

Expected behavior

It's better to have the entry removed once its corresponding file is moved away (ranger does that).

When we called dired-do-redisplay, the entry should be away, and there shouldn't be any error.

OS

MacOS

Emacs Version

28

Emacs Configurations

doomemacs

Error callstack

Debugger entered--Lisp error: (file-missing "Reading directory" "No such file or directory" "virtual-machine.pdf")
  access-file("virtual-machine.pdf" "Reading directory")
  #f(compiled-function (file switches &optional wildcard full-directory-p) "Insert directory listing for FILE, formatted according to SWITCHES.\nLeaves point after the inserted text.\nSWITCHES may be a string of options, or a list of strings\nrepresenting individual options.\nOptional third arg WILDCARD means treat FILE as shell wildcard.\nOptional fourth arg FULL-DIRECTORY-P means file is a directory and\nswitches do not contain `d', so that a full listing is expected.\n\nThis works by running a directory listing program\nwhose name is in the variable `insert-directory-program'.\nIf WILDCARD, it also runs the shell specified by `shell-file-name'.\n\nWhen SWITCHES contains the long `--dired' option, this function\ntreats it specially, for the sake of dired.  However, the\nnormally equivalent short `-D' option is just passed on to\n`insert-directory-program', as any other option." #<bytecode -0x2a95334c96e6b59>)("virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil)
  ls-lisp--insert-directory(#f(compiled-function (file switches &optional wildcard full-directory-p) "Insert directory listing for FILE, formatted according to SWITCHES.\nLeaves point after the inserted text.\nSWITCHES may be a string of options, or a list of strings\nrepresenting individual options.\nOptional third arg WILDCARD means treat FILE as shell wildcard.\nOptional fourth arg FULL-DIRECTORY-P means file is a directory and\nswitches do not contain `d', so that a full listing is expected.\n\nThis works by running a directory listing program\nwhose name is in the variable `insert-directory-program'.\nIf WILDCARD, it also runs the shell specified by `shell-file-name'.\n\nWhen SWITCHES contains the long `--dired' option, this function\ntreats it specially, for the sake of dired.  However, the\nnormally equivalent short `-D' option is just passed on to\n`insert-directory-program', as any other option." #<bytecode -0x2a95334c96e6b59>) "virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil)
  apply(ls-lisp--insert-directory #f(compiled-function (file switches &optional wildcard full-directory-p) "Insert directory listing for FILE, formatted according to SWITCHES.\nLeaves point after the inserted text.\nSWITCHES may be a string of options, or a list of strings\nrepresenting individual options.\nOptional third arg WILDCARD means treat FILE as shell wildcard.\nOptional fourth arg FULL-DIRECTORY-P means file is a directory and\nswitches do not contain `d', so that a full listing is expected.\n\nThis works by running a directory listing program\nwhose name is in the variable `insert-directory-program'.\nIf WILDCARD, it also runs the shell specified by `shell-file-name'.\n\nWhen SWITCHES contains the long `--dired' option, this function\ntreats it specially, for the sake of dired.  However, the\nnormally equivalent short `-D' option is just passed on to\n`insert-directory-program', as any other option." #<bytecode -0x2a95334c96e6b59>) ("virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil))
  insert-directory("virtual-machine.pdf" "--dired -Ghlv --group-directories-first --time-sty..." nil nil)
  dired-insert-directory("/tmp/" "-Ghlv --group-directories-first --time-style=long-..." ("virtual-machine.pdf"))
  dired-add-entry("/tmp/virtual-machine.pdf" nil t)
  dired-update-file-line("/tmp/virtual-machine.pdf")
  dired-do-redisplay(nil 1)
  funcall-interactively(dired-do-redisplay nil 1)
  call-interactively(dired-do-redisplay nil nil)
  command-execute(dired-do-redisplay)



### Anything else

_No response_

aikrahguzar commented 1 year ago

I think the problem is with the async nature of the function you wrote. Most likely what happens is that you call dired-do-redisplay before the file is deleted. So the file is included in the directory listing. However it get deleted before the file is accessed to get some extra information about it.

If you want to use such an async function, the proper way would be to use make-process with a sentinel that call dired-do-redisplay when the process actually finishes.

If the problem is not async command, you might need to revert buffer after deletion instead of dired-do-redisplay.

jcguu95 commented 1 year ago

Thanks for your hint. I need to use sentinel and #'revert-buffer indeed.

For those who are curious, here's the code I use at the end.

  (defun dired/funcall-on-marked-files (fn)
    (let ((files (dired-get-marked-files
                  t current-prefix-arg nil nil t)))
      (funcall fn files)))

  (defun dired/async/trash-marked-files ()
    "Interactively trash-put dired marked files."
    (interactive)
    (cl-flet ((async/trash-files
               (files)
               (let ((command `("trash-put"
                                ,@(mapcar (lambda (x)
                                            (format "%s" x))
                                          files))))
                 (message (format "Running command: %S" command))
                 ;; more sophisticated than #'async-shell-command
                 (make-process
                  :name     "dired/async-trash-marked-files"
                  :command  command
                  :buffer   (get-buffer "*Messages*")
                  :sentinel (lambda (proc type)
                              (declare (ignore (proc type)))
                              (revert-buffer))))))
      (dired/funcall-on-marked-files #'async/trash-files)))

aikrahguzar commented 1 year ago

:sentinel (lambda (proc type)
                              (declare (ignore (proc type)))
                              (revert-buffer))))))

This is a bit dangerous for long running async commands since if you switch buffer for whatever reason before the command finishes, wrong buffer will be reverted. It is better to let bind the current buffer at the start and then do,

(with-current-buffer buf (revert-buffer))

Where buf is the let bound symbol.

jcguu95 commented 1 year ago

@aikrahguzar I see what you mean. But it seems that the sentinel has its own environment.

For example, the following code invokes an error when the sentinel is activated, as it still doesn't know what buf means.

  (defun dired/async/arXiv-helper-on-marked-files ()
    "Interactively apply arXiv helper on dired marked files."
    (interactive)
    (cl-flet ((async/arXiv-helper-on-files
               (files)
               (let ((buf (current-buffer))
                     (command `("arXiv-helper" "-im"
                                ,@(mapcar (lambda (x)
                                            (format "%s" x))
                                          files))))
                 (message (format "Running command: %S" command))
                 ;; more sophisticated than #'async-shell-command
                 (make-process
                  :name     "dired/async/arXiv-helper-on-marked-files"
                  :command  command
                  :buffer   (get-buffer "*Messages*")
                  :sentinel (lambda (proc type)
                              (declare (ignore (proc type)))
                              (with-current-buffer buf
                                (revert-buffer)))))))
      (dired/funcall-on-marked-files #'async/arXiv-helper-on-files)))

aikrahguzar commented 1 year ago

@aikrahguzar I see what you mean. But it seems that the sentinel has its own environment.

For example, the following code invokes an error when the sentinel is activated, as it still doesn't know what buf means.

The issue is probably that you don't have lexical binding in the buffer where you are defining these. Adding

;; -*- lexical-binding: t; -*-

at the top of the file and reverting it should fix this.

alexluigit / dirvish