Support embark multitarget actions

[bdarcus]

Omar just merged support for this, discussed in #477:

oantolin/embark@b17af02

So we need to test this and get him feedback, but also make sure citar works with it. Once we get it working as expected, we can also add the pdf-search function, which would rely on this.

I'm not sure why, but this is the result I get.

Debugger entered--Lisp error: (wrong-type-argument listp "huck1975a")
  #f(compiled-function (x) #<bytecode -0x1433a9947753cda6>)("huck1975a")
  mapcar(#f(compiled-function (x) #<bytecode -0x1433a9947753cda6>) ("huck1975a" "huck1975b" "huck1975c" "huck1975d" "huck1975e" "huck1975f" "huck1975" "huck1975g"))
  citar-citeproc-format-reference(("huck1975a" "huck1975b" "huck1975c" "huck1975d" "huck1975e" "huck1975f" "huck1975" "huck1975g"))
  citar-copy-reference(("huck1975a" "huck1975b" "huck1975c" "huck1975d" "huck1975e" "huck1975f" "huck1975" "huck1975g"))
  #f(compiled-function () #<bytecode -0x1bcb76d099fa68a4>)()
  apply(#f(compiled-function () #<bytecode -0x1bcb76d099fa68a4>) nil)
  #f(compiled-function () #<bytecode -0x1ebf6128d325eb1a>)()
  #f(compiled-function () #<bytecode -0x14e4eee3a110c9d2>)()

Any ideas @localauthor @roshanshariff?

See oantolin/embark#433

[roshanshariff]

It looks like with this change, Embark will call the function non-interactively with a list of keys. So citar-select-refs is skipped entirely, and the function needs to lookup entries on its own. I can add a function to do this and change the Citar commands to use it, so they can be used as multi-actions.

[minad]

Hmm, I see. Your function is not taking a list of keys but alist of already looked-up key entries. If you change your functions accordingly such that they takes keys as argument it will work.

But maybe we can come up with an even better crm injection protocol in Embark. Embark could also temporarily advise completing-read-multiple and return the "injected" candidates directly. The advice would only be active once and then be removed. @oantolin I wonder why did you chose a minibuffer injection mechanism in the first place. Why not advise read-from-minibuffer for embark-act? This way one could also perform auto detection in embark-act-all, if CRM is called we are done, if read-from-mb is used continue with the tail of the list in a loop.

[minad]

@bdarcus I suggest you don't apply changes to citar just yet. Maybe it is worth to rethink the target injection mechanism in Embark.

[oantolin]

I think advising read-from-minibuffer never occurred to me. I had just learned about the minibuffer-setup-hook and it worked, so I never looked for other solutions.

The minibuffer injection solution is pretty flexible, though. It allows embark-allow-edit-actions which is great for, say M-!(to write which command you want run on the file) or even M-: (to add arguments to functions). Other action setup hooks like minibuffer-force-complete for package-delete, come in handy too. I guess all those things could be done by advising read-from-minibuffer but they sound a little awkward to do. I'll think about it.

[minad]

Okay I think the edit actions are a good argument to keep the minibuffer injection as is. Thanks for reminding me. While it is more heavy than an advice based mechanism the injection mechanism is relatively robust from experience since it simulates user input.

[oantolin]

Maybe we could use the advise method only for embark-act-all, where we don't need the bells and whistles of the injection method?

[minad]

I am not sure. I am out of ideas here. Maybe citar should indeed be rewritten to conform to the string list argument protocol.

The only thing I'd like to make sure is that multi actions don't require the action author to write "unnatural" code. Ideally the CRM actions look like normal interactive crm commands.

[minad]

Maybe we could use the advise method only for embark-act-all, where we don't need the bells and whistles of the injection method?

I think it is not a good idea to use either the advice method and the injection method depending on act-all/act. This will just lead to inconsistencies.

But we could use the CRM advice mechanism only for actions registered in the embark-crm-action list.

[minad]

Hmm, I kind of hate it for it's complexity but what about the nuclear option of adding an embark-action-injection-mechanism-alist. Default is 'minibuffer. Other alternatives 'list-argument, 'string-argument, 'crm-advice...

[oantolin]

That seems excessive. 😄

[oantolin]

Ah! How about this? We could leave both embark and these citar functions as they are and just do the reference lookup in an embark pre-action hook! Those hooks recieve a plist with a key :candidates that would contain all the keys, a pre-action hook could modify it to contains the looked up references.

[roshanshariff]

Regardless of the ultimate choice of Embark injection protocol, I like the idea of allowing the keys-entries function arguments to be lists of keys. I wrote a simple citar--ensure-entries function that looks up any missing entries, to be used if you really need a list of (KEY . ENTRY) pairs.

On the other hand, some functions just need keys and not entries, and they already use citar--extract-keys; this now guarantees that the elements of the list are just keys.

These two functions can be used to normalize the keys-entries list depending on the situation. The main benefit is that all the functions become easier to use from outside Citar, since you don't have to do the entry lookup yourself.

[oantolin]

Although I might argue that it would be nice if the citar functions could be called from Lisp code either with a list of looked up references or, for convenience, with a list of keys. Just like the Emacs buffer functions which can take either a buffer object or a string naming a buffer.

[oantolin]

Ah, I see @roshanshariff beat me to the punchline. I really do think it's a nice bit of convenience for possible Emacs Lisp users of citar to allow lists of keys.

[minad]

We could leave both embark and these citar functions as they are and just do the reference lookup in an embark pre-action hook! Those hooks recieve a plist with a key :candidates that would contain all the keys, a pre-action hook could modify it to contains the looked up references.

@oantolin I am not sure if this would work. Can the pre action hook lookup the candidates/transform them? I thought these hooks can transform the minibuffer content. Also it introduces a weird splitting of the responsibilities.

[oantolin]

The hooks that are meant to transform the minibuffer content are the setup hooks, the pre-action hooks get called before the action is called, and thus before there even is a minibuffer. No currently known use of the pre-action hook mutates the target plist, but it should definitely work. However I do agree it's not the best place for the lookup to happen. As I said above, I think the best thing would be to add the convenience feature to citar functions of looking things up for you if presented with bare keys.

[minad]

@roshanshariff Thanks! If you say that a list of strings is anyways more natural and flexible or if you perform normalization of a keys-or-entry-list then it is best to keep the simple mechanism we have right now! This is all I wanted to make sure - the action author should end up with commands which look natural.

[oantolin]

This is all I wanted to make sure - the action author should end up with commands which look natural.

Yes, that's the main thing! And I do think that @roshanshariff's suggestion here is not an unnatural contortion that you'd never think to do if it weren't for Embark, but a reasonable convenience features to be added to the citar calling convention. I mentioned Emacs allowing buffer objects or string name and doing the lookup for you, but I'm pretty sure there are other examples in Emacs.

[oantolin]

I guess you could say that completing-read accepting alists and hashtables is another example, it really just extracts the list of keys for you,really saving you only one function call.

[roshanshariff]

There is one (small) remaining consideration: writing multi-action functions that work with both older and newer versions of Embark. This is not a problem for interactive commands, since the old embark-act always calls them interactively and the new one never does. But for non-interactive functions, old Embark calls it with a single string argument, whereas the new version calls it with a list of strings.

So, in practice, the calling convention will need to handle both strings and lists. Maybe this isn't really worth considering? It's a relatively minor issue in Citar anyway, since we only have one non-interactive Embark action and that probably needs a rethink anyway.

[oantolin]

Oh, interesting point. As a work around, I guess embark-act could call the multitarget action first with a singleton list, and if there is an error, print a warning and try again with a string. Or does that sound too horrible?

[oantolin]

It would be temporary, like other obsolescence warnings. I think I've actually done something like that in the past when I changed the number of arguments a user supplied function is called with. 😬 😄

[minad]

I would not implement special support for old Embark versions. Embark just acquired multi action support in particular for Citar. It seems okay to ask users to update Citar and Embark to the newest version.

[roshanshariff]

In the one instance we do this in Embark (citar-run-default-action) it's easy enough to just handle strings and lists, so perhaps you could leave it as it is and consider the more complicated approach if somebody complains?

[oantolin]

Sounds good, @roshanshariff, let's do it that way. Thanks.

[roshanshariff]

Another design question: now that Embark knows how to run multi-actions, should target finders be allowed to return lists of strings rather than just one string?

In Embark, we have a target finder for multi-key citations that returns strings like "key1 & key2 & key3", relying on CRM with an appropriate separator to split the strings. I'm not entirely sure, but maybe it's easier if it can just return a list of strings?

[oantolin]

Oh! Good question. Right now there are two types of contexts for targets: embark-act is singular and wants just one target; embark-collect-*, embark-export and embark-act-all are plural and look for lists of candidates. They don't even use the same mechanism to procure targets: embark-act calls the functions on embark-target-finders, while all the "plural commands" call the functions on embark-candidate-collectors instead.

But maybe a radical reorganization is in order? If we did allow target finders to return lists of targets, we could merge target finders and candidate collectors, and merge embark-act and embark-act-all.

[oantolin]

I guess that target finder for multi-key citations could instead become a candidate collector, to be used with embark-act-all instead of with embark-act. A nice advantage of that change would be that you could directly call embark-collect-snapshot with point on a multi-key citation and get a nice little buffer showing the keys where you could act on the individually.

[roshanshariff]

The current design is that we actually have two target finders (with different target types, alas, since currently Embark only keeps one target of a particular type). One just returns the single key at point (with its bounds) and the other returns all the keys in the citation (with the bounds of the whole citation).

So the current UX is that embark-act will highlight a single key, and you can embark-cycle if you want to act on all the keys. I kind of like this, to be honest.

[minad]

@roshanshariff Honestly I think it is too late now for this change. See my comment #477 (comment) where I ha mentioned the possibility of modifying target finders such that they return a list of candidates. After this comment it was more or less agreed on that we don't go with multi targets per target finder.

We decided on the solution with embark-act-all. I kind of see your point but I am not happy with how this is going. Why come up with this now that we have the new mechanism in Embark? Moving to target finders which return a list of candidates will give yet another aspect to the current design. It does not make sense to end up with ten different approaches in Embark to do the same thing.

[roshanshariff]

@minad, I certainly didn't think my suggestion required a big redesign of Embark, and I saw it as natural consequence of the design that @oantolin already implemented. I like Embark's design (more than Helm 🙂) and I don't think there's any need to re-orient it around multi-targets. The simplest implementation is: if a target finder returns a list instead of a string, embark-act uses its existing calling convention for non-interactive functions (which happens to match the new embark-act-all calling convention). If I'm not mistaken, this is at most a couple of lines to change in embark-act and is completely backward-compatible.

Then @oantolin pointed out that this opens the door to more radical simplification in the future. But of course, I'll defer to the two of you on that front; this is not the best place to discuss that anyway. And I'm not strongly advocating for the simple change above either, so please don't read so much into it.

[minad]

@roshanshariff

I certainly didn't think my suggestion required a big redesign of Embark
I don't think there's any need to re-orient it around multi-targets.

Okay.

The simplest implementation is: if a target finder returns a list instead of a string, embark-act uses its existing calling convention for non-interactive functions (which happens to match the new embark-act-all calling convention). If I'm not mistaken, this is at most a couple of lines to change in embark-act and is completely backward-compatible.

And what should it do for interactive functions? At this point embark-act will behave like embark-act-all. I consider this a conflict. Note that the multi list target should also work well for the general actions not only Citar actions. I would not like if acting on certain targets will then implicitly behave like embark-act-all. By mixing up these two concepts I argue that we lose some clarity of the design and behavior. You can still achieve your goals in Citar with the existing design by defining a "big target string" as proposed above.

Then @oantolin pointed out that this opens the door to more radical simplification in the future.

Look, this is why I disagree with you here. It may open the door for a radical simplification. But then we are back at the big overhaul that I am not fond of. I really see some good things in the current design, even if it is limited in comparison to a generalized design ("radical simplification"). As I wrote above such a generalized design will have flaws - I don't think it is feasible to unify target finders and candidate collectors, without worsening the UI experience. Or if one tries to preserve the current UI experience as is one would still have to add special casing and wouldn't win anything on the technical side - no simplification!

But of course, I'll defer to the two of you on that front; this is not the best place to discuss that anyway. And I'm not strongly advocating for the simple change above either, so please don't read so much into it.

In the end it is Omar's decision. I am only expressing that I like the current design which distinguishes single candidates and multiple candidates and which keeps clear boundaries between the two concepts instead of mixing them up as you are proposing.

[minad]

I wonder why it is not possible for Citar to treat multiple citation keys as point as a single target. This seems like a perfectly simple solution to me within the bounds of the current Embark design. See #486 (comment) and #486 (comment).

One should also look at it like this: Since Citar is the only package which asked for such a multi target feature, it does not seem justified to change Embark to support this single use case, in particular since it is possible to implement all of this within Citar directly. This adds complications to the Embark injection mechanism and the target finder API.

In contrast, the functionality added by @oantolin with embark-act-all (and also the consult-completing-read-multiple target collector) are general features, which work in any context for any Emacs command, so they are definitely a good and justified addition. Of course it is even better that Citar profits from it!

[roshanshariff]

@minad,

You can still achieve your goals in Citar

Yes, on that I agree (and perhaps why this is a bad place to discuss this). Citar will get by anyway with or without these changes, modulo some minor annoyances. I was mostly concerned with simplifying Embark for its own sake.

And what should it do for interactive functions?

Do the same thing as embark-act-all, i.e. call the function non-interactively if it's in embark-multitarget-actions, or call it in a loop otherwise. By putting that code in embark--act, the acting side of embark-act and embark-act-all can be unified. Indeed, embark--act already has the non-interactive calling convention. All I'm saying is that if it's calling a function non-interactively (just forwarding the target as the function argument) it shouldn't care what the type of the target is, whether list or string.

I consider this a conflict.

The main difference is that embark-act-all gets its targets using collectors, whereas embark-act gets them using target finders. This is an important difference on the target-collection side, but that doesn't imply they have to have different action-execution semantics.

Look, this is why I disagree with you here.

This disagreement feels misdirected; @oantolin proposed the radical simplification of unifying target finders and collectors, not me 😛 (only kidding, since I believe @oantolin was also just brainstorming future ideas). Not that my opinion is important here, but I actually agree with you that target finders and candidate collectors seem semantically distinct (the former finds something at or near point, whereas the latter scans the entire buffer; in the minibuffer the target finder returns the current selection, whereas the collector returns all candidates).

All that said, I can think of some pain points that I've actually encountered that also get solved with this small change, and that have nothing to do with multi-target actions (rather, they rely on being able to force an interactive command to be called non-interactively by embark-act, which is one effect of embark-multitarget-actions). But since this thread is getting way too long and off-topic already, I should probably get around to opening a discussion on the embark project to discuss it further.

[minad]

@roshanshariff

Yes, on that I agree (and perhaps why this is a bad place to discuss this). Citar will get by anyway with or without these changes, modulo some minor annoyances. I was mostly concerned with simplifying Embark for its own sake.

Good, then let's proceed like this. I would like to emphasize once more that using a single target has advantages. I argue that the general actions work better on the single target string. By introducing multi targets on the target finder level all these actions won't work well anymore. For example the mark action, should this mark the closure of all targets then? In comparison, what about the action which add the target to the kill ring, should this action add each of the subtargets one by one? It will get quite incoherent I think. These are all complications on the level of Embark which you didn't consider.

The problem here also comes from the side of Citar. You neglect the general actions and you even have (or had) them removed from the actions menu in order to clean up the which-key indicator. While I understand the motivation it works a bit against the design and the spirit of Embark and it leads you to making proposals which do not generalize well.

The main difference is that embark-act-all gets its targets using collectors, whereas embark-act gets them using target finders. This is an important difference on the target-collection side, but that doesn't imply they have to have different action-execution semantics.

Of course nothing is implied. We can design it as we like. I argue that we should keep the distinction. embark-act is single target from the target finders and embark-act-all is for multiple targets from the collectors and we should not mix these up.

This disagreement feels misdirected

Maybe, but you used it as an argument in favor of making this change. I disagree with using a potential "radical simplification" as argument. It is baseless anyways since we don't have a coherent design for this.

All that said, I can think of some pain points that I've actually encountered that also get solved with this small change, and that have nothing to do with multi-target actions (rather, they rely on being able to force an interactive command to be called non-interactively by embark-act, which is one effect of embark-multitarget-actions).

I disagree. The change is not a small change and it has implications you are not foreseeing. I don't understand why you keep on trying and insisting so hard. Why not try to solve this in Citar now? Of course you don't have to take me as any kind of authority here and it is good to check the design from all sides. But you still seem to ignore my arguments and my concerns. If you say that it is only a minor inconvenience/annoyance on the side of Citar while at the same time I argue that for Embark the change would be more impactful, why are you pushing so hard? If you show me the code for Citar with the annoyance (probably only a small amount of normalization code is needed) in comparison to the cleaner or better code without the annoyance then we can judge the impact. But I am convinced that the outcome will probably be that the Citar code will be a small change in comparison to the changes which would be needed on the side of Embark.

[bdarcus]

Wow; quite the thread!

So we should look into changing all the citar interactive functions to operate on keys?

I think, as @roshanshariff says, that should be easy enough.

We just want to confirm, as that would mean reverting to what was there from the beginning. And I know there was a good reason for that at the time (that may no longer be relevant given subsequent changes).

And am I correct these embark changes also impact embark-act, so that code like this would no longer be needed here?

https://github.com/bdarcus/citar/blob/51b30f2e4091a41243ae62cfbac53e7a579f3168/citar.el#L785-L787

[roshanshariff]

For example the mark action, should this mark the closure of all targets then? These are all complications on the level of Embark which you didn't consider.

Ow, I wonder if this is the root of all the misunderstanding. I didn't mean that the target finders would return multiple targets (which would mean multiple bounds, multiple target types, etc.) I just meant that the target need not be a string, i.e. the second element of the target finder's return value. But there's still only one target type and one set of bounds. embark--mark-target would not be affected (nor any of the embark-region-map commands), since it ignores the target and cares only about its bounds. And kill-new adding all the target items to the kill ring actually seems like really nice behaviour. If you're concerned, remember that we'd still confirm before running the action on multiple targets.

In fact, of all the actions in embark-general-map, the only one that it doesn't make sense to call in a loop is embark-isearch. But that is a wrapper around isearch anyway, which can easily do something useful if it's given a list-valued target (run isearch to match any element of the list, for example). Remember that most target finders will only return string-valued targets. And my opinion (different from yours, obviously) is that it's unnecessarily restrictive to disallow list-valued targets, especially since doing so results in unnecessary inconsistency and code duplication.

But you still seem to ignore my arguments and my concerns.

I've tried to address the complications you've actually brought up. But it could also be that you overestimated the magnitude of the change I'm proposing.

it leads you to making proposals which do not generalize well.

Now this is really off-topic, but I fear you misunderstood my position on that issue. I didn't decide to remove the general actions or even advocate for that. I just wanted a way for all actions to be available but not displayed in the indicators. But please, I'd appreciate a fair consideration of what I'm trying to say without being prejudiced by half-remembered unrelated discussions (understanding, of course, that some back-and-forth might be needed for us to understand each other, and you have no obligation to engage).

I don't understand why you keep on trying and insisting so hard.

Because I like Embark and would like to see it achieve its goals with a minimum of unnecessary complexity. But you're right, I don't think this discussion is moving in a productive direction. When I have the time and energy, maybe I'll write up a better design proposal. (Might have to create a pseudonym to get a fair shake though 😛.)

[roshanshariff]

So we should look into changing all the citar interactive functions to operate on keys?

I mostly have this implemented already. PR incoming soon.

And I correct these embark changes also impact embark-act, so that code like this would no longer be needed here?

They don't; embark-act still only works with string-valued targets, which it injects into the minibuffer for interactive commands, and passes as an argument for non-interactive functions. That's one of the reasons I was proposing the change, to try to bring embark-act more in line with embark-act-all.

[bdarcus]

They don't; embark-act still only works with string-valued targets, which it injects into the minibuffer for interactive commands, and passes as an argument for non-interactive functions.

So did @oantolin implement his plan here to have non-interactive functions (I guess) work more like embark-act-all (" if it is non-interactive it would pass it a singleton list")?

oantolin/embark#433 (comment)

[minad]

@roshanshariff

Ow, I wonder if this is the root of all the misunderstanding. I didn't mean that the target finders would return multiple targets (which would mean multiple bounds, multiple target types, etc.) I just meant that the target need not be a string, i.e. the second element of the target finder's return value. But there's still only one target type and one set of bounds. embark--mark-target would not be affected (nor any of the embark-region-map commands), since it ignores the target and cares only about its bounds. And kill-new adding all the target items to the kill ring actually seems like really nice behaviour. If you're concerned, remember that we'd still confirm before running the action on multiple targets.

No, there is no misunderstanding. The problem is that your proposal introduces an incoherence. If you have the target finder and it returns a list of string and a single set of bounds then mark will mark the bounds of all these targets. So if I mark and then kill I will get the entire list of targets. While when I use the kill action directly I will get each of the targets separately on the kill ring. I don't like this incoherence.

You are mixing up the nice separation of embark-act+single-target vs embark-act-all+multi-target.

I've tried to address the complications you've actually brought up. But it could also be that you overestimated the magnitude of the change I'm proposing.

No I am not. The impact of your change will probably not that large, but it will also not be large on the side of Citar, probably even less so. I argue against your proposal since it muddies the design and mixes up the clean separation we are having now.

Note that the proposal has been discussed a few times before a while ago. And back then we also decided not to pursue this route. Now it came up again and we decided to not pursue this route. And after we got the implementation of embark-act-all you want to reiterate the discussion once again.

Now this is really off-topic, but I fear you misunderstood my position on that issue. I didn't decide to remove the general actions or even advocate for that. I just wanted a way for all actions to be available but not displayed in the indicators. But please, I'd appreciate a fair consideration of what I'm trying to say without being prejudiced by half-remembered unrelated discussions (understanding, of course, that some back-and-forth might be needed for us to understand each other, and you have no obligation to engage).

I don't blame you for this. But I still claim that Citar considers the general actions as less important, which they are of course in the context of Citar. It would still be good if Citar somehow fits coherently in the Embark picture. I am not prejudiced and I reject this accusation. I remember the old discussion well enough.

Because I like Embark and would like to see it achieve its goals with a minimum of unnecessary complexity. But you're right, I don't think this discussion is moving in a productive direction. When I have the time and energy, maybe I'll write up a better design proposal. (Might have to create a pseudonym to get a fair shake though stuck_out_tongue.)

Well you are not doing that. You are mixing up the goals of Citar and Embark and want to push Citar problems to Embark which will then lead to problems in Embark. I don't understand where you are getting the impression from that I am treating you in any way unfairly. I try to be fair. I try to bring technical arguments here. But you keep and keep on insisting on your point of view. I am also sticking to my point of view, but as I said this is the status quo and it has already been discussed.

Anyways I don't understand your behavior here. It feels too adversarial. I am not looking for a fight here and I tried on numerous occasions to help the Citar project. If this is not the case, I can just leave here. I don't know if @bdarcus appreciates my discussion points or not.

If it turns out that Citar does not fit together with the other packages, it may make sense to consider alternatives, e.g., a transient menu or your own citation key at point finder infrastructure. But one should be hesitant to try to push all the other projects in other directions. This does not seem fair to me.

In this discussion I said that I don't like to go with the multiple targets idea and Omar also seems to have the desire to preserve this design, see #486 (comment) and
#486 (comment).

Maybe one thing I should have clarified a bit better - this may explain my standpoint better. This whole set of packages has to work with "limited means", we don't have the perfect Emacs infrastructure, we have to work with what we have. We try to fit the packages well with existing infrastructure. So this should be the foremost goal and one should check if Citar is compatible with these goals. Furthermore from the experience with these packages we've learned that there is some value in sticking to simpler solutions, in particular on the API or protocol level. By extending the target finder you are introducing a significant change to how Embark works internally, even if you consider this only a minor change. In particular it introduces a duplication between collectors and target finders, which will be odd and which will then lead to ideas like a great overhaul where everything is united. This is a strong indication to not do this change since it will lead to a potential unification. What I want to say - by drawing the line here and now we make the distinction clear and the design which follows from it will be influenced by that decision. If we generalize arbitrarily the design which follows becomes less restricted and less coherent. This is not necessarily good. There is value in keeping the distinction.

[bdarcus]

I always appreciate your input @minad; I just haven't had the time yet to wrap my head around the details here :-)

I also don't have a problem with the current behavior of embark at point, BTW. And as you say, a transient or hydra is always an option at some point, whether it's in this package, or not. I experimented with such a thing here, but decided not to include it.

https://gist.github.com/bdarcus/09dff264a75ae78330d8ee1a1ee5e1b2

To me, embark-act-all is most useful in the minibuffer.

[minad]

@roshanshariff I would also like to remind you of the old discussion we had about hiding actions in keymaps. It rather seems to me that you are putting this in a light which is not entirely true. When you came up with this proposal you proposed to modify the keymaps or change the keymap mechanism even if your actual intention was to only filter on the indicator level. But it seems to me that you still remember that discussion as unpleasant and it seems there are still some unresolved hostilities here. I am not sure if we've convinced you back then or if you just left with an unchanged opinion. In any case there must be a way to have disagreements and still work productively together. It won't be as efficient as if there is mostly agreement, but it is still doable. One more lesson I took from these packages is that enforcing these package boundaries is a super helpful tool to work together, since everyone has their field that they are working on. So one should reconsider twice if a change I could make here (if it is not general here) should indeed be pushed to the other package. It seems to me that you tried (or are trying) two push problems from Citar on two occasions to Embark.

[roshanshariff]

Anyways I don't understand your behavior here. It feels too adversarial. I am not looking for a fight here and I tried on numerous occasions to help the Citar project. If this is not the case, I can just leave here. I don't know if @bdarcus appreciates my discussion points or not.

Regardless of anything else, @minad, you should not let any of this influence your involvement with Citar. I'm in no way representing this project, I'm just an interested contributor with my own views that have nothing to do with @bdarcus.

I agree that things felt too adversarial, and that was certainly not my intention. Needless to say, I have the same feeling as you about the importance of productive discussion and making solid technical arguments. But since we both feel this discussion isn't that, clearly there's room for improvement. I really don't have any hostility about that previous conversation, since I understand the pros and cons of what I was proposing and why that decision was made. I was just taken aback that you brought it up here in a seemingly unrelated context.

And, lastly, please believe that the changes I was proposing here are not, in fact, driven by Citar. Like I've said, those changes are quite trivial. Rather, I was just thinking of Embark on its own, which is why this discussion is misplaced here.

[minad]

@bdarcus Okay, thanks! Please tell me if I am intruding too much, I think there is a lot of value in keeping distinct/decentralized responsibilities. Of course this requires discussion across the package boundaries to decide on APIs. In more general terms, I consider asking another package for an API change is not a small change and it should be considered carefully.

To me, embark-act-all is most useful in the minibuffer.

Yes, it is, since in a normal buffer we don't usually have multiple targets. But as @oantolin wrote in #486 (comment) you could implement a collector which can collect multiple targets at point. I am not sure if this is a good design though. My proposal would be to write a target finder which returns a single target string, but this single string may contain multiple citation keys. Then you normalize this string target on the side of Embark and unpack it. I assume this is what @roshanshariff is currently implementing?

[roshanshariff]

Actually, for now, perhaps I can pare back what I was saying to one request: some way to indicate that an interactive command should be treated as non-interactive by embark-act (just like embark-multitarget-actions does for embark-act-all). The goal would be to have the command always called non-interactively, regardless of whether it's called through embark-act or embark-act-all. This gives us a couple of nice possibilities:

• We can use CRM in the interactive specification only when it makes sense (and plain CR if not), while still accepting string-encoded multi-keys as function arguments.
• We can create small wrappers around commands like org-cite-insert, which usually prompt for keys but sometimes prompt for something else; it would be nice to accept the embark target as a function argument, to be used when the command would otherwise prompt for keys. (But avoid having citation keys inserted into an unrelated prompt.)

[minad]

Regardless of anything else, @minad, you should not let any of this influence your involvement with Citar. I'm in no way representing this project, I'm just an interested contributor with my own views that have nothing to do with @bdarcus.

Don't worry. I appreciate that you contribute a lot to Citar and I consider myself an outside to Citar while you are one of the main Citar developers. This is how collaboration works even if we only discuss across API boundaries :-P

I really don't have any hostility about that previous conversation, since I understand the pros and cons of what I was proposing and why that decision was made. I was just taken aback that you brought it up here in a seemingly unrelated context.

Okay, I am glad about that and I didn't want to give the impression to pull something unrelated out of context. Sorry for that. The connection I was trying to make is that if you design Citar as a clean slate system (with a hydra/transient-style) interface you don't have to bother at all how targets are injected into the actions, you don't have to bother about general actions and actions introduced in user configurations.

And, lastly, please believe that the changes I was proposing here are not, in fact, driven by Citar. Like I've said, those changes are quite trivial. Rather, I was just thinking of Embark on its own, which is why this discussion is misplaced here.

These multi-actions are driven by Citar and by @bdarcus ! Of course they are. But this is nothing bad! I added consult-completing-read-multiple only thanks to discussions with @bdarcus and I am glad we added this!

You are right, the addition of embark-act-all and consult-completing-read-multiple are useful on their own. This is the convincing argument to actually add these features. On the other hand your new proposal of multiple targets returned from a target finder is not yet standing on their own. My preference would be to not go this route (for simplicity, because of the symmetries, coherence, and so on) but one could consider this as a general addition with other relevant targets (not only citar citations). But in this case I think the 99% rule applies and there are just not that many such targets where this would make sense. The few multi targets could be handled on the action level (as in this case within Citar), keeping Embark simple.

[minad]

@roshanshariff

Actually, for now, perhaps I can pare back what I was saying to one request: some way to indicate that an interactive command should be treated as non-interactive by embark-act (just like embark-multitarget-actions does for embark-act-all). The goal would be to have the command always called non-interactively, regardless of whether it's called through embark-act or embark-act-all.

This should work if you add the actions to embark-multitarget-actions. Then you will always get a list; also for the embark-act case. Alternatively if you want to have actions to be always called non-interactively just write them as plain functions. In the worst case you have to write wrappers. We could also introduce a variable embark-noninteractive-actions but I am against this. Then we have already two variables which modify the injection behavior embark-multitarget-actions and embark-noninteractive-actions! Instead I would rather propose again the addition of a generalized embark-action-injection-alist. See my #486 (comment). Also the current injection logic is already fairly complicated, we should be hesitant to extend this protocol. It is basically the same discussion as the one we already had. You are once again asking for an API extension while the issue can probably be solved on the side of Citar? Once I again I would like to ask if you can try to just stick to the existing protocols/APIs and see if you can live with it.

[roshanshariff]

Then you will always get a list; also for the embark-act case.

Ah, thank you! I hadn't realized that adding a command to that list also affects the embark-act case. This should be good enough for now. Creating a command and a separate function is not ideal because if somebody does embark-act and then M-x, they will end up running the command instead of the function but with an injected target.

On the whole, I really like the uniformity of injecting targets into minibuffer prompts, but until now there wasn't an escape hatch for those few cases where that can't be made to work for the command you're writing.

[minad]

On the whole, I really like the uniformity of injecting targets into minibuffer prompts, but until now there wasn't an escape hatch for those few cases where that can't be made to work for the command you're writing.

There was, but you had to write a wrapper function 🤷 I consider this an acceptable trade-off in comparison to maintain a configuration variable. There are many configuration variables in Embark already, it is a good idea to be careful when adding new ones. Note that for Helm and Ivy you have to write specialised action functions for every single action. Embark has a much better mechanism to catch the common case!

[oantolin]

Woah! This blew up after I went to sleep! But I gather the resolution was that embark-multitarget-actions already does what @roshanshariff wanted, namely that if a command is listed in that variable it is always called non-interactively, by both embark-act-all and embark-act? In the embark-act case it is called with a list with only one element, the target.

[bdarcus]

Woah! This blew up after I went to sleep! But I gather the resolution was that embark-multitarget-actions already does what @roshanshariff wanted, namely that if a command is listed in that variable it is always called non-interactively, by both embark-act-all and embark-act? In the embark-act case it is called with a list with only one element, the target.

Yes.

I think the upshot is we may want to list all of the commands there, so we handle them consistently, and with all of them taking a list of keys.

That may have other benefits in the end (including maybe allowing hydra or transient menus to use the functions directly?).

BTW, @roshanshariff, this is the commit where we changed from keys to keys-entries. It was in part to fix a bug; IIRC because there was code (I think for formatting the candidates) relying on the cache to be present which failed when creating the cache (or something like that). Just need to make sure we don't reintroduce this bug when making this change.

f843288

[roshanshariff]

this is the commit where we changed from keys to keys-entries

Ah, yes, I remember encountering that bug. It was an infinite recursion because citar--format-candidates needed to know if a key has associated files, and the function which checked that needed to get the entry for a given key, which in turn needed the candidates cache. Thankfully, I don't think that bug is possible any more; none of the functions needed by citar--format-candidates depend on the cache; the new citar-has-file and citar-has-note predicates are called with KEY and ENTRY arguments and don't have to look anything up.

[bdarcus]

How do you get the entry arg?

[roshanshariff]

How do you get the entry arg?

citar--format-candidates loops over every key and entry in the hash table returned by parsebib-parse and calls the file and note predicates with them. For the "has:link" test, it just checks if the entry has "doi" or "url" fields.