oantolin / embark

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

Going beyond strings #495

Closed DamienCassou closed 2 years ago

DamienCassou commented 2 years ago

I maintain a set of packages (libmpdel, mpdel and ivy-mpdel) to make it easy to control music from Emacs.

The ivy-mpdel package provides an Ivy-based interface to navigate my music and act on it. For example, a command lists in the minibuffer all artists on my disk with completion. From an artist, pressing RET would list its albums. I can also get a contextual menu on an artist to, e.g., add all songs of the artist to the playlist. When listing albums of an artist, I can basically do the same: pressing RET to get a list of the album's songs or getting a contextual menu.

Having switched to embark, I would like my package to provide similar support using the standard completion mechanism. So I created a command which lists artists using completing-read with libmpdel-artist as category metadata. Then I wanted to add a contextual menu with embark so I can quickly add the candidate artist to the playlist:

(embark-define-keymap embark-libmpdel-artist-actions
  "Keymap for actions on an artist."
  ("p" libmpdel-current-playlist-add))

(add-to-list 'embark-keymap-alist '(libmpdel-artist . embark-libmpdel-artist-actions))

And here is the definition of the function used in the keymap:

(defun libmpdel-current-playlist-add (entity)
  "Add ENTITY to the current playlist.

ENTITY can also be a list of entities to add."
  (libmpdel-playlist-add entity 'current-playlist))

As you can see, it is not a command so I would expect embark to pass the entity as argument. Problem is: embark doesn't pass an artist (a struct) as argument, it passes the artist name (a string). This obviously makes sense because the whole completing-read infrastructure seems to rely on strings which is why my call to completing-read is complicated:

(cl-defun libmpdel-completing-read (prompt entities &key category)
  "PROMPT user to select one entity among ENTITIES.
Return the selected entity.

Transform each entity to a string with `libmpdel-entity-name'.

The user is allowed to exit by typing a string not matching any
entity.  In this case, the user must confirm and the typed string
is returned.

CATEGORY may be used to specify the type of object being listed.
This is used by some packages to show additional information
about each candidate or to provide contextual menus."
  (let* ((map (make-hash-table :test 'equal :size (length entities)))
         (entity-strings (mapcar (lambda (entity) (funcall #'libmpdel-entity-name entity)) entities))
         (metadata (list 'metadata (when category (cons 'category category)))))
    (cl-mapcar (lambda (entity entity-string)
                 (puthash entity-string entity map))
               entities entity-strings)
    (let ((entity-string (completing-read prompt
                                          (lambda (string predicate action)
                                            (if (eq action 'metadata)
                                                metadata
                                              (complete-with-action
                                               action entity-strings string predicate)))
                                          nil 'confirm)))
      (gethash entity-string map entity-string))))

The first lines convert a list of structs into a list of strings to pass to completing-read. The last line finds the struct back from the return value of completing-read.

Question: Given that the completing-read architecture only works on strings, would it be possible/desirable for my code to store the struct as a "property" (text property?) of each candidate string so the code can find the struct back when using embark?

oantolin commented 2 years ago

Storing the entity struct in a text property should work as long as your actions are non-interactive functions. I recommend another approach I'll mention below, but if you do wish to use text properties you should be aware that completing-read removes all text properties from its return value and, accordingly, Embark also removes text properties when injecting targets to commands used as actions (the command was likely to read that argument via completing-read anyway). When you use a non-interactive function as an action, Embark passes the string it obtained as-is, including any text properties.

An alternative strategy would be to make all of the actions into commands and have their interactive spec turn look up the full entity struct given the string. Here's a quick mock-up of what I mean:

(defun libmpdel-current-playlist-add (entity)
  "Add ENTITY to the current playlist."
  (interactive
   (list (libmpdel-completing-read "Entity: "
                                   (get-all-entities-somehow)
                                   'libmpdel-entity)))
  (libmpdel-playlist-add entity 'current-playlist))

(Or maybe several such commands separated by type of entity with more specific prompts and categories, like the libmpdel-artist category you used.)

The advantage of doing things that way is that people may discover they use these command a lot and prefer it as an entry point, and they may bind that to a key instead.

With Ivy actions typically you pick one operation on your items to serve as entry point and all others are implemented as actions from within that one. In the recommended Embark approach all of the operations on the items are implemented as commands on the same footing, all can be used as an entry point and all can be used as actions. For example, you can call any one of find-file, rename-file, delete-file or copy-file and when it is prompting you for a file use any one of those as an Embark action on the completion candidates. In an Ivy approach you'd "bless" one of them, namely counsel-find-file, to be the only entry point, and only have actions to rename, copy or delete from there.

Hugo-Heagren commented 2 years ago

I have run into similar problems with my package (very similar in spirit), consult-emms. The second approach doesn't really work for me thought, because part of what I wanted was a package with very few entry-point commands, which were each very powerful. As a results, I also ended up solving it with text properties (there's some discussion here).

You can see how I implemented adding all an artist's tracks here. There's quite a lot of abstraction, so you might have to dig through lots of macros, but you might find it useful. I also used text properties for adding individual tracks which might be a bit clearer.

minad commented 2 years ago

@Hugo-Heagren

I have run into similar problems with my package (very similar in spirit), consult-emms. The second approach doesn't really work for me thought, because part of what I wanted was a package with very few entry-point commands, which were each very powerful. As a results, I also ended up solving it with text properties (thttps://github.com/minad/consult/issues/470's some discussion here).

I wonder what the reasons are for the "few entry point" UI. Is it because you use consult--multi based multi source commands as main entry points, which present you all the items at once? Then depending on the category of the respective sources you attach different special actions?

Anyway, there is no right or wrong here. Both approaches are well supported and it boils down to UI preferences. However the treatment of actions=commands on an equal footing as described by @oantolin in https://github.com/oantolin/embark/issues/495#issuecomment-1111462501 is somehow a natural outcome if one reuses existing commands as actions and enhances existing completion categories with actions. Embark itself defines very few custom actions. Most actions in the predefined Embark keymaps are just regular builtin toplevel commands.

Hugo-Heagren commented 2 years ago

I wonder what the reasons are for the "few entry point" UI. Is it because you use consult--multi based multi source commands as main entry points, which present you all the items at once? Then depending on the category of the respective sources you attach different special actions?

Exactly. I tend to listen to music in the background while working on other things, so I wanted a very simple, low-cognitive-load workflow for controlling what I'm listening to. With the current setup I have one command/key binding to search my whole library, and add whatever I select (in the appropriate way. As you say, there are different actions for different sources). I can move through the library in this way too -- I have actions for things like "present all tracks in the album at point in the minibuffer", but because the UI is consistent across all these commands the workflow is very intuitive (for me at least).

(I should add that the readme note about embark support not being implemented is no longer true. Embark support is almost completely finished to my satisfaction.)

Anyway, there is no right or wrong here.

Very true. My way works really well for me (and hopefully a few other people find it useful), but that doesn't mean others aren't equally useful for people with different ways of working. :smile:

minad commented 2 years ago

(I should add that the readme note about embark support not being implemented is no longer true. Embark support is https://github.com/Hugo-Heagren/consult-emms/issues/4.)

Do you plan to publish consult-emms at some point on ELPA/MELPA?

Very true. My way works really well for me (and hopefully a few other people find it useful), but that doesn't mean others aren't equally useful for people with different ways of working.

Of course!

(I have another side remark since @oantolin and I recently discussed about default actions in #494, in particular about the default action of viewing or displaying a candidate. It seems that the approach of using single entry points, which goes well together with multi sources, somewhat implies that the action is a display action. In contrast, more specialized type-dependent actions are not a good fit as general entry points and also not a good fit for consult--multi. This leads to the distinction of actions and few entry points in your consult-emms project.)

Hugo-Heagren commented 2 years ago

Do you plan to publish consult-emms at some point on ELPA/MELPA?

I did originally, though the maintainer of EMMS expressed interest in having it merged as part of that package once my CA came through. I don't really mind how it's distrubuted, and the CA has actually come through now, so thank you for reminding me! I'll get it out there as soon as I can.

In contrast, more specialized type-dependent actions are not a good fit as general entry points and also not a good fit for consult--multi. This leads to the distinction of actions and few entry points in your consult-emms project.

This is interesting, I think you're right. There's a similar patten with packages like citar. Citar relies heavily on embark for its intended workflow. It does defined all of its actions as interactive commands which can be called independently, but I have never used them (and had to check to see whether they were defined interactively before making this post, though I use citar every day!).

minad commented 2 years ago

I did originally, though the maintainer of EMMS expressed interest in having it merged as part of that package once my CA came through. I don't really mind how it's distrubuted, and the CA has actually come through now, so thank you for reminding me! I'll get it out there as soon as I can.

Good! But note that then emms will acquire a dependency to Consult/Embark if you don't use some with-eval-after-load trickery. My recommendation would be to publish it separately as we also did with the separate embark-consult package, consult-flycheck etc. I am sure you will figure out a working solution with the emms developers :)

This is interesting, I think you're right. There's a similar patten with packages like citar. Citar relies heavily on embark for its intended workflow. It does defined all of its actions as interactive commands which can be called independently, but I have never used them (and had to check to see whether they were defined interactively before making this post, though I use citar every day!).

If I recall correctly, Citar goes with the actions=commands on an equal footing approach and they still do I suppose? There were a lot of influential discussions going back and forth between Embark and Citar if you search through the issue trackers, also regarding completing-read-multiple/consult-completing-read-multiple.

Another example is the Dtache package which also provides its actions/commands in a similar way. Dtache provides a keymap which you can either bind as a prefix keymap under e.g. C-c d or you can register the same keymap as an action keymap. However I think Dtache only operates on a single candidate type (Dtache sessions), which makes this approach feasible. The nice thing about the Dtache approach is that the prefix/action keymap is both useful with or without Embark and no Embark dependency is implied!

Hugo-Heagren commented 2 years ago

But note that then emms will acquire a dependency to Consult/Embark if you don't use some with-eval-after-load trickery.

Ah yes of course. My intention was to publish the files in the same repo, but have them load as a separate package, like embark-consult living within the embark codebase, but (as I understand?) being actually a separate package.

oantolin commented 2 years ago

The second approach doesn't really work for me though, because part of what I wanted was a package with very few entry-point commands

I'm not sure I understand the reason for this, @Hugo-Heagren. If a user only wants to use a few commands as entry points they can, by simply refraining from running other commands directly. Why force the user to be unable to use other commands as entry points?

oantolin commented 2 years ago

The reason I recommend making all actions independent commands, even though I personally usually wind up picking a single one as my default entry point (for file actions I use find-file and for packages I use describe-package, for example), is that I strongly dislike restricting users's freedom and other people may prefer different workflows.

That's the same reason I've repeatedly denied requests to limit embark-act to "approved" actions, i.e., those listed in a keymap registered on embark-keymap-alist.

oantolin commented 2 years ago

But having defended freedom, let me just stress the point that Embark does also let you define actions as non-interactive functions and in that case even passes the target with its text properties intact (so Embark doesn't restrict your freedom to restrict your users's freedom, 😛.)

Hugo-Heagren commented 2 years ago

with its text properties intact.

I'm not sure, because it was a while ago, but I've got a feeling that this was the reason. I wanted to use consult for consult--multi, and discovered that if wanted to use consult, and be able to do arbitrary actions on objects (like an album, which is a list of tracks), rather than the completable strings which represented them, then I would have to use text properties to get at those objects. There are other ways of course (like maintaing an alist of album names and album objects, then looking up the selected name in the list), but I looked into it and chose text properties on balance. Then, to retain and use text properties, I had to non-interactive functions.

EDIT: I realise this doesn't really answer the original question. The reason is that consult-emms started as just something I wanted to make and use, not really designed to be good for other people or alternative workflows, because originally it was just my thing on my machine. And I knew that the workflow I wanted had very few entrypoints. Over time though I realised others might also like it and now it's on github (though as you can tell, the docs aren't very well maintained!). So it was just sort of laziness as well.

DamienCassou commented 2 years ago

I'm not sure I fully understand the recommended approach of transforming functions into commands. If we take the following function as example:

(defun libmpdel-current-playlist-add (entity)
  "Add ENTITY to the current playlist.

ENTITY can also be a list of entities to add."
  (libmpdel-playlist-add entity 'current-playlist))

This function can be passed any kind of mpdel entity which is a struct representing a playlist, an artist, an album or a song. If this function is transformed into a command, what will the user see as completion candidates? For example, it cannot simply be song names because several artists might have songs with the same name or an album might be named the same as a song.

Do you suggest having all artists, albums and songs as candidates in a form that can later be parsed to recreate the struct? Such as in

for an artist, an album and a song. I must make sure the separator character appears in no entity name on its own.

Going back to the original reason for this issue and if I go with this solution, I must make sure completing-read passes such a string to libmpdel-playlist-add. If I understand correctly, this means such strings must also be used in the top-level command calling completing-read.

It feels to me that we are working around a problem. The good solution would probably be for Emacs to get a completing-read-object command that would take a list of objects and a formatter as argument, that would list the objects formatted with the formatter and that would return one object based on what the user picked.

oantolin commented 2 years ago

Do you suggest having all artists, albums and songs as candidates in a form that can later be parsed to recreate the struct?

That is certainly one possibility and I think it is the simplest if you find the resulting string tolerable. If you find strings contianing all that information too unwieldy then you have a couple of options:

Going back to the original reason for this issue and if I go with this solution, I must make sure completing-read passes such a string to libmpdel-playlist-add. If I understand correctly, this means such strings must also be used in the top-level command calling completing-read.

Yes. You can have more than one type of formatted string of course, maybe one for artists, one for albums (that would probably have to include the artist name), and one for songs (which would also probably have to include the artist), and commands that operate on each. But yes, basically what I was suggesting is, for each type of object you want to work with standardazing on a string format that identifies such an object, implementing one function that prompts for such a string with completing-read (or consult--read if you want to add some sort of preview —album covers would be cool!), and then using that function in all the interactive forms of the commands that operate on that type of object. So all your commands that work on songs, say, would look like:

(defun add-song-to-current-playlist (song)
  (interactive (list (read-song)))
  ...)

(Or you can have a single read-entity if it makes more sense to always lump songs, artists, albums, etc., together.)

This is, for example, what Emacs's file commands do: they all use strings to specify the file (and not, say, "file objects" that include info on size, modification date, permissions, etc.), and they all use the same function read-file-name in their interactive specification, either calling it directly or through the f specifier in the string form of the interactive form.

I'm not saying it's always practical to look at long strings that contain all the necessary information, and if you decide that for your case it is not practical you can certainly go with a different approach, but bear in mind that all the user sees are some strings (either the candidates, their display properties or that plus an annotation, but some string) so if your code has the problem that the string doesn't specify a unique entity, then the user has that problem too!

It feels to me that we are working around a problem. The good solution would probably be for Emacs to get a completing-read-object command that would take a list of objects and a formatter as argument, that would list the objects formatted with the formatter and that would return one object based on what the user picked.

We do have that already! It is exactly the pattern I am advocating!

(defun completing-read-object (prompt objects formatter)
  (let ((alist (mapcar (lambda (object)
                         (cons (funcall formatter object) object))
                       objects)))
    (cdr (assoc (completing-read prompt alist) alist))))

(defun some-command-on-objects (object)
  (interactive
   (list (completing-read-object "Object: " (get-objects) #'my-formatter)))
  ...)
oantolin commented 2 years ago

I am closing this issue merely to indicate I don't think Embark needs any changes to support this use case, but feel free to continue the discussion.

DamienCassou commented 2 years ago

Thank you very much for all your help. I think I understand the solutions you suggest now. Two points:

oantolin commented 2 years ago

I know it's not in the most convenient form, but you can currently get Embark to "pass an object to actions". You just need to attach the objects to the string candidates as text properties and use non-interactive functions as actions. (I tested this just now to make sure I wasn't lying, I might as well attach the example below.) That's not too uncomfortable compared to the platonic ideal you are proposing, I hope.

(defun greet (x) ; command action
  (interactive (list (read-person '(("Bob" . 47) ("Sam" . 34) ("Lisa" . 41)))))
  (message "Hello %s!" x))

(defun show-age (x) ; non-interactive action function
  (message "%s is %d years old" x (get-text-property 0 'age x)))

(embark-define-keymap people-map
  "Keymap for actions on people"
  ("a" show-age)
  ("g" greet))

(setf (alist-get 'person embark-keymap-alist) 'people-map)

(defun read-person (age-alist)
  (completing-read
   "Pick a person: "
   (let ((collection (mapcar (lambda (x) (propertize (car x) 'age (cdr x)))
                             age-alist)))
     (lambda (string predicate action)
       (if (eq action 'metadata)
           '(metadata (category . person))
         (complete-with-action action collection string predicate))))))
minad commented 2 years ago

I think the platonic ideal of completion are strings. Completion is like the Tcl of Emacs.

DamienCassou commented 2 years ago

Thank you very much for all your help. I think I know where to go with my package now. And thank you for all your hard work on your excellent packages.