[PATCH] Allow external libraries (org-roam) to supply org-id locations

[PATCH] Allow external libraries (org-roam) to supply org-id locations

[Rick Lupton]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

Hi all,

Since updating the org-id code [1] to use the standard org-link registered functions instead of hard-coding org-id opening, I have a problem using org-roam.

org-roam overwrites the :follow function for "id" links from the build-in `org-id-open' to its own `org-roam-id-open' (https://github.com/org-roam/org-roam/blob/8667e441876cd2583fbf7282a65796ea149f0e5f/org-roam-id.el#L91).  The only change between these functions is to insert a call to try `org-roam-id-find' before trying `org-id-find', which uses org-roam's cached sqlite database to find the id (https://github.com/org-roam/org-roam/blob/8667e441876cd2583fbf7282a65796ea149f0e5f/org-roam-id.el#L70-L71)

As well as being messy, my specific problem is that the improvements to open search strings in org-id links are no longer enabled when org-roam is loaded.

It seems reasonable that a library might want to provide its own way of locating org-ids.  The attached patch adds a new hook variable `org-id-find-functions` which contains functions doing the same job as `org-id-find' that should be tried first.  Then all org-roam needs to do is add its `org-roam-id-find' to the hook.

Does this seem like a good idea?

Thanks,
Rick

[1] Link: https://list.orgmode.org/118435e8-0b20-46fd-af6a-88de8e19fac6@app.fastmail.com/

[-- Attachment #2: 0001-lisp-org-id.el-add-hook-org-id-find-functions.patch --]
[-- Type: application/octet-stream, Size: 3893 bytes --]

From bce5ed6fee217bd4cdfaa432283126446f08b66c Mon Sep 17 00:00:00 2001
From: Rick Lupton <mail@ricklupton.name>
Date: Tue, 12 Mar 2024 22:56:28 +0000
Subject: [PATCH] lisp/org-id.el: add hook `org-id-find-functions'

* lisp/org-id.el (org-id-find-functions): New hook for functions to find
location of org-ids.
(org-id-find): Try to use `org-id-find-functions' first when looking for
an org-id.
* testing/lisp/test-ol.el: Add test for `org-id-find-functions`.

This accommodates tools such as org-roam which otherwise overwrite the
org-id link opening function in order to use their own cached database
of org id locations.
---
 etc/ORG-NEWS            |  6 ++++++
 lisp/org-id.el          | 34 ++++++++++++++++++++++++----------
 testing/lisp/test-ol.el |  8 ++++++++
 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index abe62daaf..2223fb332 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -1308,6 +1308,12 @@ It does no longer use regexps.
 
 It is also faster. Large tables can be read quickly.
 
+*** New hook ~org-id-find-functions~
+
+Functions added to this hook are tried first when locating an org-id.
+Libraries can use this to provide additional methods to efficiently
+locate ids beyond the built-in caching.
+
 * Version 9.6
 
 ** Important announcements and breaking changes
diff --git a/lisp/org-id.el b/lisp/org-id.el
index 58d51deca..a7db53a59 100644
--- a/lisp/org-id.el
+++ b/lisp/org-id.el
@@ -295,6 +295,18 @@ This variable is only relevant when `org-id-track-globally' is set."
   :group 'org-id
   :type 'boolean)
 
+(defvar org-id-find-functions nil
+  "Hook for functions finding an ID location.
+These functions must take two arguments, ID and MARKERP.
+
+`org-id-find' will first try to call these functions, and the
+first non-nil return value will be returned from `org-id-find'.
+See `org-id-find' for the expected return type and meaning of ID
+and MARKERP.  ID will be passed as a string.
+
+If all functions return nil, `org-id-find' will proceed to look
+for the ID using the built-in id location cache.")
+
 ;;; The API functions
 
 ;;;###autoload
@@ -396,16 +408,18 @@ With optional argument MARKERP, return the position as a new marker."
   (cond
    ((symbolp id) (setq id (symbol-name id)))
    ((numberp id) (setq id (number-to-string id))))
-  (let ((file (org-id-find-id-file id))
-	org-agenda-new-buffers where)
-    (when file
-      (setq where (org-id-find-id-in-file id file markerp)))
-    (unless where
-      (org-id-update-id-locations nil t)
-      (setq file (org-id-find-id-file id))
-      (when file
-	(setq where (org-id-find-id-in-file id file markerp))))
-    where))
+  (or
+   (run-hook-with-args-until-success 'org-id-find-functions id markerp)
+   (let ((file (org-id-find-id-file id))
+         org-agenda-new-buffers where)
+     (when file
+       (setq where (org-id-find-id-in-file id file markerp)))
+     (unless where
+       (org-id-update-id-locations nil t)
+       (setq file (org-id-find-id-file id))
+       (when file
+         (setq where (org-id-find-id-in-file id file markerp))))
+     where)))
 
 ;;; Internal functions
 
diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 3150b4e2f..04ac3395d 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -503,6 +503,14 @@ See https://github.com/yantar92/org/issues/4."
             (test-ol-stored-link-with-text "* H1\n:PROPERTIES:\n:ID: abc\n:END:\n* H2\n** <point>Target\n"
               (org-id-store-link-maybe t))))))
 
+(ert-deftest test-org-link/org-id-find ()
+  "Test `org-id-find' specifications."
+  (should
+   (equal '("id.org" . 12)
+          (org-test-with-temp-text ""
+            (add-hook 'org-id-find-functions (lambda (id _markerp) (cons (concat id ".org") 12)) nil t)
+            (org-id-find "id")))))
+
 \f
 ;;; Radio Targets
 
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Ihor Radchenko]

"Rick Lupton" <mail@ricklupton.name> writes:

> Since updating the org-id code [1] to use the standard org-link registered functions instead of hard-coding org-id opening, I have a problem using org-roam.
>
> org-roam overwrites the :follow function for "id" links from the build-in `org-id-open' to its own `org-roam-id-open' (https://github.com/org-roam/org-roam/blob/8667e441876cd2583fbf7282a65796ea149f0e5f/org-roam-id.el#L91).  The only change between these functions is to insert a call to try `org-roam-id-find' before trying `org-id-find', which uses org-roam's cached sqlite database to find the id (https://github.com/org-roam/org-roam/blob/8667e441876cd2583fbf7282a65796ea149f0e5f/org-roam-id.el#L70-L71)
>
> As well as being messy, my specific problem is that the improvements to open search strings in org-id links are no longer enabled when org-roam is loaded.
>
> It seems reasonable that a library might want to provide its own way of locating org-ids.  The attached patch adds a new hook variable `org-id-find-functions` which contains functions doing the same job as `org-id-find' that should be tried first.  Then all org-roam needs to do is add its `org-roam-id-find' to the hook.
>
> Does this seem like a good idea?

I think that we can do it simpler.

Something like

(defun yant/message-me (&rest _) (message "I am here!") nil) ; return nil to pass the turn
(gv-define-setter org-link-get-parameter (value type key)
  `(org-link-set-parameters ,type ,key ,value))
(add-function :before-until (org-link-get-parameter "id" :follow) #'yant/message-me)

The idea is to use Emacs' advice machinery to allow third-party code
alter the functions stored in link parameters.

Either way, org-roam needs to be adapted in order to support the changes
in org-id. My variant appears to be simpler. We just need to add
`gv-define-setter ...' and maybe also document the `add-function'
approach in the manual.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Rick Lupton]

On Wed, 13 Mar 2024, at 12:30 PM, Ihor Radchenko wrote:
> I think that we can do it simpler. [...]
> The idea is to use Emacs' advice machinery to allow third-party code
> alter the functions stored in link parameters.

I was avoiding this because I thought it was only recommended (in the elisp manual) to use advice directly by users, not in libraries (like, I assume, org-roam):

> If you are writing code for release, for others to use, try to avoid including advice in it. If the function you want to advise has no hook to do the job, please talk with the Emacs developers about adding a suitable hook. Especially, Emacs’s own source files should not put advice on functions in Emacs. (There are currently a few exceptions to this convention, but we aim to correct them.) It is generally cleaner to create a new hook in foo, and make bar use the hook, than to have bar put advice in foo.

(https://www.gnu.org/software/emacs/manual/html_node/elisp/Advising-Named-Functions.html)

But I don't mind either way.  I agree your approach is simpler if it's a reasonable way for a third party library like org-roam to extend the org id functions.

Thanks,
Rick

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Ihor Radchenko]

"Rick Lupton" <mail@ricklupton.name> writes:

> On Wed, 13 Mar 2024, at 12:30 PM, Ihor Radchenko wrote:
>> I think that we can do it simpler. [...]
>> The idea is to use Emacs' advice machinery to allow third-party code
>> alter the functions stored in link parameters.
>
> I was avoiding this because I thought it was only recommended (in the elisp manual) to use advice directly by users, not in libraries (like, I assume, org-roam):
>
>> If you are writing code for release, for others to use, try to avoid including advice in it. If the function you want to advise has no hook to do the job, please talk with the Emacs developers about adding a suitable hook. Especially, Emacs’s own source files should not put advice on functions in Emacs. (There are currently a few exceptions to this convention, but we aim to correct them.) It is generally cleaner to create a new hook in foo, and make bar use the hook, than to have bar put advice in foo.
>
> (https://www.gnu.org/software/emacs/manual/html_node/elisp/Advising-Named-Functions.html)

This is absolutely right, but only when advising named functions. What
we are talking about is advising the link property value. In my previous
discussions with Emacs devs, I was told that it superior to use advice
system when a library wants to modify a function stored as a value of a
variable - see
https://yhetil.org/emacs-devel/SJ0PR10MB548885B715C9875726F70F61F34FA@SJ0PR10MB5488.namprd10.prod.outlook.com/

> But I don't mind either way.  I agree your approach is simpler if it's a reasonable way for a third party library like org-roam to extend the org id functions.

I added the setter on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=d545ad606

We may also want to document the `add-function' approach in the manual.
Maybe in a new section "Modifying Hyperlink Types", right after "Adding
Hyperlink Types".

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Rick Lupton]

Thanks, I hadn't understood the significance of named functions vs variables for advice.  But now I realise this is not solving the same problem as the original patch.

The point is that I was thinking org-roam should advise/modify/hook the specific function `org-id-find' [to find ids in its database] but NOT the more general `(org-link-get-parameter "id" :follow)' [because `org-id-open' has a lot of extra logic for search-strings, backwards compatibility, where to open the new location, which it would not be reasonable to duplicate].

Is there a way to do this using the approach you are suggesting?

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Ihor Radchenko]

"Rick Lupton" <mail@ricklupton.name> writes:

> The point is that I was thinking org-roam should advise/modify/hook the specific function `org-id-find' [to find ids in its database] but NOT the more general `(org-link-get-parameter "id" :follow)' [because `org-id-open' has a lot of extra logic for search-strings, backwards compatibility, where to open the new location, which it would not be reasonable to duplicate].
>
> Is there a way to do this using the approach you are suggesting?

No. Then, your patch makes sense.

> +(ert-deftest test-org-link/org-id-find ()
> +  "Test `org-id-find' specifications."
> +  (should
> +   (equal '("id.org" . 12)
> +          (org-test-with-temp-text ""
> +            (add-hook 'org-id-find-functions (lambda (id _markerp) (cons (concat id ".org") 12)) nil t)
> +            (org-id-find "id")))))

... although this test does not look right. You are modifying the hook
by side effect, retaining the value for all other tests.

Please remove the hook after calling ~org-id-find~ via ~unwind-protect~.

Also, before we merge your patch, may I know if you discussed this
change with org-roam developers? If they do not want to use the proposed
hook, there is no reason to add it to Org mode.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> Also, before we merge your patch, may I know if you discussed this
> change with org-roam developers? If they do not want to use the proposed
> hook, there is no reason to add it to Org mode.

It has been over a month since the last message in this thread.
May I know if there is any progress?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Rick Lupton]

I have been using the simpler version using advice (without my patch).

On reflection this seems ok to me -- I don't think there's really any need for my proposed patch, unless it turns out in future that there are multiple packages like org-roam that need to supply org-id-finding abilities in a well-defined order.  So I suggest cancelling this.

I just proposed this as a change to org-roam:

https://github.com/org-roam/org-roam/pull/2432

Best
Rick

On Sat, 20 Apr 2024, at 12:07 PM, Ihor Radchenko wrote:
> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Also, before we merge your patch, may I know if you discussed this
>> change with org-roam developers? If they do not want to use the proposed
>> hook, there is no reason to add it to Org mode.
>
> It has been over a month since the last message in this thread.
> May I know if there is any progress?
>
> -- 
> Ihor Radchenko // yantar92,
> Org mode contributor,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Allow external libraries (org-roam) to supply org-id locations

[Ihor Radchenko]

"Rick Lupton" <mail@ricklupton.name> writes:

> I have been using the simpler version using advice (without my patch).
>
> On reflection this seems ok to me -- I don't think there's really any need for my proposed patch, unless it turns out in future that there are multiple packages like org-roam that need to supply org-id-finding abilities in a well-defined order.  So I suggest cancelling this.

Sure.
Canceled.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>