radian-software / selectrum

🔔 Better solution for incremental narrowing in Emacs.
MIT License
737 stars 33 forks source link

selectrum-read-library-name returns nil when submitting exact input #577

Closed zabe40 closed 2 years ago

zabe40 commented 2 years ago

When calling selectrum-submit-exact-input or exit-minibuffer from selectrum-read-library-name, it causes selectrum-read-library-name to return nil regardless of the input. I initially noticed this when using find-library as an Embark action.

Heres a recipe for emacs -Q:

(defvar bootstrap-version)
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el" user-emacs-directory))
      (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 'selectrum)

(selectrum-mode +1)

(minibuffer-with-setup-hook
    (lambda ()
      (with-selected-window (active-minibuffer-window)
    (delete-minibuffer-contents)
    (insert "selectrum")))
  (selectrum-read-library-name))

While in the minibuffer after running the last form, if you call selectrum-submit-exact-input by pressing C-j, selectrum-read-library-name returns nil, even though selectrum is one of the given completions.

After poking around a bit with edebug, it seems like the issue stems from the submitted input not having the selectrum--lib-path text property that is expected by selectrum-read-library-name.

okamsn commented 2 years ago

Thank you for the detailed reporting of this issue.

I've created PR #580, which grabs the property from the first matching candidate in the unfiltered candidate list. This is the same candidate that would be selected automatically from typing, so this change should work the same as using selectrum-select-current-candidate on the automatically selected library name.

Would you please check to see whether this change works for you? It works for me.

EDIT: Actually, things seem a bit more complicated when there are multiple versions of the same library. The PR seems to need more testing.

zabe40 commented 2 years ago

I just tested your PR, and can confirm that it works for cases with no load-path shadowing, but returns the wrong (shadowed) path when the library is shadowed.

okamsn commented 2 years ago

I just tested your PR, and can confirm that it works for cases with no load-path shadowing, but returns the wrong (shadowed) path when the library is shadowed.

Thank you. I think that I found the cause of that error, and now just need to make sure that the Selectrum version accepts all of the variants that the built-in version accepts.