minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.24k stars 103 forks source link

Questions about a consult-notes command attempt #210

Closed hmelman closed 3 years ago

hmelman commented 3 years ago

I'm trying to use consult--multi to manage several directories of notes files. I have the following:

(defun hrm-notes-closure (dir)
  "Return a function for an hrm-notes source rooted in DIR.
The function called with no arg returns a list of filenames in DIR.
Called with an arg f opens the file in DIR."
  (lambda (&optional f)
    (if f
    (find-file (expand-file-name f dir))
      (directory-files dir))))

(defvar hrm-notes-restaurants-function (hrm-notes-closure "~/Dropbox/Restaurants/"))
(defvar hrm-notes-lectures-function (hrm-notes-closure "~/Dropbox/Lectures/"))

(defvar hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category file
    :face     consult-file
    :history  hrm-notes-history
    :items    ,hrm-notes-restaurants-function
    :action   ,hrm-notes-restaurants-function))

(defvar hrm-notes-lecture-source
  `(:name     "Lectures"
    :narrow   ?l
    :category file
    :face     consult-file
    :history  hrm-notes-history
    :items    ,hrm-notes-lectures-function
    :action   ,hrm-notes-lectures-function))

(defvar hrm-notes-sources
  (list 'hrm-notes-restaurants-source
    'hrm-notes-lecture-source))

(defvar hrm-notes-history nil
  "History variable for hrm-notes.")

(defun hrm-notes ()
  "Find a file in a notes directory."
  (interactive)
  (consult--multi
   hrm-notes-sources
   :prompt "Notes File: "
   :history 'hrm-notes-history))

First off, it works, but do you see any problems in the above?

I'd like to figure out how to add annotations to these. At first just the source name and then maybe dates and sizes like marginalia does for files. I couldn't get that going, even for just the simple fixed string case. I tried adding a :annotate to the source and to the consult--multi call with a function that returned ("Restaurants") but it didn't show. For file attributes I'll have another problem as the default-directory will be different. Do you have a recommendation for how to get it to the annotation function? I could make a different one for each source, or store it in a property. It wasn't clear to me if the return of :items had to be just a list of strings or if like candidates it could be a list of cons with a string and value that could be the full path.

I had tried before but was confused by how to ultimately call find-file, so the addition of the :action param definitely helped me. I think I now get how :state and "restore" works with the new description. IIRC I'd probably rename "restore" to "final" and say the final call is used to restore things if previewing has occurred and then to perform the desired action.

When I get this done I'll add it to wiki as an example and I'll give comments on consult--multi documentation.

minad commented 3 years ago

IIRC I'd probably rename "restore" to "final" and say the final call is used to restore things if previewing has occurred and then to perform the desired action.

Yes, I considered this and up to now I have been to lazy to change it everywhere. This is somehow "historic" since it all came out of the original preview function definition, where the restore name made more sense.

It wasn't clear to me if the return of :items had to be just a list of strings or if like candidates it could be a list of cons with a string and value that could be the full path.

It has to be a list of strings or a function returning a list of strings.

Regarding annotations, I will check if this functionality still works.

minad commented 3 years ago

Annotations work for me:

(nconc consult--source-buffer `(:annotate ,(lambda (cand) (format "==%S==" cand))))
minad commented 3 years ago

The main problem in your source definitions above is that you are using :action and :items wrong. Items should return a list of strings. Action should take the candidate and act on it, e.g., jump to it/open it. Besides that it should work what you are doing. Please report back if it you manage to get it working!

minad commented 3 years ago

Right, I missed this question:

For file attributes I'll have another problem as the default-directory will be different. Do you have a recommendation for how to get it to the annotation function?

You can add text properties to the candidates. These can then be extracted in the :annotate function.

To the problem mentioned by @oantolin, Marginalia annotations should work as expected. They won't work if the path is not correct or not relative to the default directory. I recommend using abbreviated or absolute paths. Or provide your own annotator.

oantolin commented 3 years ago

I'd suggest just changing default-directory in the minibuffer to the right place. Just add a (setq default-directory dir) to those closures.

The main problem in your source definitions above is that you are using :action and :items wrong. Items should return a list of strings. Action should take the candidate and act on it, e.g., jump to it/open it. Besides that it should work what you are doing. Please report back if it you manage to get it working!

But it does, right? Called with no arguments his closures return a list of strings, called with one argument they visit the file.

hmelman commented 3 years ago

But it does, right? Called with no arguments his closures return a list of strings, called with one argument they visit the file.

Yes, as I said it does work. But adding this doesn't show any annotations for me:

(defun hrm-annotate-rest (cand)
  "Annotation function for restaurants"
  (format "== Restaurant ==" cand))

(defvar hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category file
    :face     consult-file
    :history  hrm-notes-history
    :annotate hrm-annotate-rest
    :items    ,hrm-notes-restaurants-function
    :action   ,hrm-notes-restaurants-function))

It also doesn't work if I add this to the consult--multi call

   :annotate 'hrm-annotate-rest
minad commented 3 years ago

But it does, right? Called with no arguments his closures return a list of strings, called with one argument they visit the file.

Oh right, I was confused by the code and I missed this. I somehow see the point since @hmelman wants to reuse a single function. The intended design is to use different functions for everything. Maybe people prefer a different design. Sources could also be defined as a single function taking an action argument, e.g. 'annotate, 'items and so on. I intentionally implemented it like this since it is easier to replace fields of sources. You can only replace the annotation function etc. Otherwise you would have to write a wrapper function catching the action and delegating the rest to the original function.

I'd suggest just changing default-directory in the minibuffer to the right place. Just add a (setq default-directory dir) to those closures.

Sounds good, if each source has an associated default directory you can change there. But you don't mean in the context of Marginalia, right?

minad commented 3 years ago

@hmelman Does this work for you?

(nconc consult--source-buffer `(:annotate ,(lambda (cand) (format "==%S==" cand))))

If not my usual questions apply. Do you have the newest versions of everything? Which Emacs version and so on.

hmelman commented 3 years ago

I don't need to use the same function it just seemed convenient. For this command all my sources will be very similar, just differing in the directory and some names. so after I got it working with a few functions it was easy to refactor to one.

minad commented 3 years ago

I don't need to use the same function it just seemed convenient. For this command all my sources will be very similar, just differing in the directory and some names. so after I got it working with a few functions it was easy to refactor to one.

Sure. It is not wrong what you are doing. It just confused me since I was mentally fixated to my intended design.

hmelman commented 3 years ago

I just updated to latest from melpa (I think I was using yesterdays before). No, doing your nconc doesn't change the display of consult-buffer for me. Yes, I checked that consult--source-buffer was modified. Macport of Emacs 27.1.

minad commented 3 years ago

@hmelman Do you see the Marginalia annotations instead? Please disable marginalia-mode and recheck.

minad commented 3 years ago

@oantolin and I made this intrusive design decision in Marginalia that it overrides with its annotations if enabled, since it "knows more" and generally provides better annotations than the default annotators, in particular the ones provided by Emacs itself. If you use the 'file or 'buffer category, then Marginalia will set in and show its annotations. If you want your own annotator, you should use a different category.

hmelman commented 3 years ago

Yes, disabling marginalia-mode shows my annotations.

minad commented 3 years ago

I didn't suggest this immediately since I am using the light Marginalia annotators by default which do not override file and buffer. But if you use heavy annotators, then Marginalia overrides file and buffer. See marginalia-annotators-heavy.

hmelman commented 3 years ago

And it looks like with marginalia-mode disabled, it automatically shows the source :name value which is nice. I'm not sure what mechanism is doing that.

hmelman commented 3 years ago

Categories still aren't completely clear to me. I know they are defined in the completion api but are they used for anything in this whole stack other than to choose annotations?

minad commented 3 years ago

And it looks like with marginalia-mode disabled, it automatically shows the source :name value which is nice. I'm not sure what mechanism is doing that.

This is a consult feature. consult--multi-annotate adds the source names.

Categories still aren't completely clear to me. I know they are defined in the completion api but are they used for anything in this whole stack other than to choose annotations?

They are also used to associate Embark actions with the candidate. Furthermore the category can override the completion style, see completion-category-defaults and completion-category-overrides.

hmelman commented 3 years ago

More a marginalia question but I'll continue it here. I've changed my sources to use :category note and my own annotation function. I have:

(setq marginalia-annotators '(marginalia-annotators-heavy marginalia-annotators-light nil))
(define-key minibuffer-local-map (kbd "M-a") 'marginalia-cycle)

because I want to default to heavy and use M-a to cycle to light and no annotations. But I find my annotations don't show when heavy is selected. I thought changing category to note (which isn't in heavy or light, they are the default values) would mean marginalia wouldn't override my annotation function.

oantolin commented 3 years ago

@minad

I'd suggest just changing default-directory in the minibuffer to the right place. Just add a (setq default-directory dir) to those closures.

Sounds good, if each source has an associated default directory you can change there. But you don't mean in the context of Marginalia, right?

I did mean in the context of Marginalia. The idea was that if the default-directory setting is correct, Marginalia can use its normal file annotator, since @hmelman is using the file category and he said about the annotations that he wanted "at first just the source name and then maybe dates and sizes like marginalia does for files", so I thought he'd be OK with just reusing Marginalia's heavy file annotator.

But maybe setting the default directory in the minibuffer doesn't work, since Marginalia actually uses the value of default-directory from (buffer-window (minibuffer-selected-window)), right?

minad commented 3 years ago

@hmelman It seems you have found a bug with this custom annotator and marginalia heavy. I can confirm this, I am also not seeing an annotator in this case. The problem is probably the consult-multi dispatcher. I will push a fix.

minad commented 3 years ago

@oantolin Yes, I am sure you can somehow mess around with Marginalia to fix the directory such that local paths work. But I would not go that route it will be too brittle. The annotators are executed in the original window. So if the default-directory is correct there it should work.

hmelman commented 3 years ago

@oantolin I think it won't work because different candidates will have a different directory (that's the point of this command using multiple sources, collecting files from different directories). I'd need to change the default-director each time the annotator is run and I don't see how I'd do that? Unless the default marginalia file annotator got a setup hook. ;)

oantolin commented 3 years ago

Oh! Sorry @hmelman, I forgot you were trying to combine multiple sources. I think I'd recommend not using the file category. I would:

minad commented 3 years ago

@oantolin This sounds like what @hmelman is planning to do! @hmelman I pushed a fix https://github.com/minad/marginalia/commit/86c0461271d407f5676a8af3776e73832458364f.

hmelman commented 3 years ago

Thanks, I'll get the fix when it hits melpa. Yes I've got some of it working with the dir in a text property. I'm going to play with marginalia--fields for my annotation, any thoughts about putting some of it in a public api to help people write their own annotators?

hmelman commented 3 years ago

I seem to be hitting something else that's slightly annoying. I'll change my closure function or a source; I check the value of the source variable and it's definitely updated. But if I run hrm-notes I don't get the new behavior. If I restart emacs and load this file I do. Is consult--multi caching the sources somehow? I see the let binding of consult--cache which I read as setting it to nil so no caching, but maybe I'm missing something? If there is sources caching I'd love a way to disable it during development.

minad commented 3 years ago

I'm going to play with marginalia--fields for my annotation, any thoughts about putting some of it in a public api to help people write their own annotators?

No plans as of now to do that. The same applies to consult--multi, consult--read, etc. I don't want to give stability guarantees on these yet. As an example I opened up the consult-find-command variables too early while I still wasn't fully happy with the design. Therefore the internal APIs will stay like internal for a longer time, maybe forever. It is much easier to make an API public than to remove it. But this does not mean that you cannot use these functions, just don't complain when things break. But things are getting more and more stable.

hmelman commented 3 years ago

No plans as of now to do that.

I get it. I'll play with it and provide some feedback on how I find it to use. Maybe it will help if/when you eventually get to making a public version.

minad commented 3 years ago

Is consult--multi caching the sources somehow? I see the let binding of consult--cache which I read as setting it to nil so no caching, but maybe I'm missing something? If there is sources caching I'd love a way to disable it during development.

No, there is no caching between consult--multi sessions. These cache facilities are just there in order to share some computations between different sources, but it is probably an unnecessary optimization as long as your recentf list does not have 10k entries. Probably you are not updating the variables or functions properly, in particular if a restart fixes this. Note that reevaluating defvar for example keeps the original value.

hmelman commented 3 years ago

Probably you are not updating the variables or functions properly, in particular if a restart fixes this. Note that reevaluating defvar for example keeps the original value.

I'm typically using C-M-x in the defvar which does change a non-void value. As I said, I explicitly checked the value of the hrm-notes-restaurants-source variable and it's definitely updated. AFAICS I shouldn't have to update hrm-notes-sources which is just a list of source variable symbols, but doing so doesn't help either. Same for re-evaluating the command hrm-notes which again, I shouldn't have to do after evaluating the source variable, right? I'll keep trying to track down something reproducible.

minad commented 3 years ago

Well if I look at this:

(defun hrm-notes-closure (dir)
  "Return a function for an hrm-notes source rooted in DIR.
The function called with no arg returns a list of filenames in DIR.
Called with an arg f opens the file in DIR."
  (lambda (&optional f)
    (if f
    (find-file (expand-file-name f dir))
      (directory-files dir))))

(defvar hrm-notes-restaurants-function (hrm-notes-closure "~/Dropbox/Restaurants/"))
(defvar hrm-notes-lectures-function (hrm-notes-closure "~/Dropbox/Lectures/"))

(defvar hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category file
    :face     consult-file
    :history  hrm-notes-history
    :items    ,hrm-notes-restaurants-function
    :action   ,hrm-notes-restaurants-function))

There you initialize the hrm-notes-restaurants-function with the lambda, then you initialize the hrm-notes-restaurants-source with the lambda. When you update hrm-notes-restaurants-function or hrm-notes-closure the :items and :action is not updated automatically. If you evaluate hrm-notes-restaurants-source you should see that it is not updated.

hmelman commented 3 years ago

Ok, I was probably either not evaling hrm-notes-restaurants-source or doing so and accidentally testing with a note from another source that wasn't updated. Sorry for the noise.

hmelman commented 3 years ago

So I'm now trying embark integration, I have a map and when calling embark-act from the minibuffer while completing on hrm-notes, my commands are getting called, but the candidate strings passed to it seem to be stripped of their properties. I saw embark issue 145 and am running the latest code. Should it be working?

oantolin commented 3 years ago

By the time the action is called the candidates are stripped of text properties. This is because the action gets the candidate through the minibuffer, even if you don't see it happening, and completing-read strips all properties from its return value. But there is a stage before the action where you can get your hands on the candidates: an embark candidate transformer. There you can combine the candidate string with its properties into a plain string that the actions later receive. If you want file actions, you should write a transformer that returns (cons 'file full-path). If you want to make your own actions, then the transformer can return your own type in place of file.

minad commented 3 years ago

Text properties are a bit of a pain 😆 @hmelman I wonder why you need them in your case. Embark will already show the correct keymap corresponding to the source category by our consult--multi dispatching. Then in your action command you get the candidate string as if the user has entered it. No properties being present. This means if your action command is a proper command it should just work. The embark actions should be commands which read the candidate string from the minibuffer without further assumptions. Note that this is different from the consult :action, :state, :annotate functions which receive the full candidate. These are not comparable to Embark actions.

hmelman commented 3 years ago

My problem is I only want the filename, without the directory component in the completions because the paths are long and also I don't want to complete my input on the directory part. My ivy implementation of this basically used separate commands for each directory and set a default-directory when run. I could do that again, but consult's narrowing seemed ideal to use and I now like the idea of using a single command to search all notes and use narrowing only when needed. My old implementation didn't have annotations but they seem like a nice add. It did have extra commands to open or grep the selected directory and I'll add that with embark (also a command to open not with find-file but in an external application that renders markdown prettily).

Not using full paths meant that the standard file category didn't work for annotations or embark, which is fair. So the suggested alternative was to make a custom category which is great as I want to the tweek the annotations and the embark keymark for this. If there's an alternative to passing along the directory info via properties, I'm open to it. If I could select on filename and then pass on a full path to the rest of the system that could work.

Working on an embark tranformer now. Seems straight forward but some documentation on this in the embark README would be nice :)

hmelman commented 3 years ago

Ok, eliding some things I have:

(defvar notes-category 'note)

(defvar hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category ,notes-category
    :face     consult-file
    :annotate hrm-notes-annotate
    :items    ,hrm-notes-restaurants-function
    :action   ,hrm-notes-restaurants-function))

(defvar hrm-notes-sources
  (list 'hrm-notes-restaurants-source
    'hrm-notes-lecture-source))

(defun hrm-notes ()
  "Find a file in a notes directory."
  (interactive)
  (consult--multi
   hrm-notes-sources
   :prompt "Notes File: "
   :history 'hrm-notes-history))

(defun hrm-notes-embark-transformer (cand)
  "Convert a propertized notes string to a full path."
  (debug)
  (let ((dir (get-text-property 0 'notes-dir cand)))
    (cons 'note (expand-file-name cand dir))))

(unless (assoc notes-category embark-transformer-alist)
  (add-to-list 'embark-transformer-alist (cons notes-category 'hrm-notes-embark-transformer)))

(defun hrm-notes-marked (cand)
  "Open a notes file CAND in Marked 2."
  (interactive "sNote: ")
  (debug)
  (let ((dir (get-text-property 0 'notes-dir cand)))
    (call-process-shell-command (format "open -a \"Marked 2\" \"%s\"" (expand-file-name cand dir)))))

(embark-define-keymap embark-notes-map
  "Keymap for Embark notes actions."
  ("f" hrm-notes-find)
  ("d" hrm-notes-dired)
  ("m" hrm-notes-marked))

(add-to-list 'embark-keymap-alist '(note . embark-notes-map))

The command hrm-notes works, but from the minibuffer, if I invoke embark-act and call hrm-notes-marked it is invoked, but my transformer is not called. It looks like my notes category is in embark-transformer-alist properly and I see embark-consult-refine-multi-type getting called and returning with my notes category so that part is right. But it doesn't seem to make it all the way through embark machinery and I'm having some trouble following it through embark--act and the timer closure. Any hints welcome.

oantolin commented 3 years ago

Oh, Embark only calls transformers once, in this case for the consult-multi type, that's why your transformer isn't called. Embark doesn't repeatedly call transformers repeatedly until none applies, but I guess it could be changed to do that (leaving it up to the user to ensure there are no infinite cycles of transformers, or maybe even adding some cycle detection, or a limit on the number of times transformation is allowed).

Another option would be to change embark-consult-refine-multi-type to do one more round of transformation. Maybe that's safer and reasonable since it doesn't seem fair that if you wrote your own hrm-notes command from scratch you do get the chance to use a transformer, but because you use consult-multi you lose your chance.

Also, bear in mind that if you use hrm-notes-marked as an action it won't have access to the notes-dir text property: it gets cand from interactive, which I believe internally calls completing-read, and that function strips all text properties from its return value.

oantolin commented 3 years ago

The transformer (which, I know, isn't being called, but let's fix it for when that is sorted out), shouldn't assume the notes-dir property is present. Every transformer is potentially tried on every candidate, not just on the ones you want it for. It should check to see if notes-dir is present, if so do it's job, and if not return nil. Which is to say, just change let to when-let.

oantolin commented 3 years ago

While we discuss whether embark should do multiple rounds of transformation (I sort of think it shouldn't, but we'll see), here's a modified consult-multi transformer that does do one more round of transformation on the result:


(defun embark-consult-refine-multi-type (target)
  "Refine `consult-multi' TARGET to its real type.
This function takes a target of type `consult-multi' (from
Consult's `consult-multi' category) and transforms it to its
actual type."
  (pcase (get-text-property 0 'consult-multi target)
    (`(,category . ,stripped)
     (when-let ((transform (alist-get category embark-transformer-alist))
                (result (funcall transform stripped)))
       (setq category (car result) stripped (cdr result)))
     (put-text-property 0 1 'embark-consult-prefix (substring target 0 1)
                        stripped)
     (cons category stripped))
    ('nil (cons 'general target))))
minad commented 3 years ago

I defined consult-multi - no it should not. @hmelman should fix his commands! hrm-notes-marked is not a valid command.

Sorry, if you already said all this - I didn't read the last few comments and I am on mobile.

EDIT:

Now I read a bit through the comments above.

@oantolin

Also, bear in mind that if you use hrm-notes-marked as an action it won't have access to the notes-dir text property: it gets cand from interactive, which I believe internally calls completing-read, and that function strips all text properties from its return value.

No, the version by @hmelman uses "s" in interactive. But as I argued this is not a proper command and is not allowed in this form. Therefore there is also no need to make the transformer more complicated. Please let's not go this route. The use case discussed here can be resolved properly. It may need a little bit more code but it is well worth it since in the end all the actions will be proper commands in the end. This is a valuable design and we should not deviate from it.

@hmelman

The embark actions should be commands which read the candidate string from the minibuffer without further assumptions.

This is what I wrote above. If this assumption is respected, things will work as is without transformer. The commands will be more reusable and overall the design is also more robust since you don't have to define a transformer, you rely on. This is an additional moving part which may break.

minad commented 3 years ago

Now, rethinking the consult--multi recursive transformer idea you mentioned @oantolin. I somehow see the point and the consistency of doing it. But ideally this transformer infrastructure is something which is only rarely used as an implementation detail, to implement symbol dispatching or consult-multi dispatching which I think is a very special case. I would somehow see it as a failure in our design if one would have to use transformers for consult-multi commands. By defining custom sources the user can already filter early on when defining the sources and define proper sources with all the categories. Tangling consult-multi with transformers leads to unnecessary complications.

minad commented 3 years ago

There is a rather simple solution which allows you to reuse the Embark file map - concat the invisible directory path and the name of the notes. It is not a particularly beautiful solution, but this is the same approach I am using for project files.

(setq hrm-notes-restaurant-directory "~/restaurants/")

(setq hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category file
    :face     consult-file
    :items    ,(lambda ()
                 (mapcar (lambda (x)
                           (concat
                            (propertize hrm-notes-restaurant-directory 'invisible t)
                            x))
                         (directory-files hrm-notes-restaurant-directory)))
    :action   ,(lambda (name) (find-file (expand-file-name name hrm-notes-restaurant-directory)))))

(defun hrm-notes ()
  "Find a file in a notes directory."
  (interactive)
  (consult--multi
   '(hrm-notes-restaurants-source)
   :prompt "Notes File: "
   :history 'hrm-notes-history))
minad commented 3 years ago

In case you don't want to go with the invisible prefix string, now there is also the possibility to overwrite the consult-multi category. This way you get direct access to the candidate string with the disambiguation prefix character. It has been an oversight of mine that consult--multi did not allow overwriting the category. I think I had originally intended it to be fully flexible, but maybe this opens up too many footguns. See https://github.com/minad/consult/commit/13cb2dc9b265662814269ead30385b4a8c79d7b4.


(setq hrm-notes-restaurant-directory "~/restaurants/")

(setq hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category hrm-restaurant
    :face     consult-file
    :items    ,(lambda () (directory-files hrm-notes-restaurant-directory))
    :action   ,(lambda (name) (find-file (expand-file-name name hrm-notes-restaurant-directory)))))

(setq hrm-notes-sources '(hrm-notes-restaurants-source))

(defun hrm-notes ()
  "Find a file in a notes directory."
  (interactive)
  (consult--multi
   hrm-notes-sources
   :category 'hrm-note
   :prompt "Notes File: "
   :require-match t
   :history 'hrm-notes-history))

(defun hrm-notes-find (note)
  (interactive "s")
  (message "source-index=%S candidate-string=%S"
           (- (aref note 0) consult--tofu-char)
           (substring note 1)))

(embark-define-keymap embark-hrm-notes-map
  "Keymap for Embark notes actions."
  ("f" hrm-notes-find))

(add-to-list 'embark-keymap-alist '(hrm-note . embark-hrm-notes-map))
minad commented 3 years ago

Then there seems to be a third way - since our embark transformer does in fact not strip the properties when using (interactive "s"). I advise against this since then the actions are not proper free-standing commands. But the following seems to work for me too. In hrm-notes-find you have access to the properties and you can access the 'notes-dir property.

(setq hrm-notes-restaurant-directory "~/restaurants/")

(setq hrm-notes-restaurants-source
  `(:name     "Restaurants"
    :narrow   ?r
    :category hrm-note
    :face     consult-file
    :items    ,(lambda ()
                 (mapcar (lambda (x)
                            (propertize x 'notes-dir hrm-notes-restaurant-directory))
                         (directory-files hrm-notes-restaurant-directory)))
    :action   ,(lambda (name) (find-file (expand-file-name name hrm-notes-restaurant-directory)))))

(defun hrm-notes ()
  "Find a file in a notes directory."
  (interactive)
  (consult--multi
   '(hrm-notes-restaurants-source)
   :prompt "Notes File: "
   :require-match t
   :history 'hrm-notes-history))

(defun hrm-notes-find (note)
  (interactive "s")
  (message "%S" note))

(embark-define-keymap embark-hrm-notes-map
  "Keymap for Embark notes actions."
  ("f" hrm-notes-find))

(add-to-list 'embark-keymap-alist '(hrm-note . embark-hrm-notes-map))

@oantolin We may want to disable this possibility by always stripping the properties before injecting into the minibuffer? But maybe it is nice to keep the flexibility. Actually one could argue that the injection is always without loss of information and in the end the command itself is responsible if information is lost. This happens in only one case, namely if the function uses completing-read to read from the minibuffer.

minad commented 3 years ago

Now which of these solutions would you want to use?

For all options you may want to consider require-match=t, since you probably don't want to handle the case of an arbitrary string being returned from the consult--multi call. For such a string not-belonging to a source, no source :action is called.

After writing the solutions I am glad that my initial reaction against recursive embark transformers is justified since they are not needed :sweat_smile: (If we would go with recursive embark transformers it would make sense to allow arbitrary recursive transformers btw, opening up another neat endless loop footgun. We could also add cycle detection on top for the fun.) It seems our design allows for a bit too much flexibility, but this is Emacs. So why not :rofl:

oantolin commented 3 years ago

I guess I misremembered the results of some experiments I conducted. I seem to recall that reading strings with interactive "s" also stripped the properties.

I agree that invisible prefix is perhaps the simplest solution. Although I do like the possibility of telling consult-multi to use a different overall category. That would be a nice way to avoid the problem of not getting multiple rounds of transformation.

You seem to have had a very violent reaction towards the idea of a second round of transformation, but let me explain my rationale. It feels to me like the categories of individual sources in consult-multi are second class citizens, because they cannot use transformers of their own.

minad commented 3 years ago

lthough I do like the possibility of telling consult-multi to use a different overall category. That would be a nice way to avoid the problem of not getting multiple rounds of transformation.

Yes, this has been fixed now and should work. This was my originally intended design, but I messed this up.

You seem to have had a very violent reaction towards the idea of a second round of transformation, but let me explain my rationale. It feels to me like the categories of individual sources in consult-multi are second class citizens, because they cannot use transformers of their own.

I agree and my opinion against recursive transformers is actually not as strong (or not as strong anymore) as it sounded first. I fully agree with the first-class argument. Sorry for my initial reaction. I agree that it would be okay to have this with some cycle detection in order to prevent obvious mistakes. My initial reaction was mostly because I would to like to try first to solve things with the means we have right now instead of adding more. In particular since the problem by @hmelman has not yet been fully settled and explored. But it would be very well possible to add more flexibility here and even more potential solutions :)

If we would do multiple rounds of transformers, we would need cycle detection and I would like if it is not only done for consult-multi but for any kind of transformer. I am actually okay with having this, it would make a lot of sense for consistency.

You could also argue that it makes sense to have only two rounds for consult-multi in order to fix this second-class behavior. But in that case I would argue that consult-multi is somehow special in a sense that the candidates are already incoming well-formed and don't need further transformations. From this perspective I see the Embark transformers a bit as a hack to fixup some candidates, which should not be needed with consult-multi. Maybe I am wrong?

oantolin commented 3 years ago

From this perspective I see the Embark transformers a bit as a hack to fixup some candidates, which should not be needed with consult-multi. Maybe I am wrong?

I agree, and thus am hesitant to add multiple transformer rounds. It would be easy to do, even with cycle detection,but it just feels complicated. That's why I suggested the compromise of two rounds just for consult--multi. But if we can find idioms that make even the compromise unnecessary, even better.