embark-act-all: Allow passing a list for crm functions

[bdarcus]

Not sure how best to title this issue (feel free to change it of course), but it's a followup to this comment of mine: emacs-citar/citar#477 (comment).

I have a couple of functions where I really need to pass a list of candidates to functions that rely on CRM (for example, a list of file names for PDFs that I want to search using pdf-tools pdf-occur). Right now, if I run embark-act-all on 10 candidates, it will run the function 10 times, where in this case I need it run once.

[oantolin]

How do the functions you want to run with all targets at once usually take their arguments? In a single list? Via a &rest parameter? Interactively using completing-read-multiple?

[oantolin]

I'm not sure we can autodetect when a command uses completing-read-multiple nor can we easily figure out what to join the targets with (i.e., given the regexp matching the separator figure out a string it matches). But we could add an embark-target-list-actions variables for actions that will be called, say, with a list of targets. Then embark-act would pass those functions singleton target lists and embark-act-all the list of all targets.

[bdarcus]

How do the functions you want to run with all targets at once usually take their arguments? In a single list? Via a &rest parameter? Interactively using completing-read-multiple?

Here's one, where citar-select-refs uses CRM:

(defun citar-copy-reference (keys-entries)
  "Copy formatted reference(s) associated with the KEYS-ENTRIES."
  (interactive (list (citar-select-refs)))
  (let ((references (funcall citar-format-reference-function keys-entries)))
    (if (not (equal "" references))
        (progn
          (kill-new references)
          (message (format "Copied:\n%s" references)))
      (message "Key not found."))))

But we could add an embark-target-list-actions variables for actions that will be called, say, with a list of targets.

I think that would work.

[minad]

Seems like the sanest solution to just call the command non-interactively (like the kill-new action) with a list as argument.

[oantolin]

I was thinking I'd do this:

• add a variable called embark-multitarget-actions for users to specify which actions receive the special treatment.
• embark-act-all would call them non-interactively with a single list argument.
• embark-act would still do the normal thing if the function is interactive, but if it is non-interactive it would pass it a singleton list.

Does that behavior for embark-act sound reasonable?

[minad]

embark-act would still do the normal thing if the function is interactive, but if it is non-interactive it would pass it a singleton list.

This decision is questionable, I would probably pass a singleton list in both cases to simplify the protocol.

[oantolin]

I wanted to leave the possibility of a function that first calls completing-read-multiple and then calls completing-read for more arguments to still be used in the embark-act case. Consider a function that prompts for a list of files and then for a directory to move them all to.

[minad]

Hmm, this is an edge case. I would keep the protocol as simple as possible. Functions registered in embark-target-list-actions can be reasonably expected to be well behaved, they will follow the protocol precisely. It is still possible to implement such an action in a way to first receive a list and then query for a directory.

(defun my-action-fun (list &optional dir)
   (interactive (list (crm...) (read-directory...)))
   (unless dir
      (setq dir (read-directory...)))
   ...)

Note that there are basically no functions/commands which profit from the injection mechanism, since crm is basically unused in Emacs. Therefore the scenario is different than the one from cr, where we profit greatly from the elaborate injection mechanism. But this is just my preference, I prefer a simpler protocol at the potential cost of less reuse. But maybe you consider your proposal as more natural?

EDIT: I should add - probably the author of such a target list action function will appreciate if Embark always calls the command in a uniform way. This will reduce confusion and will make the entire mechanism more predictable.

[oantolin]

OK. I'm convinced. I'll always call the new multitarget actions non-interactively.

[oantolin]

OK. Done. Please test!

[bdarcus]

Thanks!

Edit: I think I need to explore if I need to make some local adjustments.

[oantolin]

Well, citar will be the first user, so you're definitely poised to exert an outsized influence on the design of the mechanism. 😛

[bdarcus]

One little piece of UI feedback.

Right now, after I invoke embark-act-all, here's what I see:

.... so no feedback that I've invoked that. And then, the y/n confirmation, which is a separate step.

Would it be possible to instead include the latter info in the main prompt, and forego the subsequent prompt?

So something like [ Act on all 5 citar-references ], and if one doesn't want that, can just cancel?

[oantolin]

Oh, that does look odd. You're not getting the cute "∀ct" indication.

[oantolin]

Ah, let me guess: that's from the which-key indicator, right? I forgot to update it!

[bdarcus]

Yep.

[bdarcus]

PS - what's the significance of that symbol?

[oantolin]

It's a math pun, "∀" means "for all".

[oantolin]

Well, for now you could use this updated which-key indicator instead:

(defun embark-which-key-indicator ()
  "An embark indicator that displays keymaps using which-key.
The which-key help message will show the type and value of the
current target followed by an ellipsis if there are further
targets."
  (lambda (&optional keymap targets prefix)
    (if (null keymap)
        (which-key--hide-popup-ignore-command)
      (which-key--show-keymap
       (cond
        ((eq (plist-get (car targets) :type) 'embark-become) "Become")
        ((plist-get (car targets) :multi)
         (format "%s on %d %ss"
                 (embark--act-label nil t)
                 (plist-get (car targets) :multi)
                 (plist-get (car targets) :type)))
        (t (format "%s on %s '%s'%s"
                   (embark--act-label
                    (where-is-internal #'embark-done (list keymap))
                    nil)
                   (plist-get (car targets) :type)
                   (embark--truncate-target (plist-get (car targets) :target))
                   (if (cdr targets) "…" ""))))
       (if prefix
           (pcase (lookup-key keymap prefix 'accept-default)
             ((and (pred keymapp) km) km)
             (_ (key-binding prefix 'accept-default)))
         keymap)
       nil nil t (lambda (binding)
                   (not (string-suffix-p "-argument" (cdr binding))))))))

But now there's code duplication between three different indicators, so I'll refactor that before updating the wiki.

[minad]

@oantolin What about adding the which-key indicator ootb to Embark? Even if you don't use it, it seems popular and we had to update it many times now which is painful for users. Generally we should keep the number of dynamic dependencies small (only adding completion system support), but we should make an exception for the which-key indicator.

[oantolin]

Sure, probably a good idea. I keep forgetting to fix it because it's not in Embark.

[minad]

Another question is if the wk-indicator should auto activate itself somehow. I would keep our default verbose indicator and users can setup the wk-indicator optionally in their configuration. The configuration snippet from the README could be updated to include the setup.

[oantolin]

Yes, that sounds good: replace the code in the wiki with the configuration code.

[minad]

Yes, that sounds good: replace the code in the wiki with the configuration code.

I would add it to the README as part of the configuration example (commented out as optional configuration) since it is an official feature.

[oantolin]

The wiki also includes code to dismiss the which-key indicator when you switch to the completing-read prompter. That is currently baked in for the verbose indicator. We should probably have a more general approach for that problem.

[oantolin]

Oh, I had misread what you said about the which-key indicator configuration. You are suggesting putting it in the README. I guess that would be OK too.

[bdarcus]

If the indicator makes clear what's going on, do we need the confirmation prompt?

[oantolin]

Maybe only for some actions. I do appreciate a prompt before deleting a bunch of files or even before killing a bunch of buffers.

[oantolin]

Now that I've been using embark-act-all for about a week, I'd like to finish up this issue of the confirmation prompt. Currently you always get prompted. It's probably better to have this configurable, so you only have to confirm certain (configurable) actions.

I thought at some point that maybe we could reuse embark-allow-edit-actions for this purpose, but probably not. That variable conflates two types of actions:

• Some actions are on it because they need further input besides the target at the prompt where the target is injected; an example is shell-command, you typically use it as an action on a non-executable file and want to type what program you actually want to run on the target.

• Other actions are on the default value just of embark-allow-edit-actions because they are "dangerous" (which just means I personally like a confirmation before carrying out the action), like delete-file or kill-buffer.

Maybe we should just make those two lists different? How about using embark-allow-edit-actions only for the first kind, and adding a new embark-confirm-actions list for the second type. Then embark-act and embark-act-all could both respect both lists.

I guess we could even make embark-act-all ask for confirmation query-replace-style with y, n, !, etc. But maybe that's overcomplicating things.

Maybe even just removing the confirmation is fine. Thoughts?

[minad]

Separate config variables make sense. How will act behave for the confirm actions? Also with y/n?

[oantolin]

Yes, also y/n, for consistency with embark-act-all and to make a clear distinction with embark-allow-edit-actions.

[oantolin]

I just realized confirmation can be done via a pre-action hook:

(cl-defun embark--confirm (&key action target &allow-other-keys)
  (unless (y-or-n-p (format "Really run %s on '%s'? " action target))
    (user-error "Cancelled")))

(push #'embark--confirm (alist-get 'ACTION embark-pre-action-hooks))

[minad]

Great, then let's use that instead. We should update the docs of embark-allow-edit-actions such that it is clear that these should only be used if real editing is desired and also remove actions from there and use embark--confirm.

[oantolin]

Done. 407ce26