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>
