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

[Ihor Radchenko]

"Rick Lupton"  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 .
Support Org development at ,
or support my work at 

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  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 .
> Support Org development at ,
> or support my work at 

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

[Ihor Radchenko]

Ihor Radchenko  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 .
Support Org development at ,
or support my work at 

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

[Ihor Radchenko]

"Rick Lupton"  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 .
Support Org development at ,
or support my work at 

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"  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/sj0pr10mb548885b715c9875726f70f61f3...@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 .
Support Org development at ,
or support my work at 

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"  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 ( _) (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 .
Support Org development at ,
or support my work at 

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

[Rick Lupton]

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-88de8e19f...@app.fastmail.com/


0001-lisp-org-id.el-add-hook-org-id-find-functions.patch
Description: Binary data