From: "Rick Lupton" <mail@ricklupton.name>
To: "Ihor Radchenko" <yantar92@posteo.net>
Cc: "Y. E." <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] org-id: allow using parent's existing id in links to headlines
Date: Thu, 14 Dec 2023 20:42:02 +0000 [thread overview]
Message-ID: <2cdfefbf-e9e3-4aeb-a410-1ff3a9d6168e@app.fastmail.com> (raw)
In-Reply-To: <87jzpmqiy0.fsf@localhost>
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>
next prev parent reply other threads:[~2023-12-14 20:43 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 [this message]
2023-12-15 12:55 ` Ihor Radchenko
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=2cdfefbf-e9e3-4aeb-a410-1ff3a9d6168e@app.fastmail.com \
--to=mail@ricklupton.name \
--cc=emacs-orgmode@gnu.org \
--cc=yantar92@posteo.net \
/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).