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

[Rick Lupton]

On Mon, 29 Jan 2024, at 1:00 PM, Ihor Radchenko wrote:
>>> 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?
>
> No. Do note that my instructions start from _empty_ file. With
> org-id-link-to-org-use-id, PROPERTIES drawer is not created. This is
> different from what happens with your patch - it is unexpected in your
> patch that the search string is added for text that did not exist in the
> buffer previously.

I see.  Updated to get the search string first, before the possible properties 
draw appears.

To make this work I changed `org-link-precise-link-target': instead of 
accepting the RELATIVE-TO argument and rejecting unsuitable targets internally, 
it now sets a marker `org-link-precise-target-marker' showing where the target 
that was found is, so the caller can decide if the found target is suitable.  I 
copied the approach from `org-entry-property-inherited-from', hope that doesn't 
cause any other issues.

> That's a good catch.
> The fact that links stored via `org-store-link' cannot be open with
> default settings is not good.
> Also, your patch disregards this setting - it should not match
> non-headline search strings with the default value of
> `org-link-search-must-match-exact-headline'.

`org-link-search-must-match-exact-headline' affects `org-link-search', which is 
called by `org-id-open' -- so I think the behaviour for these org-id links 
should be the same as for other file links? Am I missing something?

Or, maybe you mean links that rely on 
`org-link-search-must-match-exact-headline' should not be stored.  That would 
seem reasonable, but also doesn't need to be part of these changes here?

> Probably, changing the default value of
> `org-link-search-must-match-exact-headline' to nil is due.

It seems like the behaviour below would be desirable, but doesn't currently 
exist with any setting of `org-link-search-must-match-exact-headline'?

(org-link-search "plain text")  -->  fuzzy search for all text
(org-link-search "*heading")    -->  search only headings, optionally creating 
if missing

>> Subject: [PATCH 2/2] org-id.el: Extend links with search strings, inherit
>>  parent IDs
>
> I ran make test, and it looks like one test is failing with your patch:

Oops, fixed now I think.

> `org-context-in-file-links' is an obsolete name. Use
> `org-link-context-for-files'.
>
> Also, please add `org-id-link-use-context' to #+vindex.

Updated

> Please update the docstring of `org-store-link-functions' to specify
> that an argument is passed to :store functions.

Updated

>> -      (org-insert-heading nil t t)
>> +      ;; Find appropriate level for new heading
>> +      (let ((level (save-excursion
>> +                     (goto-char (point-min))
>> +                     (+ 1 (or (org-current-level) 0)))))
>
> This is fragile. You assume that `point-min' always contains a heading.
> That may or may not be the case - `org-link-search' may be called by
> third-party code that does not care about setting narrowing in certain
> ways.

I don't think it's a problem. (org-current-level) returns something suitable 
whether or not point-min contains a heading. Both the situations below seem 
reasonable choices for the level of the newly created heading at the end:

---start of narrowing---
Text
* H1
** H2
* A new level 1 heading is created at the end
---end of narrowing---

---start of narrowing---
* H1
** H2
** A new level 2 heading is created at the end
---end of narrowing---

(this is how it currently works, unless I'm missing something)

>> +(defun org-link-precise-link-target (&optional relative-to)
>> +  "Determine search string and description for storing a link.
>> +
>> +If a search string (see 'org-link-search') is found, return cons
>
> Quoting: `org-link-search'.

Fixed

>> +           (let* ((element (org-element-at-point))
>> +                  (name (org-element-property :name element))
>> +                  (heading (org-element-lineage element 'headline t))
>
> What about inlinetasks?

I added inlinetasks to the element types, so they are picked up the same as 
headlines now.

>> +                  (custom-id (org-entry-get nil "CUSTOM_ID")))
>
> May as well pass HEADING as the first argument of `org-entry-get'. It
> will be slightly more efficient.

Ok

>> +        (org-link--add-to-stored-links link desc)
>> +        ;; In org buffers, store an additional "human-readable" link
>> +        ;; using custom id, if available.
>> +        (when (and (buffer-file-name (buffer-base-buffer))
>> +                   (derived-mode-p 'org-mode)
>> +                   (org-entry-get nil "CUSTOM_ID"))
>> +          (setq link (concat "file:"
>> +                             (abbreviate-file-name
>> +                              (buffer-file-name (buffer-base-buffer)))
>> +                             "::#" (org-entry-get nil "CUSTOM_ID")))
>
> This is fragile - you are relying upon the exact code used to store
> file:...#CUSTOM-ID link. Instead, please refactor the function to re-use
> that code.

Ok

>> +           (id-location (or (and org-entry-property-inherited-from
>> +                                 (marker-position 
>> org-entry-property-inherited-from))
>> +                            (save-excursion 
>> (org-back-to-heading-or-point-min) (point))))
>>         (case-fold-search nil)
>>         (desc (save-excursion
>> -               (org-back-to-heading-or-point-min t)
>> +                   (goto-char id-location)
>
> You are calling `org-back-to-heading-or-point-min' without optional
> argument INVISIBLE-OK. This looks like an oversight.

Fixed

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