Support embark multitarget actions

[bdarcus]

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

https://github.com/oantolin/embark/commit/b17af024d92e5c8532ed94f157e65aa7f4cda334

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 https://github.com/oantolin/embark/issues/433

[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 https://github.com/bdarcus/citar/issues/486#issuecomment-993222093 and https://github.com/bdarcus/citar/issues/486#issuecomment-993229729.

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 https://github.com/bdarcus/citar/issues/486#issuecomment-993229729 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 https://github.com/bdarcus/citar/issues/486#issuecomment-993201675. 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 :shrug: 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.

https://github.com/bdarcus/citar/commit/f843288557750ad509c4373b7a67a6a2958a4783

[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.