Add groups to citar-open functions

[localauthor]

Changes:

• Adds completing-read to citar--open-note, for when multiple notes exist

• Changes citar-select-files to citar-select-resources, and calls the new function in both citar-open and citar-open-library-files

• Adds group-function, citar-select-group-related-resources, to citar-select-resources, to group candidates by type

The result of calling citar-open on an entry with associated files, notes, and a link (the following shows vertico with the consult-completing-read-multiple interface):

Selecting multiple file types, a note and a file:

Todo:
- Somehow set proper category metadata for links in citar-select-resources. I'm actually not sure if this is possible.
- Note that prior to this PR, citar-open did not add any metadata to candidates, so embark-act'ing on a file candidate did not make embark file functions available. Now embark file functions are available, but on both files and links. Neither case seems optimal, if it's not possible (or easy) to set category metadata by group.

Question:
- Does this work as expected with embark-act-all? I've not used that enough to properly test it.

[bdarcus]

@localauthor - I set this to merge to the no-crm branch, but before doing that, I'll put comments here:

1. I think the "Files" group maybe should be "LIbrary files"? WDYT?
2. I realize this when I was playing with this idea earlier: links have no annotations. Should they? If yes, what should they be? (we can worry about this later; just wondering)
3. What should happen to the existing more specific "open" commands? I guess we still need citar-open-notes at least?

Let me know your thoughts, and if you want to make any changes here before I merge?

Also, note: this now conflicts with the new base branch. You'll need to rebase and maybe fix a conflict or two.

[bdarcus]

Hmm ... actually, I wonder: do you think we should merge this to #496? Or should we just evaluate this on its own, and if we want to merge it, then do so, and I can rebase #496 on this, which I can refocus just on citar-insert-citation?

Really the CR vs CRM discussion varies depending on the command in question.

[bdarcus]

Does this work as expected with embark-act-all? I've not used that enough to properly test it.

I am just starting to test this branch. Here's what you see after running embark-act-all, and then embark-act:

Two things:

1. I guess notes should also have a file category, but remain a separate group? If yes, not sure how to do that.
2. I'm actually not sure how one would then open all of these results from there using embark-act-all, and running RET on a single candidate generates an error.

Any thoughts on the above @minad?

[localauthor]

Ok, I just rebased onto main.

I changed citar--open-notes to use citar-select-resources, so now notes are properly categorized as files and grouped as notes.

As for the question of where to merge this (either to main directly or to no-crm) , I suppose it depends on your plans for no-crm. :)

This branch changes CRM to CR in all the open functions anyway.

[bdarcus]

Let's keep it here for now. There are still some subtleties with embark-act-all to work out. Per my point 2 above, I can't see how to run embark-act-all to open all of the resulting resources, because the default action is then embark-act. I wonder if that needs to be the case, or what a better alternative might be.

[bdarcus]

Per my point 2 above, I can't see how to run embark-act-all to open all of the resulting resources, because the default action is then embark-act. I wonder if that needs to be the case, or what a better alternative might be.

There does seem to be a bug here though. If I only select one candidate, and then do embark-act-all on the results, where the default action is citar-open, I get a "no results found" (not sure why).

Even if I only select one resource using completing-read, I get a "wrong type" error (it's expecting a list; probably because citar-open is included in embark-multitarget-actions, since if you remove it from there, it works fine).

[localauthor]

All the open functions should be working as expected now (hopefully!), with the exception of embark-act-all, which I'm just starting to work through.

[bdarcus]

All the open functions should be working as expected now (hopefully!), with the exception of embark-act-all, which I'm just starting to work through.

When I use citar-open, I still get a type error when trying to open any resource.

[localauthor]

Latest commit allows embark-act-all to act appropriately in all contexts.

Each open command now includes a let-binding for embark-default-action-overrides in order to override the default action depending on target type. Before, the default action was always the original command (ie, citar-open, citar-open-notes, etc.). Since those cannot act on files or urls, the default action is now set in each case to a function that can act on files or urls --- namely, citar-file-open for files, find-file for notes, and browse-url for urls.

The edge case here is with citar-open and the matter of acting at the same time on candidates of multiple types. When there are multiple types together, the candidates are not associated with their actual type (file or url n this case) but remain typed as consult-multi. So I've added a function citar-open-multi (which see) and included it in the embark-default-action-overrides list in the let-binding in citar-open. So now running embark-act-all on candidates of multiple types (urls and files) should open all of them as expected.

Hopefully it works for you as it does for me.

[localauthor]

One issue with using consult-multi is that it requires embark-consult.el to be loaded and, specifically, embark-consult--multi-transform to be defined.

If only embark.el is loaded, these are not available.

[bdarcus]

One issue with using consult-multi is that it requires embark-consult.el to be loaded and, specifically, embark-consult--multi-transform to be defined.

If only embark.el is loaded, these are not available.

What if you define the variable? Do you get an error, or just not the behavior?

[localauthor]

embark-consult--multi-transform is a function, so it really needs to loaded for everything to fully work.

I say "fully" because citar-open still functions reasonably without it, ie, there are no errors and files/links can be opened with "RET", according to type --- even when calling embark-act-all on multiple types.

To explain: without embark-consult--multi-transform, files and urls remain typed as consult-multi. They are therefore handled by the default action associated with that type, namley, the new citar-open-multi, which opens each type appropriately. There aren't file- or url-specific actions available, in each case, but at least there are no errors.

If, as @minad seemed to be suggesting here, consult-multi were changed to multi (or the like) and included in embark.el rather than embark-consult.el, this (very minor, almost non-) issue would be moot.

[bdarcus]

Each open command now includes a let-binding for embark-default-action-overrides in order to override the default action depending on target type.

These result in "unused lexical variable" warnings.

On a separate matter, it's getting closer. But if I have this:

... and then do embark-act-all, I get this:

[minad]

@localauthor Yes of course. We should switch to multi from consult-multi to make this general and to avoid the loading issue. But I will wait for a few weeks to let this settle here, since Citar does not yet seem to have converged fully. Then we can change it across the different packages and tag new releases. Probably embark and marginalia should support both multi and consult-multi for some time and treat them as aliases.

[localauthor]

... and then do embark-act-all, I get this:

Before pressing "RET", what does the embark-indicator-menu look like for you? That is: what is the default action you are calling when do embark-act-all? If it is citar-open, then I would expect to see exactly what your screenshot shows (which is a symptom of another issue I'm trying to figure out, about which more soon).

Here is what I see before pressing "RET" in a similar series of steps:

Followed by this:

Followed by opened files, notes, and browser.

[localauthor]

These result in "unused lexical variable" warnings.

I'm afraid I don't know how to avoid such warnings, or what they mean really.

[bdarcus]

Followed by opened files, notes, and browser.

I see the same, except for this last step.

Maybe there's something wrong with my install of the branch. Running eval-buffer on citar.el does result in the correct behavior!

[bdarcus]

These result in "unused lexical variable" warnings.

I'm afraid I don't know how to avoid such warnings, or what they mean really.

@minad - any tips? He's let-binding embark-default-action-overrides per #491 (comment).

[minad]

Oh. These default action overrides should not be necessary. The default file and url actions should work ootb as should all the other actions. If not something else is wrong and should be fixed.

The correct approach is to define your own category if you want to overwrite or define your own actions.

[minad]

Ah I see, you have special open functions. I guess in this case the override is justified. But overriding browse-url for url and the consult-multi override should not be needed.

Probably you miss a defvar to fix the lexical error?

[bdarcus]

Probably you miss a defvar to fix the lexical error?

Yes; I just committed that change.

[minad]

This command is unnecessary and should be removed. Embark already dispatches consult-multi to file and url. That's the purpose of consult-multi and the corresponding Embark candidate transformer.

[minad]

Here the consult-multi override should go away and the url override probably too, in order to allow the user to customize the action on the side of Embark.

[localauthor]

Thanks for looking at this, and for the comments.

Embark and consult-multi work just as you describe when it comes to categorizing files and urls, but (as I describe here) because the source of the target is minibuffer completion, Embark sets the default action for all targets, regardless of type, to the command that opened the minibuffer in the first place (per the docstring of embark-default-action-overrides). That means that, in this case, the default action for any target type is set to citar open, which is a function that does not take files or urls as arguments. Hence the overrides. It's a bit of a hail mary, I guess, but I'm afraid I'm running up against the limits of my lisp/emacs knowledge here. ;)

As for the citar-open-multi function, it was only added to allow embark-act-all to work on properly on targets of multiple types. When using embark-act on single candidates, consult-multi works just as expected and this function isn't called. When calling embark-act-all on targets of multiple types, they haven't been transformed by the candidate transformer yet --- so they remain typed as consult-multi. Hence, another override. Again, bit of a hail mary -- but functional!

Thanks again for your comments. Really appreciate your taking the time.

[minad]

Okay, maybe I missed something here but this seems wrong. The default action is supposed to be the command that started the completion. Overriding it with something else is counterintuitive and goes against the usual Embark behavior.

Note that Embark has a minibuffer argument injection mechanism which works even if functions don't take arguments. I suggest you dig deeper into the Embark documentation.

[minad]

That means that, in this case, the default action for any target type is set to citar open, which is a function that does not take files or urls as arguments. Hence the overrides.

I am specifically referring to this. This is wrong. There is the minibuffer injection mechanism.

When calling embark-act-all on targets of multiple types, they haven't been transformed by the candidate transformer yet --- so they remain typed as consult-multi.

I will check this, but if this is the case it is a bug or issue in Embark.

I know that oantolin added some target type checking to embark-act-all and there is indeed a problem when acting on candidates of multiple categories at once. Anyways this should be handled properly by Embark itself.

EDIT: Okay, I checked Embark. Embark is doing everything alright. When you act on candidates of multiple categories at once then indeed the candidates are not transformed. Then the candidates remain consult-multi. They are only transformed if you act only on files, only on buffers or only on urls. But still, it should not be necessary to override the default action since the command itself should then be capable of handling these consult-multi candidates. You can look into consult--multi and consult-buffer as examples. There I also don't overwrite the embark default actions and everything works as expected :)

[bdarcus]

I experimented a bit with removing these lines, and things seem to work fine, until I run embark-act-all on the resulting resources. So to recap:

1. citar-open
2. narrow candidates and then embark-act-all
3. from there I get a list of resources, if I run embark-act-all, I then get the error below:

embark PCH: (user-error "No target found")

... which is I think why he introduced the citar-open-multi function.

[minad]

Did you load embark-consult? I don't know what is wrong here exactly, but the override should not be needed due to the design of embark. consult-buffer also works without override.

[bdarcus]

Did you load embark-consult?

I did require it now, and get:

(execute-extended-command nil "citar-open" nil)

I don't know what is wrong here exactly, but the override should not be needed due to the design of embark. consult-buffer also works without override.

I'll take another look.

[minad]

@bdarcus What's the status here? Did you figure out the issues?

[bdarcus]

Did you figure out the issues?

Not yet. Likely in the next few days.

[minad]

@bdarcus As a small demonstrator, this works for me:

(require 'embark-consult)
(defun open-example ()
  (interactive)
  (let* ((candidates
          (list (propertize "init.el" 'consult-multi
                            '(file . "~/.config/emacs/init.el"))
                (propertize "emacs.org" 'consult-multi
                            '(url . "https://www.gnu.org/software/emacs/"))))
         (result (completing-read "Open: "
                                  (lambda (str pred action)
                                    (if (eq action 'metadata)
                                        '(metadata (category . consult-multi))
                                      (complete-with-action action candidates str pred)))
                                  nil t)))
    (pcase (get-text-property 0 'consult-multi (car (member result candidates)))
      (`(url . ,url) (eww url))
      (`(file . ,file) (find-file file)))))