fix: change functions to take entry, decouple cache from formatting

[bdarcus]

This is fundamentally to fix #286 (a recursion bug), but may have other benefits for modularity and performance.

1. change bibtex-actions-select-keys to bibtex-actions-select-refs, which now returns (key . entry)
2. change functions that rely on keys to accept this richer input instead
3. decouple action and formatting functions from caches (no more looking up data there, which caused the bug)

See:

#274 (comment)

This seems to fix it @aikrahguzar, but I need to test more.

Also, maybe need to change the function names.

[bdarcus]

So you were suggesting, @aikrahguzar, something like this?

(defun bibtex-actions-open (keys-entries)

Latest commit mostly implements this, though there are some bugs I need to fix.

[aikrahguzar]

This change is going to break embark as it is, since now the functions expect an alist of keys and entries and not a list of keys. So the choice needs to be changed to something like

(cdr (seq-find (lambda (cand) (equal choice (cadr cand))) candidates))

to complete the key at point into a key entry cons.

[bdarcus]

This change is going to break embark as it is ...

Yeah, I was aware of that; was going to figure that out next, so thanks!

[bdarcus]

So the choice needs to be changed to something like

(cdr (seq-find (lambda (cand) (equal choice (cadr cand))) candidates))

Actually, I had been thinking the functions in the "Embark" section would have to be changed; not this?

If I'm right, I'm not even sure how to do that ATM, as we only have access to the keys in the buffer.

[aikrahguzar]

Actually, I had been thinking the functions in the "Embark" section would have to be changed; not this?

According to what Omar said last time, emark will basically call the commands interactively and that in turn means they will call select-keys and embark will simulate typing the keys at point. Here we couple the passed key with the corresponding entry. I think this should work for embark and I don't see anywhere else we can do this coupling

[aikrahguzar]

If I'm right, I'm not even sure how to do that ATM, as we only have access to the keys in the buffer.

The only difference between the normal usage of select-keys and embark is that in the first case the returned strings are the display strings and in the case of embark they are keys. But we can use either to find the key entry pair, so they are not that different from that perspective.

[aikrahguzar]

Are you comfortable with me pulling the trigger on this generally, once I have it sorted out?

E.g. changing the interactive functions so dramatically, and diverging clearly from other similar packages?

I think I am, but just want a sanity check!

I think overall it is a change for the better to make cache more of an internal implementation detail to be used sparingly. But it probably will break setup of some people with customization in their config. Might be a good idea to make an issue and allow people to chime in.

But I am fairly trigger happy with changes and I think we have changed internals a few times since I started working on it without too many problems.

[aikrahguzar]

I want to put a clear comment there, to replace the earlier one you had; does this work?

           ;; When calling embark at-point, use keys to look up and return the
           ;; selected candidates.

This seems better. Might be a good idea to link to Omar's reply there so if someone new comes along, they can pick up how it is working quickly. It cleared up a few things for me.

BTW, I changed select-keys to select-refs.

Seems good!

[bdarcus]

I don't understand the notes function but I think two options are

1. There is no need for the notes function to consider itself with the details of the entry so in that case we use the extract-keys internally and expose only the key to be used as an input.

The function does need to concern itself with the "details of the entry" because it has to run format-entry on the note template for new notes.

But ...

2. The rest of the information might be useful but the default using org-roam-bibtex doesn't need it.

... this is also true, I guess, and less-than-ideal, since ORB relies on the bibtex-completion cache; why they can get away with only using the key.

Perhaps I could ask them to allow an optional "entry" arg.

In any case, given status quo, I'm faced with a somewhat difficult choice:

1. do the "right" thing from a technical POV
2. do the "compatible" thing

Or I could do a hybrid; take key, but get the entry from the cache.

[aikrahguzar]

The function does need to concern itself with the "details of the entry" because it has to run format-entry on the note template for new notes.

Then I think this change should be better since now the function is given entry upfront instead of having to find it.

... this is also true, I guess, and less-than-ideal, since ORB relies on the bibtex-completion cache; why they can get away with only using the key.

This is indeed unfortunate and I think a potential problem in removing bibtex-completion as a dependency since it will get loaded as a transitive one.

Perhaps I could ask them to allow an optional "entry" arg.

This seems like a good idea especially in view of minimizing dependencies.

In any case, given status quo, I'm faced with a somewhat difficult choice:

do the "right" thing from a technical POV
do the "compatible" thing

Or I could do a hybrid; take key, but get the entry from the cache.

I think for now the best option seems to be to have the customizable function take (key . entry), but extract the key in the default and pass it to ORB and hope that they change to implementation so we can pass the entry directly in the future.

[bdarcus]

I think for now the best option seems to be to have the customizable function take (key . entry), but extract the key in the default and pass it to ORB and hope that they change to implementation so we can pass the entry directly in the future.

I pinged the lead maintainer, and he's open to it.

#270 (reply in thread)