Open bdarcus opened 3 months ago
@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 ...
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.
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.)
Thanks @kslutsky - yes, I was effectively meaning how to modify citar-select-refs
to address this.
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.)
Related to #837 also.
A different possibility would be to advise functions in the
citar-embark--multitarget-actions
list.
Why would we advise internal functions, @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.
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?
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.
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
(citar--some-action)
called interactively followed by citekey "Foo"
injected into minibuffer.(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).(citar--some-action "Foo")
for a single target on a non-interactive command; not applicable for Citar because all our commands are interactive.(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.
@roshanshariff - should we ask Omar to weigh in?
@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".
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 withcitar-citation
targets andcitar-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 fromcitar--select-multiple
or via an embark action on acitar-key
target, all works well. But if the argument is from acitar-citation
target with multiple keys, thencitar-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 thecitar--stringify-keys
function. Thuscitar-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 thecitar-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:
The same issue, in fact, applies to functions
citar-open
,citar-open-links
,citar-open-files
, andcitar-open-notes
, each of which fails, in my case at least, to act on acitar-citation
target with multiple keys.Originally posted by @kslutsky in https://github.com/emacs-citar/citar/issues/834#issuecomment-2196233740