From: Ihor Radchenko <yantar92@posteo.net>
To: Rick Lupton <mail@ricklupton.name>
Cc: "Y. E." <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] org-id: allow using parent's existing id in links to headlines
Date: Fri, 15 Dec 2023 12:55:14 +0000 [thread overview]
Message-ID: <87zfybzkul.fsf@localhost> (raw)
In-Reply-To: <2cdfefbf-e9e3-4aeb-a410-1ff3a9d6168e@app.fastmail.com>
"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>
next prev parent reply other threads:[~2023-12-15 12:52 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-24 11:40 [PATCH] org-id: allow using parent's existing id in links to headlines Rick Lupton
2023-07-25 7:43 ` Ihor Radchenko
2023-07-25 15:16 ` Max Nikulin
2023-07-26 8:10 ` Ihor Radchenko
2023-07-27 0:16 ` Samuel Wales
2023-07-27 7:42 ` IDs below headline level (for paragraphs, lists, etc) (was: [PATCH] org-id: allow using parent's existing id in links to headlines) Ihor Radchenko
2023-07-28 20:00 ` Rick Lupton
2023-07-28 19:56 ` [PATCH] org-id: allow using parent's existing id in links to headlines Rick Lupton
2023-07-29 8:33 ` Ihor Radchenko
2023-11-09 20:56 ` Rick Lupton
2023-11-10 10:03 ` Ihor Radchenko
2023-11-19 15:21 ` Rick Lupton
2023-12-04 13:23 ` Rick Lupton
2023-12-10 13:35 ` Ihor Radchenko
2023-12-14 20:42 ` Rick Lupton
2023-12-15 12:55 ` Ihor Radchenko [this message]
2023-12-15 16:16 ` Rick Lupton
2023-12-16 14:20 ` Ihor Radchenko
2023-12-17 19:07 ` [PATCH v2] " Rick Lupton
2023-12-18 12:27 ` Ihor Radchenko
2024-01-02 16:13 ` Rick Lupton
2024-01-03 14:17 ` Ihor Radchenko
2024-01-28 22:47 ` Rick Lupton
2024-01-29 0:20 ` Samuel Wales
2024-01-29 13:06 ` Ihor Radchenko
2024-01-30 0:03 ` Samuel Wales
2024-02-03 15:08 ` Ihor Radchenko
2024-01-29 13:00 ` Ihor Radchenko
2024-01-31 18:11 ` Rick Lupton
2024-02-01 12:13 ` Ihor Radchenko
2024-02-01 16:37 ` Rick Lupton
2024-02-03 13:10 ` Ihor Radchenko
2024-02-08 8:24 ` [PATCH] lisp/ol.el: Improve docstring Rick Lupton
2024-02-08 14:52 ` Ihor Radchenko
2024-02-08 8:46 ` [PATCH v2] org-id: allow using parent's existing id in links to headlines Rick Lupton
2024-02-08 13:02 ` Ihor Radchenko
2024-02-08 22:30 ` Rick Lupton
2024-02-09 12:09 ` Ihor Radchenko
2024-02-09 12:47 ` Rick Lupton
2024-02-09 12:57 ` Ihor Radchenko
2024-02-24 10:48 ` Bastien Guerry
2024-02-24 13:02 ` Ihor Radchenko
2024-02-24 15:57 ` Rick Lupton
2024-03-05 14:05 ` Stefan
2024-03-05 14:51 ` Ihor Radchenko
2023-11-04 23:01 ` [PATCH] " Rick Lupton
2023-11-05 12:31 ` Ihor Radchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zfybzkul.fsf@localhost \
--to=yantar92@posteo.net \
--cc=emacs-orgmode@gnu.org \
--cc=mail@ricklupton.name \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).