oantolin / embark

Emacs Mini-Buffer Actions Rooted in Keymaps
GNU General Public License v3.0
941 stars 56 forks source link

Embark giving errors with selectrum #77

Closed hmelman closed 3 years ago

hmelman commented 3 years ago

Originally mentioned on https://github.com/oantolin/embark/commit/e9aa0d7a941c1fc31cfd9112f2446701201aceed

I think this broke the suggestion selectrum integration described in the Selectrum Wiki Additional Configuration page. I am not sure how to fix it. I'm getting errors with that config about values not being lists and removing it completely works better, though I see some other errors.

minad commented 3 years ago

Could you please paste a stacktrace and the error message?

hmelman commented 3 years ago
Debugger entered--Lisp error: (wrong-type-argument listp "~/elisp/")
  embark--prompt-for-action(exit)
  embark-act()
  funcall-interactively(embark-act)
  call-interactively(embark-act nil nil)
  command-execute(embark-act)
  read-from-minibuffer("Find file: " "~/elisp/" (keymap (15 . embark-occur) (9 . selectrum-insert-current-candidate) (10 . selectrum-submit-exact-input) (C-M-backspace . backward-kill-sexp) (27 keymap (116 . hrm-selectrum-toggle-show-indices) (115 keymap (114 . selectrum-prescient-toggle-regexp) (112 . selectrum-prescient-toggle-prefix) (108 . selectrum-prescient-toggle-literal) (105 . selectrum-prescient-toggle-initialism) (102 . selectrum-prescient-toggle-fuzzy) (97 . selectrum-prescient-toggle-anchored)) (67108991 . backward-kill-sexp)) (remap keymap (previous-matching-history-element . selectrum-select-from-history) (kill-ring-save . selectrum-kill-ring-save) (end-of-buffer . selectrum-goto-end) (beginning-of-buffer . selectrum-goto-beginning) (minibuffer-beginning-of-buffer . selectrum-goto-beginning) (scroll-up-command . selectrum-next-page) (scroll-down-command . selectrum-previous-page) (exit-minibuffer . selectrum-select-current-candidate) (next-line-or-history-element . selectrum-next-candidate) (previous-line-or-history-element . selectrum-previous-candidate) (next-line . selectrum-next-candidate) (previous-line . selectrum-previous-candidate) (minibuffer-keyboard-quit . abort-recursive-edit) (keyboard-quit . abort-recursive-edit)) keymap (remap keymap (previous-matching-history-element . selectrum-select-from-history)) (menu-bar keymap (minibuf "Minibuf" keymap (previous menu-item "Previous History Item" previous-history-element :help "Put previous minibuffer history element in the min...") (next menu-item "Next History Item" next-history-element :help "Put next minibuffer history element in the minibuf...") (isearch-backward menu-item "Isearch History Backward" isearch-backward :help "Incrementally search minibuffer history backward") (isearch-forward menu-item "Isearch History Forward" isearch-forward :help "Incrementally search minibuffer history forward") (return menu-item "Enter" exit-minibuffer :key-sequence "\15" :help "Terminate input and exit minibuffer") (quit menu-item "Quit" abort-recursive-edit :help "Abort input and exit minibuffer") "Minibuf")) (10 . exit-minibuffer) (13 . exit-minibuffer) (7 . minibuffer-keyboard-quit) (C-tab . file-cache-minibuffer-complete) (9 . self-insert-command) (XF86Back . previous-history-element) (up . previous-line-or-history-element) (prior . previous-history-element) (XF86Forward . next-history-element) (down . next-line-or-history-element) (next . next-history-element) (27 keymap (97 . marginalia-cycle) (60 . minibuffer-beginning-of-buffer) (114 . previous-matching-history-element) (115 . next-matching-history-element) (112 . previous-history-element) (110 . next-history-element))) nil file-name-history)
  selectrum-read("Find file: " #f(compiled-function (input) #<bytecode 0x1ff19ea9f9d5>) :default-candidate "~/elisp/" :initial-input "~/elisp/" :history file-name-history :require-match nil :may-modify-candidates t :minibuffer-completion-table read-file-name-internal :minibuffer-completion-predicate file-exists-p)
  selectrum--completing-read-file-name("Find file: " read-file-name-internal file-exists-p confirm-after-completion "~/elisp/" file-name-history "~/elisp/" nil)
  apply(selectrum--completing-read-file-name ("Find file: " read-file-name-internal file-exists-p confirm-after-completion "~/elisp/" file-name-history "~/elisp/" nil))
  #f(compiled-function (&rest args) #<bytecode 0x1ff19e945cc5>)("Find file: " read-file-name-internal file-exists-p confirm-after-completion "~/elisp/" file-name-history "~/elisp/" nil)
  completing-read("Find file: " read-file-name-internal file-exists-p confirm-after-completion "~/elisp/" file-name-history "~/elisp/")
  read-file-name-default("Find file: " nil "/Users/hmelman/elisp/" confirm-after-completion nil nil)
  selectrum-read-file-name("Find file: " nil "/Users/hmelman/elisp/" confirm-after-completion nil nil)
  read-file-name("Find file: " nil "/Users/hmelman/elisp/" confirm-after-completion)
  find-file-read-args("Find file: " confirm-after-completion)
  byte-code("\300\301\302 \"\207" [find-file-read-args "Find file: " confirm-nonexistent-file-or-buffer] 3)
  call-interactively(find-file nil nil)
  command-execute(find-file)
oantolin commented 3 years ago

Oh my God, sorry! I completely forgot about Selectrum when I unified acquisition and classification. The integration does need to be updated since the expected return values of target finders and candidate collectors has changed.

oantolin commented 3 years ago

I sort of thought that with the changes in Selectrum instigated by @clemera, that old integration wasn't necessary any more. But you are saying without any integration it only partially works?

I guess I'll install Selectrum to test...

hmelman commented 3 years ago

So far the only error I'm seeing is in #78 but that was the first thing I tried. If there's no specific integration needed, great, I'll work from that and report any other issues.

oantolin commented 3 years ago

I've been a Selectrum user for about 10 minutes now! I'll explore a bit and see if some integration really is needed or not. It certainly does something out of the box, I'm just not sure it does the right thing.

oantolin commented 3 years ago

Wow, Selectrum is pretty snappy, maybe I should keep it...

oantolin commented 3 years ago

Could you please try this rewritten integration?

(add-hook 'embark-target-finders
      (defun selectrum-current-candidate+category ()
        (cons (selectrum--get-meta 'category)
          (selectrum-get-current-candidate))))

(add-hook 'embark-candidate-collectors
          (defun selectrum-current-candidates+category ()
            (when selectrum-active-p
          (cons (selectrum--get-meta 'category) 
            (selectrum-get-current-candidates
             ;; Pass relative file names for dired.
             minibuffer-completing-file-name)))))
hmelman commented 3 years ago

Playing with it quickly it does seem embark-occur works in all the cases I'm trying.

oantolin commented 3 years ago

Good! Does it also take care of the empty occur buffer issue?

hmelman commented 3 years ago

Wow, Selectrum is pretty snappy, maybe I should keep it...

I came from ivy and am quite happy with it. I was happy with ivy as a user but like the selectrum README explains I ran into issues with it's api writing new commands.

hmelman commented 3 years ago

Good! Does it also take care of the empty occur buffer issue?

Yes.

oantolin commented 3 years ago

Excellent. I'm still a little curious about what exactly embark does if you have selectrum on and no integration at all, I don't quite understand it yet. I'll dig a little more, but it seems likely that I was just plain wrong in my impression that the integration is not needed anymore and that the functions I put above are what's needed now.

minad commented 3 years ago

@oantolin For reference, this is my current configuration (still not adjusted for the recent changes)

(use-package embark
  :bind (:map minibuffer-local-map
              ("M-SPC" . embark-act-noexit)
              ("C-M-SPC" . embark-act)
         :map embark-symbol-map
         ("h" . helpful-symbol)
         :map embark-become-help-map
         ("v" . helpful-variable)
         ("f" . helpful-function)
         ("s" . helpful-symbol)
         :map embark-meta-map
         ("C-M-a" . marginalia-cycle))
  :config
  (unbind-key "C-h" embark-meta-map)
  (add-to-list 'which-key-replacement-alist '((nil . "embark-") . (nil . "e-")))
  (add-hook 'embark-target-finders 'selectrum-get-current-candidate)
  (add-hook 'embark-candidate-collectors
            (lambda ()
              (when selectrum-active-p
                (selectrum-get-current-candidates
                 minibuffer-completing-file-name))))
  (add-hook 'embark-setup-hook 'selectrum-set-selected-candidate)
  (add-hook 'embark-input-getters
            (lambda ()
              (when selectrum-active-p
                (if minibuffer-completing-file-name
                    (file-name-nondirectory (selectrum-get-current-input))
                  (selectrum-get-current-input)))))
  (setq embark-action-indicator
        (lambda (map) (which-key--show-keymap "Embark" map nil nil 'no-paging)
          #'which-key--hide-popup-ignore-command)
        embark-become-indicator embark-action-indicator))
oantolin commented 3 years ago

Yeah, if you update Embark to after the unification, @minad, you should also update your target finder and candidate collector, they just need to cons the category on the front of what they return, as in my code block above.

oantolin commented 3 years ago

I'm pretty sure I understand what Embark does if you use Selectrum without any integration: it just relies on the completion-styles, as if you were using the default tab completion. This is obvious, the reason I got confused is that I tested Embark + Selectrum (without integration) in two settings and got wildly different behaviors: in my Emacs it seemed to work absolutely fine, in emacs -q I got @hmelman's empty buffer issue plus other issues.

The difference is that I use orderless in my Emacs and emacs -q doesn't even have the decency of including substring among the completion styles!

OK, so mystery solved, integration fixed. I'll close this issue and the other one abotu the empty buffer and we have to see about updating the Selectrum wiki.

oantolin commented 3 years ago

Oh no, I also got rid of embark-input-getters in the unification, so also get rid of that in your configuration, @minad. I think embark-become should work under Selectrum anyway, but I'll make sure it does.

oantolin commented 3 years ago

OK, pretty sure embark-become does work with Selectrum even now that the input getter isn't used.

minad commented 3 years ago

Great! I like it when the config is getting simpler! I have to test it - I didn't update the melpa packages for a few days.

oantolin commented 3 years ago

I updated the Selectrum wiki. I'll start using Selectrum, with the configuration I put there and fix any bugs I notice, but I'm prettu sure it's correct now.

oantolin commented 3 years ago

@hmelman, @minad there was a slight bug in the target finders I wrote above, it also needs the selectrum-active-p check that the candidate collector has (without that check it doesn't let you act outside the minibuffer). I update the Selectrum wiki.

I want to add a minor correction to my earlier statement about Selectrum: it's very fast once it gets started but it's pretty slow at starting up. :( This is very noticeable when you act on candidates outside the minibuffer: the action feels instant with default completion or with icomplete but has a noticeable pause if you use Selectrum. I think that pause may be enough to drive me back to embark-live-occur.

hmelman commented 3 years ago

Thanks, I'm using the config now in the updated selectrum wiki.

I'm not sure I follow when you find things slow. Could you give me steps to reproduce?

oantolin commented 3 years ago

I'm not sure I follow when you find things slow. Could you give me steps to reproduce?

Any time Selectrum start with a long candidate list I notice a pause. For example C-h f or C-h v have noticeable pauses. If you type C-h f RET very quickly to get help on the function at point, the help appears instantly with default completion or icomplete, but there is a noticeable pause with Selectrum.

If you put point on a function name in an Emacs Lisp buffer and type C-S-a h (where C-S-a is your binding for embark-act), on my computer there is a noticeable pause before the help buffer appears. I believe this is basically the same pause as with C-h f RET so if that's fast for you, probably C-S-a h will be fast too.

hmelman commented 3 years ago

Any time Selectrum start with a long candidate list I notice a pause. For example C-h f or C-h v have noticeable pauses. If you type C-h f RET very quickly to get help on the function at point, the help appears instantly with default completion or icomplete, but there is a noticeable pause with Selectrum.

Hmmm, I see that pause with those commands, but only the first time I invoke them. The second time it's fast. I think it's more generation of the completions list, which I'd think would be slow the first time for any completion system? I'll keep an eye out for more.

oantolin commented 3 years ago

Are you saying Selectrum caches the list of completions for C-h v? It shouldn't because what if I have defined more variables since the last time I ran it?

hmelman commented 3 years ago

No but I think emacs generates a completions table from the obarry the first time its needed.

oantolin commented 3 years ago

Well, I notice the pause in C-h f and C-h v will Selectrum every time. I think it extracts the strings from the obarray every time. My computer is a 7 year old $250 USD Chromebook running Emacs in a Linux VM, so it's probably fine on faster computers.

minad commented 3 years ago

@oantolin I also notice a slight delay the first time when doing C-h f and C-h v. But it gets better the second time (could also be some os memory caching effect?). I believe selectrum goes over the obarray once as you say. But M-x is very snappy, always! Maybe @clemera can say more about that. For me having a slightly slower C-h f is acceptable - I usually care a lot about snappiness, also when we made marginalia.

oantolin commented 3 years ago

Yes M-x is very fast. I wonder if that has some caching (and if it does pick up newly defined commands).

minad commented 3 years ago

@oantolin I have also be wondering about this. We should open an issue on the selectrum tracker and just ask what is going on.

clemera commented 3 years ago

Thanks @oantolin for updating our Wiki! We don't cache things on the Selectrum side, I think the issue with the initial delay is because we compute the candidates right away, with default completion the candidates are only computed on TAB and icomplete uses a delay by default.

walseb commented 3 years ago

Thanks @oantolin for updating our Wiki! We don't cache things on the Selectrum side, I think the issue with the initial delay is because we compute the candidates right away, with default completion the candidates are only computed on TAB and icomplete uses a delay by default.

I've been having the same problem with slow startup too (although for me it's only a problem with describe-variables), but for me the ivy variant starts up instantly, which I believe also fetches all candidates at once when called, but I could be wrong.

minad commented 3 years ago

I think there is still some optimization potential in selectrum.

See https://github.com/raxod502/selectrum/issues/312 and https://github.com/raxod502/selectrum/issues/75

oantolin commented 3 years ago

I suspected as much @clemera. The delay is really only noticeable on my slowest computer, and neither of the two is recent, so I think it's pretty good. But maybe Selectrum should introduce a small delay? Currently typing C-h f RET or M-x M-p RET as fast as I can still pops up the candidates briefly. I like that all the other completions UIs I use don't display anything at all in those cases.

oantolin commented 3 years ago

Oh, by the way @clemera: I'm a Selectrum user now too! Not all the time, but I have it configured along with default tab completion, icomplete-vertical and embark-live-occur-after-{delay/input} and have a little command to select a completion UI among them.

I have to say that with Embark and Marginalia, even the default tab completion with its *Completions* buffer is pretty usable.

It turns out I'm not really attached to any completion UI but you can pry orderless from my cold dead hands.

minad commented 3 years ago

I like it that there is no delay. I agree that selectrum is already pretty fast. But there is https://github.com/raxod502/selectrum/issues/75 where selectrum is significantly slower than ivy. Would be good to at least check what the reason is. Maybe it is not possible to make this faster for a good reason. But maybe there is a bit of optimization in potential.

clemera commented 3 years ago

@walseb

although for me it's only a problem with describe-variables

describe-variables is currently slow with Selectrum for some reason, see https://github.com/raxod502/selectrum/issues/75.

@oantolin

But maybe Selectrum should introduce a small delay?

I use sorting so M-x M-p RET is usually M-x RET for me but I know you don't like sorting by default. The C-h f RET case is also something which annoys me from time to time but I also don't like to have delays in all other cases to avoid that. I see that if you don't use sorting seeing the initial candidate set isn't interesting so if you don't use sort or are okay with an initial delay it would be nice to have an option to configure that.

It turns out I'm not really attached to any completion UI but you can pry orderless from my cold dead hands.

That is nice I think the completion UI should really be just another component, I also don't think I will stop using orderless but it is nice that I could and all other components of my setup would keep working like I'm used to.

hmelman commented 3 years ago

It turns out I'm not really attached to any completion UI but you can pry orderless from my cold dead hands.

I came from ivy and didn't think about completion-styles. When I switched to this "stack", I started with selectrum and quickly added prescient and really like it. I've only briefly played with orderless and it seems that prescient uses it's own completion-style and I think they don't play nice together (I could be wrong). @oantolin Could you describe what you love about orderless?

minad commented 3 years ago

My pov: Orderless is a completion style in contrast to prescient, orderless is faster, orderless is more flexible. Ideally I would like to see a prescient which is downstripped and does only the sorting part based on frecency.

oantolin commented 3 years ago

@hmelman Prescient does both filtering and sorting, while orderless does only filtering. The type of filtering they provide is pretty similar: matching components in any order, with the components being able to match in a variety of styles: as initialisms, fuzzy or flex matching, as regexps, etc.; though I believe orderless comes with many more matching styles than prescient.

@clemera and @minad seem to think orderless is faster, which wouldn't surprise me since it is a relatively thin wrapper around Emacs functionality written in C, while Prescient does the bulk of its work in Emacs Lisp.

But for me the main advantage of orderless is that it is highly configurable (even ridisculously so), and you can use it to craft your own personal query syntax. Prescient, I believe, is less configurable: you can toggle each matching style on or off but when it is on it applies to all the components of your search string. With my orderless configuration I can type C-h f .pmu !-- to get all function names that have dash separated parts starting with p, m and u in that order, but that do not contain -- (i.e., are not private). I chose to use . and ! with those meanings, they are not built-in to orderless.

In summary, orderless can be configured to be more precise about how each component matches, but it's not too different from prescients filtering, so if you are happy with prescient there is no strong motivation to switch to orderless. Note that it is easy to use prescient just for sorting and orderless for filtering. @clemera does that.

hmelman commented 3 years ago

Thanks, that's really great info. I suggest you put in the Orderless README.