radian-software / prescient.el

☄️ Simple but effective sorting and filtering for Emacs.
MIT License
603 stars 25 forks source link

`vertico-directory` does not behave as expected when used alongside `vertico-prescient-mode` #155

Closed leungbk closed 8 months ago

leungbk commented 9 months ago

Repro:

  1. Load the following with emacs -q -l my-init.el
(let ((bootstrap-file (concat user-emacs-directory "straight/repos/straight.el/bootstrap.el"))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'use-package)
(setq straight-use-package-by-default t)

(use-package vertico
  :straight ( :host github
              :repo "minad/vertico"
              :files (:defaults "extensions/*")
              :includes (vertico-buffer
                         vertico-directory
                         vertico-flat
                         vertico-indexed
                         vertico-mouse
                         vertico-quick
                         vertico-repeat
                         vertico-reverse))
  :demand t
  :config
  (vertico-mode 1))

(use-package vertico-directory
  :bind
  ( :map vertico-map
    ("RET" . #'vertico-directory-enter)))

(use-package prescient
  :defer t
  :config
  (prescient-persist-mode 1))

(use-package vertico-prescient
  :after vertico
  :config
  (vertico-prescient-mode 1))
  1. mkdir foo
  2. C-x C-f foo RET RET to exit from find-file into a dired buffer.
  3. Within the dired buffer, press ^ to re-enter the parent directory.
  4. C-x C-f again.

After step 5, foo is not at the top of the completions list; however, when using vertico-directory without vertico-prescient-mode on, performing the same experiment leads to foo appearing at the top of the completions.

leungbk commented 9 months ago

cc @okamsn

okamsn commented 9 months ago

The problem seems to be these:

  1. We currently only advise vertico-insert, which vertico-exit uses to insert candidates onto the prompt line. We currently don't record anything if the user accepts what's on the prompt line (vertico--index = -1). I forget why we don't; maybe we should.
    • If the minibuffer contents are blank, can we get the default value?
  2. When vertico-directory-enter is run for a directory candidate that is not on the prompt line, it uses delete-minibuffer-contents and insert to put a potentially modified form of the candidate on the prompt line. We currently don't run any advice in that case.
okamsn commented 9 months ago

I believe that a simple solution would be if Vertico ran a special hook that received that relevant candidate in the public commands and that those public commands would run a simpler internal version without the hook. However, that complicates Vertico for the needs of (probably only) Prescient. I believe that @minad rejected this idea in the past.

@minad, do you have any thoughts on receiving the string inserted by vertico-directory-enter into the minibuffer? It's been a long while since I've worked with these things.

okamsn commented 8 months ago

I'm now of the opinion that a hook isn't needed. I think that a simpler solution would be if the user-facing commands were wrappers for the implementations, and that the user-facing commands only used the implementations, and not other user-facing commands. For example, vertico-exit and the directory command could both use a vertico--exit. That way, we could advise the user-facing commands without worrying whether the remembering advice is run twice, such as if, as it is today, we advised vertico-exit and vertico-insert (used itself by vertico-exit).

I will create a PR to see if this idea has any traction.

minad commented 8 months ago

@okamsn Would it be possible to use a post-command-hook/minibuffer-exit-hook in the minibuffer? It could run after all the relevant commands and record the minibuffer content. Ideally this would even work for other UIs like the default completion UI. (Unrelated, but also hooking into the sorting functionality of Vertico seems not necessary, since completion styles can use completion--adjust-meta-data to provide their own sorting routine. Maybe most of the vertico-prescient code could be made more generic?)

okamsn commented 8 months ago

@okamsn Would it be possible to use a post-command-hook/minibuffer-exit-hook in the minibuffer? It could run after all the relevant commands and record the minibuffer content. Ideally this would even work for other UIs like the default completion UI.

I've tried that in PR #156. @leungbk, would you please try the changes made in PR #156 and see if it works for you? It now uses the function file-name-split, which was added in more recent Emacs versions and is available from Compat. In that PR, I've added Compat as a dependency of Vertico Prescient.

(Unrelated, but also hooking into the sorting functionality of Vertico seems not necessary, since completion styles can use completion--adjust-meta-data to provide their own sorting routine. Maybe most of the vertico-prescient code could be made more generic?)

We did it the current way so that the sorting was applied even if the prescient completion style wasn't used (such as when falling back to basic). I originally wrote it to use the completion metadata in PR #125, but the feedback/consensus was that having it done both ways was confusing, so the metadata was removed in #130.