Re: [PATCH v2] org-id: allow using parent's existing id in links to headlines

[Rick Lupton]

Hi,

Thanks for trying it out.  Updated patches attached, comments below.

On Mon, 18 Dec 2023, at 12:27 PM, Ihor Radchenko wrote:
> I played around with the patch a bit and found a couple of rough edges:
>
> 1. When I try to open a link to non-existing search target, like
>    <id:some-id::non-existing-target>, I get a query to create a new
>    heading. If I reply "yes", a new heading is created. However, the
>    heading is created at the end of the file and is always level 1,
>    regardless of the "some-id" parent context.
>    It would make more sense to create a new heading at the end of the
>    id:some-id subtree.

Fixed in updated patches -- first patch adds generic new flexibility to 
`org-insert-heading', second patch uses it so new headings now added at correct 
level at the end of the id:sub-id subtree.

> 2. Consider the following setting:
>    (setq org-id-link-consider-parent-id t)
>    (setq org-id-link-to-org-use-id 'create-if-interactive-and-no-custom-id)
>
>    Then, create the following Org file
>
> * Sub
> * Parent here
> ** This is test
> :PROPERTIES:
> :ID:       fe40252e-0527-44c1-a990-12498991f167
> :END:
>
> *** Sub <point here>
> :PROPERTIES:
> :CUSTOM_ID:       subid
> :END:
>
>    When you M-x org-store-link, the stored link has ::*Sub instead of
>    the expected ::#subid

Updated so that search strings prefer custom-ids (::#subid) to headline matches 
(::*Sub).  This makes this example behave as you expect.

The correct behaviour of org-store-link doesn't seem totally obvious to me 
about id vs custom-id links.  Currently org-store-link has special logic to 
store TWO links (one <file:xx::#subid>, one <file:xx::*Sub>) when a CUSTOM_ID 
is present. In the manual, it says:

     If the headline has a ‘CUSTOM_ID’ property, store a link to this
     custom ID.  In addition or alternatively, depending on the value of
     ‘org-id-link-to-org-use-id’, create and/or use a globally unique
     ‘ID’ property for the link(1).  So using this command in Org
     buffers potentially creates two links: a human-readable link from
     the custom ID, and one that is globally unique and works even if
     the entry is moved from file to file.  The ‘ID’ property can be
     either a UUID (default) or a timestamp, depending on
     ‘org-id-method’.  Later, when inserting the link, you need to
     decide which one to use.

That refers to ID links specifically, but now, using the generic link store 
functions, there is only the possibility to store one link type, so it's not 
possible to neatly keep exactly the same behaviour (i.e. for ID links but not 
for other external link types).

I think the intention of what's described in the manual is to distinguish 
"human-readable" vs "persistent id" links.  There could be other types of 
"persistent id" links apart from org-id links, such as mu4e: links to email 
message-ids.  Therefore I've updated org-store-link to simply store a 
<file:xx.org::#custom-id> link as an additional option, whether or not the 
first matched link was an org-id link (this is the current behaviour) or 
another external link type (this is changed behaviour).

Added a note to ORG-NEWS about this.

> 3. Consider
>    (setq org-id-link-consider-parent-id t)
>    (setq org-id-link-to-org-use-id t)
>
>    Then, create a new empty Org file
>    M-x org-store-link with create a top-level properties drawer with ID
>    and store the link. However, that link will not be a simple ID link,
>    but also have ::PROPERTIES search string, which is not expected.

This is because it is trying to link to the current line of the file, which 
contains the text "PROPERTIES".  On main, with (setq org-id-link-to-org-use-id 
nil), you see the equivalent behaviour (a link to 
[[file:test.org:::PROPERTIES:]]) when point is before the first heading.  So, 
this seems consistent with non-org-id links?

(these links don't actually work with the default value of 
`org-link-search-must-match-exact-headline', but I think that's a separate 
issue).

>> +  #+vindex: org-id-link-consider-parent-id
>> +  When ~org-id-link-consider-parent-id~ is ~t~, parent =ID= properties
>> +  are considered.  This allows linking to specific targets, named
>> +  blocks, or headlines (which may not have a globally unique =ID=
>> +  themselves) within the context of a parent headline or file which
>> +  does.
>
> It would be nice to add an example, similar to what you did in the docstring.

Added.

>
>> -(defun org-man-store-link ()
>> +(defun org-man-store-link (&optional _interactive?)
>>    "Store a link to a man page."
>>    (when (memq major-mode '(Man-mode woman-mode))
>>      ;; This is a man page, we do make this link.
>> @@ -21312,13 +21324,15 @@ A review of =ol-man.el=:
>
> Please, update the actual built-in :store functions in lisp/ol-*.el to
> handle the new optional argument as well.

Updated.

>> +**** =org-link= store functions are passed an ~interactive?~ argument
>> +
>> +The ~:store:~ functions set for link types using
>> +~org-link-set-parameters~ are now passed an ~interactive?~ argument,
>> +indicating whether ~org-store-link~ was called interactively.
>
> Please also explain that the existing functions are not broken.

Done.

>> +*** ~org-id-store-link~ now adds search strings for precise link targets
>> +
>> +This new behaviour can be disabled generally by setting
>> +~org-id-link-use-context~ to ~nil~, or when storing a specific link by
>> +passing a prefix argument to ~org-store-link~.
>
> universal argument.
> There are several possible prefix arguments in `org-store-link', but
> only C-u (universal argument) will give the described effect.
> Also, won't the behavior be _toggled_ by the universal argument?

Updated.

>> +When using this feature, IDs should not include =::=, which is used in
>> +links to indicate the start of the search string.  For backwards
>> +compability, existing IDs including =::= will still be matched (but
>> +cannot be used together with precise link targets).
>
> Please add an org-lint checker that warns about such IDs and mention
> this checker in the above.

Added.

> Also, this paragraph belongs to "Breaking changes", not "new and changed
> options".

That's where it is, I think.

>> +*** New option ~org-id-link-consider-parent-id~ to allow =id:= links to 
>> parent headlines
>> +
>> +For =id:= links, when this option is enabled, ~org-store-link~ will
>> +look for ids from parent/ancestor headlines, if the current headline
>> +does not have an id.
>> +
>> +Combined with the new ability for =id:= links to use search strings
>> +for precise link targets (when =org-id-link-use-context= is =t=, which
>> +is the default), this allows linking to specific headlines without
>> +requiring every headline to have an id property, as long as the
>> +headline is unique within a subtree that does have an id property.
>> +
>> +By giving files top-level id properties, links to headlines in the
>> +file can be made more robust by using the file id instead of the file
>> +path.
>
> Please, provide an example here as well.

Done.

>> +(defun org-link--try-link-store-functions (interactive?)
>> +  "Try storing external links, prompting if more than one is possible.
>> +
>> +Each function returned by `org-store-link-functions' is called in
>> +turn.  If multiple functions return non-nil, prompt for which
>> +link should be stored.
>> +
>> +Return t when a link has been stored in `org-link-store-props'."
>
> Please document INTERACTIVE? argument in the docstring.

Done.

>> +  (let ((results-alist nil))
>> +    (dolist (f (org-store-link-functions))
>> +      (when (condition-case nil
>> +                (funcall f interactive?)
>> +              ;; XXX: The store function used (< Org 9.7) to accept no
>> +              ;; arguments; provide backward compatibility support for
>> +              ;; them.
>
> Use FIXME, not XXX. (I have no idea why it is XXX in the existing code).

Changed.

>> +(defun org-link-precise-link-target (&optional relative-to)
>> +  "Determine search string and description for storing a link.
>> +
>> +If a search string is found, return cons cell (SEARCH-STRING
>> +. DESC).  Otherwise, return nil.
>> +
>> +If there is an active region, the contents is used (see
>> +`org-link--context-from-region').
>
> It is not clear from this sentence whether the contents is used for
> SEARCH-STRING of DESC.
>
>> +In org-mode buffers, if point is at a named element (e.g. a
>> +source block), the name is used. If within a heading, the current
>> +heading is used.
>
> Please use double space between sentences.
>
>> +Optional argument RELATIVE-TO specifies the buffer position where
>> +the search will start from.  If the search target that would be
>> +returned is already at this location, return nil to avoid
>> +unnecessary search strings (for example, when using search
>> +strings to find targets within org-id links)."
>
> It is not clear what will happen if RELATIVE-TO is before/after point.

Updated the docstring.

>> -    (let (link cpltxt desc search custom-id agenda-link) ;; description
>> +    (let ((org-link-context-for-files (org-xor org-link-context-for-files
>> +                                               (equal arg '(4))))
>> +          link cpltxt desc search custom-id agenda-link) ;; description
>>        (cond
>>         ;; Store a link using an external link type, if any function is
>> -       ;; available. If more than one can generate a link from current
>> -       ;; location, ask which one to use.
>> +       ;; available.  If more than one can generate a link from
>> +       ;; current location, ask which one to use.  Negate
>> +       ;; `org-context-in-file-links' when given a single prefix arg.
>
> The part of the comment about negation, should probably be moved near
> the let binding of `org-link-context-for-files'.

Done.

>> +For example, given this org file:
>> +
>> +* Parent
>> +:PROPERTIES:
>> +:ID: abc
>> +:END:
>> +** Child 1
>> +** Child 2
>> +
>> +With `org-id-link-consider-parent-id' set to t, storing a link
>> +with point at \"Child 1\" will produce a link \"id:abc\" to
>> +\"Parent\".
>
> This is actually confusing. May we only consider parent when
> `org-id-link-use-context' is enabled?

Yes, I was trying to keep them independent but I agree it's probably more 
useful to only consider parent when `org-id-link-use-context' is enabled (which 
in turn depends on `org-context-in-file-links' being enabled).

>> -(defun org-id-get (&optional epom create prefix)
>> +(defun org-id-get (&optional epom create prefix inherit)
>>    "Get the ID property of the entry at EPOM.
>>  EPOM is an element, marker, or buffer position.
>>  If EPOM is nil, refer to the entry at point.
>>  If the entry does not have an ID, the function returns nil.
>> +If INHERIT is non-nil, parents' IDs are also considered.
>>  However, when CREATE is non-nil, create an ID if none is present already.
>>  PREFIX will be passed through to `org-id-new'.
>>  In any case, the ID of the entry is returned."
>
> What about both CREATE and INHERIT being non-nil?

Rewrote the docstring.

Also removed INHERIT argument for `org-id-get-create' again, as other functions 
can be re-written to use `org-id-get' directly, and INHERIT isn't particularly 
useful when using `org-id-get-create' interactively.

>> +;;;###autoload
>> +(defun org-id-store-link-maybe (&optional interactive?)
>> +  "Store a link to the current entry using its ID if enabled.
>> +
>> +The value of `org-id-link-to-org-use-id' determines whether an ID
>> +link should be stored, using `org-id-store-link'.
>> +
>> +Assume the function is called interactively if INTERACTIVE? is
>> +non-nil."
>> +  (interactive "p")
>
> Do we really need to make it interactive?

No, removed.

Thanks,
Rick

0001-lisp-org.el-org-insert-heading-allow-specifying-head.patch
Description: Binary data

0002-org-id.el-Extend-links-with-search-strings-inherit-p.patch
Description: Binary data