Support Vertico sorting functions

olnw commented 1 week ago

Closes #18

This commit makes hotfuzz aware of vertico-sort-override-function and vertico-sort-function. This allows candidates to be sorted by minibuffer history, such as with Vertico's default sorting function, vertico-sort-history-length-alpha.

axelf4 commented 1 week ago

Thanks! This can get messy, however, if it is to also use corfu--sort-function if Corfu is the active UI, etc.

@minad Could Vertico instead alter the metadata it passes to completion-all-completions such that display-sort-function is set to (vertico--sort-function)? That way, the hotfuzz/flex/... completion--adjust-metadata functions should just do the right thing.

olnw commented 1 week ago

Upon investigating this a bit further, I found a bug report for fido-mode wherein it was not sorting candidates by their history positions,¹ and the corresponding fix.² This was solved in flex's metadata adjustment function by only specifying a display-sort-function when the prompt was non-empty, thereby allowing icomplete to handle sorting.

hotfuzz could do something similar:

diff --git a/hotfuzz.el b/hotfuzz.el
index 50ceaa5..9fe2e08 100644
--- a/hotfuzz.el
+++ b/hotfuzz.el
@@ -167,7 +167,8 @@ will lead to inaccuracies."
 (defun hotfuzz--adjust-metadata (metadata)
   "Adjust completion METADATA for hotfuzz sorting."
   (let ((existing-dsf (completion-metadata-get metadata 'display-sort-function))
-        (existing-csf (completion-metadata-get metadata 'cycle-sort-function)))
+        (existing-csf (completion-metadata-get metadata 'cycle-sort-function))
+        (filtering-p (or (not (minibufferp)) (> (point-max) (minibuffer-prompt-end)))))
     (cl-flet ((compose-sort-fn (existing-sort-fn)
                 (lambda (completions)
                   (if (or (null completions)
@@ -175,8 +176,9 @@ will lead to inaccuracies."
                       completions
                     (funcall existing-sort-fn completions)))))
       `(metadata
-        (display-sort-function . ,(compose-sort-fn (or existing-dsf #'identity)))
-        (cycle-sort-function . ,(compose-sort-fn (or existing-csf #'identity)))
+        ,@(when filtering-p
+            `((display-sort-function . ,(compose-sort-fn (or existing-dsf #'identity)))
+              (cycle-sort-function . ,(compose-sort-fn (or existing-csf #'identity)))))
         . ,(cdr metadata)))))

 ;;;###autoload

However, as João commented, this was not optimal:

(let ((flex-is-filtering-p
         ;; JT@2019-12-23: FIXME: this is kinda wrong.  What we need
         ;; to test here is "some input that actually leads/led to
         ;; flex filtering", not "something after the minibuffer
         ;; prompt".  E.g. The latter is always true for file
         ;; searches, meaning we'll be doing extra work when we
         ;; needn't.
         (or (not (window-minibuffer-p))
             (> (point-max) (minibuffer-prompt-end))))

The solution was later updated to check that completion-pcm--regexp was non-nil, which is a variable set by completion-pcm--hilit-commonality, which is called by completion-flex-all-completions.³

axelf4 commented 1 week ago

Perhaps something like:

diff --git a/hotfuzz.el b/hotfuzz.el
index 50ceaa5..c90cd15 100644
--- a/hotfuzz.el
+++ b/hotfuzz.el
@@ -43,6 +43,8 @@ Large values will decrease performance."
 (defvar hotfuzz--d (make-vector hotfuzz--max-needle-len 0))
 (defvar hotfuzz--bonus (make-vector hotfuzz--max-haystack-len 0))

+(defvar hotfuzz--filtering-p)
+
 (defconst hotfuzz--bonus-lut
   (eval-when-compile
     (let ((state-special (make-char-table 'hotfuzz-bonus-lut 0))
@@ -154,6 +156,7 @@ will lead to inaccuracies."
      (t (cl-loop for x in-ref all do (setf x (cons (hotfuzz--cost needle x) x))
                  finally (setq all (mapcar #'cdr (sort all #'car-less-than-car))))))
     (when all
+      (setq hotfuzz--filtering-p (not (string= needle "")))
       (unless (string= needle "")
         (defvar completion-lazy-hilit-fn) ; Introduced in Emacs 30 (bug#47711)
         (if (bound-and-true-p completion-lazy-hilit)
@@ -174,10 +177,12 @@ will lead to inaccuracies."
                           (get-text-property 0 'completion-sorted (car completions)))
                       completions
                     (funcall existing-sort-fn completions)))))
-      `(metadata
-        (display-sort-function . ,(compose-sort-fn (or existing-dsf #'identity)))
-        (cycle-sort-function . ,(compose-sort-fn (or existing-csf #'identity)))
-        . ,(cdr metadata)))))
+      (if hotfuzz--filtering-p
+          `(metadata
+            (display-sort-function . ,(compose-sort-fn (or existing-dsf #'identity)))
+            (cycle-sort-function . ,(compose-sort-fn (or existing-csf #'identity)))
+            . ,(cdr metadata))
+        metadata))))

 ;;;###autoload
 (progn

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.64%. Comparing base (1afac1f) to head (9f9a38c). Report is 1 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #20 +/- ## ========================================== - Coverage 93.50% 92.64% -0.86% ========================================== Files 1 1 Lines 77 68 -9 ========================================== - Hits 72 63 -9 Misses 5 5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

axelf4 / hotfuzz

Support Vertico sorting functions #20

Codecov Report