joostkremers / ebib

A BibTeX database manager for Emacs.
https://joostkremers.github.io/ebib/
BSD 3-Clause "New" or "Revised" License
275 stars 37 forks source link

Provide example integration with Citar for notes #299

Open swflint opened 5 months ago

swflint commented 5 months ago

Pending #298, this PR provides an example integration with Citar to demonstrate how external packages can be used to manage notes.

joostkremers commented 5 months ago

This looks pretty cool. Thanks!

Hugo-Heagren commented 5 months ago

@swflint am I right that this fixes #263?

swflint commented 5 months ago

It should. If you are a citar user, I'd appreciate testing of the citar-specific functionality, as note creation is definitely somewhat unstable. Else, please test with the patch to ebib-notes.el and your notes management infrastructure of choice.

Edit:

I'd like to note, I pretty heavily considered your comment on #263, and had initially written a version that did exactly that. I realized however, that it would be simpler to use a single function with a defined interface instead of having 3--4 variables that have to be checked. This means simpler code, and hopefully simpler, more likely to be consistent, configuration.

As a note for ebib-notes-extract-note, I didn't initially see how note extraction operates, so I hadn't included it as an action. I'd definitely appreciate thoughts on how to integrate the options, because it in particular seems a bit tricky.

joostkremers commented 5 months ago

I pushed a few commits, mostly cosmetic. I also added an option for Citar to ebib-notes-storage, which is just a convenience thing, but nice to have, I think.

Personally, I would also change the signature of the function from operation args to operation &rest args (and make appropriate changes elsewhere, of course), but that's personal preference.

One serious issue, though: citar-has-notes apparently (I don't use Citar) takes zero arguments, and it returns a function to test whether a key has a note, it is not a predicate itself. That would need to be fixed, because right now, showing notes does not work. I may be able to take a look at it later, but feel free to take it up yourself, of course.

swflint commented 5 months ago

I pushed a few commits, mostly cosmetic. I also added an option for Citar to ebib-notes-storage, which is just a convenience thing, but nice to have, I think.

Cool! Thanks -- I was hesitant to include that, though would a function-item make more sense?

Personally, I would also change the signature of the function from operation args to operation &rest args (and make appropriate changes elsewhere, of course), but that's personal preference.

I think that's a reasonable change, and I've gone ahead and made that

One serious issue, though: citar-has-notes apparently (I don't use Citar) takes zero arguments, and it returns a function to test whether a key has a note, it is not a predicate itself. That would need to be fixed, because right now, showing notes does not work. I may be able to take a look at it later, but feel free to take it up yourself, of course.

That's what I get for ignoring docstrings! I've pushed a fix.

swflint commented 5 months ago

I've also made it so that the note checking closure is cached to improve performance.

swflint commented 5 months ago

Opening citar notes should work now as well.

joostkremers commented 5 months ago

Cool! Thanks -- I was hesitant to include that, though would a function-item make more sense?

Absolutely, and if I had known about it, I would have used it. :smile:

One serious issue, though: citar-has-notes apparently (I don't use Citar) takes zero arguments, and it returns a function to test whether a key has a note, it is not a predicate itself. That would need to be fixed, because right now, showing notes does not work. I may be able to take a look at it later, but feel free to take it up yourself, of course.

That's what I get for ignoring docstrings! I've pushed a fix.

Thanks!

Though just to be on the safe side, perhaps we should put the funcall inside an (if ebib-citar--notes-checker ...? Because when I tested it (having Citar installed but not configured), citar-has-notes returns nil, which causes the funcall to raise an error...

swflint commented 5 months ago

I've wrapped it in a when. That said, I suspect it's because citar is unconfigured.

Hugo-Heagren commented 5 months ago

I've just had a look at this -- it's really cool but I have some worries. It seems that ebib-citar-backend assumes:

  1. The values in the hash table returned by (citar-get-notes citekey) will be lists of? filepaths
  2. therefore, the thing to do with them is to visit a file (the last file in the list?), which will open a buffer, which can be handed back to ebib to display

I have a citar setup in which neither is true. I thought this might be an abuse of the code, but looking at how citar handles this sort of thing, I think actually the opposite is the case: citar's documentation carefully avoids assuming that notes objects are filepaths -- the docstring for citar-get-notes calls them 'notes' and the internal citar--get-resources calls them 'resources'. So I think this part might need to be changed to allow for the full flexibility of citar.

I use org-roam for all my note taking. This means:

Concerning 1: in my setup, each note object is a list of the titles of every org-roam node which has the citekey as in its ROAM_REFS property. I spent a long time working out how to make citar and org-roam play nicely together and this was the best way (for me, even better than using the integration package which citar advertises).

Concerning 2: because of this tight integration, my citar setup uses org-roam-capture-. This function takes care of display for me, and (because of the vagaries of org-mode) I can't return the buffer to which I'm capturing (I'm sure I could if I did enough hacking, but it would take fragile advice, and my setup is complex enough as it is). (ETA: the importance of this is that at the moment, I think ebib assumes the notes backend will eventually return a buffer which ebib will then display, but in my setup that won't be true -- perhaps the way forward is to interpret certain kinds of return value as signallin that tha backend has dealt with display itself and ebib doesn't need to? (e.g. a t return))

I don't have the time right now to look into how to solve all this, but I hope it's relatively straightforward.

For possible reference, here are the relevant parts my citar setup:

(leaf citar
  :require bibtex-completion
  :config
  ;; Use org-roam for citar notes
  (require 'cl-lib)
  (require 'org-roam)
  (defun my/citar-org-roam-has-notes ()
    (lambda (key) (org-roam-node-from-ref (concat "@" key))))
  (defun my/citar-org-roam-titles-hash-table (keys)
    "Return hash table of KEYS and lists of notes.
Each key in KEYS is a key in the hash table. Each value is a list
of strings: the titles of every org roam node which has a
ROAM_REFS property with that KEY."
    (let ((table (make-hash-table)))
      (mapcar
       (lambda (key)
     ;; TODO relies on an open PR
     (when-let ((nodes (org-roam-nodes-from-ref (concat "@" key)))
            (formatted (mapcar #'org-roam-node-title nodes)))
       (puthash key formatted table)))
       keys)
      table))
  (defun my/citar-capture-org-roam (key entry)
    "Capture to "
    (let ((title (citar-format--entry "${title}" entry)))
      (org-roam-capture-
       :templates `(,my/org-roam-reference-capture-template)
       :props '(:finalize find-file)
       :node (org-roam-node-create
          :title title
          :refs key))))
  (defun my/citar-open-org-roam-note-title (note-title)
    "Capture to the Org Roam node with with title NOTE-TITLE."
    (when-let ((node (org-roam-node-from-title-or-alias note-title)))
      (org-roam-capture- :node node)))
  (defun my/citar-transform-title-to-org-roam-cand (title)
    "Return `org-roam-node-read'-like candidate string for TITLE."
    (let ((node (org-roam-node-from-title-or-alias
         (substring-no-properties title))))
      (org-roam-node-formatted-candidate node)))
  (citar-register-notes-source
   'my/citar-org-roam
   (list :name "Org-Roam Notes"
     :category 'org-roam-node
     :hasitems #'my/citar-org-roam-has-notes
     :open #'my/citar-open-org-roam-note-title
     :items #'my/citar-org-roam-titles-hash-table
     :create #'my/citar-capture-org-roam
     :transform #'my/citar-transform-title-to-org-roam-cand))
  :custom
  (citar-bibliography . `(,my/bib-file))
  (citar-library-paths . `(,my/bib-dir))
  (citar-notes-paths . `(,my/bib-dir))
  (citar-latex-prompt-for-cite-style . nil)
  (citar-latex-default-cite-command . "autocite")
  (citar-notes-source . 'my/citar-org-roam))
swflint commented 4 months ago

I've just had a look at this -- it's really cool but I have some worries. It seems that ebib-citar-backend assumes:

1. The values in the hash table returned by `(citar-get-notes citekey)` will be lists of? filepaths
2. therefore, the thing to do with them is to visit a file (the last file in the list?), which will open a buffer, which can be handed back to ebib to display

I have a citar setup in which neither is true. I thought this might be an abuse of the code, but looking at how citar handles this sort of thing, I think actually the opposite is the case: citar's documentation carefully avoids assuming that notes objects are filepaths -- the docstring for citar-get-notes calls them 'notes' and the internal citar--get-resources calls them 'resources'. So I think this part might need to be changed to allow for the full flexibility of citar.

Well that adds a wrinkle in design. I wonder if there's a way to determine how to resolve that into a buffer without knowing the specific citar backend (not having a way to do so seems like a miss in the design of citar as it is so generic).

I use org-roam for all my note taking. This means:

Concerning 1: in my setup, each note object is a list of the titles of every org-roam node which has the citekey as in its ROAM_REFS property. I spent a long time working out how to make citar and org-roam play nicely together and this was the best way (for me, even better than using the integration package which citar advertises).

Hmm. That does make things difficult. How would you want it to decide which note(s) would be displayed given the integration?

Concerning 2: because of this tight integration, my citar setup uses org-roam-capture-. This function takes care of display for me, and (because of the vagaries of org-mode) I can't return the buffer to which I'm capturing (I'm sure I could if I did enough hacking, but it would take fragile advice, and my setup is complex enough as it is). (ETA: the importance of this is that at the moment, I think ebib assumes the notes backend will eventually return a buffer which ebib will then display, but in my setup that won't be true -- perhaps the way forward is to interpret certain kinds of return value as signallin that tha backend has dealt with display itself and ebib doesn't need to? (e.g. a t return))

nil should be sufficient (though it may be necessary to add an additional nil check).

I don't have the time right now to look into how to solve all this, but I hope it's relatively straightforward.

I certainly hope so as well, but I suspect there will probably be some other work that's necessary.

For possible reference, here are the relevant parts my citar setup:

(leaf citar
  :require bibtex-completion
  :config
  ;; Use org-roam for citar notes
  (require 'cl-lib)
  (require 'org-roam)
  (defun my/citar-org-roam-has-notes ()
    (lambda (key) (org-roam-node-from-ref (concat "@" key))))
  (defun my/citar-org-roam-titles-hash-table (keys)
    "Return hash table of KEYS and lists of notes.
Each key in KEYS is a key in the hash table. Each value is a list
of strings: the titles of every org roam node which has a
ROAM_REFS property with that KEY."
    (let ((table (make-hash-table)))
      (mapcar
       (lambda (key)
   ;; TODO relies on an open PR
   (when-let ((nodes (org-roam-nodes-from-ref (concat "@" key)))
          (formatted (mapcar #'org-roam-node-title nodes)))
     (puthash key formatted table)))
       keys)
      table))
  (defun my/citar-capture-org-roam (key entry)
    "Capture to "
    (let ((title (citar-format--entry "${title}" entry)))
      (org-roam-capture-
       :templates `(,my/org-roam-reference-capture-template)
       :props '(:finalize find-file)
       :node (org-roam-node-create
        :title title
        :refs key))))
  (defun my/citar-open-org-roam-note-title (note-title)
    "Capture to the Org Roam node with with title NOTE-TITLE."
    (when-let ((node (org-roam-node-from-title-or-alias note-title)))
      (org-roam-capture- :node node)))
  (defun my/citar-transform-title-to-org-roam-cand (title)
    "Return `org-roam-node-read'-like candidate string for TITLE."
    (let ((node (org-roam-node-from-title-or-alias
       (substring-no-properties title))))
      (org-roam-node-formatted-candidate node)))
  (citar-register-notes-source
   'my/citar-org-roam
   (list :name "Org-Roam Notes"
   :category 'org-roam-node
   :hasitems #'my/citar-org-roam-has-notes
   :open #'my/citar-open-org-roam-note-title
   :items #'my/citar-org-roam-titles-hash-table
   :create #'my/citar-capture-org-roam
   :transform #'my/citar-transform-title-to-org-roam-cand))
  :custom
  (citar-bibliography . `(,my/bib-file))
  (citar-library-paths . `(,my/bib-dir))
  (citar-notes-paths . `(,my/bib-dir))
  (citar-latex-prompt-for-cite-style . nil)
  (citar-latex-default-cite-command . "autocite")
  (citar-notes-source . 'my/citar-org-roam))
Hugo-Heagren commented 4 months ago

Well that adds a wrinkle in design.

Haha yes! I seem to remember running up against similar things last time I looked into this, though you've got a lot further than I have.

I wonder if there's a way to determine how to resolve that into a buffer without knowing the specific citar backend

If there was, why would that be useful? Citar's infrastructure does not assume that the background application (citar or ebib) will always have to handle display (in fact I think citar assumes the opposite -- that the notes backend will deal with this). In my setup, it would definitely not be useful, since org-roam-capture- deals with this for me.

(not having a way to do so seems like a miss in the design of citar as it is so generic).

... so I think this sort of gets it the wrong way round: citar is so generic that it doesn't assume this is the sort of thing you would need (I think?)

Hmm. That does make things difficult. How would you want it to decide which note(s) would be displayed given the integration?

This is one thing that citar does handle. If an entry has more than one note, citar:

  1. transforms each note using the function stored in the :transformer prop of the notes backend
  2. prompts you to select between these (transformed) candidates with completing-read
  3. uses clever magic with text properties to get back the actual notes object from the transformed string

This is all handled by citar--select-resource.

Ideally, if we integrate with citar, I would want exactly this to happen, using citar's infrastructure. Ebib shouldn't reimplement citar--select-resource or something like it. (that is, for integration with citar. Of course, Ebib might need somethig like this for other purposes and that would be fine)

joostkremers commented 4 months ago

Since I don't use Citar, I can't really be of much help, but if you guys manage to sort this out, I'd love to add it to Ebib.

There's one question above that I wanted to get back to:

As a note for ebib-notes-extract-note, I didn't initially see how note extraction operates, so I hadn't included it as an action. I'd definitely appreciate thoughts on how to integrate the options, because it in particular seems a bit tricky.

Do you mean specifically the option ebib-notes-extract-text-function (and its default value ebib-extract-note-text-default). Yeah, it's tricky, because what it does is to try and extract the text of the note, but remove property drawers, because in the entry buffer, we only want to show the first few lines of the note, and if all we show is the property drawer, it'd be pretty useless.

This extraction is done using org-element, which reads the Org text and turns it into a data structure, from which we can then remove the property drawer. In order to get to the text again, we need to write the text back into a (temp) buffer and extract it from there.

In order to be able to format the text in the entry buffer, it is then split into lines and returned as a list of strings.

ebib-extract-note-text-default assumes Ebib handles the notes, so it cannot be used as-is for any other note-taking system, though it may be possible to use parts of it.

joostkremers commented 4 months ago

Ideally, Ebib's note-management would be split into a frontend and a backend, with the frontend flexible enough to be able to plug in different backends. This PR could be a first step in that direction, by defining an interface and an example backend. The traditional options, one-file-per-note and multiple-notes-per-file, and the option to use org-capture, could then be reimplemented as backends as well.

Hugo-Heagren commented 4 months ago

Ideally, Ebib's note-management would be split into a frontend and a backend, with the frontend flexible enough to be able to plug in different backends. This PR could be a first step in that direction, by defining an interface and an example backend. The traditional options, one-file-per-note and multiple-notes-per-file, and the option to use org-capture, could then be reimplemented as backends as well.

This sounds good. Do you envisage that the current user-facing api for configuring notes would remain the same (i.e. setting ebib-notes-storage to a symbol) or that this would be deprecated and a new api used, which would allow users to configure identical behaviour? I ask because I imagine the flexible front/backend architecture you suggest would (I imagine...) be easier to implement without the constraint of user-facing backwards compatibility (i.e. would be easier to implement in the second scenario).

joostkremers commented 4 months ago

This sounds good. Do you envisage that the current user-facing api for configuring notes would remain the same (i.e. setting ebib-notes-storage to a symbol) or that this would be deprecated and a new api used, which would allow users to configure identical behaviour? I ask because I imagine the flexible front/backend architecture you suggest would (I imagine...) be easier to implement without the constraint of user-facing backwards compatibility (i.e. would be easier to implement in the second scenario).

I think starting anew would be better in this case. A new user option, ebib-notes-system or something similar, that would take a function as value, which would implement the interface that @swflint suggests: a symbol to indicate the action and a specific set of additional arguments for each action.

I think a lot of the current code can be kept, once such an interface has been set up, so ideally, users who want to keep using Ebib's note-taking system (or one of them, as there are actually two) shouldn't have to do more than set the new user option. But if it turns out that bigger changes would be better in the long run, I wouldn't rule them out just to maintain backward compatibility.

joostkremers commented 4 months ago

In fact, one thing I might decide to remove from the current system is the ability to use other formats than Org, at least for the multiple-notes-per-file option. It would be possible to implement it as a separate backend, of course, but I'm not sure that's worth the trouble. I suspect most Emacs users will use Org as their markup language.

joostkremers commented 4 months ago

Ideally, Ebib's note-management would be split into a frontend and a backend, with the frontend flexible enough to be able to plug in different backends.

I've now started working on this, in a separate branch based off of this PR. So @swflint , for any work you may do on ebib-citar.el, you can assume that the interface is the one that you defined (except that the user option is now ebib-notes-backend).

joostkremers commented 4 months ago

I've now pushed an initial implementation of the notes back-end idea to https://github.com/joostkremers/ebib/tree/devel/notes-redesign. I've had to make a small modification to the interface suggested by @swflint , in that :create-note and :open-note don't return a cons but a list with an optional third element, a list of functions that should be run after positioning point. I also added two more actions, :extract-text and :delete-note, which are both optional, however. They are used in case ebib-notes-show-note-method is set to top-lines. A back-end may decide to do nothing and always return nil for those actions and suggest to set ebib-notes-show-note-method to all instead.

I've also indicated in the doc string of ebib-notes-backend which actions may raise an error. Some can, some shouldn't, because raising an error would disrupt Ebib. (For example, the :has-note action may be used when setting up the index buffer, which would not be completed if :has-note raised an error.)

joostkremers commented 4 months ago

I've also made it so that the note checking closure is cached to improve performance.

Good idea. You could actually do that in ebib-citar-mode, and have it raise an error if citar hasn't been configured (i.e., if calling citar-has-notes returns nil).

BTW, I added ebib-citar-backend as a possible value for ebib-notes-backend, but given that ebib-citar implemented as a minor mode, it doesn't really make sense to offer it as an option, because if the user selects it, the minor mode activation isn't run.

swflint commented 4 months ago

Sorry for the slow response -- it's end of term around these parts.

This is one thing that citar does handle. If an entry has more than one note, citar:

transforms each note using the function stored in the :transformer prop of the notes backend prompts you to select between these (transformed) candidates with completing-read uses clever magic with text properties to get back the actual notes object from the transformed string

This is all handled by citar--select-resource.

I'd missed that -- but I'll have to see if I can get it to work.

I've now pushed an initial implementation of the notes back-end idea to https://github.com/joostkremers/ebib/tree/devel/notes-redesign.

Do you want me working on swflint:ebib-citar or your new branch? If nothing else, I'd like to see if I can get the citar integration working well.

joostkremers commented 4 months ago

Sorry for the slow response -- it's end of term around these parts.

Don't worry. :slightly_smiling_face: I happen to have some time now, but that may change any time.

I've now pushed an initial implementation of the notes back-end idea to https://github.com/joostkremers/ebib/tree/devel/notes-redesign.

Do you want me working on swflint:ebib-citar or your new branch?

Good question... Your PR is included in the new branch, so it would make the most sense to work on that (the new branch, I mean). I'm not sure how that would work from a practical point of view, though. Suggestions welcome.

If nothing else, I'd like to see if I can get the citar integration working well.

That'd be great.

swflint commented 4 months ago

Good question... Your PR is included in the new branch, so it would make the most sense to work on that (the new branch, I mean). I'm not sure how that would work from a practical point of view, though. Suggestions welcome.

I can branch off of that & rebase regularly to get the citar implementation working (and work only on ebib-citar.el).

swflint commented 4 months ago

I think my updates should get it pretty close (https://github.com/swflint/ebib/tree/improve-ebib-citar). The only thing is, it may make sense to have an option to return buffer focus to the index or entry buffer.