Add embark support for citation keys at point

[ilupin]

This PR enhances embark support for bibtex-actions-at-point, which is inspired by the discussion in #142 and the code presented by @publicimageltd.

If embark is installed, bibtex-actions-at-point will call embark-dwim (the default) or embark-act, depending on the option bibtex-actions-embark-dwim, when one or more citation keys are found at point, where embark-dwim runs the default action and embark-act prompts the user for an action in bibtex-actions-embark-map. If no key is found, bibtex-actions-read is used for choosing entries, and functions in bibtex-actions-map can still be called via embark-act.

This PR uses bibtex-actions-get-key-org-cite and bibtex-completion-key-at-point to find citation keys, and should work with org-cite and latex-mode/bibtex-mode buffers.

BTW, embark-target-finders requires the TARGET at point being a string, while bibtex-completion-* only accept a list of citation keys as input. Therefore, bibtex-completion-* cannot be used in bibtex-actions-embark-map directly, and bibtex-actions-embark-* does the argument conversion.

[bdarcus]

Thanks!

Any thoughts on the CI warning on "with-eval-after-load" @ilupin @publicimageltd?

Any general comments on this embark-focused PR @oantolin?

[publicimageltd]

Any thoughts on the CI warning on "with-eval-after-load" @ilupin @publicimageltd?

AFAIU it's a warning which we can ignore insofar we do know what we do. But I don't know offhand how to silence this warning.

[bdarcus]

Any thoughts on the CI warning on "with-eval-after-load" @ilupin @publicimageltd?

AFAIU it's a warning which we can ignore insofar we do know what we do. But I don't know offhand how to silence this warning.

And are we comfortable ignoring it?

Just want to confirm :-)

BTW, I tried to test this, but I have some problems on my end (long story).

So a question. if in org-mode, I run C-c C-o on an org-cite citation, what's intended behavior?

Edit: BTW, just for context for some of my perspective, bibtex actions will be included in Doom Emacs, as part of the forthcoming monster selectrum (embark, consult, etc) module merger. The intention is that this completion module replace the current ivy default. The ideas here are a future-facing alternative to a current PR (submitted more than a year ago) focused on org-ref. Of course, none of my perspective is specific to Doom though.

[bdarcus]

BTW, embark-target-finders requires the TARGET at point being a string, while bibtex-completion-* only accept a list of citation keys as input. Therefore, bibtex-completion-* cannot be used in bibtex-actions-embark-map directly, and bibtex-actions-embark-* does the argument conversion.

@ilupin - are there any other alternatives you considered for this piece?

[oantolin]

BTW, embark-target-finders requires the TARGET at point being a string, while bibtex-completion-* only accept a list of citation keys as input.

I haven't read any of this new code, but this sounded a little suspicious to me. Here's what I thought would happen:

• The bibtex-completions-* functions take lists of citation key as input, but
• they are interactive commands that call completing-read-multiple; interactively they take a crm-separator-separated list of strings which gets split and parsed into a list; therefore
• the target finder can return a single string, which will be injected into the minibuffer prompt and "split" into a singleton list.

But as I said, I haven't looked at this carefully.

[bdarcus]

• the target finder can return a single string, which will be injected into the minibuffer prompt and "split" into a singleton list.

Edit: I read the above too quickly and skipped the "minibuffer prompt" bit somehow. We just need to pass the list of in-buffer keys as argument to the functions; ultimately like:

(bibtex-actions-open-notes '("one" "two" "three"))

Thanks @oantolin - this sounds familiar from our discussions of embark and CRM.

If this indeed the case, and we could avoid having to add specialized functions to do this, would have a huge advantage:

We could then just have a standardized API for these functions; take a list of keys and do X (edit: maybe not that simple in its own, but ...).

This would then allow mixing-and-matching these functions from different packages via embark.

Right???

[ilupin]

Any thoughts on the CI warning on "with-eval-after-load"

How about (require 'embark nil t)?

So a question. if in org-mode, I run C-c C-o on an org-cite citation, what's intended behavior?

You need the following code to make C-c C-o work on citations.

 (org-cite-register-processor 'bibtex-actions
    :follow (lambda (_ _) (call-interactively 'bibtex-actions-at-point)))

(setq org-cite-follow-processor 'bibtex-actions)

[ilupin]

BTW, embark-target-finders requires the TARGET at point being a string, while bibtex-completion-* only accept a list of citation keys as input.

I haven't read any of this new code, but this sounded a little suspicious to me. Here's what I thought would happen:

• The bibtex-completions-* functions take lists of citation key as input, but
• they are interactive commands that call completing-read-multiple; interactively they take a crm-separator-separated list of strings which gets split and parsed into a list; therefore
• the target finder can return a single string, which will be injected into the minibuffer prompt and "split" into a singleton list.

But as I said, I haven't looked at this carefully.

As @bdarcus said, we are dealing with in-buffer citation keys and there is no completing-read things. The keys need to be passed to bibtex-completion-open-pdf, etc., as a list.

bibtex-actions-citation-key-at-point returns (citation-key . "key1;key2") for multiple keys now, but I find it actually works if it returns (citation-key . ("key1" "key2")) and its CDR is directly passed to bibtex-completion-open-pdf. I don't know if it causes other problems.

[bdarcus]

Any thoughts on the CI warning on "with-eval-after-load"

How about (require 'embark nil t)?

I'm not really sure myself. Does that just silence the warning, but the setup code then works if embark is present?

I was also wondering, will there be any problem with a user using different maps in their config?

So a question. if in org-mode, I run C-c C-o on an org-cite citation, what's intended behavior?

You need the following code to make C-c C-o work on citations.

While I can probably help you, could you please include README additions and modifications for all this, when the code is settled? Particularly the config details.

[bdarcus]

Next big question: why do we need these bibtex-actions-embark-* functions?

Can we not just have a little function to split if a string, and leave alone otherwise, something like:

(defun bibtex-actions--process-keys (keys)
  (if (stringp keys) (split-string keys ";") keys))

... and then something like:

(defun bibtex-actions-add-pdf-to-library (keys)
  (interactive (list (bibtex-actions-read :rebuild-cache current-prefix-arg)))
  (bibtex-completion-add-pdf-to-library (bibtex-actions--process-keys keys)))

... and then remove these functions entirely?

Aside from raising the maintenance burden and decreasing flexibility, these additional functions result in really long names that are a problem in which-key (which Doom, for example, uses), at least.

BTW, if anyone has a great shorter and more general (non-bibtex-specific, because I'm expecting to be able to add support for CSL JSON in the future) name for this package ... :-)

[ilupin]

I agree that bibtex-actions-embark-* functions should be removed finally.

... and then something like:

It doesn't seem to work as expected. The tricky part is the entries are always chosen via completing-read when bibtex-actions-add-pdf-to-library is interactively called. embark also interactively calls bibtex-actions-add-pdf-to-library, which means the keys found at point are not used by bibtex-actions-add-pdf-to-library. I need to find a proper way to handle this.

embark treats interactive functions specially according to #143 (comment). I think we can just use non-interactive bibtex-completion-* functions as embark actions and pass a list of keys to them directly (see bb07e66).

[bdarcus]

I hadn't thought about the "add" commands. Could always defer those to a later PR.

[ilupin]

The problem is the same for bibtex-actions-open-* actually.

[bdarcus]

Another question (and it is a question; I'm not sure) for both @publicimageltd and @ilupin:

Right now, one effectively cannot get to the default action directly. One must open embark, and then hit RET.

And both bibtex-actions-at-point and embark-act do the exact same thing; call embark-act.

Is that the right UX?

I could see also the first running "default" instead.

Or it could be this is a user-preference thing. Do we need a defcustom to configure this?

[bdarcus]

See this new comment, and linked new commit, from @minad; it's definitely relevant.

doomemacs/doomemacs#4664 (review)

The command embark-dwim executes the default action at point.

Seems we want to bind default to embark-dwim (the new name for embark-default-action), and suggest users have a keybinding for it in the README.

So default action is embark-dwim, embark-act brings up the option menu.

From the monster doom PR I mentioned, which includes this package.

[ilupin]

I'm fine with adding a defcustom to make embark-dwim the default (see a80cb02). It will be consistent with the case where embark is not installed.

[publicimageltd]

Any thoughts on the CI warning on "with-eval-after-load"

How about (require 'embark nil t)?

I wouldn't do that, because we do not want to interfere with the way the user wants embark to be loaded. The require statement forces to load it, which I consider to be a bad style for a package (users might not expect it to be loaded).

After looking at the info page for with-eval-after-load, I still think it's the right choice. It says we should set the variables of an external packages if we need to set them anyways. This is true for all variables declared via defvar and defcustom. But still, we need to add something to a list -- so we need embark to initialize the variable before we set it, which is exactly what can be accomplished with with-eval-after-load.

See this example code:

(progn
  (setq my-var nil)
  (add-to-list 'my-var :element)
  (defvar my-var '(a b c))
  my-var)
;; -> yields (:element), but we would want (a b c :element)

[bdarcus]

After looking at the info page for with-eval-after-load, I still think it's the right choice

I don't have time ATM to look into it, but what about (when (featurep?

I just remembered that's an approach used a lot in Doom.

[publicimageltd]

After looking at the info page for with-eval-after-load, I still think it's the right choice

I don't have time ATM to look into it, but what about (when (featurep?

featurep can only check the presence of a package at runtime, that is in our case, when bibtex-actions is loaded. So what happens when embark is loaded afterwards? Nothing, unless we explicitly bind our code to the event "package is loaded", which is what with-eval-after-load does.

Maybe we should ask at reddit or stackoverflow for good ways to silence the compiler warning and best practices. But as I said above, the case of adding elements to a variable which has to be set by the package itself is exactly one of those cases, in my eyes, for which with-eval-after-load is made. It makes perfect sense to me.

[bdarcus]

Maybe we should ask at reddit or stackoverflow for good ways to silence the compiler warning and best practices.

This post seems helpful?

https://emacs.stackexchange.com/a/51463

Maybe the "eval" option he gives makes sense?

Could also just require embark, or remove this code (three lines) and put it in the README config section.

[publicimageltd]

Maybe we should ask at reddit or stackoverflow for good ways to silence the compiler warning and best practices.

This post seems helpful?

https://emacs.stackexchange.com/a/51463

The code at stackexchange wraps some definitions into an eval as to trick the byte compiler, because the called code is a macro. We don't use a macro, just some vars which can be announced correctly, so that's another use case.

Here's a good solution (I didn't try it, but it looks good): https://github.com/tarao/bundle-el/blob/original/eval-after-load-compile.el

The idea seems to be to "load" or "require" the feature only when the code is compiling. This seems to be the core part:

(let (loaded)
  (eval-when-compile
    (setq loaded
          (condition-case ()
              ;; don't eval after-loads during compilation
              (let ((after-load-alist nil))
                (require 'embark))
            (error nil))))
  (with-eval-after-load 'embark
    ....))

There's some byte compile vodoo going on in the snipped; I didn't quite get why the macro calls byte-compile in the case it is compiled. That might be a trick my snipped above doesn't catch. So maybe we just copy paste this macro, give it credit where due, and if it works, then it works, right?

I am currently procrastinating, I should do my work instead of this stuff. But it is an interesting problem. :-)

[bdarcus]

That seems clever, but OTOH, it's simpler to just require embark, or remove those lines and add to the example config in the README.

Whether to require embark or not is not only a technical question, I'm thinking. It also signals priorities and such.

I'm not sure what I think ATM.

Maybe once this is ready for review (the other issues are addressed) we can ask for feedback on the whole thing. @ilupin could add an explicit "REVIEW" comment to this piece.

But it would be good to resolve the compiler warnings to facilitate review (else I need to turn off CI or something).

[bdarcus]

@ilupin

You need the following code to make C-c C-o work on citations.

 (org-cite-register-processor 'bibtex-actions
    :follow (lambda (_ _) (call-interactively 'bibtex-actions-at-point)))

(setq org-cite-follow-processor 'bibtex-actions)

So when org-cite is merged to org master, we probably want to add the first piece to the package, and the second to the README?

[publicimageltd]

Another solution would be to stuff all embark-related initializations into a function, and then offer a minor mode for embark. Initializiation from the user pov could then look like this:

(use-package bibtex-actions
 :config
 ;; for those who want to use it with embark:
 (use-package embark
   :config 
   (bibtex-actions-embark-minor-mode))) ;; or less verbose name

The require would then be in the minor mode definition:

(define-minor-mode bibtex-actions-minor-mode
   :lighter ""
   :group 'bibtex-actions
   :global t
  (unless (require 'embark nil t)
     (user-error "Bibtex actions embark mode requires embark to be installed."))
  (if bibtex-actions-minor-mode
     ....setup...
  ....teardown...))

[bdarcus]

Another solution would be to stuff all embark-related initializations into a function, and then offer a minor mode for embark.

This is another detail I'm not super familiar with (from a developer POV), but seems worth considering.

Also occurs to me we might defer all this for a separate PR, if it holds up merging.

[bdarcus]

@oantolin - on this:

• the target finder can return a single string, which will be injected into the minibuffer prompt and "split" into a singleton list.

... I think we may have misunderstood you were saying here. Can you clarify?

Are you saying, for example, that the in-buffer at-point target-finder somehow gets run through, internally, completing-read? And since embark will convert the CRM string to a list, it should work here too, without additional code?

[oantolin]

Oh, thanks for pinging me again, @bdarcus! It gives me a chance to correct myself. My earlier comment is completing wrong because I confused two package names: bibtex-completion and bibtex-actions!

When I wrote that earlier comment I thought you wanted to use the functions from bibtex-actions (even though I mistakenly wrote bibtex-completions I was really thinking of bibtex-actions) as actions. Those functions are interactive commands, so when used as actions, Embark runs them interactively and inserts the target at the minibuffer prompt. The target must be a string for this to work. And, in this case, there would be a completing-read call involved: the completing-read call present in the bibtex-actions commands.

But I was confused, you were really talking about bibtex-completion, and that's a different ball game because the functions there are not commands (i.e., the do not have an (interactive ...) specification). When given a function which is not a command as an action, what Embark does is simply to call the function (non-interactively) passing the target as an argument.

This is good news for this PR, I believe, because for non-interactive functions, Embark does not require the argument to be a string! I believe the functions in bibtex-completion take an argument which is a list of citation keys, right? The target finder can simply return a cons of the form (cons 'citation-key list-of-keys). The cdr of the cons does not need to be a string, if you only plan to use the target with non-interactive functions.

[bdarcus]

But I was confused, you were really talking about bibtex-completion.

@oantolin - sorry, this is indeed confusing, and I guess we didn't help :-)

To recap:

• bibtex-actions interactive commands wrap bibtex-completion (non-interactive) functions
• @ilupin recently added a bibtex-actions-at-point command, which wraps bibtex-completion-at-point
• this PR, then, extends that to "embarify" at-point command access
• but for a reason I don't understand, @ilupin added new bibtex-actions-embark-* functions for these, I guess in part because of the string issue he mentioned

I personally think ideally we want to remove the bibtex-actions-embark-* functions, and instead call the bibtex-actions ones directly, passing them the keys extracted with the target finder.

Edit: in other words, we want ideally to be able to use the same bibtex-actions interactive functions both with CRM, and at-point.

I think we just weren't sure how best to do that. See, for example, #143 (comment).

Does that make better sense now?

[ilupin]

While I can probably help you, could you please include README additions and modifications for all this, when the code is settled? Particularly the config details.

Of course. I will update README when there is no problem with the code.

So when org-cite is merged to org master, we probably want to add the first piece to the package, and the second to the README?

Yes.

[bdarcus]

Maybe better for a separate PR, but ...

If so, I guess this goes back to menus: figuring out how to get which-key to display labels instead of function names. That way the menus could at least be consistent.... But I think that's waiting on an unmerged which-key PR.

For short-to-medium run, we could add config info (probably on the README or the wiki), that makes use of this, to replace the command names in the menus with their descriptions:

https://github.com/justbur/emacs-which-key#custom-string-replacement-options

So rather than seeing "bibtex-completion-open-any" a user would see, for example, "Open (PDF or link)", both in buffer and minibuffer. Incomplete example for illustration.

(which-key-add-keymap-based-replacements bibtex-actions-map
  "o" '("Open source (PDF or link)" . bibtex-actions-open)
  "p" '("Open source (PDF)" . bibtex-actions-open-pdf)
  "l" '("Open source (link)" . bibtex-actions-open-link)
  "n" '("Open notes" . bibtex-actions-open-notes))

(which-key-add-keymap-based-replacements bibtex-actions-map-buffer
  "o" '("Open source (PDF or link)" . bibtex-completion-open-any)
  "p" '("Open source (PDF)" . bibtex-completion-open-pdf)
  "l" '("Open source (link)" . bibtex-completion-open-doi-or-url)
  "n" '("Open notes" . bibtex-completion-open-notes))

Incidentally, this is the same mechanism that Doom uses for its which-key-based menus (though hidden behind a macro). So we could hopefully include this in that biblio module as well, so the Doom experience is as expected upfront.

Edit: not sure if this is new (looking more, I don't think so), but it might be worth experimenting with which-key-enable-extended-define-key, as that seems to suggest #57 should work.

Ideally whatever we do works the same in which-key and in the default embark menu. It seems embark does support 'simple menus" as in #57, so then perhaps just doing that and adding the above which-key config would get what we need?

[ilupin]

If so, I guess this goes back to menus: figuring out how to get which-key to display labels instead of function names. That way the menus could at least be consistent.... But I think that's waiting on an unmerged which-key PR.

For short-to-medium run, we could add config info (probably on the README or the wiki), that makes use of this, to replace the command names in the menus with their descriptions:

With the workaround in d11dc1d, interactive commands bibtex-actions-* can be used in the bibtex-actions-buffer-map. Now bibtex-actions shows same embark action names in minibuffer and regular buffers.

The workaround, i.e. overriding embark--act, is only effective in bibtex-actions-at-point by design and doesn't affect any other code. Therefore, users cannot use bibtex-actions-buffer-map normally outside bibtex-actions-at-point, and the keymap should not be added to embark-keymap-alist.

[bdarcus]

The workaround, i.e. overriding embark--act, is only effective in bibtex-actions-at-point by design and doesn't affect any other code. Therefore, users cannot use bibtex-actions-buffer-map normally outside bibtex-actions-at-point, and the keymap should not be added to embark-keymap-alist.

Do you want to add a warning to that effect in the docstring?

PS - this is looking good!

[bdarcus]

@ilupin - could you update the first post with the commit message, so I can squash merge it, and use that?

I'm a bit distracted ATM.

[bdarcus]

This looks RTM.

Last call for feedback. I'll plan to merge later today (I'm in US West Coast ATM) or tomorrow.

[bdarcus]

Thanks again @ilupin; this is a nice addition!

[bdarcus]

@ilupin - back from travel, and so have more time to play with the new embark at-point functionality.

Are we missing something here to configure, or should it "just work" if I have embark configured?

https://github.com/bdarcus/bibtex-actions#basic

It currently doesn't appear to be working for me. embark-act shows no bibtex-actions commands.

Edit: if I'm in a latex buffer, and run bibtex-actions-at-point, if it doesn't find the item, it now opens a warnings buffer. I thought it should instead open the the minibuffer?

[ilupin]

It doesn't need additional configuration.

embark-act shows no bibtex-actions commands.

It is expected if you call embark-act, rather than bibtex-actions-at-point, on citation keys in buffer. bibtex-actions-buffer-map and bibtex-actions-citation-key-at-point is only used inside bibtex-actions-at-point.

Edit: if I'm in a latex buffer, and run bibtex-actions-at-point, if it doesn't find the item, it now opens a warnings buffer. I thought it should instead open the the minibuffer?

Yes. What is the warning message?

[bdarcus]

It doesn't need additional configuration.

What I thought. Hmm ...

embark-act shows no bibtex-actions commands.

It is expected if you call embark-act, rather than bibtex-actions-at-point, on citation keys in buffer.
bibtex-actions-buffer-map and bibtex-actions-citation-key-at-point is only used inside bibtex-actions-at-point.

So how do you get the embark-act menu at-point?

Edit: and for now (until org-cite), is latex the only context where this will work?

Edit: if I'm in a latex buffer, and run bibtex-actions-at-point, if it doesn't find the item, it now opens a warnings buffer. I thought it should instead open the the minibuffer?

Yes. What is the warning message?

That the item key isn't found. In this case, the key was wrong; if I correct it, bibtex-actions-open correctly runs.

[ilupin]

So how do you get the embark-act menu at-point?

(setq bibtex-actions-embark-dwim nil). It is described in README.

Edit: and for now (until org-cite), is latex the only context where this will work?

No. There is a bibtex-completion-get-key-org-bibtex in bibtex-completion-key-at-point-functions for org-mode. It also works in bibtex-mode.

That the item key isn't found. In this case, the key was wrong; if I correct it, bibtex-actions-open correctly runs.

I see. In this case, bibtex-actions-citation-key-at-point does find a key, but it has no means to check whether the key is valid or not. I think it is reasonable for bibtex-actions-at-point to pass the key to an action, resulting in a warning if the action fails though, rather than fallback to completing-read.

[bdarcus]

(setq bibtex-actions-embark-dwim nil). It is described in README.

Is there a way we can have both though?

If the basic metaphor of embark-dwim, as described by Daniel Mendler, is left-click, and embark-act is right-click contextual menu, seems ideally both should be accessible at-point?

Like, hit RET (well, maybe not this, but ...) for embark-dwim and some binding for embark-act?

That's what I was assuming with the question at least.

I think it is reasonable for bibtex-actions-at-point to pass the key to an action, resulting in a warning if the action fails though, rather than fallback to completing-read.

OK.

[ilupin]

If the basic metaphor of embark-dwim, as described by Daniel Mendler, is left-click, and embark-act is right-click contextual menu, seems ideally both should be accessible at-point?

Like, hit RET (well, maybe not this, but ...) for embark-dwim and some binding for embark-act?

It is what bibtex-actions-at-point does when bibtex-actions-embark-dwim is nil.

In fact, when bibtex-actions-embark-dwim is t, call bibtex-actions-at-point == call embark-dwim == call the action binded to RET in bibtex-actions-buffer-map.

If bibtex-actions-embark-dwim is nil, call bibtex-actions-at-point == call embark-act. In this case, RET runs the action embark-dwim would call.

[bdarcus]

Now that I committed the follow processor (I've opened a Doom PR to add OC support, so helpful to have that merged), back to this @ilupin:

So my big question: is there a way for us to have both bibtex-actions-embark-dwim t (for example using the OC follow processor where it's available via org-open-at-point), AND be able to call embark-act from point and get our map options?

That would be ideal. embark-act and embark-dwim are designed to be used together, after all.

It's also a bit confusing to me that when on an org-citation, calling embark-act gives a menu without contextual actions.

If yes, what's involved in adding that capability?

If no, maybe we should make that nil by default?

Hmm ... I just tested #156 with bibtex-actions-embark-dwim set to nil, and it didn't work (does work with t). But bibtex-actions-at-point also isn't working ATM, so maybe it's some other weird thing. Please let me know if you have a chance to test it.