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

[Ihor Radchenko <yantar92@posteo.net>]

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

> Thanks for trying it out.  Updated patches attached, comments below.

Thanks!

>> 3. Consider
>>    (setq org-id-link-consider-parent-id t)
>>    (setq org-id-link-to-org-use-id t)
>>
>>    Then, create a new empty Org file
>>    M-x org-store-link with create a top-level properties drawer with ID
>>    and store the link. However, that link will not be a simple ID link,
>>    but also have ::PROPERTIES search string, which is not expected.
>
> This is because it is trying to link to the current line of the file, which contains the text "PROPERTIES".  On main, with (setq org-id-link-to-org-use-id nil), you see the equivalent behaviour (a link to [[file:test.org:::PROPERTIES:]]) when point is before the first heading.  So, this seems consistent with non-org-id links?

No. Do note that my instructions start from _empty_ file. With
org-id-link-to-org-use-id, PROPERTIES drawer is not created. This is
different from what happens with your patch - it is unexpected in your
patch that the search string is added for text that did not exist in the
buffer previously.

> (these links don't actually work with the default value of
> `org-link-search-must-match-exact-headline', but I think that's a
> separate issue).

That's a good catch.
The fact that links stored via `org-store-link' cannot be open with
default settings is not good.
Also, your patch disregards this setting - it should not match
non-headline search strings with the default value of
`org-link-search-must-match-exact-headline'.

Probably, changing the default value of
`org-link-search-must-match-exact-headline' to nil is due.

> Subject: [PATCH 2/2] org-id.el: Extend links with search strings, inherit
>  parent IDs

I ran make test, and it looks like one test is failing with your patch:

1 unexpected results:
   FAILED  test-org-link/id-store-link-using-parent  ((should (equal '("id:abc" "H1") (test-ol-stored-link-with-text "* H1\n:PROPERTIES:\n:ID: abc\n:END:\n** H2\n<point>" (org-id-store-link)))) :form (equal ("id:abc" "H1") ("id:88e0c8d7-90a6-4628-b35a-cea989e3561b" "H2")) :value nil :explanation (list-elt 0 (arrays-of-different-length 6 39 "id:abc" "id:88e0c8d7-90a6-4628-b35a-cea989e3561b" first-mismatch-at 3)))

> +  #+vindex: org-id-link-consider-parent-id
> +  When ~org-id-link-consider-parent-id~ is ~t~ (and
> +  ~org-context-in-file-links~ and ~org-id-link-use-context~ are both

`org-context-in-file-links' is an obsolete name. Use
`org-link-context-for-files'.

Also, please add `org-id-link-use-context' to #+vindex.

> +**** =org-link= store functions are passed an ~interactive?~ argument
> +
> +The ~:store:~ functions set for link types using
> +~org-link-set-parameters~ are now passed an ~interactive?~ argument,
> +indicating whether ~org-store-link~ was called interactively.
> +
> +Existing store functions will continue to work.

Please update the docstring of `org-store-link-functions' to specify
that an argument is passed to :store functions.

(org-link-parameters docstring says

:store

  Function responsible for storing the link.  See the function
  org-store-link-functions for a description of the expected
  arguments.
)

> -      (org-insert-heading nil t t)
> +      ;; Find appropriate level for new heading
> +      (let ((level (save-excursion
> +                     (goto-char (point-min))
> +                     (+ 1 (or (org-current-level) 0)))))

This is fragile. You assume that `point-min' always contains a heading.
That may or may not be the case - `org-link-search' may be called by
third-party code that does not care about setting narrowing in certain
ways.

It is more reliable to do something like (while (org-up-heading-safe)
...) to find the lowest-level ancestor.

> +(defun org-link-precise-link-target (&optional relative-to)
> +  "Determine search string and description for storing a link.
> +
> +If a search string (see 'org-link-search') is found, return cons

Quoting: `org-link-search'.

> +           (let* ((element (org-element-at-point))
> +                  (name (org-element-property :name element))
> +                  (heading (org-element-lineage element 'headline t))

What about inlinetasks?

> +                  (custom-id (org-entry-get nil "CUSTOM_ID")))

May as well pass HEADING as the first argument of `org-entry-get'. It
will be slightly more efficient.

> +        (org-link--add-to-stored-links link desc)
> +        ;; In org buffers, store an additional "human-readable" link
> +        ;; using custom id, if available.
> +        (when (and (buffer-file-name (buffer-base-buffer))
> +                   (derived-mode-p 'org-mode)
> +                   (org-entry-get nil "CUSTOM_ID"))
> +          (setq link (concat "file:"
> +                             (abbreviate-file-name
> +                              (buffer-file-name (buffer-base-buffer)))
> +                             "::#" (org-entry-get nil "CUSTOM_ID")))

This is fragile - you are relying upon the exact code used to store
file:...#CUSTOM-ID link. Instead, please refactor the function to re-use
that code.

> +           (id-location (or (and org-entry-property-inherited-from
> +                                 (marker-position org-entry-property-inherited-from))
> +                            (save-excursion (org-back-to-heading-or-point-min) (point))))
>  	   (case-fold-search nil)
>  	   (desc (save-excursion
> -		   (org-back-to-heading-or-point-min t)
> +                   (goto-char id-location)

You are calling `org-back-to-heading-or-point-min' without optional
argument INVISIBLE-OK. This looks like an oversight.

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