feat: add new format-entry function, note template

[bdarcus]

Per #269, this adds a template for formatting new note titles, and a function to handle it.

It has occurred to me maybe this function should be called format-entry, and the original one renamed format-entry-columns?

[aikrahguzar]

Note: there's a little change I made here that exposed a much bigger infinite recursion bug that I can't figure out how to fix otherwise.

Is it adding the if statement causing the infinite loop? That is very weird, because there is no recursion there at all. I checked by evaluating

(defun my-ba-get-entry (key)
  "Return the cached entry for KEY."
  (if (or (eq 'uninitialized bibtex-actions--candidates-cache)
          (eq 'uninitialized bibtex-actions--local-candidates-cache))
      (message "Something is wrong; your library is not initialized.")
    (cddr (seq-find
           (lambda (entry)
             (string-equal key (cadr entry)))
           (bibtex-actions--get-candidates)))))

and the calling it with a key and it seems to return the entry as expected.

I am setting up at a new place so won't do much anytime soon but I will try to track down infinite recursion bug if I can reproduce it.

[bdarcus]

Is it adding the if statement causing the infinite loop?

No; the reverse. Adding that statement, along with changing the load order of some functions, originally "fixed" it.

I don't see why it should make any difference, and in retrospect, the check didn't make sense there (and the "or" should be "and").

I am setting up at a new place so won't do much anytime soon but I will try to track down infinite recursion bug if I can reproduce it.

OK.

[bdarcus]

Adding that statement, along with changing the load order of some functions, originally "fixed" it.

I added this check back (which again "fixed" it), but also added a "FIX" comment, because I don't think this check should be necessary if everything else is correctly coded.

[aikrahguzar]

@bdarcus, does changing ba--get-candidates to

(defun bibtex-actions--get-candidates (&optional force-rebuild-cache)
  "Get the cached candidates.
If the cache is unintialized, this will load the cache.
If FORCE-REBUILD-CACHE is t, force reload the cache."
  (when force-rebuild-cache
    (bibtex-actions-refresh force-rebuild-cache))
  (when (eq 'uninitialized bibtex-actions--candidates-cache)
    (bibtex-actions-refresh nil 'global))
  (when (eq 'uninitialized bibtex-actions--local-candidates-cache)
    (bibtex-actions-refresh nil 'local))
  (seq-concatenate 'list
                   bibtex-actions--local-candidates-cache
                   bibtex-actions--candidates-cache))

helps with the recursion blowing up? There is certainly a bug as currently the 'local and 'global checks are inside an if statement that won't be executed unless force-rebuild-cache is non-nil. Which it won't be in the normal first usage and that behavior would be fixed by checking for uninitialized variables for in ba-get-entry.

I don't see why it would cause infinite recursion instead of a type error though, unless it is something that happens during completing-read part. That seems unlikely to me but I don't understand that part well.

I would test and do a pr but I need to setup my laptop since the last one went with my last job.

[bdarcus]

does changing ba--get-candidates to ... helps with the recursion blowing up?

Alas, no.

[aikrahguzar]

@bdarcus I figured out the problem, but I don't have a good solution yet. It is the use of bibtex-actions-file--files-for-key in the bibtex-actions-file--format-candidates. bibtex-actions-file--files-for-key calls bibtex-actions-file--possible-names which calls ba-get-entry which calls ba-get-candidates and ba-get-candidates will call bibtex-actions-file--format-candidates because the cache isn't built yet causing bad recursion. The check for initialization then allow us to proceed. This is a pretty subtle result of recent changes to parse the file field. I think the solution should be to change ba-file functions to take the extra entry argument and pass the entry directly for ba--format-candidates and use the ba-get-entry in interactive functions that need the files to supply this new entry argument.

This has me thinking that ba-get-entry exists only to overcome a shortcoming in the ba-select-keys. We store the entry but then throw it away when we return only the keys in ba-select-keys and then we need to go through the whole candidate list to find the entry later when we need it. It is inefficient but that doesn't really matter since we only need to lookup a few selected keys but it can also cause this kind of issue. We can change ba-select-keys to return an alist of (key . entry) but that will probably need some adjustment for embark integration too.

[bdarcus]

Maybe get-entry (or just however to get the necessary data) needs to grab from the candidate cddr then? If that's possible; not sure it is.

[aikrahguzar]

Maybe get-entry (or just however to get the necessary data) needs to grab from the candidate cddr then? If that's possible; not sure it is.

Something like this would be my preferred solution. Where select-keys returns alist but a lot of (I think all the interactive) functions would needed to be update to deal with this change. Though the change would just be to change the key to key-entry and then get the key by (car key-entry). The other problem is embark's at point and we would need to change that case to fetch the entry like we do in get-entry now. That is the only place where I think the current solution is the optimal one.

[bdarcus]

Looks like the only field files-for-key is currently accessing (indirectly) is files.

[aikrahguzar]

Looks like the only field files-for-key is currently accessing (indirectly) is files.

Yes, but I think generally the set of functions that depend on the caches being populated should be fairly limited, I guess ideally only to interactive functions.

[bdarcus]

One issue, the details of which I'm forgetting such that I'm unsure if it applies here, is that completing-read throws out non-candidate data at a certain point.

[bdarcus]

Occurs to me also that it might be wise at some to add some tests, that we can also hook up to the CI.

[aikrahguzar]

One issue, the details of which I'm forgetting such that I'm unsure if it applies here, is that completing-read throws out non-candidate data at a certain point.

I think we already take care of it to find the key corresponding to the formatted string. I was thinking of changing this

(cl-loop for choice in chosen
             ;; Collect citation keys of selected candidate(s).
             collect (or (cadr (assoc choice candidates))
             ;; Key is literal coming from embark, just pass it on
                          choice))

to something like this

(seq-map (lambda (choice) (seq-find
                            (lambda (cand) (cdr (or (equal choice (car cand))
                                             ;; For embark, find the cand with correct key
                                                    (equal choice (cadr cand)))))
                          candidates))
             chosen)

Basically change cadr to cdr so that the function calling select-keys can use both the key and entry.

[bdarcus]

Basically change cadr to cdr so that the function calling select-keys can use both the key and entry.

I pushed #274 before seeing this, but will take a look sometime today or tomorrow.

Are you thinking of this as a simpler alternative to #274?

[aikrahguzar]

I pushed #274 before seeing this, but will take a look sometime today or tomorrow.

Are you thinking of this as a simpler alternative to #274?

I briefly looked at #274 and it looks good. This is almost unrelated and the purpose is not to go through candidate list too many times. I think it cleaner if we have a separation of the part of the library that does caching and the rest of the library interacts with cached candidates only through select-keys.

[bdarcus]

I think it cleaner if we have a separation of the part of the library that does caching and the rest of the library interacts with cached candidates only through select-keys.

Yes, ideally they really should be cleanly-separated.

[bdarcus]

I pushed #274 before seeing this ...

Sigh ... I of course meant #281!