search

jgru / consult-org-roam

A bunch of convenience functions for operating org-roam with the help of consult
GNU General Public License v3.0
121 stars 6 forks source link

Quick review #2

Closed minad closed 2 years ago

minad commented 2 years ago

Hi,

thank you for this package! I quickly looked over the code and have a few comments and improvement proposals.

jgru commented 2 years ago

Hi @minad,

thank you very much for looking at the code. Your interest in this package and the input on this is much appreciated! PR #4 implements almost all of you proposals for improvement. However, I refrained from reusing consult--temporary-files for consult-org-roam--temporary-nodes for two reasons: First, I need to derive the filename via org-roam-node-file node for each node, and, secondly, I intend to improve this function to open the actual org-roam-node and not just the housing file. Is this okay for you? I hope #4 is fine, do you have any further recommendations?

Thanks again and best regards, jgru

minad commented 2 years ago

Of course not using the temporary file function is perfectly okay if it is sufficiently different. But I don't get what the difference is between a node and a file. From my understanding I thought these are just the same? Or at least you have to open the file first and then jump to the headline. In this case you should still reuse consult--temporary-files for the file opening.

jgru commented 2 years ago

Of course not using the temporary file function is perfectly okay if it is sufficiently different. But I don't get what the difference is between a node and a file. From my understanding I thought these are just the same? Or at least you have to open the file first and then jump to the headline. In this case you should still reuse consult--temporary-files for the file opening.

Actually, a node is represented by an ID (of a headline) within an org-file. In code a node is a struct created by cl-defstruct. I was wondering whether there is a better approach, but I didn't find a way to pass a filepath instead of a node-struct to consult-org-roam--temporary-nodes. Can you give me a hint, how to pass filenames (at best with a precise position inside it) instead of nodes to consult--temporary-files?

minad commented 2 years ago

It should be possible to retrieve the node id and file path from the org roam node struct, or not?

jgru commented 2 years ago

It should be possible to retrieve the node id and file path from the org roam node struct, or not?

Sure, I did this in my "sodomized" version of consult--temporary-files, but now I moved this functionality into consult-org-roam--node-preview. (See 6885fb1701bb32e0fc8a2fc65423bb319a044ca2) I think this resolves all of the valuable remarks brought up by you. However, in the future I will have to figure out how to pass the relevant position of the headline to the open-function. (If you have any ideas on this, please let me know.

Thanks again for helping out and best regards, jgru

jgru commented 2 years ago

Closed by #4.

minad commented 2 years ago

Very good. Note that I am currently improving the file preview (https://github.com/minad/consult/commit/3b0d745a480220d3eb8367fe73e8bf1321034a4e), so it makes a lot of sense to not duplicate consult--temporary-files.

However, in the future I will have to figure out how to pass the relevant position of the headline to the open-function. (If you have any ideas on this, please let me know.

You can use the buffer returned by consult--temporary-files and then move the point inside that buffer. The point is made available by the org-roam-node struct it seems. Or you use the title string and jump there.

jgru commented 2 years ago

Very good. Note that I am currently improving the file preview (minad/consult@3b0d745), so it makes a lot of sense to not duplicate consult--temporary-files.

Good to hear. I will keep an eye on this improvement. :)

You can use the buffer returned by consult--temporary-files and then move the point inside that buffer. The point is made available by the org-roam-node struct it seems. Or you use the title string and jump there.

Okay, then I will just have to modify the preview function. Thanks for the hint on this.