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

[Rick Lupton]

Dear Ihor,

Thanks for taking a look at the patch and for your feedback.

Re various docstrings, documenting new optional argument -- I will try to 
address these.

Other comments/questions below.

On Sun, 10 Dec 2023, at 1:35 PM, Ihor Radchenko wrote:
> "This change..." part sounds like a commit message, not a NEWS item.

I think there are lots of other examples that are written like this in ORG-NEWS 
(but I agree my sentence was unnecessary and have removed it).

>> +(defcustom org-id-link-use-context t
>> +  "Non-nil means org-id links from `org-id-store-link' contain context.
>> +\\<org-mode-map>
>> +A search string is added to the id with \"::\" as separator and
>> +used to find the context when the link is activated by the
>> +command `org-open-at-point'.  When this option is t, the entire
>> +active region is be placed in the search string of the file link.
>> +If set to a positive integer N, only the first N lines of context
>> +are stored.
>
> It does not look like integer value is respected in the patch.

You're right. Do you have a preference between

(a) sticking to this docstring, which creates the possibility of using 
different numbers of lines for id: and file: links' context, and makes the code 
slightly more complicated, but keeps the meaning of 
`org-link-context-for-files' specifically `for files'; or

(b) Always use `org-link-context-for-files' to set the number of lines of 
context used for all links; `org-id-link-use-context' is just a boolean. The 
code is simpler.

?

>> -  (let ((id (org-entry-get epom "ID")))
>> +  (let ((id (org-entry-get epom "ID" inherit)))
>
> This makes your description of INHERIT argument slightly inaccurate - for
> `org-entry-get', INHERIT can also be a special symbol 'selective.

Good point; I think the answer is to force INHERIT to t or nil, rather than 
documenting and continuing to accept 'selective (when INHERIT is used, it 
should definitely take effect).

>> -(defun org-id-store-link ()
>> +(defun org-id-store-link (interactive?)
>
> Please make this new argument optional and document the argument in the
> docstring and NEWS. Non-optional new argument is a breaking change that
> may break third-party code.

Oops, yes -- but in fact this argument is only needed on 
`org-id-store-link-maybe' (as below), so I can remove it again here.

>>    "Store a link to the current entry, using its ID.
>>  
>> +See also `org-id-link-to-org-use-id`,
>> +`org-id-link-use-context`,
>> +`org-id-link-consider-parent-id`.
>> +
>>  If before first heading store first title-keyword as description
>>  or filename if no title."
>> -  (interactive)
>> -  (when (and (buffer-file-name (buffer-base-buffer)) (derived-mode-p 
>> 'org-mode))
>> -    (let* ((link (concat "id:" (org-id-get-create)))
>> +  (interactive "p")
>> +  (when (and (buffer-file-name (buffer-base-buffer))
>> +             (derived-mode-p 'org-mode)
>> +             (or (eq org-id-link-to-org-use-id t)
>
> I do not like this change - `org-id-store-link' is not only used by
> `org-store-link'. Suddenly honoring `org-id-link-to-org-use-id' will be
> a breaking change. Instead, I suggest you to write a wrapper function,
> like `org-id-store-link-maybe' and use it as :store id link property.
> That function will call `org-id-store-link' as necessary according to
> user customization.

Ok, yes that makes sense.

>> +      ;; Prefix to `org-store-link` negates preference from 
>> `org-id-link-use-context`.
>> +      (when (org-xor current-prefix-arg org-id-link-use-context)
>
> This is not reliable. `org-id-store-link' may be called from a completely
> different command, not `org-store-link'. Then, the effect of prefix
> argument will be unexpected. You should instead process prefix argument
> right in `org-store-link' by let-binding `org-id-link-use-context'
> around the call to `org-id-store-link'.

Now that `org-id-store-link' is called via :store link property, 
`org-store-link` does not have special logic for org-id, which I thought was an 
improvement, so it would be a step backwards to add in special logic for 
`org-id-link-use-context'?

Instead, I think this logic could be in `org-id-store-link-maybe' as above. 
That is, it is safe to take account of `current-prefix-arg' within a link 
:store function, and assume it represents prefix args as used with 
`org-store-link'? 

>
>> +        (pcase (org-link-precise-link-target id-location)
>
> Why not passing the RELATIVE-TO argument?

The `id-location' is the RELATIVE-TO argument. Or do I misunderstand you?

>>  (defun org-id-open (id _)
>>    "Go to the entry with id ID."
>> -  (org-mark-ring-push)
>> -  (let ((m (org-id-find id 'marker))
>> -    cmd)
>> +  (let* ((option (and (string-match "::\\(.*\\)\\'" id)
>> +                  (match-string 1 id)))
>> +     (id (if (not option) id
>> +               (substring id 0 (match-beginning 0))))
>> +         m cmd)
>> +    (org-mark-ring-push)
>> +    (setq m (org-id-find id 'marker))
>
> This means that the existing IDs that happen to contain :: will be
> broken. For such IDs, we should (1) document the problem in the news;
> (2) try harder to match them calling `org-id-find' with all the possible
> ID values until one matches.

Good point, ok, I'll try this.

>> +    (when option
>> +      (org-link-search option))
>>      (org-fold-show-context)))
>
> `org-link-search' does not always search from point. So, you may end up
> matching, for example, a duplicate CUSTOM_ID above.
> Moreover, regular expression match option will be broken -
> `org-link-search' creates sparse tree in the whole buffer and will
> disregard the ID part of the link. I suspect that you will need to make
> dedicated modifications to `org-link-search' as well in order to
> implement opening ID links with search option cleanly.

Thanks, yes. It looks to me (from the code and some testing) that narrowing to 
the target heading first before calling `org-link-search' does the right thing. 
Was there a particular reason you thought `org-link-search' would need to be 
changed?

Thanks,
Rick

>
> -- 
> 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>