Revise note API (again)

[bdarcus]

We need to revise again the note API so that notes work correctly with citar-open, while also being configurable, but I suggest we do this after #634.

---

The problem

With notes, the issue is that we don't can't assume the category is always a file; it could also be some sub-file section.

For purposes of this discussion, the completion category can be file or org-roam-node.

If the category is file, then the candidates need be no different than citar-get-files; it's only with the org-roam-node case that it gets complicated.

And even more specifically, it's only if we insist on allowing different categories within the same group, which I'm skeptical about. I'm not even clear how we do that from a technical POV.

Proposed solution

Per minad below:

... use the multi-category category in the same way as consult--multi uses it. The multi-category is also supported by Marginalia and Embark. The candidates must carry a multi-category text property, see consult--multi-candidates.

So have citar-get-notes return a list of completion strings regardless of completion category, and change citar-open-note-functions to a plist something like this, which is how consult--multi does it:

(defvar citar-org-roam-open-notes-config
  `(:name "Org-Roam Ref Notes"
    :category 'org-roam-node
    :key-predicate ,#'citar-org-roam-has-note
    :action ,#'citar-org-roam-open-note-from-id
    :items ,#'citar-org-roam--get-candidates))

That's simple and flexible, with the sole constraint that all notes within a group must be the same category.

Then with citar-org-roam can just do:

(setq citar-notes-config citar-org-roam-open-notes-config) ; the package can include the plist for easy config

On the candidates: I have an experimental citar-org-roam--get-ref-nodes function in the citar-org-roam repo that returns a list of propertized strings, using the hidden id trick, with citekey and title in the visible part.

Conclusion

I see no downside to this approach. I think the question is just whether the defcustom becomes citar-notes-config, or citar-notes-configs. The latter would allow multiple plists, and hence notes groups. I have no opinion on this question.

Except, if it's a list, the whole citar-open UI may as well be configured with this approach; not just notes? So citar-open-config?

(setq citar-open-config '(citar-open-note-config citar-open-file-config citar-open-link-config))

Originally posted by @bdarcus in #634 (comment)

[bdarcus]

@roshanshariff I had an epiphany on this just now, and so have revised the proposal to simplify.

OTOH, I don't want to hold up #634 too much longer; we've been working on that for I think more than a month at this point.

It might be good to keep status quo there, and worry about proper support for multi-category, including whether we should just use consult--multi for citar-open, in a separate PR?

In any case, some further info, and demo code ...

The code in citar-org-roam (the config plist and the functions it specifies) should be ready for you to play with. As in, this works to return a candidate list:

(funcall (plist-get citar-org-roam-open-notes-config :items) keys)

... and to open a selected candidate:

(funcall (plist-get citar-org-roam-open-notes-config :action) candidate)

Note also that multi-category is used by embark (as noted in a docstring for citar--open-multi), but also by consult in consult--multi. And here's what those candidates look like:

         (multi-category
          ('org-roam-node .
            #("fd1e8dfa-8bfd-43fb-884a-b350a7a0850a agnew2003                     Notes on Agnew, From Megalopolis to Global City-Region?" 0 37
              (invisible t)
              37 46
              (face citar-highlight)
              67 122
              (face citar)))
          face citar)
         122 123

So we seem to have a cons embedded in a string propertized with "multi-category".

This again raises the question for me of whether we should just use consult--multi instead. This, for example, works:

(defun citar-consult-open ()
  "Open related resources."
  (interactive)
  (let* ((keys (citar-select-refs))
         (sources
          `((:name "Org-Roam Ref Notes"
             :category org-roam-node
             :action ,#'citar-org-roam-open-note-from-id
             :annotate ,#'citar-org-roam--annotate
             :items ,(citar-org-roam--get-candidates keys))
            (:name "Related Files"
             :category file
             :action ,#'citar-file-open-external
             :items  ,(citar-get-files keys))
            (:name "Links"
             :category url
             :action ,#'browse-url
             :items ,(citar-get-links keys)))))
    (consult--multi sources)))

I just can't figure out how to avoid let-binding the sources because of that keys arg. Edit: see minad's comment below; consult--multi isn't meant for this use case.

[bdarcus]

@minad we're again thinking about consult--mutli (or writing something very similar) for citar-open. In the working example in the preceding comment, is there a way for us to avoid let-binding the sources?

We need to dynamically generate the :items depending on what reference keys the user selects.

[minad]

This doesn't look like a good use case of consult--multi. consult--multi is mostly useful if the sources can be configured flexibly and if they are not hard coded. If you have hard coded sources anyway I suggest to use completing-read with the group-function.

[bdarcus]

I suggest to use completing-read with the group-function.

That's what we do now.

Any tips how to address the fact that we have multiple categories, and in the case of notes, we won't know what they are upfront (can be files, as now, or with Org-Roam can be files or sub-file nodes)?

I don't really understand that part of completing-read, though perhaps @roshanshariff does.

[minad]

You can use the multi-category category in the same way as consult--multi uses it. The multi-category is also supported by Marginalia and Embark. The candidates must carry a multi-category text property, see consult--multi-candidates. You should read through the Consult source for the details.

[bdarcus]

You should read through the Consult source for the details.

Yes, I have. I just wanted to confirm that.

So no to consult--multi, but yes to multi-category and to consult--multi-like source config plists.

I guess we're already using multi-category in citar--select-resource , so probably just involves extending to notes. Or rather, right now, we only have file or url categories there; need to allow notes to be something else.

As always, thank you!

[bdarcus]

I have an idea for a possible solution:

Add an optional note-nodes arg to citar--select-resource.

Actually, have all the args optional then, and have them keywords.

I just pushed a commit for this to #634, and I think it's all working correctly ATM. For example:

ELISP> (citar--get-resource-candidates '("toly2017") :files t :notes t :links t)
(#("https://doi.org/10.1080/14747731.2016.1233679" 0 45
   (multi-category
    (url . "https://doi.org/10.1080/14747731.2016.1233679")))
  #("/home/bruce/Zotero/storage/PKZ88BS7/14747731.2016.html" 0 54
    (multi-category
     (file . "/home/bruce/Zotero/storage/PKZ88BS7/14747731.2016.html")))
  #("/home/bruce/Zotero/storage/4XRNSQEB/Toly_2017_Brexit, global cities, and the future of world order.pdf" 0 102
    (multi-category
     (file . "/home/bruce/Zotero/storage/4XRNSQEB/Toly_2017_Brexit, global cities, and the future of world order.pdf"))))

[bdarcus]

Looking now at note config, here's the new plist as it stands:

(defvar citar-notes-config-file
  `(:name "Notes"
    :category file
    :key-predicate ,#'citar-file-has-notes
    :action ,#'citar-file--open-note
    :items ,#'citar--get-file-notes)
  "Default file-per-note configuration.")

And here's the current defcustoms:

1. citar-create-note-function
2. citar-has-notes-functions
3. citar-open-note-function
4. citar-open-note-functions

Observations/thoughts:

• Obviously the last two are redundant, and all of them need to be deprecated.
• As I said above, I don't think we need multiple open functions here. If we need that flexibility, we can perhaps expand to allow multiple plists, where there could be multiple notes groups.
• I think the single :action function is enough for opening and creating, but am not sure. Probably need to check how ORB would fit here.
• If it's not, we can always add another property for this to distinguish :open and :create.

[bdarcus]

@myshevchuk - turns out our earlier discussion on note config got a little more complex. Any feedback, particularly on my last post?

These changes will allow a package like ORB to directly update the citar UI for presence of notes, configure the candidate display (including annotations), and open the selected candidate from citar-open or citar-open-notes.

The idea would be to keep the current defcustom around a bit, until I tag v1.0 sometime in the next month or so.

Or I suppose we could iterate the current config approach.

[roshanshariff]

@bdarcus, I like the new API design. I would just make one small change: instead of just having the citar-notes-config variable be a plist, it should be an alist (maybe called citar-notes-sources). The keys should be the names of notes backends, and the values should be plists. Something like

(defcustom citar-notes-sources ((citar-file . PLIST))

Then there should be another variable which selects the source (or sources) to actually use, which will actually be modified by the user:

(defcustom citar-notes-source 'citar-file)

Then citar-org-roam can add itself to citar-notes-sources when it's loaded, but there won't be any change in behaviour until the user changes citar-notes-source. This is a common pattern in other libraries (e.g. org-export-registered-backends).

[bdarcus]

@roshanshariff OK, will take a look tomorrow.

While I have you, do you know how to modify our group function so it will work with non-file notes? This is the primary bug I'm stuck on.

Can we just inspect the multi-category metadata?

(defun citar--select-group-related-resources (resource transform)
  "Group RESOURCE by type or TRANSFORM."
  (let ((extension (file-name-extension resource)))
    (if transform
        (if (file-regular-p resource)
            (file-name-nondirectory resource)
          resource)
      (cond
       ;; FIX doesn't work with node-id notes
       ((member extension citar-file-note-extensions) "Notes")
       ((string-prefix-p "http" resource 'ignore-case) "Links")
       (t "Library Files")))))

[roshanshariff]

Maybe citar--select-resource could add its own text property to the candidates? Maybe something like 'citar--resource-type which can be 'file, 'note, or 'link. Then citar--select-group-related-resources would just read that property and return the group name.

[bdarcus]

I was thinking it's valuable to use 'file as category for file notes, given annotations, embark, etc.

So distinguish file notes from node-notes.

[roshanshariff]

Maybe the 'multi-category property annotation can be left to the note backend providers? That way. for example, citar-org-roam could apply the file category to Org IDs that map to files, but not to those that map to nodes within a file. Then citar--select-resource would just use the candidates it got from the back-end as-is (just adding its own property for the grouping function to use).

[bdarcus]

Right now, the category is specified in the plist, so I think it's already as you suggest.

I was really just thinking for the default plist and the org-roam one.

Edit: also I was wondering about using that metadata.

I was wanting to do something like this (which actually works with the org-roam nodes, but probably won't with the default file notes), or even have all of these configured similarly.

(defun citar--select-group-related-resources (resource transform)
  "Group RESOURCE by type or TRANSFORM."
  (if transform
      (if (file-regular-p resource)
          (file-name-nondirectory resource)
        resource)
    (let ((cat (car (get-text-property 0 'multi-category resource))))
      (pcase cat
        ('file "Files")
        ('url "Links")
        (_ (plist-get citar-notes-config :name))))))

I think I can use a variation of this trick I just figured out for the annotation function here as well:

  (let* ((nodecat (car (get-text-property 0 'multi-category cand)))
         (notecat (plist-get citar-notes-config :category))
         (annotate (plist-get citar-notes-config :annotate))
         (ann (when (and annotate (string= nodecat notecat)) ; check that category matches the plist

Just need to fix the annotation alignment, and get file note grouping working again.

[bdarcus]

Maybe we could define default category as 'note-file and use the marginalia 'file annotation for that?

[bdarcus]

I pushed the defcustoms, but can't figure out how to access the plists from them.

ELISP> (plist-get (cdr (assoc citar-notes-source citar-notes-sources)) :name)
nil
ELISP> citar-notes-config-file
(:name "Notes" :category file :key-predicate citar-file-has-notes :action citar-file--open-note :items citar-file--get-note-files)

And here's what I have:

(defcustom citar-notes-sources
  '((citar-file . citar-notes-config-file))

Once I get that sorted, I'll add a citar--get-note-property convenience function.

Or maybe cl-defstruct would be useful here too?

[bdarcus]

Maybe citar--select-resource could add its own text property to the candidates?

I ended up adding a notep property to note candidates, and then the group function uses that, or falls back to the completion category, which is known and fixed for files and links:

      (if notep (plist-get citar-notes-config :name) ; if note, get the group name from plist
        (pcase cat ; else use the completion category
          ('file "Library Files")
          ('url "Links"))))))

So this allows us to use 'file as the default note type without problem, which not only brings the automatic marginalia annotation, but also means embark will recognize it.

[bdarcus]

I pushed the defcustoms, but can't figure out how to access the plists from them.

I implemented it with some help from someone suggesting:

A better approach is to initialize t/config like this
(defcustom t/config `((test1 . ,(list :one 1 :two 2))))

Then a user can modify (cdr (assq 'test1 t/config)) directly using setf or alike

[myshevchuk]

@myshevchuk - turns out our earlier discussion on note config got a little more complex. Any feedback, particularly on my last post?

These changes will allow a package like ORB to directly update the citar UI for presence of notes, configure the candidate display (including annotations), and open the selected candidate from citar-open or citar-open-notes.

The idea would be to keep the current defcustom around a bit, until I tag v1.0 sometime in the next month or so.

Or I suppose we could iterate the current config approach.

I definitely love the approach where an external package like ORB could feed the necessary information to Citar using the exposed API. I remember the difficulties I had with ORB trying to hijack Ivy-bibtex to update the "has-note" indicator, and I think it ended up buggy anyway.

I can't speak much about the new Citar API right now because I haven't had time to look at it in detail yet. In any case, the suggested fine-grained control over the different citation resources looks very thorough and systematic. Whatever the new Citar API will look like, ORB will support it.

[bdarcus]

@myshevchuk - sounds good.

Note I'm developing this small extension alongside this to make sure the API is correct.

https://github.com/emacs-citar/citar-org-roam/blob/main/citar-org-roam.el

I'm not sure how it will evolve, but am thinking it will remain pretty small and just handle the integration, and other packages can build on it. Or they write their own integration functions, and that becomes a demonstration of how.

[bdarcus]

Here's another example of a notes source, using the new API; from today!

https://github.com/localauthor/zk/blob/main/zk-citar.el

PS - any comments on the notes API can go here.

#651

I'll likely make one tweak based on the feedback so far.

[bdarcus]

@myshevchuk

Whatever the new Citar API will look like, ORB will support it.

FYI, I'll be tagging 1.0 in the coming days, and then bumping the doom biblio module.

I will post this on the wiki, I think, for folks who want to continue to use ORB:

(citar-register-notes-source
 'orb-citar-source
 (list :name "Org-Roam Notes"
        :category 'org-roam-node
        :items #'citar-org-roam--get-candidates
        :hasitems #'citar-org-roam-has-notes
        :open #'citar-org-roam-open-note
        :create #'orb-citar-edit-note
        :annotate #'citar-org-roam--annotate))

(setq citar-notes-source 'orb-citar-source)

This relies on https://github.com/emacs-citar/citar-org-roam.

[myshevchuk]

@bdarcus excellent! I will update the ORB readme too.

[cdlm]

FYI seems like citar-register-notes-source wants two arguments now (and while debugging that last night, I didn't understand the need for defconst?)

[bdarcus]

It's always had two arguments, since it's just adding to an alist.

Oh sorry; my bad, fixed. :-)

I wasn't sure on your second question either. I copied and pasted it from the citar-org-roam example (though just removed it to avoid confusion). @roshanshariff?

[roshanshariff]

I'm not sure I understand, but there's never any "need" for defconst. You can always just use a defvar instead if you prefer. Or even skip giving the plist a name:

(citar-register-notes-source 'my-notes-source '(:name "My Notes Source" :create my-note-create-function ...))

But in citar-org-roam, the citar-org-roam-config plist constitutes some metadata about the package itself (specifically, the functions it exposes to manipulate notes) so it's semantically a constant. There shouldn't be any need to modify it unless you're changing the package.

The defconst also reminds you that changing the variable won't affect the actual notes source in citar unless you again run citar-register-notes-source. But if you find yourself wanting to change it, you're probably better of making your own notes source by copying and modifying. For example, to make your own source that is like the Org-Roam one but with a different create function:

(citar-register-notes-source 'my-notes-source (plist-put (copy-sequence citar-org-roam-config) :create #'my-note-create-function)
(setq citar-notes-source 'my-notes-source)

I hope that answers your question, but I'm happy to clarify otherwise.