Review, refine `citar-org-roam--create-capture-note `

bdarcus commented 2 years ago

This function is adapted from @jethrokuan's post:

https://jethrokuan.github.io/org-roam-guide/#orgc48eb0d

I'm not super familiar with the org-roam capture system, so would help if someone who is can review this and suggest any changes.

Aside from fixing the variable bug noted in the code, it might be nice if this was accessible and configurable from org-roam-capture, which it isn't now. Per below, I'm still unsure of the ideal UI and config details here.

Note: this reflects changes in https://github.com/emacs-citar/citar/pull/634; also, org citations aren't allowed in property drawers, so I changed that.

roshanshariff commented 2 years ago

I can help get this working when I'm done with emacs-citar/citar#634 and have some more bandwidth. I don't use Org-Capture myself, but I it has some features that the create-note function in Citar doesn't. For example, it can store notes as sub-headings inside a single file, insert the last stored Org link, prompt for certain template fields that are expanded within the note, etc.

bdarcus commented 2 years ago

For example, it can store notes as sub-headings inside a single file, insert the last stored Org link, prompt for certain template fields that are expanded within the note, etc.

OK, those are good reasons to keep it and fix it then.

It occurs to me also, as I noted in an edit above, that if possible, it'd be good to be able to add a ref note from org, using capture.

So I merged that branch to main.

I did add back the entry argument to the create-note function for now.

I go back-and-forth on whether this should be in citar, but am leaning towards keeping it here, since I don't really know how it will evolve, and it could be good to keep it independent.

PS - I sent you an invite to the org, just in case it's easier to manage permissions, if it should be useful.

Vidianos-Giannitsis commented 2 years ago

Hi, I gave this a look and I am pretty sure there is a small mistake in the function. Aren't the :node and :props arguments supposed to go to org-roam-capture- and not the list generated in :info? If I am not mistaken that is something that needs to be changed.

(defun citar-org-roam--create-note (key entry) 
   "Create org-roam node for KEY and ENTRY." 
   ;; adapted from https://jethrokuan.github.io/org-roam-guide/#orgc48eb0d 
   ;; FIX doesn't work ATM. 
   (let ((title (citar-format--entry 
                 "${author editor} :: ${title}" entry))) 
     (org-roam-capture- 
      :templates 
      '(("r" "reference" plain "%?" :if-new 
         (file+head 
          "%(concat citar-org-roam-subdir \"/\" \"${key}.org\")" 
          ":PROPERTIES: 
 :ROAM_REFS: @${key} 
 :END: 
 #+title: ${title}\n") 
         :immediate-finish t 
         :unnarrowed t)) 
      :info (list :citekey key)
      :node (org-roam-node-create :title title) 
      :props '(:finalize find-file))))

I have tinkered a bit with the org-roam capture system (mainly for making my own specialized templates) so I can probably help with this, but I have 2 questions. I wanted to run this function as a test to try its behavior but I am a bit confused with the entry argument. What are you supposed to pass to it? And my other question is what exactly does this plan to achieve. Is it just supposed to create a ref node for a bibtex entry with the minimum amount of metadata for it to be functional or do you want to add more things to it? I am asking because how I look at it this should be a working function for just initializing the note, but there are other cool things you could add to something like this.

bdarcus commented 2 years ago

I wanted to run this function as a test to try its behavior but I am a bit confused with the entry argument. What are you supposed to pass to it?

Probably easier to show you; entry is the metadata alist, which is valuable for constructing the node title, for example.

(defun citar-open-notes (keys)
  "Open notes associated with the KEYS."
  (interactive (list (citar-select-refs)))
  (dolist (key keys)
    (let ((entry (citar-get-entry key)))
      (funcall
       (citar--get-notes-config :action) key entry))))

And my other question is what exactly does this plan to achieve. Is it just supposed to create a ref node for a bibtex entry with the minimum amount of metadata for it to be functional or do you want to add more things to it? I am asking because how I look at it this should be a working function for just initializing the note, but there are other cool things you could add to something like this.

Its limitations are my limitations; specifically my lack of knowledge of org/roam-capture. I just tried to adapt Jethro's minimal example.

So if you'd like to submit a PR with those "other cool things", I'd love to see it!

EDIT: I think, ideally, the same function would open existing notes or create new ones (using :if-new). At least from the POV of the note source plist; the API. But that was one of my little questions; whether we can get away with only the :action function.

Vidianos-Giannitsis commented 2 years ago

I see, I will probably play around with this more and suggest anything I come up with.

bdarcus commented 2 years ago

FYI, I just renamed that function to citar-org-roam--open-capture-note, since it does both open existing and create new notes.

Except, the way I went with the citar integration is to use completion candidates that are node-ids, and then use those to open the notes.

So there would need to be some melding of this functionality somehow. I hadn't yet gotten to that part; was focused more on the other functions.

bdarcus commented 2 years ago

Also, just realized there was a bug on the citar side, that I just fixed.

Turning on citar-org-roam-mode and then running citar-open should work again (as it did before I made a late change and forgot to test).

I just added a screenshot to the README.

EDIT: the basics now work; both open existing (by id) and creating new (by citekey, via capture) notes.

lazzalazza commented 2 years ago

Hi! Wouldn't there be a way to simply use the default org-roam-capture-templates like it was possible with the old (citar-open-note-function 'orb-citar-edit-note) setting? Also, is there a way to configure citar-org-roam-subdir from outside the citar-org-roam.el file? Thanks!

bdarcus commented 2 years ago

Hi! Wouldn't there be a way to simply use the default org-roam-capture-templates like it was possible with the old (citar-open-note-function 'orb-citar-edit-note) setting?

IDK. Why I was asking for help here.

Also, is there a way to configure citar-org-roam-subdir from outside the citar-org-roam.el file?

Sure; it's a defcustom.

Or did you mean something different?

lazzalazza commented 2 years ago

A couple of things: if you set the citar-org-roam-subdir variable to be "", the concat command here at line 112 adds an extra "/" to the string. Do you think it should be removed by an extra if statement inside the concatenation? I have tried something like "%(concat (if (equal citar-org-roam-subdir \"\") nil (concat citar-org-roam-subdir \"/\")) \"${citekey}.org\")" but it proceeds to directly create the note and moving to it without opening a proper capture buffer.

I was able to get the 'orb-citar-edit-note to work as I was expecting by calling it directly from the defconst statement.

(defconst citar-org-roam-notes-config
  (list :name "Org-Roam Notes"
        ...
        :create 'orb-citar-edit-note
        ...)

I have tried to set it from my configuration file but it doesn't work from there, and this is most likely due to my ignorance in elisp (is there way to simply access the :create value without re-declaring all the others?). May I ask you how you would have proceeded to set the variable externally?

bdarcus commented 2 years ago

I'll fix the spurious "/". Should be set to nil in that case.

May I ask you how you would have proceeded to set the variable externally?

Edit: sorry. I misread.

Utimately, orb will probably be its own note source, so you would just choose that for citar-notes-source.

You may be able use setf there, but I haven't tried it, and not sure we'd recommend it.

bdarcus commented 2 years ago

I'll fix the spurious "/". Should be set to nil in that case.

Fixed.

If you don't want to use a subdir, set that value to nil.

... but it proceeds to directly create the note and moving to it without opening a proper capture buffer.

Yes; that's just how it works ATM.

If we know we're creating a bibliographic note here, because calling citar-open-notes, do we need a capture buffer?

Or is the idea that one might want to do this from org-roam?

If yes, that's one of the enhancements I don't know how to implement, and I don't have time ATM to figure it out.

Perhaps the others in this thread can.

But I do think there are some UX issues to think about.

Like, if one clicks on a citation within an org-roam document, which runs embark-act or a hydra (see below demo screenshot; or transient menu, and then selects to open a note, which command is doing that?

I think it can indeed just, as it would now, be citar-open-notes; that there's no need for capture.

In fact, it should work now, if you have citar and citar-embark installed and configued.

So we cover now two note-opening/creating contexts that work with org-roam:

from the citar UI
from the buffer, at point

Would you need to create a bibliographic note in some other context, then?

If from org-roam, via the capture menu, what benefit does that provide, and could that not just call citar-open-notes?

Maybe if one wanted more than one template for bibliographic notes?

I do know @roshanshariff had some ideas, however, to enhance this function. Perhaps he could weigh in here?

roshanshariff commented 2 years ago

I know that Org Capture is a very user-oriented feature and doesn't easily allow other packages to participate in the note formatting (for example to extend its template system, except by including arbitrary Elisp code). Org-Roam-Bibtex goes to heroic lengths to make it possible.

When I have some time, I'm planning to look into whether there's a way to do it without quite so much complexity, but it will probably involve some trade-offs. For example, you might have to invoke a Citar-specific capture command instead of org-roam-capture. But we'll see how the design works out.

bdarcus commented 2 years ago

What value does the capture menu provide for this particular use case, however?

roshanshariff commented 2 years ago

I don't personally use Org Capture very much, but I can imagine how its features might fit into someone's workflow:

Having multiple notes in a file, and possibly choosing which file to put them in. You might have different capture templates for different projects, and choose to put all the notes relating to a project in its own file.
If you're keeping a reading journal, you might want to use the the "datetree" feature to file each note under the current date.
The capture template can include the selected region. You might use this to store relevant snippets as you encounter them.

Pretty much any time you otherwise want to use Org Capture, but would like the template to be filled in using data from a Citar reference.

lazzalazza commented 2 years ago

I totally agree with @roshanshariff. I also like the way in which Org Capture lets you preemptively confirm or discard the new buffer without automatically saving it. Plus, people that integrated Org Capture in their workflow might like at least to have the option of using their general templates even with Citar...

bdarcus commented 2 years ago

Maybe we should have two notes sources:

The default citar one (without capture)
A capture one (where user can their own also)

Or a single one, with additional defcustom to choose?

I still don't understand how the UX would work on the second though.

lazzalazza commented 2 years ago

Here are some pictures of my UX.

Two more things:

I still can't modify citar-org-roam-notes-config with a setq from my init file: it would be nice, at least for the moment, to have a way to customize the note-creating function from outside citar-org-roam.el.
If I try to create a note with the standard citar-org-roam--create-capture-note command, after setting the citar-org-roam-subdir to "", I get a "file not found" and "buffer read-only" error.

bdarcus commented 2 years ago

Thanks for the screenshots!

Except, there you only have one bibliographic template; so what's the value of the capture menu there?

2. If I try to create a note with the standard citar-org-roam--create-capture-note command, after setting the citar-org-roam-subdir to "",

As I mentioned to you earlier (twice): has to be set to nil ;-)

lazzalazza commented 2 years ago

Sorry, my bad! Thanks!

Il giorno 19 lug 2022, alle ore 12:33, bdarcus @.***> ha scritto:

Thanks for the screenshots!

If I try to create a note with the standard citar-org-roam--create-capture-note command, after setting the citar-org-roam-subdir to "",

As I mentioned to you earlier (twice): has to be set to nil ;-)

— Reply to this email directly, view it on GitHub https://github.com/emacs-citar/citar-org-roam/issues/2#issuecomment-1188888155, or unsubscribe https://github.com/notifications/unsubscribe-auth/AONRCSKJRYX7BQH35LURWXLVUZ767ANCNFSM5ZX3AV5A. You are receiving this because you commented.

bdarcus commented 2 years ago

I do also want to mention that ORB will be supporting the new citar note API, and could in theory even use parts of this package.

So these are potentially complementary.

And to go back to your other point above:

I still can't modify citar-org-roam-notes-config with a setq from my init file: it would be nice, at least for the moment, to have a way to customize the note-creating function from outside citar-org-roam.el.

You have two options ATM:

You can create and register your own note source, and then use that.
I think you can use setf (not setq) to change the existing note source.

I'm not sure if @roshanshariff has any thoughts about how we should handle this piece. I was really contemplating two options:

per above, use note source plist for this
add a defcustom just for :create

IDK myself.

This is still kind of an experimental package at this point, that we put together to test out what we needed for the new citar notes API. It still needs a bit of work before we release it more formally.

bdarcus commented 2 years ago

Granting there's more work to be done here, anyone see any problem with announcing this and submitting to MELPA along with citar 1.0 (soon)?

It's functional and bug-free; no?

bdarcus commented 2 years ago

@lazzalazza - can you try this please; preferably today or tomorrow (the weekend)?

I want to recommend this to folks whose configs break :-)

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

lazzalazza commented 2 years ago

Hi! I put the last line of code in my config file and the rest of it in citar-org-roam.el and it works like a charm (there's an extra parens at the end though)... Thank you so much!

Il giorno 6 ago 2022, alle ore 13:17, bdarcus @.***> ha scritto:

@lazzalazza https://github.com/lazzalazza - can you try this?

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

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

(setq citar-notes-source 'orb-citar-source)) — Reply to this email directly, view it on GitHub https://github.com/emacs-citar/citar-org-roam/issues/2#issuecomment-1207196969, or unsubscribe https://github.com/notifications/unsubscribe-auth/AONRCSIHU2FVJNYMTCZZF53VXZCTZANCNFSM5ZX3AV5A. You are receiving this because you were mentioned.

bdarcus commented 2 years ago

Really the only change should be using ORB to create new notes. Is that the case?

lazzalazza commented 2 years ago

It is! It works!

Il giorno 6 ago 2022, alle ore 14:41, bdarcus @.***> ha scritto:

Really the only change should be using ORB to create new notes. Is that the case?

— Reply to this email directly, view it on GitHub https://github.com/emacs-citar/citar-org-roam/issues/2#issuecomment-1207207979, or unsubscribe https://github.com/notifications/unsubscribe-auth/AONRCSOO2ZTEUBJKFHDL5F3VXZMQJANCNFSM5ZX3AV5A. You are receiving this because you were mentioned.

bdarcus commented 2 years ago

Maybe we should have two notes sources:

Maybe we should do three, including the orb one I posted just above?

Or add it to the README

bdarcus commented 2 years ago

I know that Org Capture is a very user-oriented feature and doesn't easily allow other packages to participate in the note formatting (for example to extend its template system, except by including arbitrary Elisp code). Org-Roam-Bibtex goes to heroic lengths to make it possible.

I wonder if there are tweaks to upstream that could make this easier?

joyguar commented 2 years ago

I am trying to integrate citar/citar-org-roam with org-roam's capture functionality. I am not sure what exact solution has been arrived at on this issue. I see that you have provided the internal function citar-org-roam--create-capture-note. Do you see this function as the main entry point for a user to implement org-roam capture functionality?

The key difference I see between this function and the one in https://jethrokuan.github.io/org-roam-guide/ is that Jethro's method of accessing the citekey and entry as the car and cdr of a key value pair retrieved by citar-select-ref no longer works with Citar since a breaking change that you mention in https://github.com/emacs-citar/citar/issues/696. What would be the best way to get the key and associated entry to pass into citar-org-roam--create-capture-note as arguments?

bdarcus commented 2 years ago

Ideally we can modify citar-org-roam--create-capture-note to allow users to define their own capture templates. But I"m not sure how best to do that, and haven't had time to figure it out

What would be the best way to get the key and associated entry to pass into citar-org-roam--create-capture-note as arguments?

If you have the key, you can get its entry with citar-get-entry. Is that what you mean?

But the citar functions that open notes already pass the key and entry to these note functions.

In the meantime, see:

https://github.com/emacs-citar/citar/wiki/Notes-configuration#org-roam-bibtex

joyguar commented 2 years ago

citar-get-entry was exactly the function I needed, thank you!

emacs-citar / citar-org-roam

Review, refine `citar-org-roam--create-capture-note ` #2