embark-related parsing problems when multiple citekeys

[bdarcus]

Am unsure why this would turn up as a bug at this point, given how many people use this package, and how long that code has been stable.

---

I ran into one more issue, which I thought I would like to report here. It affects multiple functions in the citar-citation-map, and has to do with the way they interact with citar-citation targets and citar-embark package.

Let me explain the problem by taking citar-copy-reference function as a specific example. This function accepts a list of citekeys, whose formatted references are to be inserted in the current buffer. When it receives its argument from citar--select-multiple or via an embark action on a citar-key target, all works well. But if the argument is from a citar-citation target with multiple keys, then citar-copy-reference fails to find the relevant bibliography entries.

I believe, the reason is that a citar-citation target produces a string obtained by concatenating individual keys with the " & " separator as done by the citar--stringify-keys function. Thus citar-copy-reference needs to unstringify the keys first, but its current implementation does not do this.

To give a concrete example, if citar-copy-reference is applied to the citar-citation target that corresponds to the LaTeX citation \cite{key1,key2}, then its argument becomes ("key1 & key2") instead of ("key1" "key2").

The problem can be fixed by unstringifying the argument first, e.g., by pre-processing it as follows:

(setq citekeys (mapcan #'citar--unstringify-keys citekeys))

The same issue, in fact, applies to functions citar-open, citar-open-links, citar-open-files, and citar-open-notes, each of which fails, in my case at least, to act on a citar-citation target with multiple keys.

Originally posted by @kslutsky in #834 (comment)

[bdarcus]

@kslutsky:

EDIT:

Confirmed. Now that I look, citar--unstringify-keys isn't used anywhere ATM!

As I say in the preface above, it's just surprising to me to see this, what appears to be silly, bug at this point :-)

But I also know we sometimes struggled with embark and multi targets, so it could just be vestige of that.

Are we missing something @roshanshariff?

Assuming no, I'm unsure of how to fix those interactive commands ATM. In particular, where would one do:

The problem can be fixed by unstringifying the argument first, e.g., by pre-processing it as follows ...

citar/citar.el

Lines 1527 to 1536 in 07d2a63

	(defun citar-open (citekeys)
	"Open related resources (links, files, or notes) for CITEKEYS."
	(interactive (list (citar-select-refs)))
	(pcase (let ((embark-default-action-overrides
	(cons (cons t #'citar--open-resource)
	(bound-and-true-p embark-default-action-overrides))))
	(apply #'citar--select-resource citekeys
	(mapcan (lambda (type) (list type t)) citar-open-resources)))
	(`(,type . ,resource) (citar--open-resource resource type))
	(_ (error "No associated resources: %s" citekeys))))

[roshanshariff]

I think this is indeed a vestige of when Embark removed the completing-read-multiple functionality and added multi-target actions. I believe there's no way of triggering a multi-target action from a single string target.

I don't have all the details in mind right now, but maybe we can deal with this in citar-select-refs and make sure it deals with the way Embark does its target injection.

[kslutsky]

In particular, where would one do

My point was simply that adding (setq citekeys (mapcan #'citar--unstringify-keys citekeys)) right after the interactive line makes citar-open (and other multitarget citar-embark actions) work correctly both when it is called on an multiple target within a minibuffer and within a regular buffer through embark-act:

(defun citar-open (citekeys)
  "Open related resources (links, files, or notes) for CITEKEYS."
  (interactive (list (citar-select-refs)))
  ;; The following line can be added to unstringify citar-citation targets
  (setq citekeys (mapcan #'citar--unstringify-keys citekeys))
  (pcase (let ((embark-default-action-overrides
  ...

(This is just a way to test the source of the issue, and I didn't mean to suggest that inserting such preprocessing calls into all multitarget actions is a good idea from the architectural standpoint.)

[bdarcus]

Thanks @kslutsky - yes, I was effectively meaning how to modify citar-select-refs to address this.

[kslutsky]

A different possibility would be to advise functions in the citar-embark--multitarget-actions list. For instance, if we define

(defun citar-embark--unstringify-args (citekeys)
  (list (mapcan #'citar--unstringify-keys (car citekeys))))

then citar-embark--enable can be amended with a dolist loop as follows

...
;; Unstringify arguments of all multitarget actions
(dolist (function citar-embark--multitarget-actions)
  (advice-add function :filter-args #'citar-embark--unstringify-args))
(cl-callf cl-union embark-multitarget-actions citar-embark--multitarget-actions)
...

whereas citar-embark--disable would remove the advice with

(dolist (function citar-embark--multitarget-actions)
  (advice-remove function #'citar-embark--unstringify-args))

(Since citar--unstringify-keys is currently not used at all, maybe it can be redefined to work as citar-embark--unstringify-args instead.)

[bdarcus]

Related to #837 also.

[bdarcus]

A different possibility would be to advise functions in the citar-embark--multitarget-actions list.

Why would we advise internal functions, @kslutsky?

[kslutsky]

Why would we advise internal functions, @kslutsky?

Multitarget actions receive as their argument either a list of keys or a string of keys produced by the citar--stringify-keys function. One can, of course, modify each such action individually to unstringify the argument first. It seems best to avoid this. Advising all these actions with an argument filter, which unstrignifies keys prior to calling the action itself, solves the issue uniformly. Any multitarget action can be implemented assuming an argument of list type and it will work automatically with stringified arguments.

I have a feeling, however, that I am missing the crux of your question, @bdarcus.

[bdarcus]

I have a feeling, however, that I am missing the crux of your question

Really it was just my understanding that one only should ever need to advise external functions, since we have control over our own code.

Does that clarify?

[kslutsky]

Really it was just my understanding that one only should ever need to advise external functions, since we have control over our own code.

Well, when we deal with an individual internal function, I would agree that it is typically better either to modify its code or to introduce a wrapper function. In the issue at hand, however, we want to apply the same modification to a whole list of functions. Advising seems to be perfectly suited for this job. Among other things, it plays well with the describe-function command, automatically telling the user that such-and-such function has been advised. An alternative might be to define wrapper functions with a macro, but I feel that this obscures the implementation logic considerably.

Having said all that, elisp is not my forte, so I might well be unaware of some pitfalls with function advising.

[roshanshariff]

Multitarget actions receive as their argument either a list of keys or a string of keys produced by the citar--stringify-keys function.

Just to clarify, Embark actions can be either invoked on a single target or multiple targets. If they're invoked on a single target and they have an interactive specification, the action is called as an interactive command and the target string is inserted as minibuffer input. Otherwise, if there are multiple targets or the command is not interactive, then it is called as a function and either the single target string or a list of target strings is passed as the first argument to the function. To summarize, actions can be called in any of these ways

1. (citar--some-action) called interactively followed by citekey "Foo" injected into minibuffer.
2. (citsr--some-action) called interactively followed by string "Foo & Bar" injected into minibuffer, when called on a multi-key citation (which after all is a single Embark target containing multiple &-separated keys).
3. (citar--some-action "Foo") for a single target on a non-interactive command; not applicable for Citar because all our commands are interactive.
4. (citar--some-action '("Foo" "Bar")) when called from Embark after selecting multiple references from the minibuffer.

Ideally we would come up with a nice unified way of handling all these different Embark calling conventions. I haven't looked into it deeply, but installing a special Embark hook might be the way to go. (Maybe embark-around-action-hooks?) The hook could detect a citar-citation target (representing an in-text citation with potentially multiple keys) and, if the action happens to be a Citar command, call that command passing all the keys as separate arguments.

[bdarcus]

@roshanshariff - should we ask Omar to weigh in?

[roshanshariff]

@bdarcus Absolutely! Omar might also be able to propose an alternative we haven't considered, or add some facility in Embark to make it easier. I think ideally Embark would be able to treat a "single target string representing multiple things" conceptually as a "multi target action".

[bdarcus]

@oantolin - can you take a look at this, starting from Roshan's summary?

No rush.

TIA!