emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
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>


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