org-roam / org-roam-bibtex

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

Can't create new notes if org-ref isn't loaded #172

Closed bigodel closed 3 years ago

bigodel commented 3 years ago

Describe the bug

When org-ref is installed but not yet loaded (I only load it after loading an Org-mode file because I don't need all of its functionalities outside Org-mode), orb-edit-notes fails with the message cite:foo-bar is not a valid ref on entries that do not have a note yet. It doesn't fire an error, it only shows this message on the minibuffer and nothing else happens.

To Reproduce

Steps to reproduce the behavior:

  1. Install and enable org-roam-bibtex-mode;
  2. Call M-x ivy-bibtex;
  3. Select an entry without a note;
  4. Call M-o (ivy-dispatching-done);
  5. Type e to call orb-edit-notes.

Expected behavior

It should show me the capture buffer for creating the note for the entry, but instead it just shows the message on the minibuffer and quits.

ORB configuration

  1. (add-hook 'org-roam-mode-hook #'org-roam-bibtex-mode);
  2. No configuration whatsoever (for org-roam-bibtex).

Environment (please complete the following information):

bigodel commented 3 years ago

i have tried my best to understand why that is happening and to try to fix to open the issue together with a PR, but i wasn't able to pin point exactly what is causing the issue.

i've also noticed some minor things that could be fixed and i could send a PR or open another issue, if applicable.

  1. instead of setting org-ref-notes-function to org-ref-notes-function-one-file it should revert back to whatever the user has set (in my case, org-ref-notes-function-multiple-files;
  2. on line 12 of org-roam-bibtex.el there is a typo and it is written "Verstion" instead of "Version".
myshevchuk commented 3 years ago

Hi, thanks for your report. Just a few days ago Sam reported a similar issue on Slack. He told he solved it before I had a chance to look into it. I don't know the details, it could be that stale *.elc files were to blame. Maybe you could ask him on Slack (I don't know his tag here). Or just try re-installing your Org-related packages. The thing is ORB does not produce this error message, e.g. grep the source code. The message is coming from a different package, most probably Org-roam, although it seems to be triggered by ORB. This is often an indication of some files need to be re-compiled.

It can also be a dependency problem. As far as I remember, Org-roam does not depend on Org-ref explicitly, but it still requires it for its citekey-related functionality. You are absolutely right, no need to load Org-ref on startup. Load it after Org is loaded. Org-roam and ORB depend on Org, so Org will be loaded anyway. Just load Org-ref additionally.

Instead of setting org-ref-notes-function to org-ref-notes-function-one-file it should revert back to whatever the user has set (in my case, org-ref-notes-function-multiple-files;

Yeah, I know. I was waiting until someone files a bug report for this. Could you please file a separate bug report? No need to fill the template, just reference this issue.

on line 12 of org-roam-bibtex.el there is a typo and it is written "Verstion" instead of "Version".

Thanks, there are many more typos, I guess. I'll keep a note of this one and will fix it in one of the upcoming commits.

bigodel commented 3 years ago

The thing is ORB does not produce this error message, e.g. grep the source code. The message is coming from a different package, most probably Org-roam, although it seems to be triggered by ORB.

yeah, it produced indeed by org-roam (found by grepping there), more specifically it is being triggered on org-roam-capture.el if i'm not mistaken.

Or just try re-installing your Org-related packages.

i just now tried reinstalling it by removing the folders from elpa, then by using package.el and lastly i tried force recompiling all of the org related packages, but the error still persists. i have just messaged Sam, and he mentioned that it was something to do with some custom capture templates, but i don't have any custom templates and i tried commenting most of my org and ivy-bibtex configuration and nothing worked.

Load it after Org is loaded. Org-roam and ORB depend on Org, so Org will be loaded anyway. Just load Org-ref additionally.

i actually load it after org-mode is loaded, not the feature org, which is loaded on startup. don't know if this might be useful information, but it appears to be somewhat connected since if i open an Org-mode buffer and thus load org-ref, org-roam-bibtex works as expected.

Yeah, I know. I was waiting until someone files a bug report for this. Could you please file a separate bug report? No need to fill the template, just reference this issue.

sure, no problem. i could even send a PR since it is such a small fix so that you wouldn't need to bother with it. it would just be the case of storing the value of the variable in another variable and resetting it when the mode is unloaded, right? i could even fix some typos along the way.

bigodel commented 3 years ago

ok, after fiddling around a bit more, i managed to understand what is going on. appearently, org-roam requires the ROAM_KEY property to be a link type recognized by Org-mode. since i don't load org-ref until i open an Org-mode buffer, the function that sets the cite: link type isn't called. one solution here would be to load this function, which is org-ref-generate-cite-links. i used the following code snippet

(with-eval-after-load "org-ref-autoloads"
  (autoload 'org-ref-generate-cite-links "org-ref-core")
  (org-ref-generate-cite-links))

but you could probably do with

(declare-function org-ref-generate-cite-links "ext:org-ref-core")

and call the function before creating a new note, so that the list of org-mode links is updated correctly. it would also be advisable to load the variables org-ref-cite-types with its default value if it was not changed by the user (maybe checking (boundp 'org-ref-cite-types) and setting a default value for it), since it is needed in order for org to know what to do when clicking on the link (this variable is used in the referred funciton to build the link types for all the cite-types on the variable). i don't know if there is a way to load a variable from a package, but it is a custom variable so maybe there is a way. you still don't need to have org-ref as a hard dependency, at least not in what regards this issue i'm having, as the above solution worked for me.

myshevchuk commented 3 years ago

Regarding the main issue with Org-ref, I actually don't think ORB should make any attempts to init the Org-ref citation links. It is Org-roam's headache. ORB uses what Org-roam gives it. In principle and what a textbook on Elisp would tell if there was one, ORB should ensure Org-ref is loaded with a top-level (require 'org-ref) statement, but almost a year ago when this package consisted of only a couple of functions, a weighed decision was made not to do so. Back then Org-roam actually did load Org-ref in order to support the cite:-style keys, but eventually they also dropped the require statement. It seems no-one wants to load Org-ref, even the users. ORB explicitly lists Org-ref as its dependency in the README, warning that the notes-actions functionality will be impaired. With the issue you are reporting, it looks like other parts can also be affected.

My anticipated solution would be to update the README file describing the caveats of not loading Org-ref in time and perhaps provide an example of how to do that in a reasonable way. By design, arguably not a very good one, the task of loading Org-ref is delegated to the user.

Alternatively, the functions that require Org-ref can be supplied with (require 'org-ref) in their body to ensure the package is loaded whenever the function is called. I'd prefer this approach, which is basically harmless on successive runs after the package has been loaded, to running org-ref-generate-cite-links each time the function is called.

i actually load it after org-mode is loaded, not the feature org, which is loaded on startup. don't know if this might be useful information, but it appears to be somewhat connected since if i open an Org-mode buffer and thus load org-ref, org-roam-bibtex works as expected.

Could you share the way you do this, please?

sure, no problem. i could even send a PR since it is such a small fix so that you wouldn't need to bother with it.

Thanks, I replied you in the respective issue #173.

bigodel commented 3 years ago

as much as i use org-ref and like what it does, i don't really like how it does. it is a somewhat clunky package and has helm as a dependency which might not be an immediate let-down, but is definitely a warning sign. unfortunately, we still don't have anything close to it (yet!).

My anticipated solution would be to update the README file describing the caveats of not loading Org-ref in time and perhaps provide an example of how to do that in a reasonable way.

i guess this would be a reasonable solution. at least it would make me happy, and i believe i'm an edge case.

Alternatively, the functions that require Org-ref can be supplied with (require 'org-ref) in their body to ensure the package is loaded whenever the function is called.

i'm not sure if i like this. org-ref is a pretty heavy package. i defer its loading to after i load and Org-mode buffer because it takes a couple of seconds to get loaded, so packages that only need one or two functionalities from it shouldn't need to load the entire thing. i guess this is a criticism of how the package is built, which has nothing to do with ORB directly.

Could you share the way you do this, please?

sure:

(add-hook 'org-mode-hook (lambda () "Load `org-ref' with and idle timer."
                             (run-with-idle-timer
                              0.5
                              nil
                              (lambda () (require 'org-ref)))))

i actually have a macro for the run-with-idle-timer since i use it frequently:

(defmacro delayed-init (&rest body)
  "Run BODY after idle for 0.5 seconds."
  `(run-with-idle-timer 0.5 nil (lambda () ,@body)))
myshevchuk commented 3 years ago

I updated the README to warn about the Org Ref issue, see #179. Think this can be closed now.

bigodel commented 3 years ago

Think this can be closed now.

yeah, i think its fair, thank you for your attention and work!