User instructions to add embark support to a well-behaved package

[DamienCassou]

Hi,

I would like users of my mpdel package to be able to use embark to get contextual menu actions on music artists, albums and songs. I would like to do that without adding a dependency toward embark and I have experimented with this: https://gitea.petton.fr/mpdel/mpdel/pulls/37/files.

What you can see in this PR is:

• A minibuffer-based command (implemented in mpdel-minibuffer-list) where the user first has to pick an artist, then an album from this artist and then a song from this album. At every step, the user can call embark to act on the current entity (e.g., an album or an artist) without diving deeper. This is based on not yet merged changes to the libmpdel package that provides completion.
• A macro mpdel-minibuffer--unwrap-command that creates a function extracting a text property from a candidate string to get the underlying entity (the rationale is described in a former embark issue).
• Some automatically-executed code to create the unwrapping functions from a list of commands I want to see in embark.
• A dynamically-generated keymap containing the commands I want to see in embark (mpdel-minibuffer-map).

This alone is not enough to get embark working on candidate entities. I also need to tell my users to add this to their init file so mpdel-minibuffer.el doesn't itself depend on embark:

(with-eval-after-load "embark"
      (defvar my/embark-mpdel
        (make-composed-keymap mpdel-minibuffer-map embark-general-map)
        "Embark keymap for mpdel.")

      (dolist (category '(libmpdel-artist libmpdel-album libmpdel-song))
        (add-to-list 'embark-keymap-alist
                     `(,category my/embark-mpdel))))

I have a few problems with this approach:

• I would prefer not to ask users to introduce such code in their init file.
• I find the implementation of mpdel-minibuffer.el complicated, especially around the macro; the main reason for it to be complex is because I want embark to show documented actions.

Am I missing something?

[oantolin]

I haven't looked at the implementation of mpdel-minibuffer.el yet, so I won't comment on that, but as for the configuration issue I agree it's not great to ask your users to add that stuff to their init files. There are basically only two options, I think:

• Make a tiny extra package with the embark integration. That package can depend on mpdel-minibuffer and on embark. For example, embark-consult takes this approach.
• Just put that with-eval-after-load block in mpdel-minibuffer but don't declare Embark as a dependency. For example, citar takes this approach. The only thing from Embark you need is the single variable embark-keymap-alist, it's not worth making it a dependency.

[minad]

For these tiny integration pieces a with-eval-after-load block seems perfectly justified. If Embark actions are normal commands (normal entry points) it makes sense to define a prefix map which can be bound like a normal keymap or used with Embark. For example dtache.el/dtached.el uses that approach.

But from what I've understood your mpdel package doesn't take that approach and defines separate actions or action functions - so in a strict sense it is not a well-behaved package from the perspective of Embark. Or at least it does not follow the Embark way idiomatically. These unwrap helpers are only needed because of that. ;)

[oantolin]

By the way, I'm glad you compose your keymap with embark-general-map. The citar package does not do this and I often tried to copy a citation key, or to search for it in isearch or consult-line only to be frustrated. :)

[DamienCassou]

For these tiny integration pieces a with-eval-after-load block seems perfectly justified.

package-lint won't like that and I can't configure it beyond disabling it. Moreover, I realize I need a bit more integration if I want embark to work in the tabulated-list views that are core to mpdel. Here is some more integration code I wrote:

(defun my/mpdel-minibuffer-entity-package-at-point ()
  "Target the mpdel entity on the current line in a mpdel tablist buffer."
  (when (derived-mode-p 'mpdel-tablist-mode)
    (when-let ((entity (get-text-property (point) 'tabulated-list-id)))
      `(
        ,(aref entity 0) ;; type of entity
        ,(propertize (libmpdel-entity-name entity) 'libmpdel-entity entity)
        ,(line-beginning-position) . ,(line-end-position)))))

(add-to-list 'embark-target-finders #'my/mpdel-minibuffer-entity-package-at-point)

The line with propertize is really ugly here. I have to do that because mpdel-minibuffer.el uses the text property to find which entity is represented by a string (discussed in #495).

Make a tiny extra package with the embark integration. That package can depend on mpdel-minibuffer and on embark. For example, embark-consult takes this approach.

I think this is the only way forward but I'm unhappy. There has to be a simpler way than having all this glue code.

By the way, I'm glad you compose your keymap with embark-general-map. The citar package does not do this and I often tried to copy a citation key, or to search for it in isearch or consult-line only to be frustrated. :)

I'm only following the great manuals you write :-). Thank you for your effort here, it's greatly appreciated.

[oantolin]

Now that I've read your code I think the mpdel-minibuffer--unwrap-command is unnecessary. instead use the mpdel-core-* functions directly and add an Embark transformer that fetches the text property:

(defun get-entity-string (type target)
  (cons type (get-text-property 0 'entity-string target)))

(dolist (category '(libmpdel-artist libmpdel-album libmpdel-song))
  (setf (alist-get category embark-transformer-alist) #'get-actual-object))

[oantolin]

With the transformer I suggested above handling the minibuffer case, you can simplify my/mpdel-minibuffer-entity-package-at-point to remove the propertize call you didn't like.

[oantolin]

package-lint won't like that [the with-eval-after-load form]

I just ignore this particular package-lint complaint, because I feel I know better. :P

[oantolin]

Maybe between the keymap, the target finder for the tabulated list buffers and the tansformer for minibuffer candidates, a small mpdel-embark package is warranted after all.

[DamienCassou]

But from what I've understood your mpdel package [...] defines separate actions or action functions - so in a strict sense it is not a well-behaved package [...]. These unwrap helpers are only needed because of that.

I agree. But because my code is mostly asynchronous, I haven't found any better way.

I think the mpdel-minibuffer--unwrap-command is unnecessary. instead use the mpdel-core-* functions directly

I followed this approach and the code is much simpler. Thank you very much. I had to add a pre-action-hook though because the mpdel-core-* commands don't accept any minibuffer input: they read the list of entities from the selection in the current buffer. The pre-action-hook let me override this behavior to return the minibuffer element instead:

(cl-defun mpdel-embark-fake-selected-entities (&key target &allow-other-keys)
  (cl-labels ((fake-selected-entities
               ()
               (advice-remove 'mpdel-core-selected-entities #'fake-selected-entities)
               (list target)))
    (advice-add 'mpdel-core-selected-entities :override #'fake-selected-entities)))

With the transformer I suggested above handling the minibuffer case, you can simplify my/mpdel-minibuffer-entity-package-at-point to remove the propertize call you didn't like.

For this to work, the transformer must be changed to test if target is a string or not. I wrote that:

(defun mpdel-embark--get-target-from-string (type target)
  (cons type (if (stringp target)
                 (get-text-property 0 'libmpdel-entity target)
               target)))

This seems to work great, thank you very much.

a small mpdel-embark package is warranted after all.

indeed. You can read my code at https://github.com/mpdel/mpdel-embark/pull/1/.

[DamienCassou]

Should we close this issue?