From: Ihor Radchenko <yantar92@posteo.net>
To: Rick Lupton <mail@ricklupton.name>
Cc: emacs-orgmode list <emacs-orgmode@gnu.org>
Subject: Re: [PATCH] org-id: allow using parent's existing id in links to headlines
Date: Sun, 10 Dec 2023 13:35:19 +0000 [thread overview]
Message-ID: <87jzpmqiy0.fsf@localhost> (raw)
In-Reply-To: <e66407c2-cd4f-4cb8-b933-4fd036b8d94a@app.fastmail.com>
"Rick Lupton" <mail@ricklupton.name> writes:
> Here's an updated patch, which adds (optional) search strings to ID links, and the option to inherit ID targets from parent headline / the top level file properties. I've also updated ORG-NEWS and the manual, and added tests.
>
> I think I've fixed all the issues with my first patch about which headline gets used for the description when inheriting IDs, what happens if there is no ID, etc.
Thanks!
> +*** ~org-id-store-link~ now adds search strings for precise link targets
> +
> +Previously, search strings are supported for =file:= links but not for
> +=id:= links. This change adds support for search strings to =id:=
> +links.
"This change..." part sounds like a commit message, not a NEWS item.
You may probably remove this paragraph.
> \f
> +(defun org-link--try-link-store-functions (interactive?)
> + "Try storing external links, prompting if more than one is possible.
> +
> +Each function returned by `org-store-link-functions` is called in
> +turn. If multiple functions return non-nil, prompt for which link
> +should be stored.
We use `...' Elisp quoting, not markdown.
Also, please keep two spaces between sentences - you do it
inconsistently across the patch.
> +Return t if a function has successfully stored a link, which will
> +be stored in `org-link-store-props`.
I'd better say "Return t when the link is stored in `org-store-link-plist'."
> +(defcustom org-id-link-consider-parent-id nil
> + "Non-nil means storing a link to an Org file will consider parent entry IDs.
> +
> +Use this with `org-id-link-use-context` set to `t` to allow
> +linking to uniquely-named sub-entries within a parent entry with
> +an ID, without requiring every sub-entry to have its own ID."
1. `...' quoting
2. The docstring is slightly confusing. Having an example would be helpful.
> +(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.
> +Using a prefix argument to the commands `org-store-link' \
> +\(`\\[universal-argument] \\[org-store-link]') or
> +`org-id-store-link` negates this setting for the duration of the
> +command."
You should also update the docstring of `org-store-link' accordingly.
> ;;;###autoload
> -(defun org-id-get-create (&optional force)
> +(defun org-id-get-create (&optional force inherit)
>...
> ;;;###autoload
> -(defun org-id-get (&optional epom create prefix)
> +(defun org-id-get (&optional epom create prefix inherit)
Please document this new optional arguments in the NEWS.
> - (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.
> -(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.
> "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.
> + ;; 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'.
> + (pcase (org-link-precise-link-target id-location)
Why not passing the RELATIVE-TO argument?
> (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.
> + (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.
--
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-10 13:33 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 [this message]
2023-12-14 20:42 ` Rick Lupton
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=87jzpmqiy0.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).