Improve ebib citar

[joostkremers]

@swflint , thanks, I pulled in your changes, and made some cosmetic changes myself.

I've also removed the option ebib-citar-backend from the definition of ebib-notes-backend, because the Citar backend is invoked through a minor mode. That actually makes sense, because it also sets a Citar-related variable (citar-open-entry-function), but that also means that setting the option through Customize wouldn't achieve the same result.

I also added actions for :extract-text and :delete-note, which just say that those actions aren't possible. I'm not sure what's better, though: informing the user or just have them fail silently. WDYT?

[Hugo-Heagren]

I'm not sure what's better, though: informing the user or just have them fail silently. WDYT?

I would support raising an error here. As Steve Purcell once said:

Some folks think that quietly working around unexpected cases is "defensive programming", but real defensive programming is failing fast and loudly when something seems off.

[swflint]

I'm not sure what's better, though: informing the user or just have them fail silently. WDYT?

I would support raising an error here. As Steve Purcell once said:

A warning (with display-warning) would make more sense. This situation is not catastrophic, shouldn't cause the debugger to trip if debug-on-error is set. Additionally, it's easier to configure which warnings are ignored.

However: with this design, it's the responsibility of backend writers to error or warn when they choose not to implement some bit of functionality.

[joostkremers]

Hmm, I didn't think of display-warning... Ebib has its own function for the purpose of logging warnings and errors, ebib--log, but it may make sense to make use of display-warning instead.

Anyway, to the topic at hand: I actually now also think it makes sense to issue an error or at least a warning: even though :extract-text and :delete-note are only used when ebib-notes-display-note-method is set to top-lines, that is the default value, and when those actions don't do anything, there is no immediate feed-back that something is wrong, unless they tell the user so.

Perhaps, though, it would make sense to change the default value of ebib-notes-display-note-method from top-lines to all, which displays the (entire) note in a separate buffer. Or, more conservatively, ebib-citar-mode could check if ebib-notes-display-note-method is set to top-lines and if so, change it to all. (If it's set to nil, the user explicitly asked notes not to be shown, so we shouldn't mess with it.)

[swflint]

Hmm, I didn't think of display-warning... Ebib has its own function for the purpose of logging warnings and errors, ebib--log, but it may make sense to make use of display-warning instead.

I saw ebib--log used, but display-warning has the benefit of being generic, and lets users easily configure what warnings are or are not displayed. It may also make sense, if we do that, to have some sort of "warn once per session" logic, which only shows the warning the first time.

Anyway, to the topic at hand: I actually now also think it makes sense to issue an error or at least a warning: even though :extract-text and :delete-note are only used when ebib-notes-display-note-method is set to top-lines, that is the default value, and when those actions don't do anything, there is no immediate feed-back that something is wrong, unless they tell the user so.

I can agree with that, though again, I think a warning makes more sense than an error. That said, if we're using pcase, we could just have a fall-through case and warn something along the lines of "Operation %s not implemented by backend" and use that.

Perhaps, though, it would make sense to change the default value of ebib-notes-display-note-method from top-lines to all, which displays the (entire) note in a separate buffer. Or, more conservatively, ebib-citar-mode could check if ebib-notes-display-note-method is set to top-lines and if so, change it to all. (If it's set to nil, the user explicitly asked notes not to be shown, so we shouldn't mess with it.)

I think that is a reasonable change. It's a fairly simple one, and given the use of an external system for notes management, that should work. However, as I noted in #299, we'll need to make sure that focus eventually ends up in the right place (i.e., the index buffer) when notes are shown.

[joostkremers]

I saw ebib--log used, but display-warning has the benefit of being generic, and lets users easily configure what warnings are or are not displayed.

Yes. Probably my next project: replace ebib--log with display-warning.

It may also make sense, if we do that, to have some sort of "warn once per session" logic, which only shows the warning the first time.

:+1:

I can agree with that, though again, I think a warning makes more sense than an error.

Yes, I agree. I've only used an error in cases where the error is a direct response to a user command, though, so it's almost like a warning, and I've been using user-error (at least in the notes backend), so it doesn't trigger the debugger.

Still, display-warning seems a better fit, so I'm happy to use that.

However, as I noted in #299, we'll need to make sure that focus eventually ends up in the right place (i.e., the index buffer) when notes are shown.

In the system as I envisage it, the back-end makes sure the buffer exists but does not display it nor select it. It just returns the buffer info, after which it's up to Ebib to display it and possibly select it (in case the note was opened for editing).

The reason is that displaying the note and editing both use :open-note, so the back-end doesn't know what to do with the buffer.

The exception is :create-note, because then it's clear that the note buffer should be displayed and selected. That's why the back-end can do it (as e.g., the org-capture backend), but it can also leave it up to Ebib.

If those assumptions don't work for Citar, we should discuss how we can handle it.

[swflint]

However, as I noted in #299, we'll need to make sure that focus eventually ends up in the right place (i.e., the index buffer) when notes are shown.

In the system as I envisage it, the back-end makes sure the buffer exists but does not display it nor select it. It just returns the buffer info, after which it's up to Ebib to display it and possibly select it (in case the note was opened for editing).

The reason is that displaying the note and editing both use :open-note, so the back-end doesn't know what to do with the buffer.

I agree that is ideal. I've tried to prevent the buffer from being displayed, but, even in the most basic notes configuration (citar-notes-sources default entry citar-file) the function to open a note is find-file, which given the normal usecase of Citar makes sense.

The exception is :create-note, because then it's clear that the note buffer should be displayed and selected. That's why the back-end can do it (as e.g., the org-capture backend), but it can also leave it up to Ebib.

That makes sense.

If those assumptions don't work for Citar, we should discuss how we can handle it.

Agreed. I think the assumptions work in general, we may just need a bit more careful coding to make the focus work correctly with a Citar backend.

[Hugo-Heagren]

Sorry I haven't been following the discussion as closely as I could have, but...

In the system as I envisage it, the back-end makes sure the buffer exists but does not display it nor select it. It just returns the buffer info, after which it's up to Ebib to display it and possibly select it (in case the note was opened for editing).

The reason is that displaying the note and editing both use :open-note, so the back-end doesn't know what to do with the buffer.

The exception is :create-note, because then it's clear that the note buffer should be displayed and selected. That's why the back-end can do it (as e.g., the org-capture backend), but it can also leave it up to Ebib.

If those assumptions don't work for Citar, we should discuss how we can handle it.

If I understand you correctly, I can say that these assumptions are not consistent with citar, as I discussed here. The important point is bullet point 2. It is possible to write a backend, entirely consistent with citar's system, which does all the buffer handling itself, including opening and displaying the buffer and selecting the window in which it is displayed. In this situation, it might not be (easily) possible to return a buffer at all.

[joostkremers]

If I understand you correctly, I can say that these assumptions are not consistent with citar, as I discussed here. The important point is bullet point 2. It is possible to write a backend, entirely consistent with citar's system, which does all the buffer handling itself, including opening and displaying the buffer and selecting the window in which it is displayed. In this situation, it might not be (easily) possible to return a buffer at all.

For :create-note, that's perfectly fine. The note buffer needs to be selected in that case and it's OK if the back-end handles that and just returns nil to Ebib.

For :open-note, it's more problematic, because Ebib calls this action in two cases: 1) if the buffer needs to be displayed but not selected (focus should stay/return to Ebib's index buffer), and 2) if the user wants to edit the note (in which case the note buffer must be selected). This is why :open-note should just return the buffer info.

So the question is, what is possible with Citar? Does opening the note always select the note buffer? In that case, we might split up :open-note into two actions, :display-note and :edit-note. :display-note would then open the note and call (ebib--pop-to-buffer (ebib--buffer 'index)) afterward to return focus to the index buffer.

We could also allow :edit-note to create a new note if that would make it easier. Right now, :open-note should return nil if there is no note for the current entry, but Ebib immediately calls :create-note if that happens, so it would be OK if the back-end did that itself.

I'm open to making chances to Ebib to make the Citar back-end work. I just need to know which changes to make... :slightly_smiling_face:

[Hugo-Heagren]

AFAICT the API is not properly documented. But here is what I can find.

Citar expects the notes backend will include a plist with (some of?) the following keys:

• :items
• :hasitems
• :open
• :create
• :annotate

:items has to hold a function taking one argument (keys, which is a list), and returning a hash table. The keys in the table should be the elements of the argument keys and the values should be lists of the notes objects associated with that key. Citar makes no assumptions about what a notes object is. In my setup, notes objects are the titles of org-roam nodes.

:hasitems takes no arguments, and must return a function which takes one argument (key), and returns non-nil if the entry with that key has any notes.

:open is a function of one argument (note, a note object) which opens the note given the argument. My :open looks like this:

(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)))

org-roam-capture- takes care of buffer display.

:create is a function which takes two args (key and entry) and creates a note. Citar makes no assumptions about this means, and does not do anything with the return value. The definition for the user command citar-create-note looks like this:

(defun citar-create-note (key &optional entry)
  "Create a note for KEY and ENTRY.
If ENTRY is nil, use `citar-get-entry' with KEY."
  (interactive (list (citar-select-ref)))
  (funcall (citar--get-notes-config :create) key (or entry (citar-get-entry key))))

As you can see, citar just selects a key/entry pair, and then calls the function in :create-note with those values. In particular, citar:

• makes no assumptions about the return value of the function for creating notes
• does not treat creating and opening as separate operations (i.e. does not expect to have to call an 'open-note' function on the return value the :create-note function)

Does that help?

There's some other stuff (:annotate, :transform) but its just cosmetic.

[Hugo-Heagren]

Sorry, to come back to your concrete question:

Does opening the note always select the note buffer?

Citar's notes API does not ensure that it does. I imagine most users who write custom setups will (themselves, in the code for those setups) ensure that it does, but this is independent of citar.

[swflint]

That the Citar API is not well-documented in this regard is certainly making it more difficult. There's an open issue about display, and whether or not it's even in the purview of Citar (emacs-citar/citar#785). I'm going to comment and ask for clarification there as well.