mohkale / consult-yasnippet

Other
35 stars 3 forks source link

Feature Request: Support for calling snippet from any table, not just the currently active buffer #4

Closed kings2u closed 2 years ago

kings2u commented 2 years ago

First of all, thank you for this package! One feature I've been after is the ability to search for a snippet in any Yasnippet table, not just those available in the current buffer. This same feature would also be helpful to visit any snippet you want to edit, regardless of the current buffer. Maybe passing a prefix argument to consult-yasnippet and consult-yasnippet-visit-snippet-file could accomplish this?

mohkale commented 2 years ago

Hmmm... I don't really mind adding this but we may have to consider how to present it. Atm the format for candidates is "%(template-name)s [%(template-key)]". If we're supporting across all snippet tables then we should to include the name of the snippet table as well because otherwise there'll be a lot of overlap across languages. Do you have any suggestions for this? Cause simply prefixing the candidate with the table name doesn't seem ideal to me.

kings2u commented 2 years ago

we should to include the name of the snippet table as well

I definitely agree. Perhaps the marginalia package could be used to annotate each snippet with the table that it derives from? Or the name of the table could just be inserted in brackets and a different face after the template-key? If you don’t want to pollute the existing functionality, I think adding new commands to the package for this feature would be fine, like consult-yasnippet-all and consult-yasnippet-visit-any-snippet-file. My lisp skills are still progressing, so I probably can’t submit helpful code at the moment :/ Thank you for your willingness to consider this feature!

mohkale commented 2 years ago

I definitely agree. Perhaps the marginalia package could be used to annotate each snippet with the table that it derives from?

Issue with that is you can't filter on annotations :/. But I think if we just add it as hidden text before what we already show and then have annotations showing it as well it could work. Kind of a hacky solution but better than nothing.

mohkale commented 2 years ago

@kings2u

I've implemented this on the all-tables branch. Now consult-yasnippet takes an optional argument which causes snippets to be loaded from all snippet tables. You can do C-u M-x consult-yasnippet to run it. Note in this case you can also filter for the name of the snippet table (Example: python-mode) since each candidate is prefixed with the table name. I've also added an annotation function which includes a right-aligned string showing the snippet-table for each snippet.

Please try it out and let me know whether it works and meets what you wanted from this issue.

Note: If you still want a dedicated command for this instead of an option for the existing consult-yasnippet command you can simply add the following to your config:

(defun consult-yasnippet-all ()
  (interactive)
  (consult-yasnippet 1))
kings2u commented 2 years ago

This is fantastic, thank you! The only issue I notice is that when I try to load snippets from all tables, the new function only loads those tables whose major mode has been used before in the current emacs session. For example, if I start emacs, calling the function with a prefix arg will give me snippets from fundamental-mode and emacs-lisp-mode, but not snippet-mode even though I have several snippets that exist for that mode. It's not until I open up a snippet-mode buffer that snippet-mode snippets start showing up when I press C-u M-x consult-yasnippet. This is not an issue per se, and it's fine if it can't be fixed, though FWIW my ideal function would probably display all snippets right away, even for tables that haven't yet been accessed.

Were you planning to extend this feature to consult-yasnippet-visit-snippet-file too? No big deal if not, as I could use Embark to visit a snippet from the consult-yasnippet view with the help of your idea at https://github.com/minad/consult/pull/173. It's great that both functions now annotate snippets with the table they belong to :)

Thanks again!

kings2u commented 2 years ago

I could use Embark to visit a snippet from the consult-yasnippet view with the help of your idea at minad/consult#173.

I actually just noticed that I cannot use your embark command when I generate the consult-yasnippet list using a prefix argument. Doing so generates the error: (wrong-type-argument yas--template nil)...

mohkale commented 2 years ago

the new function only loads those tables whose major mode has been used before in the current emacs session

Looks like yas defers the loading of snippets for modes that haven't been required. Not much we can do about that, if it really bothers you you can add (with-eval-after-load 'yasnippet (yas-reload-all)) to your config. This'll probably have a noticeable slowdown though.

Were you planning to extend this feature to consult-yasnippet-visit-snippet-file too?

I tested this yesterday and it was working, but it isn't now. I don't really know why.

@oantolin could you help me understand why this is happening? For your convenience here's a minimal reproducible config.

(defun foo (&optional arg)
  (consult--read
   (if arg
       '(("e" . 5)
         ("f" . 6)
         ("g" . 7)
         ("h" . 8))
     '(("a" . 1)
       ("b" . 2)
       ("c" . 3)
       ("d" . 4)))
   :prompt "Foo: "
   :lookup 'consult--lookup-cdr
   :category 'foo))

(defun bar (it)
  (interactive (list (foo)))
  (message "%s" it))

(embark-define-keymap embark-foo-completion-actions
  "Embark actions for `foo' and derivatives."
  ("d" bar))

(push '(foo . embark-foo-completion-actions)
      embark-keymap-alist)

(defun baz (arg)
  (interactive "P")
  (when-let ((it (foo arg)))
    (message "baz: %s" it)))

(global-set-key (kbd "M-'") #'baz)

If you run C-u M-' followed by M-x embark-act d you'll get nil messaged out. But if you do so without the prefix arg it works like you'd expect.

oantolin commented 2 years ago

In the code above, foo uses a different alist to lookup the candidate depending on the argument; baz calls foo with the prefix argument but bar always calls (foo nil). So in the C-u M-' case, your target is one of e, f, g, h and then when you call embark-act d it looks that target up in the a, b, c, d alist, returning nil.

Notice that bar simply cannot do the correct lookup without knowing what the prefix argument was earlier, at the time baz was called.

mohkale commented 2 years ago

@oantolin

I'm not well aware of how embark is implemented but I'm not sure why that's an issue. Consult already has all the candidates by the time I call embark doesn't it? Does embark recall the function and regenerate the candidates or something like that?

Either way how would you recommend implementing a action that can be shared across slightly different caller functions. In the case of this issue consult-yasnippet-visit-snippet-file. Since the prefix-arg was the issue here I tried creating a new separate function for consult-yasnippet-all (at #5) but it still fails if I try to call M-x embark-act d (which is bound to consult-yasnippet-visit-snippet-file) from this second function. It still works completely fine if I call it from the original consult-yasnippet function.

oantolin commented 2 years ago

Consult already has all the candidates by the time I call embark doesn't it? Does embark recall the function and regenerate the candidates or something like that?

No command is called twice. Let me explain again with more detail what happens in the example you gave above (by the way, thanks for the small example, since reading it is much easier for me than diving into this package).

When you call C-u M-', the function baz is called interactively. It calls (foo arg) which produces a completing-read session using the alist with keys e, f, g, h. Then you select a candidate, say f, and call embark-act d. This will call the function bar as an action.

Now embark can use any function as an action and respects it's interactivity, that is, interactive functions are called interactively (and other functions are called via a normal function call). Since bar is interactive, embark-act calls it interactively and will insert the target f at the first minibuffer prompt. When called interactively, what bar does is call (foo nil) to start a completing-read session with the alist having a, b, c, d as keys. Then embark inserts the target f at that prompt, and consult--read looks up the entered target, namely f, in the alist with a, b, c, d as keys. The result of that lookup is nil, which becomes the value of the it parameter of bar; so bar prints nil in the echo area.

Does that make sense? Embark just runs the action interactively and inserts the target at the first minibuffer prompt created by the target. In this example, that winds up inserting the target, which was e, f, g or h, at the (foo) prompt which then looks it up in the a, b, c, d alist.

oantolin commented 2 years ago

Since the prefix-arg was the issue here I tried creating a new separate function for consult-yasnippet-all (at #5) but it still fails if I try to call M-x embark-act d (which is bound to consult-yasnippet-visit-snippet-file) from this second function. It still works completely fine if I call it from the original consult-yasnippet function.

The issue wasn't really the prefix-arg but simply that consult-yasnippet and consult-yasnippet-visit-snippet-file have the same format of candidates and consult-yasnippet-all has a different format for candidates, which the other functions don't understand. Said more directly: consult-yasnippet-visit-snippet-file doesn't work with a completion candidate for consult-yansnippet-all.

This has nothing to do with Embark, actually: try running consult-yasnippet-visit-snippet-file and instead of selecting one of its completion candidates submit one of the candidates for consult-yansnippet-all (i.e., prefix it with the table name). You'll see it doesn't work. If it doesn't work when run manually it won't work when run through embark, since embark just automates the manual steps.

oantolin commented 2 years ago

Maybe the cheapest way to fix this is to make consult-yasnippet-visit-snippet-file universal, that is, to work with both types of candidates:

(defun consult-yasnippet-visit-snippet-file (template)
  "Visit the snippet file associated with TEMPLATE.
When called interactively this command previews snippet completions in
the current buffer, and then opens the selected snippets template file
using `yas--visit-snippet-file-1'."
  (interactive (list (consult-yasnippet--read-template
                      (append (consult-yasnippet--candidates)
                              (consult-yasnippet--all-candidates)))))
  (yas--visit-snippet-file-1 template))

It will look a little funny if called directly as a command (since the current buffer templates will be listed twice: once with an invisible prefix), but if you use it as an Embark action you never see its completion candidates!

oantolin commented 2 years ago

You know, maybe you could always use a big list of all templates from all tables, and allow consult-narrowing to the current buffer templates. You could remove consult-yasnippet-all and just have consult-yasnippet start with initial narrowing to the current buffer templates. If one wants a template from a different table, you just consult-widen first.

mohkale commented 2 years ago

@oantolin

Okay, your expanded example helps make things a lot easier to understand. Thank you very much. Always nice to get a better understanding of how something you use day-to-day works under the hood. :smile:.

I've managed to fix the issue using what you've shared. Firstly now the table-name is always included in a candidate (since even in the current-mode it may have multiple table names, meaning it should make the interface more powerful). And I've changed consult-yasnippet-visit-snippet-file to read from all tables, since there's no need for it to be limited to the current modes table list.

@kings2u

Could you try the current all-tables branch. I've tested and everythings working for me, I'd just like a confirmation before merging.

Once again, thanks @oantolin. :grinning:.

kings2u commented 2 years ago

Everything works great on my end! Thank you @mohkale and @oantolin!