org-roam / org-roam-bibtex

Org Roam integration with bibliography management software
GNU General Public License v3.0
576 stars 47 forks source link

Ensure bibtex-completion is initialized #256

Closed meliache closed 2 years ago

meliache commented 2 years ago

bibtex-completion-format-entry fails if bibtex-completion-display-formats-internal is nil, which is the case if bibtex completion has not been initialized via (bibtex-completion-init) and I run orb-note-actions.

This happened to me because I install bibtex-completion via straight.el without helm-bibtex or ivy-bibtex. Calling commands of either of those packages seems to initialize it. When it is not initialized, that resulted in the error message

s-format: Wrong type argument: arrayp, nil

because bibtex-completion-format-entry then tries to format a nil string.

Calling (bibtex-completion-init) seems to be cheap after subsequent invocations, but I'm not sure that calling it in every orb-format-entry is the best solution or we can somehow make sure to call it only once. But that function call also present in orb-insert-generic, so I assume it's no problem. Anyway, this fixed my issues.

myshevchuk commented 2 years ago

Hi @meliache , thank you for debugging and addressing this issue.

Calling (bibtex-completion-init) seems to be cheap after subsequent invocations, but I'm not sure that calling it in every orb-format-entry is the best solution or we can somehow make sure to call it only once. But that function call also present in orb-insert-generic, so I assume it's no problem. Anyway, this fixed my issues.

I completely share your feelings about (not) calling bibtex-completion-init on every orb-format-entry invocation. Foremost, because an "init" function is probably meant to be called once - conceptually - no matter what the function does or how cheap it is. I'd suggest putting it into org-roam-bibtex-mode, the latter actually being more of an "init" than a "mode" function itself. E.g. here:

    (add-hook 'org-roam-capture-new-node-hook #'orb--insert-captured-ref-h)
+++ (bibtex-completion-init)
    (orb-make-notes-cache)
meliache commented 2 years ago

@myshevchuk

completely share your feelings about (not) calling bibtex-completion-init on every orb-format-entry invocation. Foremost, because an "init" function is probably meant to be called once - conceptually - no matter what the function does or how cheap it is.

Tbh, I'm really not sure whether this is what the bibtex-completion developers intended, because the function is called for every invocation of the ivy-bibtex and helm-bibtex commands. So the fact that orb calls it in every invocations of orb-insert-generic is at least consistent with the above commands, so I would keep it there. The function also seems to do some cross-checks like checking that the bibliography exists and it might be useful to have those at runtime sometimes, e.g. if the bibliography is moved while emacs is running.

But I admit we possibly don't need that in a formatting function at every invocation, so I implemented your suggestion in a6b4af915b709c4149507f1399adf38dbc66f3a8

myshevchuk commented 2 years ago

@meliache

In fact, there is a call to bibtex-completion-init in orb-insert-generic! So this issue has likely already arisen in that context.

What you are experiencing is an issue with orb-note-actions because this is where the call to orb-format-entry and respectively bibtex-completion-format-entry happens. But orb-note-actions acts on the current note's citekey and does not consult bibtex-completion-bibliography at all, so at least the bibliography-checking part of bibtex-completion-init is irrelevant to it. As the comments within orb-format-entry state, it uses bibtex-completion-format-entry as a temporary replacement for a yet-to-be-written native function. Putting bibtex-completion-init in org-roam-bibtex-mode would be another temporary solution to address your specific edge-case situation. Please add the following comment line, and I'll merge it:

;; HACK: temporary to address #256; see also `orb-format-entry'
(bibtex-completion-init)
(orb-make-notes-cache))
meliache commented 2 years ago

;; HACK: temporary to address #256; see also `orb-format-entry'

I added this comment in 5e117272da91379758c3080f0ac6e842cf299382. What might be a bit weird that #256 is not an issue but a PR, and I wouldn't usually say that a hack addresses a PR, but that's my fault for creating a PR without making an issue first. Though this PR describes the issue, I think any future person reading this will understand what is meant, but if you prefer I could also create an issue, link it to this PR and reference it in the comment.

myshevchuk commented 2 years ago

@meliache, thanks a lot!

What might be a bit weird that #256 is not an issue but a PR, and I wouldn't usually say that a hack addresses a PR, but that's my fault for creating a PR without making an issue first.

I think we can safely ignore these minor language inconsistencies :) ORB's tracker is a relatively quiet place, and I don't think anyone will ever be confused.