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

Support Vertico sorting functions #20

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

olnw
Copy link
Contributor

@olnw olnw commented Sep 6, 2024

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
Copy link
Owner

axelf4 commented Sep 6, 2024

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
Copy link
Contributor Author

olnw commented Sep 7, 2024

Upon investigating this a bit further, I found a bug report for fido-mode wherein it was not sorting candidates by their history positions,1 and the corresponding fix.2 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.3

  1. https://lists.gnu.org/archive/html/emacs-bug-tracker/2021-08/msg00321.html
  2. emacs-mirror/emacs@2c699b8
  3. emacs-mirror/emacs@dfffb91#diff-9fe92bd4449f8efe346fc61ec3ba7e772b1d500ea79254dac58b040a9d1f688bR4324

@axelf4
Copy link
Owner

axelf4 commented Sep 7, 2024

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

Completion frontends would forgo their default sorting when the
completion--adjust-metadata function added a display-sort-function
property even if it did nothing, e.g. due to an empty search string.
This commit conditionally omits the sort function properties, allowing
candidates to be sorted by minibuffer history, such as with Vertico's
default sorting function, vertico-sort-history-length-alpha.

Closes axelf4#18

Co-authored-by: Oliver Nikolas Winspear <dev@oliverwinspear.com>
Copy link

codecov bot commented Sep 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@axelf4 axelf4 merged commit aa6bf36 into axelf4:master Sep 7, 2024
4 of 5 checks passed
@JohnWick95
Copy link

This feature appears to be partially implemented, as the history is currently ignored once typing begins.

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.

Feature request: support for savehist-mode
3 participants