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

[Ihor Radchenko]

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

>>> +(defcustom org-id-link-use-context t
>> ...
>> 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.
>
> ?

I think that (b) makes sense. There is no reason to make the
customization yet more granular and complex when there is no clear need.
We can always do it later, if necessary, anyway.

>>> -  (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).

Agree.

>>> +      ;; 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'? 

No, it is generally not safe. For a different reason.

Let me illustrate with an example:

(defun yant/test2 ()
  (message "current-prefix-arg: %S" current-prefix-arg))

(defun yant/test (arg)
  (interactive "P")
  (yant/test2))

When you call M-x yant/test, you will see "current-prefix-arg: nil".
However, when you call C-u M-x yant/test, you will see
"current-prefix-arg: (4)".

Similar logic applies to the non-interactive calls to `org-store-link'.
If some Elisp code implements a command like

(defun yant/my-command (arg)
  (interactive "P")
  <do staff>
  (org-store-link nil))

then, `org-store-link' may call `org-id-store-link-maybe' and
`org-id-store-link-maybe' will still "see" the top-level prefix argument
passed to `yant/my-command' - the prefix argument that has nothing at
all to do with prefix arguments of `org-store-link'.

Conclusion: It is unsafe to use `current-prefix-arg' value. We need to
pass this information some other way.

The way I proposed is actually not any special for ID links. What I
meant it to let-bind `org-link-context-for-files' around the whole call
to `org-store-link-functions', so that the custom :store functions will
get access to the adjusted value of `org-link-context-for-files'.
Does this explanation make more sense?

>>> +        (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?

I just did not notice ID-LOCATION :facepalm:

>>> +    (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?

Because a lot of the code (except some part `org-link-open') assumes
that `org-link-search' searches the whole buffer, not just the narrowed
part. But that's not your problem - I will update the docstring of
`org-link-search' to explicitly specify that it is searching within the
accessible portion of the buffer and update the callers to account for
this.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=89164e605
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=5c543cd9d
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=cb71bde7c
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=63ef7b924

But does your code do narrowing? I did not notice it.

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