ol.el: add description format parameter to org-link-parameters

ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

Suggested patch to add a new link parameter controlling how the
default description is generated. As explained in the commit, this
behaviour is often similar between links of the same type, but differs
between those types, so a parameter seems like a good place to specify
it.

I've tried to make `org-insert-link' behave sensibly with regard to
all the possible options -- hope it's alright.

Blue skies, Hugo

[PATCH] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

* ol.el (org-link-parameters): add parameter `:default-description', a
string or a function.
* (org-insert-link): if no description is provided (pre-existing or as
an argument), next option is to use the `:default-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types. A
type-parameter is thus a good place to store information on the
default description.
---
 lisp/ol.el | 49 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index 1b2bb9a9a..af0aaaf35 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -140,6 +140,13 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description. A string is used as-is, a function is
+  called with the full link text as the sole argument, and should
+  return a single string.
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1761,11 +1768,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:default-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1775,7 +1785,7 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 entry all-prefixes auto-desc type)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1842,6 +1852,7 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 		      (and (equal ":" (substring link -1))
 			   (member (substring link 0 -1) all-prefixes)
 			   (setq link (substring link 0 -1))))
+              (setq type link)
 	      (setq link (with-current-buffer origbuf
 			   (org-link--try-special-completion link)))))
 	(set-window-configuration wcf)
@@ -1918,14 +1929,24 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (let ((initial-input
 	     (cond
 	      (description)
-	      ((not org-link-make-description-function) desc)
-	      (t (condition-case nil
-		     (funcall org-link-make-description-function link desc)
-		   (error
-		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
-		    (sit-for 2)
-		    nil))))))
+              (desc)
+              ((org-link-get-parameter
+                type
+                :default-description)
+               (let ((def (org-link-get-parameter
+                           type
+                           :default-description)))
+                 (cond
+                  ((stringp def) def)
+                  ((functionp def)
+                   (funcall def link)))))
+	      (org-link-make-description-function
+               (funcall org-link-make-description-function link desc))
+              (t (error
+		  (message "Can't get link description from %S"
+			   (symbol-name org-link-make-description-function))
+		  (sit-for 2)
+		  nil)))))
 	(setq desc (if (called-interactively-p 'any)
 		       (read-string "Description: " initial-input)
 		     initial-input))))
-- 
2.20.1

Re: [PATCH] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

> * ol.el (org-link-parameters): add parameter `:default-description', a
> string or a function.

LGTM in general. Note that I proposed (and never followed up on) a
similar patch in the past:
https://orgmode.org/list/87pnauvxht.fsf@localhost

I like that you introduced an option to set default description to
string, but I have several comments on the patch itself.


> -	      (t (condition-case nil
> -		     (funcall org-link-make-description-function link desc)
> +	      (org-link-make-description-function
> +               (funcall org-link-make-description-function link desc))
> +              (t (error
> +		  (message "Can't get link description from %S"
> +			   (symbol-name org-link-make-description-function))
> +		  (sit-for 2)
> +		  nil)))))

It looks like you removed condition-case used to catch errors on
org-link-make-description function. I am pretty sure that it is not
intentional.

Similar condition-case could also be used for the :default-description
function.

+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description. A string is used as-is, a function is
+  called with the full link text as the sole argument, and should
+  return a single string.

Why not two arguments like in org-link-make-description-function?

Best,
Ihor

[PATCH v2] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

* ol.el (org-link-parameters): add parameter `:default-description', a
string or a function.
* (org-insert-link): if no description is provided (pre-existing or as
an argument), next option is to use the `:default-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types. A
type-parameter is thus a good place to store information on the
default description.
---

I've added the condition-case back to the check on
`org-link-make-description', and added a new one to the check for the
`:default-description' parameter, as Ihor suggested. I've also
modified the handling of that parameter, to reflect
`org-link-make-description', and updated the docstring accordingly.

Apologies if the subject formatting is not correct, I'm still getting
the hang of git-send-email.

Hugo

 lisp/ol.el | 43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index 1b2bb9a9a..e74ef8dda 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -140,6 +140,15 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description. A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-linke'. It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1761,11 +1770,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:default-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1775,7 +1787,7 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 entry all-prefixes auto-desc type)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1842,6 +1854,7 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 		      (and (equal ":" (substring link -1))
 			   (member (substring link 0 -1) all-prefixes)
 			   (setq link (substring link 0 -1))))
+              (setq type link)
 	      (setq link (with-current-buffer origbuf
 			   (org-link--try-special-completion link)))))
 	(set-window-configuration wcf)
@@ -1918,7 +1931,23 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (let ((initial-input
 	     (cond
 	      (description)
-	      ((not org-link-make-description-function) desc)
+              (desc)
+              ((org-link-get-parameter
+                type
+                :default-description)
+               (condition-case nil
+                   (let ((def (org-link-get-parameter
+                               type
+                               :default-description)))
+                     (cond
+                      ((stringp def) def)
+                      ((functionp def)
+                       (funcall def link desc))))
+                 (error
+                  (message "Can't get link description from %S"
+			   (symbol-name def))
+		  (sit-for 2)
+                  nil)))
 	      (t (condition-case nil
 		     (funcall org-link-make-description-function link desc)
 		   (error
-- 
2.20.1

Re: [PATCH v2] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

> I've added the condition-case back to the check on
> `org-link-make-description', and added a new one to the check for the
> `:default-description' parameter, as Ihor suggested. I've also
> modified the handling of that parameter, to reflect
> `org-link-make-description', and updated the docstring accordingly.

Thanks!

Some more comments:

> +               (condition-case nil
> +                   (let ((def (org-link-get-parameter
> +                               type
> +                               :default-description)))
> +                     (cond
> +                      ((stringp def) def)
> +                      ((functionp def)
> +                       (funcall def link desc))))
> +                 (error
> +                  (message "Can't get link description from %S"
> +			   (symbol-name def))

Firstly, def will be undefined inside the error clause.

Secondly, when :default-description is a lambda expression and that
expression fails, your code will fail with "wrong-type-argument symbolp"
when executing (symbol-name def). Please consider cases when `def' is
not a string and not a function symbol.

I recommend writing tests for org-insert-link in testing/lisp/test-ol.el
and testing expected behaviour for various values of
:default-description.

> Apologies if the subject formatting is not correct, I'm still getting
> the hang of git-send-email.

You don't have to use git-send-email to submit simple patches. An easier
(as for me) alternative is a simple email with attached patch. magit
makes it trivial to create patch files.

Best,
Ihor

Re: [PATCH v3] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

I've finally had time to the `:default-description' parameter in
`org-link-parameters'.

Ihor Radchenko <yantar92@gmail.com> writes:

> I recommend writing tests for org-insert-link in
> testing/lisp/test-ol.el and testing expected behaviour for various
> values of :default-description.

Good advice! I haven't written many tests before, so this was a
challenge. I've attached a patch with some tests for the expected
behaviour. These include a couple of macros and a function, to stop
the definitions being too huge and unwieldy.

All the tests pass on my machine.

They're very nearly just right, but for some reason
`test-ol-with-link-parameters-as' doesn't always reset the parameters
correctly for me. However I have them set to originally, it sets
`:default-description' to a lambda. This might be a quirk at my end
though -- if someone else could have a look I'd be very grateful.

The tests showed that my previous approach doesn't work generally
enough. I've moved where `type' is defined in the code, to cover all
cases.

> Firstly, def will be undefined inside the error clause.

I've defined def a bit further up in a `let', and run the error clause
inside this.

> Secondly, when :default-description is a lambda expression and that
> expression fails, your code will fail with "wrong-type-argument
> symbolp" when executing (symbol-name def). Please consider cases
> when `def' is not a string and not a function symbol.

Dealt with this by just using `message "<text> %S: "', since %S will
print a symbol, a string or a lambda correctly.

Hope this is a bit better!

Best,
Hugo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-test-ol-tests-for-default-description-param-when-ins.patch --]
[-- Type: text/x-diff, Size: 3476 bytes --]

From 5a44edfda4e09a95bba1327c2758b2637e5b16bd Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Tue, 21 Jun 2022 12:45:50 +0100
Subject: [PATCH 2/2] test-ol: tests for default-description param when
 inserting links

Add tests for various values of `:default-description' in
`org-link-parameters'.
---
 testing/lisp/test-ol.el | 77 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..7524d73af 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -625,5 +625,82 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  `(progn
+     (setq orig-parameters org-link-parameters)
+     (org-link-set-parameters ,type ,@parameters)
+     (let ((rtn (progn ,@body)))
+       (setq org-link-parameters orig-parameters)
+       rtn)))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:default-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-default-description', for the case where
+`:default-description' is a function symbol."
+  "foobar")
+
+(ert-deftest test-ol/insert-link-default-description ()
+  "Test `:default-description' parameter handling."
+  ;; String case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description (lambda (_link-test _desc) "foobar"))
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description #'test-ol/return-foobar)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:default-description' parameter is defined, but doesn't return a
+  ;; string
+  (should-error
+   (test-ol-with-link-parameters-as
+    "id" (:default-description #'ignore)
+    (test-ol-insert-link-get-desc "id:foo-bar")))
+  ;; Description argument should override `:default-description'
+  (should
+   (string=
+    "notfoobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar" "notfoobar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 4870 bytes --]

From 437155c2394b5c9961ecc0af4e1e1a54b63416fe Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH 1/2] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): add parameter `:default-description', a
string or a function.
* (org-insert-link): if no description is provided (pre-existing or as
an argument), next option is to use the `:default-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types. A
type-parameter is thus a good place to store information on the
default description.
---
 lisp/ol.el | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..1ab3a5f76 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,15 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-link'.  It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1804,11 +1813,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:default-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1830,7 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 entry all-prefixes auto-desc type)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1893,6 +1905,13 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (or entry (push link org-link--insert-history))
       (setq desc (or desc (nth 1 entry)))))
 
+    (setq type
+          (cond
+           ((string-match (rx (: string-start (submatch (eval `(or ,@all-prefixes))) ":")) link)
+            (match-string 1 link))
+           ((file-name-absolute-p link) "file")
+           ((string-match "\\`\\.\\.?/" link) "file")))
+
     (when (funcall (if (equal complete-file '(64)) 'not 'identity)
 		   (not org-link-keep-stored-after-insertion))
       (setq org-stored-links (delq (assoc link org-stored-links)
@@ -1961,12 +1980,24 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (let ((initial-input
 	     (cond
 	      (description)
-	      ((not org-link-make-description-function) desc)
+              (desc)
+              ((org-link-get-parameter type :default-description)
+               (let ((def (org-link-get-parameter type :default-description)))
+                 (condition-case nil
+                     (cond
+                      ((stringp def) def)
+                      ((functionp def)
+                       (funcall def link desc)))
+                   (error
+                    (message "Can't get link description from org link parameter `:default-description': %S"
+			     def)
+		    (sit-for 2)
+                    nil))))
 	      (t (condition-case nil
 		     (funcall org-link-make-description-function link desc)
 		   (error
 		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
+		             org-link-make-description-function)
 		    (sit-for 2)
 		    nil))))))
 	(setq desc (if (called-interactively-p 'any)
-- 
2.20.1

Re: [PATCH v3] ol.el: add description format parameter to org-link-parameters

[Robert Pluim]

>>>>> On Tue, 21 Jun 2022 13:03:32 +0100, Hugo Heagren <hugo@heagren.com> said:

    Hugo> They're very nearly just right, but for some reason
    Hugo> `test-ol-with-link-parameters-as' doesn't always reset the parameters
    Hugo> correctly for me. However I have them set to originally, it sets
    Hugo> `:default-description' to a lambda. This might be a quirk at my end
    Hugo> though -- if someone else could have a look I'd be very grateful.

They way your macro is defined, it uses `orig-parameters' in a way
that it will leak into the global namespace. Better would be to do

(let ((orig-parameters org-link-parameters))
       ...
      (setq org-link-parameters orig-parameters)

Robert
-- 

[PATCH v4] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

Since the last version of this patch, I have:
- moved the code which sets `type' in `org-insert-link' to a position
  where it covers more cases
- rewritten the macros used in the tests, so that always (and
  correctly) restore the original state after running, even after
  errors. Thanks to Max Nikulin for suggesting using `condition-case'

There is only one thing left which I am not happy with. Currently, the
test fails in the case where :default-description is 'ignore. This was
intended to test for situations where the parameter is set to a
function, but the function doesn't return a string (I used ignore
because it returns 'nil). Accordingly, the test is a `should-error'
(because the function *should* return a string, so we should error if
it doesn't, right?).

But the error condition is inside the error clause of a condition
case---which is only triggered if the code errors. Calling 'ignore
with any arguments and getting 'nil back doesn't cause any errors, so
the error clause is never triggered, and 'nil is just used as the
default description (which is then ignored, because it is 'nil --
other non-string values like numbers would not be ignored).

I'd like some input on what to do about this: I could rewrite the
tests so that a nil-value doesn't matter. In the case of this test, a
non-interactive call would just insert a link without a description.
Alternatively I could rewrite the function so that if the
:default-description parameter returns something, we error unless it
is a string.

tl;dr The question is: what is the Good Behaviour when
:default-description is set to something, which is meant to return a
string and returns 'nil instead? Should it be treated like an empty
string, or as an error?

Thanks,

Hugo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-test-ol-tests-for-default-description-param-when-ins.patch --]
[-- Type: text/x-diff, Size: 4182 bytes --]

From ae17e87436def764f99b24add4debb5d7a481e1a Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Tue, 21 Jun 2022 12:45:50 +0100
Subject: [PATCH 2/2] test-ol: tests for default-description param when
 inserting links

Add tests for various values of `:default-description' in
`org-link-parameters'.
---
 testing/lisp/test-ol.el | 92 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..9114c6497 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -625,5 +625,97 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  ;; Copy all keys in `parameters' and their original values to
+  ;; `orig-parameters'. For `parity': nil = odd, non-nill = even
+  `(let (parity orig-parameters)
+     (dolist (p ',parameters)
+       (unless parity
+         (setq orig-parameters
+               (plist-put orig-parameters p (org-link-get-parameter ,type p))))
+       (setq parity (not parity)))
+     ;; Set `parameters' values
+     (condition-case err
+         (let ((_ (org-link-set-parameters ,type ,@parameters))
+               ;; Do body
+               (rtn (progn ,@body)))
+           ;; Restore original values
+           (apply 'org-link-set-parameters ,type orig-parameters)
+           ;; Return whatever the body returned
+           rtn)
+       ;; In case of error, restore state anyway AND really error
+       (error
+        (apply 'org-link-set-parameters ,type orig-parameters)
+        (signal (car err) (cdr err))))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:default-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-default-description', for the case where
+`:default-description' is a function symbol."
+  "foobar")
+
+(ert-deftest test-ol/insert-link-default-description ()
+  "Test `:default-description' parameter handling."
+  ;; String case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description (lambda (_link-test _desc) "foobar"))
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description #'test-ol/return-foobar)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:default-description' parameter is defined, but doesn't return a
+  ;; string
+  (should-error
+   (test-ol-with-link-parameters-as
+    "id" (:default-description #'ignore)
+    (test-ol-insert-link-get-desc "id:foo-bar")))
+  ;; Description argument should override `:default-description'
+  (should
+   (string=
+    "notfoobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar" "notfoobar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 5472 bytes --]

From 12726d18487298907ff63374120d17ee733383a7 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH 1/2] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): add parameter `:default-description', a
string or a function.
* (org-insert-link): if no description is provided (pre-existing or as
an argument), next option is to use the `:default-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types. A
type-parameter is thus a good place to store information on the
default description.
---
 lisp/ol.el | 53 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..00f3bce5c 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,15 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-link'.  It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1804,11 +1813,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:default-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1830,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc type)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1874,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1893,6 +1905,13 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (or entry (push link org-link--insert-history))
       (setq desc (or desc (nth 1 entry)))))
 
+    (setq type
+          (cond
+           ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+            (match-string 1 link))
+           ((file-name-absolute-p link) "file")
+           ((string-match "\\`\\.\\.?/" link) "file")))
+
     (when (funcall (if (equal complete-file '(64)) 'not 'identity)
 		   (not org-link-keep-stored-after-insertion))
       (setq org-stored-links (delq (assoc link org-stored-links)
@@ -1961,12 +1980,24 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (let ((initial-input
 	     (cond
 	      (description)
-	      ((not org-link-make-description-function) desc)
+              (desc)
+              ((org-link-get-parameter type :default-description)
+               (let ((def (org-link-get-parameter type :default-description)))
+                 (condition-case nil
+                     (cond
+                      ((stringp def) def)
+                      ((functionp def)
+                       (funcall def link desc)))
+                   (error
+                    (message "Can't get link description from org link parameter `:default-description': %S"
+			     def)
+		    (sit-for 2)
+                    nil))))
 	      (t (condition-case nil
 		     (funcall org-link-make-description-function link desc)
 		   (error
 		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
+		             org-link-make-description-function)
 		    (sit-for 2)
 		    nil))))))
 	(setq desc (if (called-interactively-p 'any)
-- 
2.20.1

Re: [PATCH v4] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

> Since the last version of this patch, I have:
> - moved the code which sets `type' in `org-insert-link' to a position
>   where it covers more cases
> - rewritten the macros used in the tests, so that always (and
>   correctly) restore the original state after running, even after
>   errors. Thanks to Max Nikulin for suggesting using `condition-case'

Thanks!

> tl;dr The question is: what is the Good Behaviour when
> :default-description is set to something, which is meant to return a
> string and returns 'nil instead? Should it be treated like an empty
> string, or as an error?

Currently, the internal implementation will treat nil return value as if
there was no :default-description and org-link-make-description-function
were set to nil. We may probably document this. It sounds like a useful
behavior.

If the :default-description function returns non-string and not nil, the
behavior is simply undefined. It was also the case for
org-link-make-description-function. Though we might add a cl-assert
somewhere near the end of org-insert-link to deliberately throw an
error.

> Subject: [PATCH 2/2] test-ol: tests for default-description param when
>  inserting links

> Add tests for various values of `:default-description' in
> `org-link-parameters'.

Could you also add the proper changelog entry for the test-ol.el file?

> +;;; Insert Links
> +
> +(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
> +  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
> +
> +Save the original value of `org-link-parameters', execute
> +`org-link-set-parameters' with the relevant args, execute BODY
> +and restore `org-link-parameters'.
> +
> +TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
> +be passed to `org-link-set-parameters'."
> +  ;; Copy all keys in `parameters' and their original values to
> +  ;; `orig-parameters'. For `parity': nil = odd, non-nill = even

*non-nil

Also, please end each sentence in the comment with ".".

> +  `(let (parity orig-parameters)
> +     (dolist (p ',parameters)
> +       (unless parity
> +         (setq orig-parameters
> +               (plist-put orig-parameters p (org-link-get-parameter ,type p))))
> +       (setq parity (not parity)))

This is a bit awkward code. You can instead use cl-loop by 'cddr or
a simple while loop with two (pop parameters) statements inside.

Also, ',parameters will fail if PARAMETERS argument is a variable name.

> +     ;; Set `parameters' values
> +     (condition-case err
> +         (let ((_ (org-link-set-parameters ,type ,@parameters))
> +               ;; Do body
> +               (rtn (progn ,@body)))
> +           ;; Restore original values
> +           (apply 'org-link-set-parameters ,type orig-parameters)
> +           ;; Return whatever the body returned
> +           rtn)
> +       ;; In case of error, restore state anyway AND really error
> +       (error
> +        (apply 'org-link-set-parameters ,type orig-parameters)
> +        (signal (car err) (cdr err))))))

unwind-protect is more suitable here and will lead to more concise code.

> Subject: [PATCH 1/2] ol.el: add description format parameter to
>  org-link-parameters
>
> * ol.el (org-link-parameters): add parameter `:default-description', a
> string or a function.
> * (org-insert-link): if no description is provided (pre-existing or as
> an argument), next option is to use the `:default-description' (if
> non-nil) parameter to generate one.
>
> Default descriptions are predictable within a link type, but because
> link types are quite diverse, are NOT predictable across many types. A
> type-parameter is thus a good place to store information on the
> default description.

Please start sentences after ":" with capital letter. Also, you missed
double space between sentences.

> +`:default-description'
> +
> +  String or function used as a default when prompting users for a
> +  link's description.  A string is used as-is, a function is
> +  called with two arguments: the full link text, and the
> +  description generated by `org-insert-link'.  It should return
> +  the description to use (this reflects the behaviour of
> +  `org-link-make-description-function').
> +
>  `:display'

I think that we should also document the new parameter in ORG-NEWS.
  
Best,
Ihor

Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

Ihor Radchenko <yantar92@gmail.com> writes:

> Currently, the internal implementation will treat nil return value as if
> there was no :default-description and org-link-make-description-function
> were set to nil. We may probably document this. It sounds like a useful
> behavior.

I added some relevant documentation to the `:default-description' part
of the `org-link-parameters' docstring. I also changed the
previosly-failing test to expect the right value. All the tests pass
now.

> Could you also add the proper changelog entry for the test-ol.el
> file?

Think I've got this right now, and other formatting errors.

> unwind-protect is more suitable here and will lead to more concise
> code.

I didn't know about `unwind-protect', thanks! You're right, much better.

> I think that we should also document the new parameter in ORG-NEWS.

Done.

Thanks, Hugo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 6372 bytes --]

From cd5268bc05324140a4cf384ce12b99bba1ddab47 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): Add parameter `:default-description', a
string or a function.
* (org-insert-link): If no description is provided (pre-existing or as
an argument), next option is to use the `:default-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types.
A type-parameter is thus a good place to store information on the
default description.
---
 etc/ORG-NEWS |  7 +++++++
 lisp/ol.el   | 56 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 35af94f92..d51899b20 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -322,6 +322,13 @@ purpose of the variable.  The replacement variable
 accepts =listings= and =verbatim= in place of =t= and =nil= (which
 still work, but are no longer listed as valid options).
 
+*** ~org-link-parameters~ has a new ~:default-description~ parameter
+
+The value of ~:default-description~ is used as the initial input when
+prompting for a link description.  It can be a string (used as-is) or
+a function (called with the same arguments as
+~org-make-link-description-function~ to return a string to use).
+
 * Version 9.5
 
 ** Important announcements and breaking changes
diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..e568cd6fc 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,18 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:default-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-link'.  It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').  If it returns nil, no
+  default description is used, but no error is thrown (from the
+  user's perspective, this is equivalent to a default description
+  of \"\").
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1804,11 +1816,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:default-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1833,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc type)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1877,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1893,6 +1908,13 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (or entry (push link org-link--insert-history))
       (setq desc (or desc (nth 1 entry)))))
 
+    (setq type
+          (cond
+           ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+            (match-string 1 link))
+           ((file-name-absolute-p link) "file")
+           ((string-match "\\`\\.\\.?/" link) "file")))
+
     (when (funcall (if (equal complete-file '(64)) 'not 'identity)
 		   (not org-link-keep-stored-after-insertion))
       (setq org-stored-links (delq (assoc link org-stored-links)
@@ -1961,12 +1983,24 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
       (let ((initial-input
 	     (cond
 	      (description)
-	      ((not org-link-make-description-function) desc)
+              (desc)
+              ((org-link-get-parameter type :default-description)
+               (let ((def (org-link-get-parameter type :default-description)))
+                 (condition-case nil
+                     (cond
+                      ((stringp def) def)
+                      ((functionp def)
+                       (funcall def link desc)))
+                   (error
+                    (message "Can't get link description from org link parameter `:default-description': %S"
+			     def)
+		    (sit-for 2)
+                    nil))))
 	      (t (condition-case nil
 		     (funcall org-link-make-description-function link desc)
 		   (error
 		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
+		             org-link-make-description-function)
 		    (sit-for 2)
 		    nil))))))
 	(setq desc (if (called-interactively-p 'any)
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-test-ol-tests-for-default-description-param-when-ins.patch --]
[-- Type: text/x-diff, Size: 4186 bytes --]

From 16f4ba5ccb73d09cf9c61eeb67f1ce21c367c043 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Tue, 21 Jun 2022 12:45:50 +0100
Subject: [PATCH] test-ol: tests for default-description param when inserting
 links

* test-ol (test-ol-with-link-parameters-as): Convenience macro for
testing.
(test-ol-insert-link-get-desc): Convenience macro for testing.
(test-ol/return-foobar): Convenience function for testing.
(test-ol/insert-link-default-description): Test for various values of
`:default-description' in `org-link-parameters' (including
`test-ol/return-foobar').
---
 testing/lisp/test-ol.el | 85 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..a5bf5758f 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -19,6 +19,8 @@
 
 ;;; Code:
 
+(require 'cl-lib)
+
 \f
 ;;; Decode and Encode Links
 
@@ -625,5 +627,88 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  (let (orig-parameters)
+    ;; Copy all keys in `parameters' and their original values to
+    ;; `orig-parameters'.
+    (cl-loop for param in parameters by 'cddr
+             do (setq orig-parameters
+                      (plist-put orig-parameters param (org-link-get-parameter type param))))
+    `(unwind-protect
+         ;; Set `parameters' values and execute body.
+         (progn (org-link-set-parameters ,type ,@parameters) ,@body)
+       ;; Restore original values.
+       (apply 'org-link-set-parameters ,type ',orig-parameters))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:default-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-default-description', for the case where
+`:default-description' is a function symbol."
+  "foobar")
+
+(ert-deftest test-ol/insert-link-default-description ()
+  "Test `:default-description' parameter handling."
+  ;; String case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description (lambda (_link-test _desc) "foobar"))
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description #'test-ol/return-foobar)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:default-description' parameter is defined, but doesn't return a
+  ;; string.
+  (should
+   (null
+    (test-ol-with-link-parameters-as
+     "id" (:default-description #'ignore)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Description argument should override `:default-description'.
+  (should
+   (string=
+    "notfoobar"
+    (test-ol-with-link-parameters-as
+     "id" (:default-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar" "notfoobar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1

Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

>> Currently, the internal implementation will treat nil return value as if
>> there was no :default-description and org-link-make-description-function
>> were set to nil. We may probably document this. It sounds like a useful
>> behavior.
>
> I added some relevant documentation to the `:default-description' part
> of the `org-link-parameters' docstring. I also changed the
> previosly-failing test to expect the right value. All the tests pass
> now.

Thanks! Can you also update the documentation for
org-link-make-description-function?

> From cd5268bc05324140a4cf384ce12b99bba1ddab47 Mon Sep 17 00:00:00 2001
> From: Hugo Heagren <hugo@heagren.com>
> Date: Mon, 28 Mar 2022 23:18:45 +0100
> Subject: [PATCH] ol.el: add description format parameter to
>  org-link-parameters

Also, can you please review the comments from Max:
https://list.orgmode.org/877d4flu3x.fsf@heagren.com/T/#m3b48d0076a25eaf24b1af48df317984dd4e33fb0

> +    ;; Copy all keys in `parameters' and their original values to
> +    ;; `orig-parameters'.
> +    (cl-loop for param in parameters by 'cddr
> +             do (setq orig-parameters
> +                      (plist-put orig-parameters param (org-link-get-parameter type param))))

Thinking about this part a bit more, I have realized that even better
option would be using map-do.

Best,
Ihor

Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

> Can you also update the documentation for
> org-link-make-description-function?

I'm not sure what sort of documentation you have in mind? What should
I add?

> I have realized that even better option would be using map-do.

Agreed. Should be in the current version.

With regard to Max's comments:

> I am in doubts concerning "default-description" as the parameter
> name. I would consider either more specific "insert-description"

Having read your thoughts, I agree. I've converted the patch to use
"insert-description" where-ever relevant.

> Have I missed something or the whole macro may be simplified to just
> copy `org-link-parameters'?
>
>    `(let ((org-link-parameters (copy-tree org-link-parameters)))
>       (org-link-set-parameters ,type ,@parameters)
>       ,@body))

I had the same thought. I doesn't work though. `copy-tree' and
`copy-sequence' only make shallow copies of each element in a
sequence. `org-link-set-parameters' then uses side effects to alter
the `org-link-parameters', so these changes are propagated to the
copy. This means the values in the copy and the real
`org-link-parameters' are always the same, and so we can't use the
copy to store the original values (which is what we need it to do).

> In addition, `declare' form should be added to `defmacro' to specify
> which argument is considered as its body.

I have never used `declare' before. I looked it up in the Elisp manual
and read the docstring, but I couldn't understand how it specifies
which argument is considered the body. I'm also not aware of what this
does/why this is useful? (not a criticism, I just haven't learned this
yet).

> My opinion is that it should be inside
>    (let ((initial-input ...

This is a good idea -- done.

> and maybe even be a sibling of
>     (def (org-link-get-parameter type
> to keep related code more local.

This isn't possible, because that's a clause in a `let' call, which is
itself inside a `cond' clause, and the `CONDITION' of that clause uses
`type', so it has to be defined at a higher level.

> I have not tested, so I may be wrong in respect to the following. It
> seems, you rises priority of desc value that earlier was a fallback.
> The reason why I am in doubts, is that it seems, desc is initialized
> from current link description when point is withing an existing link
> and in such cases desc likely should be preserved. I can not say
> that I like that it is responsibility of all description functions
> to return the desc argument if it is supplied.

It's right that `desc' is initialized from current link description
when point is withing an existing link.

Previously, `desc' was only used when
`org-link-make-description-function' was nil. This seems to me a very
odd behaviour: preserve the current link description, but only when
`org-link-make-description-function' is nil. Especially considering
that `org-insert-link' is also used for editing already-existing
links. So in the original version, in some situations,
`org-link-make-description-function' had a higher priority than
`desc', which seems wrong.

Thanks,

Hugo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 6402 bytes --]

From aac4b04e8448436dd2713116870ee86d87fa2005 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): Add parameter `:insert-description', a
string or a function.
* (org-insert-link): If no description is provided (pre-existing or as
an argument), next option is to use the `:insert-description' (if
non-nil) parameter to generate one.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types.
A type-parameter is thus a good place to store information on the
default description.
---
 etc/ORG-NEWS |  7 +++++
 lisp/ol.el   | 73 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 35af94f92..86128d100 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -322,6 +322,13 @@ purpose of the variable.  The replacement variable
 accepts =listings= and =verbatim= in place of =t= and =nil= (which
 still work, but are no longer listed as valid options).
 
+*** ~org-link-parameters~ has a new ~:insert-description~ parameter
+
+The value of ~:insert-description~ is used as the initial input when
+prompting for a link description.  It can be a string (used as-is) or
+a function (called with the same arguments as
+~org-make-link-description-function~ to return a string to use).
+
 * Version 9.5
 
 ** Important announcements and breaking changes
diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..5c702e33a 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,18 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:insert-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-link'.  It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').  If it returns nil, no
+  default description is used, but no error is thrown (from the
+  user's perspective, this is equivalent to a default description
+  of \"\").
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -1804,11 +1816,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:insert-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1833,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1877,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1958,17 +1973,35 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	    (setq desc path)))))
 
     (unless auto-desc
-      (let ((initial-input
-	     (cond
-	      (description)
-	      ((not org-link-make-description-function) desc)
-	      (t (condition-case nil
-		     (funcall org-link-make-description-function link desc)
-		   (error
-		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
-		    (sit-for 2)
-		    nil))))))
+      (let* ((type
+              (cond
+               ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+                (match-string 1 link))
+               ((file-name-absolute-p link) "file")
+               ((string-match "\\`\\.\\.?/" link) "file")))
+             (initial-input
+	      (cond
+	       (description)
+               (desc)
+               ((org-link-get-parameter type :insert-description)
+                (let ((def (org-link-get-parameter type :insert-description)))
+                  (condition-case nil
+                      (cond
+                       ((stringp def) def)
+                       ((functionp def)
+                        (funcall def link desc)))
+                    (error
+                     (message "Can't get link description from org link parameter `:insert-description': %S"
+			      def)
+		     (sit-for 2)
+                     nil))))
+	       (t (condition-case nil
+		      (funcall org-link-make-description-function link desc)
+		    (error
+		     (message "Can't get link description from %S"
+		              org-link-make-description-function)
+		     (sit-for 2)
+		     nil))))))
 	(setq desc (if (called-interactively-p 'any)
 		       (read-string "Description: " initial-input)
 		     initial-input))))
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-test-ol-tests-for-insert-description-param-when-inse.patch --]
[-- Type: text/x-diff, Size: 4204 bytes --]

From b8be9031c58a88e7d3294d587847b3edbbb43d36 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Sat, 16 Jul 2022 19:50:15 +0100
Subject: [PATCH] test-ol: tests for insert-description param when inserting
 links

* test-ol (test-ol-with-link-parameters-as): Convenience macro for
testing.
(test-ol-insert-link-get-desc): Convenience macro for testing.
(test-ol/return-foobar): Convenience function for testing.
(test-ol/insert-link-insert-description): Test for various values of
`:insert-description' in `org-link-parameters' (including
`test-ol/return-foobar').
---
 testing/lisp/test-ol.el | 88 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..d2af972bd 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -19,6 +19,8 @@
 
 ;;; Code:
 
+(require 'cl-lib)
+
 \f
 ;;; Decode and Encode Links
 
@@ -625,5 +627,91 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  (let (orig-parameters)
+    ;; Copy all keys in `parameters' and their original values to
+    ;; `orig-parameters'.
+    (map-do
+     (lambda (key _) (setq orig-parameters
+                      (plist-put
+                       orig-parameters key
+                       (org-link-get-parameter type key))))
+     parameters)
+    `(unwind-protect
+         ;; Set `parameters' values and execute body.
+         (progn (org-link-set-parameters ,type ,@parameters) ,@body)
+       ;; Restore original values.
+       (apply 'org-link-set-parameters ,type ',orig-parameters))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:insert-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-insert-description', for the case where
+`:insert-description' is a function symbol."
+  "foobar")
+
+(ert-deftest test-ol/insert-link-insert-description ()
+  "Test `:insert-description' parameter handling."
+  ;; String case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:insert-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:insert-description (lambda (_link-test _desc) "foobar"))
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+     "id" (:insert-description #'test-ol/return-foobar)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:insert-description' parameter is defined, but doesn't return a
+  ;; string.
+  (should
+   (null
+    (test-ol-with-link-parameters-as
+     "id" (:insert-description #'ignore)
+     (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Description argument should override `:insert-description'.
+  (should
+   (string=
+    "notfoobar"
+    (test-ol-with-link-parameters-as
+     "id" (:insert-description "foobar")
+     (test-ol-insert-link-get-desc "id:foo-bar" "notfoobar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1

Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

Hi,

Ihor, Hugo raised a question how to interpret nil returned by 
:insert-description functions. Do you have any opinion? I do not like 
the idea to consider it as an error since string is expected. I would 
prefer to call `org-link-make-description-function' as a fallback.

On 17/07/2022 04:20, Hugo Heagren wrote:
>> Can you also update the documentation for
>> org-link-make-description-function?
> 
> I'm not sure what sort of documentation you have in mind? What should
> I add?

I have no idea what Ihor means. From my point of view, mention of 
:insert-description and `org-link-parameters' may give a hint to users.

>> Have I missed something or the whole macro may be simplified to just
>> copy `org-link-parameters'?
>>
>>     `(let ((org-link-parameters (copy-tree org-link-parameters)))
>>        (org-link-set-parameters ,type ,@parameters)
>>        ,@body))
> 
> I had the same thought. I doesn't work though. `copy-tree' and
> `copy-sequence' only make shallow copies of each element in a
> sequence. `org-link-set-parameters' then uses side effects to alter
> the `org-link-parameters', so these changes are propagated to the
> copy. This means the values in the copy and the real
> `org-link-parameters' are always the same, and so we can't use the
> copy to store the original values (which is what we need it to do).

I have tried it again (Emacs-26.3). I agree that `copy-sequence' and 
shallow copy is not enough, but `copy-tree' works for me. It rather a 
curiosity than demanding related changes.

(defmacro nm-with-org-link-parameters (type parameters &rest body)
   (declare (indent 2))
   `(let ((org-link-parameters (copy-tree org-link-parameters)))
      (org-link-set-parameters ,type ,@parameters)
      ,@body))

(nm-with-org-link-parameters "help" (:insert-description (lambda (_link 
_descr) "h1"))
   (assoc "help" org-link-parameters))
=> ("help" :follow org-link--open-help :store org-link--store-help 
:insert-description (lambda (_link _descr) "h1"))

(assoc "help" org-link-parameters)
=> ("help" :follow org-link--open-help :store org-link--store-help)

> I have never used `declare' before. I looked it up in the Elisp manual
> and read the docstring, but I couldn't understand how it specifies
> which argument is considered the body. I'm also not aware of what this
> does/why this is useful? (not a criticism, I just haven't learned this
> yet).

See info "(elisp) Indenting Macros". Without `declare' automatic 
indentation is the following:

(nm-with-org-link-parameters "help" (:insert-description (lambda (_link 
_descr) "h1"))
			     (assoc "help" org-link-parameters))

>> and maybe even be a sibling of
>>      (def (org-link-get-parameter type
>> to keep related code more local.
> 
> This isn't possible, because that's a clause in a `let' call, which is
> itself inside a `cond' clause, and the `CONDITION' of that clause uses
> `type', so it has to be defined at a higher level.

It was written with assumption that `or' is used instead of `cond' to 
call `org-link-make-description-function' if :insert-description returns 
nil.

>> I have not tested, so I may be wrong in respect to the following. It
>> seems, you rises priority of desc value that earlier was a fallback.
>> The reason why I am in doubts, is that it seems, desc is initialized
>> from current link description when point is withing an existing link
>> and in such cases desc likely should be preserved. I can not say
>> that I like that it is responsibility of all description functions
>> to return the desc argument if it is supplied.
> 
> It's right that `desc' is initialized from current link description
> when point is withing an existing link.
> 
> Previously, `desc' was only used when
> `org-link-make-description-function' was nil. This seems to me a very
> odd behaviour: preserve the current link description, but only when
> `org-link-make-description-function' is nil. Especially considering
> that `org-insert-link' is also used for editing already-existing
> links. So in the original version, in some situations,
> `org-link-make-description-function' had a higher priority than
> `desc', which seems wrong.

In addition to changing behavior, you decision is still a trade-off. 
Consider the case when, editing some link, a user changes link path and 
expects to get new default description. When description function is 
called, to respect existing description, it may be defined as

(lambda (link desc)
  (or (org-string-nw-p desc)
      (format-string "My link %s" link)))

I do not like that in such approach checking of DESC is mandatory, so it 
is OK for me to commit your variant with high priority of DESC and to 
postpone discussion of possible improvements.

A side-note: see the following thread for a feedback form a user who is 
not happy with current prompt for description:
Visuwesh. [BUG] org-insert-link should use DEFAULT in read-string when 
asking for description. Fri, 25 Feb 2022 19:49:23 +0530. 
https://list.orgmode.org/87sfs7jafo.fsf@gmail.com

> +	       (t (condition-case nil
> +		      (funcall org-link-make-description-function link desc)

You do not check that `org-link-make-description-function' is not nil. 
You might even add a test for such configuration (no :insert-description 
is defined).

I have not tested the latest version of the patch and I am sorry if I 
missed some changes intended to address my suggestions.

Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Ihor, Hugo raised a question how to interpret nil returned by 
> :insert-description functions. Do you have any opinion? I do not like 
> the idea to consider it as an error since string is expected. I would 
> prefer to call `org-link-make-description-function' as a fallback.
>
> On 17/07/2022 04:20, Hugo Heagren wrote:
>>> Can you also update the documentation for
>>> org-link-make-description-function?
>> 
>> I'm not sure what sort of documentation you have in mind? What should
>> I add?
>
> I have no idea what Ihor means. From my point of view, mention of 
> :insert-description and `org-link-parameters' may give a hint to users.

Let me clarify.

The new :insert-description parameter is a more fine-grained equivalent
to already existing org-link-make-description-function. The latter can,
de facto, return string or nil - it is passed to `read-string' as
INITIAL-INPUT argument.

So, I suggested interpreting nil return value in :insert-description
just as it is already interpreted in org-link-make-description-function.
Further, I asked to document that nil value is allowed - both in
:insert-description docstring section and in
org-link-make-description-function docstring.

Best,
Ihor

Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

>> Can you also update the documentation for
>> org-link-make-description-function?
>
> I'm not sure what sort of documentation you have in mind? What should
> I add?

I referred to my earlier statement:

	> tl;dr The question is: what is the Good Behaviour when
	> :default-description is set to something, which is meant to return a
	> string and returns 'nil instead? Should it be treated like an empty
	> string, or as an error?

	Currently, the internal implementation will treat nil return value as if
	there was no :default-description and org-link-make-description-function
	were set to nil. We may probably document this. It sounds like a useful
	behaviour.

	If the :default-description function returns non-string and not nil, the
	behaviour is simply undefined. It was also the case for
	org-link-make-description-function. Though we might add a cl-assert
	somewhere near the end of org-insert-link to deliberately throw an
	error.

The return value of org-link-make-description-function can also be nil
in addition to string.  I suggest documenting this, just as you did for
:insert-description parameter.

Best,
Ihor

Re: [PATCH v6] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On 17/07/2022 04:20, Ihor Radchenko wrote:
> The return value of org-link-make-description-function can also be nil
> in addition to string.  I suggest documenting this, just as you did for
> :insert-description parameter.

Ah I understand. Done. Also updated the changelog commit message to
reflect the new documentation.

On 17/07/2022 04:20, Max Nikulin wrote:
> See info "(elisp) Indenting Macros".

I understand now. Fixed in this version. Thanks for clarifying.

> In addition to changing behavior, you decision is still a trade-off.
> Consider the case when, editing some link, a user changes link path
> and expects to get new default description.

Yes, I see what you mean. I suppose that without a user option or
similar to control this behaviour, it will always be a trade-off. I
think the behaviour I have written makes the most sense, and as you
say, a solution to completely resolve it might be unwieldy. If someone
else really wants different behaviour I suppose they can submit a
patch.

> You do not check that `org-link-make-description-function' is not nil.

This is a good point. I've substituted
`org-link-make-description-function' for `t', which has exactly the
effect of only running that clause if
`org-link-make-description-function' is not nil. This also means that
in the case where `:insert-description' *and*
`org-link-make-description-function' are both nil then the user won't
get a 2-second error message. Since this configuration is the default
state which ships with org, an error message seems rather odd, so this
is probably a good result!

This also means that if we reach that clause and
`org-link-make-description-function' *is* nil, then the whole `cond'
just returns nil, and so the link inserted with no default
description, which makes sense to me.

> You might even add a test for such configuration (no
> :insert-description is defined).

Good idea. Done in the latest version.

Thanks,

Hugo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-test-ol-tests-for-insert-description-param-when-inse.patch --]
[-- Type: text/x-diff, Size: 4544 bytes --]

From fbe030ad3a2aafd09d491aefb9c56242b7ec669b Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Sat, 16 Jul 2022 19:50:15 +0100
Subject: [PATCH] test-ol: tests for insert-description param when inserting
 links

* test-ol (test-ol-with-link-parameters-as): Convenience macro for
testing.
(test-ol-insert-link-get-desc): Convenience macro for testing.
(test-ol/return-foobar): Convenience function for testing.
(test-ol/insert-link-insert-description): Test for various values of
`:insert-description' in `org-link-parameters' (including
`test-ol/return-foobar').
---
 testing/lisp/test-ol.el | 97 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..a3aa302c3 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -19,6 +19,8 @@
 
 ;;; Code:
 
+(require 'cl-lib)
+
 \f
 ;;; Decode and Encode Links
 
@@ -625,5 +627,100 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  (declare (indent 2))
+  (let (orig-parameters)
+    ;; Copy all keys in `parameters' and their original values to
+    ;; `orig-parameters'.
+    (map-do
+     (lambda (key _) (setq orig-parameters
+                      (plist-put
+                       orig-parameters key
+                       (org-link-get-parameter type key))))
+     parameters)
+    `(unwind-protect
+         ;; Set `parameters' values and execute body.
+         (progn (org-link-set-parameters ,type ,@parameters) ,@body)
+       ;; Restore original values.
+       (apply 'org-link-set-parameters ,type ',orig-parameters))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:insert-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-insert-description', for the case where
+`:insert-description' is a function symbol."
+  "foobar")
+
+(ert-deftest test-ol/insert-link-insert-description ()
+  "Test `:insert-description' parameter handling."
+  ;; String case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar")
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description (lambda (_link-test _desc) "foobar"))
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case.
+  (should
+   (string=
+    "foobar"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'test-ol/return-foobar)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:insert-description' parameter is defined, but doesn't return a
+  ;; string.
+  (should
+   (null
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'ignore)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Description argument should override `:insert-description'.
+  (should
+   (string=
+    "notfoobar"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar")
+      (test-ol-insert-link-get-desc "id:foo-bar" "notfoobar"))))
+  ;; When neither `:insert-description' nor
+  ;; `org-link-make-description-function' is defined, there should be
+  ;; no description
+  (should
+   (null
+    (let ((org-link-make-description-function nil)
+	  (org-link-parameters nil))
+      (test-ol-insert-link-get-desc "id:foo-bar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1


[-- Attachment #3: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 7268 bytes --]

From 4f6868175fc8204a5a1e7fd5a9ae430ff760a96d Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): Add parameter `:insert-description', a
string or a function.
* (org-insert-link): If no description is provided (pre-existing or as
an argument), next option is to use the `:insert-description' (if
non-nil) parameter to generate one.
* (org-link-make-description-function): Add documentation to describe
behaviour of nil return value, like that of `:insert-description'.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types.
A type-parameter is thus a good place to store information on the
default description.
---
 etc/ORG-NEWS |  7 +++++
 lisp/ol.el   | 78 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 35af94f92..86128d100 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -322,6 +322,13 @@ purpose of the variable.  The replacement variable
 accepts =listings= and =verbatim= in place of =t= and =nil= (which
 still work, but are no longer listed as valid options).
 
+*** ~org-link-parameters~ has a new ~:insert-description~ parameter
+
+The value of ~:insert-description~ is used as the initial input when
+prompting for a link description.  It can be a string (used as-is) or
+a function (called with the same arguments as
+~org-make-link-description-function~ to return a string to use).
+
 * Version 9.5
 
 ** Important announcements and breaking changes
diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..f49105e17 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,18 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:insert-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the full link text, and the
+  description generated by `org-insert-link'.  It should return
+  the description to use (this reflects the behaviour of
+  `org-link-make-description-function').  If it returns nil, no
+  default description is used, but no error is thrown (from the
+  user's perspective, this is equivalent to a default description
+  of \"\").
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -200,7 +212,9 @@ You can interactively set the value of this variable by calling
 This function must take two parameters: the first one is the
 link, the second one is the description generated by
 `org-insert-link'.  The function should return the description to
-use."
+use.  If it returns nil, no default description is used, but no
+error is thrown (from the user’s perspective, this is equivalent
+to a default description of \"\")."
   :group 'org-link
   :type '(choice (const nil) (function))
   :safe #'null)
@@ -1804,11 +1818,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:insert-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1835,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1879,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1958,17 +1975,36 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	    (setq desc path)))))
 
     (unless auto-desc
-      (let ((initial-input
-	     (cond
-	      (description)
-	      ((not org-link-make-description-function) desc)
-	      (t (condition-case nil
-		     (funcall org-link-make-description-function link desc)
-		   (error
-		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
-		    (sit-for 2)
-		    nil))))))
+      (let* ((type
+              (cond
+               ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+                (match-string 1 link))
+               ((file-name-absolute-p link) "file")
+               ((string-match "\\`\\.\\.?/" link) "file")))
+             (initial-input
+	      (cond
+	       (description)
+               (desc)
+               ((org-link-get-parameter type :insert-description)
+                (let ((def (org-link-get-parameter type :insert-description)))
+                  (condition-case nil
+                      (cond
+                       ((stringp def) def)
+                       ((functionp def)
+                        (funcall def link desc)))
+                    (error
+                     (message "Can't get link description from org link parameter `:insert-description': %S"
+			      def)
+		     (sit-for 2)
+                     nil))))
+	       (org-link-make-description-function
+                (condition-case nil
+		    (funcall org-link-make-description-function link desc)
+		  (error
+		   (message "Can't get link description from %S"
+		            org-link-make-description-function)
+		   (sit-for 2)
+		   nil))))))
 	(setq desc (if (called-interactively-p 'any)
 		       (read-string "Description: " initial-input)
 		     initial-input))))
-- 
2.20.1

Re: [PATCH v6] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

Hugo,

I am sorry if I am wrong, but I do not see you among known Org 
contributors. You patch is likely greater than it is allowed for 
TINYCHANGE, so before you patch can be committed, copyright assignment 
should be signed, see 
https://orgmode.org/worg/org-contribute.html#copyright for details.

On 18/07/2022 03:59, Hugo Heagren wrote:
>  From fbe030ad3a2aafd09d491aefb9c56242b7ec669b Mon Sep 17 00:00:00 2001
> From: Hugo Heagren<hugo@heagren.com>
> Date: Sat, 16 Jul 2022 19:50:15 +0100
> Subject: [PATCH] test-ol: tests for insert-description param when inserting
>   links

Emacs-26.3:
make test-dirty BTEST_RE=test-ol/insert-link-insert-description

selected tests: test-ol/insert-link-insert-description
Running 1 tests (2022-07-18 12:21:46+0200)
Test test-ol/insert-link-insert-description backtrace:
   signal(wrong-type-argument (listp :insert-description))
   apply(signal (wrong-type-argument (listp :insert-description)))
   (setq value-7565 (apply fn-7563 args-7564))
   (unwind-protect (setq value-7565 (apply fn-7563 args-7564)) (setq fo
   (if (unwind-protect (setq value-7565 (apply fn-7563 args-7564)) (set
   (let (form-description-7567) (if (unwind-protect (setq value-7565 (a
   (let ((value-7565 (quote ert-form-evaluation-aborted-7566))) (let (f
   (let* ((fn-7563 (function signal)) (args-7564 (condition-case err (l
   (closure (t) nil (let* ((fn-7563 (function signal)) (args-7564 (cond
   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
   ert-run-test(#s(ert-test :name test-ol/insert-link-insert-descriptio
   ert-run-or-rerun-test(#s(ert--stats :selector "test-ol/insert-link-i
   ert-run-tests("test-ol/insert-link-insert-description" #f(compiled-f
   ert-run-tests-batch("test-ol/insert-link-insert-description")
   ert-run-tests-batch-and-exit("test-ol/insert-link-insert-description
   (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
   org-test-run-batch-tests("test-ol/insert-link-insert-description")
   eval((org-test-run-batch-tests org-test-select-re))
   command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
   command-line()
   normal-top-level()
Test test-ol/insert-link-insert-description condition:
     (wrong-type-argument listp :insert-description)
    FAILED  1/1  test-ol/insert-link-insert-description

> +(ert-deftest test-ol/insert-link-insert-description ()
> +  "Test `:insert-description' parameter handling."
> +  ;; String case.

The cases might be improved by using different values, so when 
particular `should' form fail it is easier to find it in the code

> +  (should
> +   (string=
> +    "foobar"
> +    (test-ol-with-link-parameters-as
> +        "id" (:insert-description "foobar")

E.g. "foobar-string"

> +      (test-ol-insert-link-get-desc "id:foo-bar"))))
> +  ;; Lambda case.
> +  (should
> +   (string=
> +    "foobar"
> +    (test-ol-with-link-parameters-as
> +        "id" (:insert-description (lambda (_link-test _desc) "foobar"))
> +      (test-ol-insert-link-get-desc "id:foo-bar"))))

"foobar-lambda"

Further "foobar-desc-arg", etc.

> +`:insert-description'
> +
> +  String or function used as a default when prompting users for a
> +  link's description.  A string is used as-is, a function is
> +  called with two arguments: the full link text, and the

"link text" might be a bit ambiguous here. I would consider "link 
location", "string containing link type and target", or something else.

> +  description generated by `org-insert-link'.  It should return
> +  the description to use (this reflects the behaviour of
> +  `org-link-make-description-function').  If it returns nil, no
> +  default description is used, but no error is thrown (from the
> +  user's perspective, this is equivalent to a default description
> +  of \"\").

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On 18/07/2022 17:55, Max Nikulin wrote:

> I am sorry if I am wrong, but I do not see you among known Org, 
> contributors. You patch is likely greater than it is allowed for
> TINYCHANGE, so before you patch can be committed, copyright assignment
> should be signed, see
> <https://orgmode.org/worg/org-contribute.html#copyright> for details.

I've already assigned copyright to the FSF for work on Emacs (see
attached certificate). Do I need to do anything else for org-mode in
particular?

> The cases might be improved by using different values, so when
> particular `should' form fail it is easier to find it in the code

Nice idea. Done.

> "link text" might be a bit ambiguous here. I would consider "link
> location", "string containing link type and target", or something
> else.

Clarified, and added some examples in the docstring as well. You're
right about this---I've found such language ambiguous in the past.

> Emacs-26.3:
> make test-dirty BTEST_RE=test-ol/insert-link-insert-description
> [failing test]

Hmmmm. I ran:

,----
| make test-dirty BTEST_RE=test-ol/insert-link-insert-description
`----

at the top-level of the org dir on my machine, and I got:

,----
| Running 1 tests (2022-07-21 12:55:40+0100, selector ‘"test-ol/insert-link-insert-description"’)
|    passed  1/1  test-ol/insert-link-insert-description (0.016860 sec)
| 
| Ran 1 tests, 1 results as expected, 0 unexpected (2022-07-21 12:55:40+0100, 0.017086 sec)
| 
| /usr/bin/make cleantest
| make[1]: Entering directory '/home/hugo/.config/emacs/straight/repos/org'
| rm -fr /tmp/tmp-orgtest || { \
|   find /tmp/tmp-orgtest -type d -exec chmod u+w {} + && \
|   rm -fr /tmp/tmp-orgtest ; \
| }
| make[1]: Leaving directory '/home/hugo/.config/emacs/straight/repos/org'
`----

So the test passes on my machine.

Are you sure you're using the most recent version of the patch? The
quote from me you used is dated from the 16th, which was at least one
version ago. Could you retest with the version attached to this email?

If it still fails, could you try isolating whether it's a specific
`should' clause which is the problem, if it fails with any of them?

Thanks,

Hugo

[-- Attachment #2: FSF-assignment.pdf --]
[-- Type: application/pdf, Size: 366616 bytes --]

[-- Attachment #3: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 7353 bytes --]

From 039c2131462f5454f2804e809e085a055b5bf552 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH 1/2] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): Add parameter `:insert-description', a
string or a function.
* (org-insert-link): If no description is provided (pre-existing or as
an argument), next option is to use the `:insert-description' (if
non-nil) parameter to generate one.
* (org-link-make-description-function): Add documentation to describe
behaviour of nil return value, like that of `:insert-description'.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types.
A type-parameter is thus a good place to store information on the
default description.
---
 etc/ORG-NEWS |  7 +++++
 lisp/ol.el   | 79 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 35af94f92..86128d100 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -322,6 +322,13 @@ purpose of the variable.  The replacement variable
 accepts =listings= and =verbatim= in place of =t= and =nil= (which
 still work, but are no longer listed as valid options).
 
+*** ~org-link-parameters~ has a new ~:insert-description~ parameter
+
+The value of ~:insert-description~ is used as the initial input when
+prompting for a link description.  It can be a string (used as-is) or
+a function (called with the same arguments as
+~org-make-link-description-function~ to return a string to use).
+
 * Version 9.5
 
 ** Important announcements and breaking changes
diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..abea2af5e 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,19 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:insert-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the link location (a string such as
+  \"~/foobar\", \"id:some-org-id\" or \"https://www.foo.com\")
+  and the description generated by `org-insert-link'.  It should
+  return the description to use (this reflects the behaviour of
+  `org-link-make-description-function').  If it returns nil, no
+  default description is used, but no error is thrown (from the
+  user's perspective, this is equivalent to a default description
+  of \"\").
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -200,7 +213,9 @@ You can interactively set the value of this variable by calling
 This function must take two parameters: the first one is the
 link, the second one is the description generated by
 `org-insert-link'.  The function should return the description to
-use."
+use.  If it returns nil, no default description is used, but no
+error is thrown (from the user’s perspective, this is equivalent
+to a default description of \"\")."
   :group 'org-link
   :type '(choice (const nil) (function))
   :safe #'null)
@@ -1804,11 +1819,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:insert-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1836,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1880,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1958,17 +1976,36 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	    (setq desc path)))))
 
     (unless auto-desc
-      (let ((initial-input
-	     (cond
-	      (description)
-	      ((not org-link-make-description-function) desc)
-	      (t (condition-case nil
-		     (funcall org-link-make-description-function link desc)
-		   (error
-		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
-		    (sit-for 2)
-		    nil))))))
+      (let* ((type
+              (cond
+               ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+                (match-string 1 link))
+               ((file-name-absolute-p link) "file")
+               ((string-match "\\`\\.\\.?/" link) "file")))
+             (initial-input
+	      (cond
+	       (description)
+               (desc)
+               ((org-link-get-parameter type :insert-description)
+                (let ((def (org-link-get-parameter type :insert-description)))
+                  (condition-case nil
+                      (cond
+                       ((stringp def) def)
+                       ((functionp def)
+                        (funcall def link desc)))
+                    (error
+                     (message "Can't get link description from org link parameter `:insert-description': %S"
+			      def)
+		     (sit-for 2)
+                     nil))))
+	       (org-link-make-description-function
+                (condition-case nil
+		    (funcall org-link-make-description-function link desc)
+		  (error
+		   (message "Can't get link description from %S"
+		            org-link-make-description-function)
+		   (sit-for 2)
+		   nil))))))
 	(setq desc (if (called-interactively-p 'any)
 		       (read-string "Description: " initial-input)
 		     initial-input))))
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-test-ol-tests-for-insert-description-param-when-inse.patch --]
[-- Type: text/x-diff, Size: 4528 bytes --]

From fbf29e47436e7ca8dcf091f4ebcccd7a6a65027c Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Sat, 16 Jul 2022 19:50:15 +0100
Subject: [PATCH 2/2] test-ol: tests for insert-description param when
 inserting links

* test-ol (test-ol-with-link-parameters-as): Convenience macro for
testing.
(test-ol-insert-link-get-desc): Convenience macro for testing.
(test-ol/return-foobar): Convenience function for testing.
(test-ol/insert-link-insert-description): Test for various values of
`:insert-description' in `org-link-parameters' (including
`test-ol/return-foobar').
---
 testing/lisp/test-ol.el | 95 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..608e38257 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -625,5 +625,100 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  (declare (indent 2))
+  (let (orig-parameters)
+    ;; Copy all keys in `parameters' and their original values to
+    ;; `orig-parameters'.
+    (map-do
+     (lambda (key _) (setq orig-parameters
+                      (plist-put
+                       orig-parameters key
+                       (org-link-get-parameter type key))))
+     parameters)
+    `(unwind-protect
+         ;; Set `parameters' values and execute body.
+         (progn (org-link-set-parameters ,type ,@parameters) ,@body)
+       ;; Restore original values.
+       (apply 'org-link-set-parameters ,type ',orig-parameters))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:insert-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-insert-description', for the case where
+`:insert-description' is a function symbol."
+  "foobar-from-function")
+
+(ert-deftest test-ol/insert-link-insert-description ()
+  "Test `:insert-description' parameter handling."
+  ;; String case.
+  (should
+   (string=
+    "foobar-string"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar-string")
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case.
+  (should
+   (string=
+    "foobar-lambda"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description (lambda (_link-test _desc) "foobar-lambda"))
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case.
+  (should
+   (string=
+    "foobar-from-function"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'test-ol/return-foobar)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:insert-description' parameter is defined, but doesn't return a
+  ;; string.
+  (should
+   (null
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'ignore)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Description argument should override `:insert-description'.
+  (should
+   (string=
+    "foobar-desc-arg"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar")
+      (test-ol-insert-link-get-desc "id:foo-bar" "foobar-desc-arg"))))
+  ;; When neither `:insert-description' nor
+  ;; `org-link-make-description-function' is defined, there should be
+  ;; no description
+  (should
+   (null
+    (let ((org-link-make-description-function nil)
+	  (org-link-parameters nil))
+      (test-ol-insert-link-get-desc "id:foo-bar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

On 23/07/2022 14:48, Hugo Heagren wrote:
> 
> I've already assigned copyright to the FSF for work on Emacs (see
> attached certificate). Do I need to do anything else for org-mode in
> particular?

Great, I hope, it is enough, but my answer is not authoritative.

>> Emacs-26.3:
>> make test-dirty BTEST_RE=test-ol/insert-link-insert-description
>> [failing test]
> 
> If it still fails, could you try isolating whether it's a specific
> `should' clause which is the problem, if it fails with any of them?

It is `map-do' that causes the problem. Maybe earlier implementation in 
Emacs-26 has some peculiarities. It is the version available from the 
system repository for Ubuntu-20.04 LTS focal.

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> If it still fails, could you try isolating whether it's a specific
>> `should' clause which is the problem, if it fails with any of them?
>
> It is `map-do' that causes the problem. Maybe earlier implementation in 
> Emacs-26 has some peculiarities. It is the version available from the 
> system repository for Ubuntu-20.04 LTS focal.

Tests do not fail on my side using Emacs 26.3

Best,
Ihor

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

On 23/07/2022 20:06, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>>> If it still fails, could you try isolating whether it's a specific
>>> `should' clause which is the problem, if it fails with any of them?
>>
>> It is `map-do' that causes the problem. Maybe earlier implementation in
>> Emacs-26 has some peculiarities. It is the version available from the
>> system repository for Ubuntu-20.04 LTS focal.
> 
> Tests do not fail on my side using Emacs 26.3

It is strange. I tried the latest 2 patches applied on the top of

127e7fee4 2022-07-23 22:16:54 +0800 Ihor Radchenko: ox-latex: Keep 
obsolete variable values removed in 97cfb959d

and I got the same failure as a week ago. Maybe I will try to debug 
`map-do' tomorrow. Previous time my impression was that map-do was 
completely broken. I never used it before, but I assumed that it is used 
in the patch in a proper way. I do not expect that the cause is minimal 
testing environment in a LXC container (e.g. missing XDG directories).

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

On 23/07/2022 20:06, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>>> If it still fails, could you try isolating whether it's a specific
>>> `should' clause which is the problem, if it fails with any of them?
>>
>> It is `map-do' that causes the problem. Maybe earlier implementation in
>> Emacs-26 has some peculiarities. It is the version available from the
>> system repository for Ubuntu-20.04 LTS focal.
> 
> Tests do not fail on my side using Emacs 26.3

Ihor, what is the origin of your Emacs-26.3? The patch uses `map-do' 
with plist argument. I have tried:

test-map-do.el
---- >8 ----
(require 'map)
(map-do (lambda (k v) (message "alist map-do: %S: %S" k v)) '((1 . 'a)))
(setq pl nil)
(setq pl (plist-put pl :a 'a))
(message "plist: %S" pl)
(map-do (lambda (k v) (message "plist map-do %S: %S" k v)) pl)
---- 8< ----

Emacs-26.3: failure
emacs -Q --batch -l test-map-do.el
alist map-do: 1: (quote a)
plist: (:a a)
Wrong type argument: listp, :a

Emacs-27: works
emacs -Q --batch -l test-map-do.el
alist map-do: 1: 'a
plist: (:a a)
plist map-do :a: a

It is consistent with
f68f2eb472 2018-12-20 08:40:43 -0500 Stefan Monnier: * 
lisp/emacs-lisp/map.el: Add support for plists

I see no equivalent change in emacs-26 branch and emacs-26.3 tag.

So I am puzzled how tests using a macro with map-do and plist may pass 
under Emacs-26.

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>>> It is `map-do' that causes the problem. Maybe earlier implementation in
>>> Emacs-26 has some peculiarities. It is the version available from the
>>> system repository for Ubuntu-20.04 LTS focal.
>> 
>> Tests do not fail on my side using Emacs 26.3
>
> Ihor, what is the origin of your Emacs-26.3? The patch uses `map-do' 
> with plist argument. I have tried:
>
> test-map-do.el
> ---- >8 ----
> (require 'map)
> (map-do (lambda (k v) (message "alist map-do: %S: %S" k v)) '((1 . 'a)))
> (setq pl nil)
> (setq pl (plist-put pl :a 'a))
> (message "plist: %S" pl)
> (map-do (lambda (k v) (message "plist map-do %S: %S" k v)) pl)
> ---- 8< ----

It also fails on my side.

Yet,
make test EMACS="emacs-26" does _not_ fail.

I tried harder then and found that
make test EMACS="emacs-26" BTEST_RE="test-ol/insert-link-insert-description"
_does_ fail.

> So I am puzzled how tests using a macro with map-do and plist may pass 
> under Emacs-26.

I am also puzzled now. Running all the tests works, while running a
single test does not.

In any case, we cannot, unfortunately, use map-do here and should
probably fall back to the previous version with cl-loop.

Best,
Ihor

Re: [PATCH v8] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

> In any case, we cannot, unfortunately, use map-do here and should
> probably fall back to the previous version with cl-loop.

You're right, this is strange. Oh well, I've moved back to the version
of the macro which used `cl-loop' in the current patch. Nothing else
is changed. All the tests pass for me.

Thanks, Hugo

[-- Attachment #2: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 7353 bytes --]

From 039c2131462f5454f2804e809e085a055b5bf552 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH 1/2] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): Add parameter `:insert-description', a
string or a function.
* (org-insert-link): If no description is provided (pre-existing or as
an argument), next option is to use the `:insert-description' (if
non-nil) parameter to generate one.
* (org-link-make-description-function): Add documentation to describe
behaviour of nil return value, like that of `:insert-description'.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types.
A type-parameter is thus a good place to store information on the
default description.
---
 etc/ORG-NEWS |  7 +++++
 lisp/ol.el   | 79 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 35af94f92..86128d100 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -322,6 +322,13 @@ purpose of the variable.  The replacement variable
 accepts =listings= and =verbatim= in place of =t= and =nil= (which
 still work, but are no longer listed as valid options).
 
+*** ~org-link-parameters~ has a new ~:insert-description~ parameter
+
+The value of ~:insert-description~ is used as the initial input when
+prompting for a link description.  It can be a string (used as-is) or
+a function (called with the same arguments as
+~org-make-link-description-function~ to return a string to use).
+
 * Version 9.5
 
 ** Important announcements and breaking changes
diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..abea2af5e 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,19 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:insert-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the link location (a string such as
+  \"~/foobar\", \"id:some-org-id\" or \"https://www.foo.com\")
+  and the description generated by `org-insert-link'.  It should
+  return the description to use (this reflects the behaviour of
+  `org-link-make-description-function').  If it returns nil, no
+  default description is used, but no error is thrown (from the
+  user's perspective, this is equivalent to a default description
+  of \"\").
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -200,7 +213,9 @@ You can interactively set the value of this variable by calling
 This function must take two parameters: the first one is the
 link, the second one is the description generated by
 `org-insert-link'.  The function should return the description to
-use."
+use.  If it returns nil, no default description is used, but no
+error is thrown (from the user’s perspective, this is equivalent
+to a default description of \"\")."
   :group 'org-link
   :type '(choice (const nil) (function))
   :safe #'null)
@@ -1804,11 +1819,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:insert-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1836,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1880,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1958,17 +1976,36 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	    (setq desc path)))))
 
     (unless auto-desc
-      (let ((initial-input
-	     (cond
-	      (description)
-	      ((not org-link-make-description-function) desc)
-	      (t (condition-case nil
-		     (funcall org-link-make-description-function link desc)
-		   (error
-		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
-		    (sit-for 2)
-		    nil))))))
+      (let* ((type
+              (cond
+               ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+                (match-string 1 link))
+               ((file-name-absolute-p link) "file")
+               ((string-match "\\`\\.\\.?/" link) "file")))
+             (initial-input
+	      (cond
+	       (description)
+               (desc)
+               ((org-link-get-parameter type :insert-description)
+                (let ((def (org-link-get-parameter type :insert-description)))
+                  (condition-case nil
+                      (cond
+                       ((stringp def) def)
+                       ((functionp def)
+                        (funcall def link desc)))
+                    (error
+                     (message "Can't get link description from org link parameter `:insert-description': %S"
+			      def)
+		     (sit-for 2)
+                     nil))))
+	       (org-link-make-description-function
+                (condition-case nil
+		    (funcall org-link-make-description-function link desc)
+		  (error
+		   (message "Can't get link description from %S"
+		            org-link-make-description-function)
+		   (sit-for 2)
+		   nil))))))
 	(setq desc (if (called-interactively-p 'any)
 		       (read-string "Description: " initial-input)
 		     initial-input))))
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-test-ol-tests-for-insert-description-param-when-inse.patch --]
[-- Type: text/x-diff, Size: 4582 bytes --]

From 6a6c7a1084e510a510a50226840ed20593079e10 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Sat, 16 Jul 2022 19:50:15 +0100
Subject: [PATCH 2/2] test-ol: tests for insert-description param when
 inserting links

* test-ol (test-ol-with-link-parameters-as): Convenience macro for
testing.
(test-ol-insert-link-get-desc): Convenience macro for testing.
(test-ol/return-foobar): Convenience function for testing.
(test-ol/insert-link-insert-description): Test for various values of
`:insert-description' in `org-link-parameters' (including
`test-ol/return-foobar').
---
 testing/lisp/test-ol.el | 94 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..0f2f2e35c 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -19,6 +19,8 @@
 
 ;;; Code:
 
+(require 'cl-lib)
+
 \f
 ;;; Decode and Encode Links
 
@@ -625,5 +627,97 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  (declare (indent 2))
+  (let (orig-parameters)
+    ;; Copy all keys in `parameters' and their original values to
+    ;; `orig-parameters'.
+    (cl-loop for param in parameters by 'cddr
+             do (setq orig-parameters
+                      (plist-put orig-parameters param (org-link-get-parameter type param))))
+    `(unwind-protect
+         ;; Set `parameters' values and execute body.
+         (progn (org-link-set-parameters ,type ,@parameters) ,@body)
+       ;; Restore original values.
+       (apply 'org-link-set-parameters ,type ',orig-parameters))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:insert-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-insert-description', for the case where
+`:insert-description' is a function symbol."
+  "foobar-from-function")
+
+(ert-deftest test-ol/insert-link-insert-description ()
+  "Test `:insert-description' parameter handling."
+  ;; String case.
+  (should
+   (string=
+    "foobar-string"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar-string")
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case.
+  (should
+   (string=
+    "foobar-lambda"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description (lambda (_link-test _desc) "foobar-lambda"))
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case.
+  (should
+   (string=
+    "foobar-from-function"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'test-ol/return-foobar)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:insert-description' parameter is defined, but doesn't return a
+  ;; string.
+  (should
+   (null
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'ignore)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Description argument should override `:insert-description'.
+  (should
+   (string=
+    "foobar-desc-arg"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar")
+      (test-ol-insert-link-get-desc "id:foo-bar" "foobar-desc-arg"))))
+  ;; When neither `:insert-description' nor
+  ;; `org-link-make-description-function' is defined, there should be
+  ;; no description
+  (should
+   (null
+    (let ((org-link-make-description-function nil)
+	  (org-link-parameters nil))
+      (test-ol-insert-link-get-desc "id:foo-bar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1

Re: [PATCH v8] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

On 25/07/2022 18:55, Hugo Heagren wrote:
>> In any case, we cannot, unfortunately, use map-do here and should
>> probably fall back to the previous version with cl-loop.
> 
> You're right, this is strange. Oh well, I've moved back to the version
> of the macro which used `cl-loop' in the current patch. Nothing else
> is changed. All the tests pass for me.

Hugo, almost certainly you are tired by so many iterations, but I still 
can not approve your patch. It is again Emacs-26 and likely the last 
`should' form with (org-link-parameters nil). I hope, you have not lost 
motivation yet and you will fix this test failure.

Test test-ol/insert-link-insert-description backtrace:
   signal(error ("rx form ‘or’ requires at least 1 args"))
   apply(signal (error ("rx form ‘or’ requires at least 1 args")))
   (setq value-7590 (apply fn-7588 args-7589))
   (unwind-protect (setq value-7590 (apply fn-7588 args-7589)) (setq fo
   (if (unwind-protect (setq value-7590 (apply fn-7588 args-7589)) (set
   (let (form-description-7592) (if (unwind-protect (setq value-7590 (a
   (let ((value-7590 (quote ert-form-evaluation-aborted-7591))) (let (f
   (let* ((fn-7588 (function null)) (args-7589 (condition-case err (let
   (closure (t) nil (let* ((fn-7563 (function string=)) (args-7564 (con
   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
   ert-run-test(#s(ert-test :name test-ol/insert-link-insert-descriptio
   ert-run-or-rerun-test(#s(ert--stats :selector "test-ol/" :tests [#s(
   ert-run-tests("test-ol/" #f(compiled-function (event-type &rest even
   ert-run-tests-batch("test-ol/")
   ert-run-tests-batch-and-exit("test-ol/")
   (let ((org-id-track-globally t) (org-test-selector (if org-test-sele
   org-test-run-batch-tests("test-ol/")
   eval((org-test-run-batch-tests org-test-select-re))
   command-line-1(("--eval" "(setq vc-handled-backends nil org-startup-
   command-line()
   normal-top-level()
Test test-ol/insert-link-insert-description condition:
     (error "rx form ‘or’ requires at least 1 args")
    FAILED   5/13  test-ol/insert-link-insert-description

Re: [PATCH v9] ol.el: add description format parameter to org-link-parameters

[Hugo Heagren]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

> Hugo, almost certainly you are tired by so many iterations, but I
> still can not approve your patch.

No worries! I has to be right before it can be merged. I wouldn't want
to submit faulty code. And besides, debugging today makes my code
tomorrow better right? This was an interesting case.


> signal(error ("rx form ‘or’ requires at least 1 args"))
> apply(signal (error ("rx form ‘or’ requires at least 1 args")))

The test fails because of an error in `rx-to-string', which is called
by `org-insert-link'. It was failing because I have the following
code:

,----
| (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":"))
`----

> It is again Emacs-26

In the final test case, `all-prefixes' is nil at this point, and
before Emacs 27.1, the rx form `or' /required/ arguments (it no longer
does).

So we need to fix the last case so that `or' has some arguments
(`all-prefixes' is non-nil).

The point of the last case is to test the behaviour when there is no
relevant parameter for the link type. I /had/ done this by removing
all the link parameters, but this makes `all-prefixes' nil, so we
can't do that.

We could just as easily do it by leaving the parameters as they are,
and using a link 'type' which is definitely not in the list. I have
taken this approach in the new version of the patch. I've used
"fake-link-type", which will surely not be used, even in anyone's
strange personal config. Admittedly it /could/ be used though (it
would be possible to add it if someone wanted), so if you'd rather, I
can develop something which uses a fake link type which is /by
definition/ not in `org-link-parameters', it would just be rather a
lot more work and the test case might subsequently be less clear to
understand.

Hope that helps -- do the tests pass for you now?

Hugo

[-- Attachment #2: 0001-ol.el-add-description-format-parameter-to-org-link-p.patch --]
[-- Type: text/x-diff, Size: 7353 bytes --]

From 039c2131462f5454f2804e809e085a055b5bf552 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Mon, 28 Mar 2022 23:18:45 +0100
Subject: [PATCH 1/2] ol.el: add description format parameter to
 org-link-parameters

* ol.el (org-link-parameters): Add parameter `:insert-description', a
string or a function.
* (org-insert-link): If no description is provided (pre-existing or as
an argument), next option is to use the `:insert-description' (if
non-nil) parameter to generate one.
* (org-link-make-description-function): Add documentation to describe
behaviour of nil return value, like that of `:insert-description'.

Default descriptions are predictable within a link type, but because
link types are quite diverse, are NOT predictable across many types.
A type-parameter is thus a good place to store information on the
default description.
---
 etc/ORG-NEWS |  7 +++++
 lisp/ol.el   | 79 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 35af94f92..86128d100 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -322,6 +322,13 @@ purpose of the variable.  The replacement variable
 accepts =listings= and =verbatim= in place of =t= and =nil= (which
 still work, but are no longer listed as valid options).
 
+*** ~org-link-parameters~ has a new ~:insert-description~ parameter
+
+The value of ~:insert-description~ is used as the initial input when
+prompting for a link description.  It can be a string (used as-is) or
+a function (called with the same arguments as
+~org-make-link-description-function~ to return a string to use).
+
 * Version 9.5
 
 ** Important announcements and breaking changes
diff --git a/lisp/ol.el b/lisp/ol.el
index d4bd0e40c..abea2af5e 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -141,6 +141,19 @@ link.
   Function that inserts a link with completion.  The function
   takes one optional prefix argument.
 
+`:insert-description'
+
+  String or function used as a default when prompting users for a
+  link's description.  A string is used as-is, a function is
+  called with two arguments: the link location (a string such as
+  \"~/foobar\", \"id:some-org-id\" or \"https://www.foo.com\")
+  and the description generated by `org-insert-link'.  It should
+  return the description to use (this reflects the behaviour of
+  `org-link-make-description-function').  If it returns nil, no
+  default description is used, but no error is thrown (from the
+  user's perspective, this is equivalent to a default description
+  of \"\").
+
 `:display'
 
   Value for `invisible' text property on the hidden parts of the
@@ -200,7 +213,9 @@ You can interactively set the value of this variable by calling
 This function must take two parameters: the first one is the
 link, the second one is the description generated by
 `org-insert-link'.  The function should return the description to
-use."
+use.  If it returns nil, no default description is used, but no
+error is thrown (from the user’s perspective, this is equivalent
+to a default description of \"\")."
   :group 'org-link
   :type '(choice (const nil) (function))
   :safe #'null)
@@ -1804,11 +1819,14 @@ prefix negates `org-link-keep-stored-after-insertion'.
 If the LINK-LOCATION parameter is non-nil, this value will be used as
 the link location instead of reading one interactively.
 
-If the DESCRIPTION parameter is non-nil, this value will be used as the
-default description.  Otherwise, if `org-link-make-description-function'
-is non-nil, this function will be called with the link target, and the
-result will be the default link description.  When called non-interactively,
-don't allow to edit the default description."
+If the DESCRIPTION parameter is non-nil, this value will be used
+as the default description.  If not, and the chosen link type has
+a non-nil `:insert-description' parameter, that is used to
+generate a description as described in `org-link-parameters'
+docstring. Otherwise, if `org-link-make-description-function' is
+non-nil, this function will be called with the link target, and
+the result will be the default link description.  When called
+non-interactively, don't allow to edit the default description."
   (interactive "P")
   (let* ((wcf (current-window-configuration))
 	 (origbuf (current-buffer))
@@ -1818,7 +1836,10 @@ don't allow to edit the default description."
 	 (desc region)
 	 (link link-location)
 	 (abbrevs org-link-abbrev-alist-local)
-	 entry all-prefixes auto-desc)
+	 (all-prefixes (append (mapcar #'car abbrevs)
+			       (mapcar #'car org-link-abbrev-alist)
+			       (org-link-types)))
+         entry auto-desc)
     (cond
      (link-location)		      ; specified by arg, just use it.
      ((org-in-regexp org-link-bracket-re 1)
@@ -1859,9 +1880,6 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	  (unless (pos-visible-in-window-p (point-max))
 	    (org-fit-window-to-buffer))
 	  (and (window-live-p cw) (select-window cw))))
-      (setq all-prefixes (append (mapcar #'car abbrevs)
-				 (mapcar #'car org-link-abbrev-alist)
-				 (org-link-types)))
       (unwind-protect
 	  ;; Fake a link history, containing the stored links.
 	  (let ((org-link--history
@@ -1958,17 +1976,36 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 	    (setq desc path)))))
 
     (unless auto-desc
-      (let ((initial-input
-	     (cond
-	      (description)
-	      ((not org-link-make-description-function) desc)
-	      (t (condition-case nil
-		     (funcall org-link-make-description-function link desc)
-		   (error
-		    (message "Can't get link description from %S"
-			     (symbol-name org-link-make-description-function))
-		    (sit-for 2)
-		    nil))))))
+      (let* ((type
+              (cond
+               ((string-match (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":")) link)
+                (match-string 1 link))
+               ((file-name-absolute-p link) "file")
+               ((string-match "\\`\\.\\.?/" link) "file")))
+             (initial-input
+	      (cond
+	       (description)
+               (desc)
+               ((org-link-get-parameter type :insert-description)
+                (let ((def (org-link-get-parameter type :insert-description)))
+                  (condition-case nil
+                      (cond
+                       ((stringp def) def)
+                       ((functionp def)
+                        (funcall def link desc)))
+                    (error
+                     (message "Can't get link description from org link parameter `:insert-description': %S"
+			      def)
+		     (sit-for 2)
+                     nil))))
+	       (org-link-make-description-function
+                (condition-case nil
+		    (funcall org-link-make-description-function link desc)
+		  (error
+		   (message "Can't get link description from %S"
+		            org-link-make-description-function)
+		   (sit-for 2)
+		   nil))))))
 	(setq desc (if (called-interactively-p 'any)
 		       (read-string "Description: " initial-input)
 		     initial-input))))
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-test-ol-tests-for-insert-description-param-when-inse.patch --]
[-- Type: text/x-diff, Size: 4564 bytes --]

From 708916f8d4f31af1e786e87154b5a4479d0ed1b8 Mon Sep 17 00:00:00 2001
From: Hugo Heagren <hugo@heagren.com>
Date: Sat, 16 Jul 2022 19:50:15 +0100
Subject: [PATCH 2/2] test-ol: tests for insert-description param when
 inserting links

* test-ol (test-ol-with-link-parameters-as): Convenience macro for
testing.
(test-ol-insert-link-get-desc): Convenience macro for testing.
(test-ol/return-foobar): Convenience function for testing.
(test-ol/insert-link-insert-description): Test for various values of
`:insert-description' in `org-link-parameters' (including
`test-ol/return-foobar').
---
 testing/lisp/test-ol.el | 93 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 429bb52ee..ac01c21eb 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -19,6 +19,8 @@
 
 ;;; Code:
 
+(require 'cl-lib)
+
 \f
 ;;; Decode and Encode Links
 
@@ -625,5 +627,96 @@ See https://github.com/yantar92/org/issues/4."
     (test-ol-parse-link-in-text
         "The <point>http://foo.com/(something)?after=parens link"))))
 
+;;; Insert Links
+
+(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
+  "Pass TYPE/PARAMETERS to `org-link-parameters' and execute BODY.
+
+Save the original value of `org-link-parameters', execute
+`org-link-set-parameters' with the relevant args, execute BODY
+and restore `org-link-parameters'.
+
+TYPE is as in `org-link-set-parameters'.  PARAMETERS is a plist to
+be passed to `org-link-set-parameters'."
+  (declare (indent 2))
+  (let (orig-parameters)
+    ;; Copy all keys in `parameters' and their original values to
+    ;; `orig-parameters'.
+    (cl-loop for param in parameters by 'cddr
+             do (setq orig-parameters
+                      (plist-put orig-parameters param (org-link-get-parameter type param))))
+    `(unwind-protect
+         ;; Set `parameters' values and execute body.
+         (progn (org-link-set-parameters ,type ,@parameters) ,@body)
+       ;; Restore original values.
+       (apply 'org-link-set-parameters ,type ',orig-parameters))))
+
+(defun test-ol-insert-link-get-desc (&optional link-location description)
+  "Insert link in temp buffer, return description.
+
+LINK-LOCATION and DESCRIPTION are passed to
+`org-insert-link' (COMPLETE-FILE is always nil)."
+  (org-test-with-temp-text ""
+    (org-insert-link nil link-location description)
+    (save-match-data
+      (when (and
+             (org-in-regexp org-link-bracket-re 1)
+             (match-end 2))
+        (match-string-no-properties 2)))))
+
+(defun test-ol/return-foobar (_link-test _desc)
+  "Return string \"foobar\".
+
+Take (and ignore) arguments conforming to `:insert-description'
+API in `org-link-parameters'.  Used in test
+`test-ol/insert-link-insert-description', for the case where
+`:insert-description' is a function symbol."
+  "foobar-from-function")
+
+(ert-deftest test-ol/insert-link-insert-description ()
+  "Test `:insert-description' parameter handling."
+  ;; String case.
+  (should
+   (string=
+    "foobar-string"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar-string")
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Lambda case.
+  (should
+   (string=
+    "foobar-lambda"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description (lambda (_link-test _desc) "foobar-lambda"))
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Function symbol case.
+  (should
+   (string=
+    "foobar-from-function"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'test-ol/return-foobar)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; `:insert-description' parameter is defined, but doesn't return a
+  ;; string.
+  (should
+   (null
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description #'ignore)
+      (test-ol-insert-link-get-desc "id:foo-bar"))))
+  ;; Description argument should override `:insert-description'.
+  (should
+   (string=
+    "foobar-desc-arg"
+    (test-ol-with-link-parameters-as
+        "id" (:insert-description "foobar")
+      (test-ol-insert-link-get-desc "id:foo-bar" "foobar-desc-arg"))))
+  ;; When neither `:insert-description' nor
+  ;; `org-link-make-description-function' is defined, there should be
+  ;; no description
+  (should
+   (null
+    (let ((org-link-make-description-function nil))
+      (test-ol-insert-link-get-desc "fake-link-type:foo-bar")))))
+
 (provide 'test-ol)
 ;;; test-ol.el ends here
-- 
2.20.1

Re: [PATCH v9] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

> The test fails because of an error in `rx-to-string', which is called
> by `org-insert-link'. It was failing because I have the following
> code:
>
> ,----
> | (rx-to-string `(: string-start (submatch (or ,@all-prefixes)) ":"))
> `----
> ...
> We could just as easily do it by leaving the parameters as they are,
> and using a link 'type' which is definitely not in the list. I have
> taken this approach in the new version of the patch. I've used
> "fake-link-type", which will surely not be used, even in anyone's
> strange personal config. Admittedly it /could/ be used though (it
> would be possible to add it if someone wanted), so if you'd rather, I
> can develop something which uses a fake link type which is /by
> definition/ not in `org-link-parameters', it would just be rather a
> lot more work and the test case might subsequently be less clear to
> understand.
>
> Hope that helps -- do the tests pass for you now?

The tests are passing on my side.

However, even though you fixed the tests, you did nothing to fix the
actual problem revealed by the tests. The rx-to-string call may still
suffer from the described edge case. Why not simply shield the
rx-to-string call with (and all-prefixes ...)? I'd leave the previous
version of the tests as they had a benefit of testing this edge case.

Best,
Ihor

Re: [PATCH v9] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Adding Org ML back to CC.

Hugo Heagren <hugo@heagren.com> writes:

>> The rx-to-string call may still suffer from the described edge case.
>
> Yes, I think this is a good idea.
>
>> (and all-prefixes ...)
>
> I'm not sure what you mean, but I don't think it will work.

I mean

(and ,all-prefixes (rx-to-string `(: string-start (submatch (or (and ,@all-prefixes))) ":")))

Best,
Ihor

[PATCH] ol-info: Enable :insert-description feature

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

On 30/07/2022 15:02, Ihor Radchenko wrote:
> 
> Hugo Heagren writes:
> 
>>> The rx-to-string call may still suffer from the described edge case.
>>
>> Yes, I think this is a good idea.
>>
>>> (and all-prefixes ...)
>>
>> I'm not sure what you mean, but I don't think it will work.
> 
> I mean
> 
> (and ,all-prefixes (rx-to-string `(: string-start (submatch (or (and ,@all-prefixes))) ":")))

I agree that it is better than the trick with test, but, I think, it is 
not critical taking into account version of the patch set. I do not see 
any issues that really prevent committing of changes. This time tests 
pass on my side.

When committing the current version or if you decided to prepare a new 
one, please, fix the name of the test. Ihor committed my patch to run ol 
tests, the variant that renames test instead of adjustment of selector.

diff --git a/testing/lisp/test-ol.el b/testing/lisp/test-ol.el
index 5f2cca538..6802f0333 100644
--- a/testing/lisp/test-ol.el
+++ b/testing/lisp/test-ol.el
@@ -673,7 +673,7 @@ API in `org-link-parameters'.  Used in test
  `:insert-description' is a function symbol."
    "foobar-from-function")

-(ert-deftest test-ol/insert-link-insert-description ()
+(ert-deftest test-org-link/insert-link-insert-description ()
    "Test `:insert-description' parameter handling."
    ;; String case.
    (should

To get impression of the new feature in action, I modified ol-info.el. 
First patch does not depend on :insert-description and might be useful 
per se. Second one just enables the feature.

[-- Attachment #2: 0001-ol-info-New-function-to-generate-description.patch --]
[-- Type: text/x-patch, Size: 9053 bytes --]

From 7d46a872ea653c3a64522463fe3f35412ac6753e Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sat, 30 Jul 2022 19:13:01 +0700
Subject: [PATCH 1/2] ol-info: New function to generate description

* lisp/ol-info.el (org-info-link-file-node): New helper to parse info
link info file (manual) name and node.
(org-info-description-as-command): New function to create description
for info links that may executed to view a manual.
(org-info-follow-link, org-info-export): Use `org-info-link-file-node'.
* testing/lisp/test-ol-info.el (test-org-link-info/link-file-node)
(test-org-link-info/description-as-command): New file for tests of new
functions `org-info-link-file-node', `org-info-description-as-command'.

Prepare to :insert-description new feature of `org-link'.
---
 lisp/ol-info.el              | 80 ++++++++++++++++++++++++++-------
 testing/lisp/test-ol-info.el | 87 ++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+), 17 deletions(-)
 create mode 100644 testing/lisp/test-ol-info.el

diff --git a/lisp/ol-info.el b/lisp/ol-info.el
index dc5f6d5ba..008b4d34f 100644
--- a/lisp/ol-info.el
+++ b/lisp/ol-info.el
@@ -63,24 +63,70 @@
   "Follow an Info file and node link specified by PATH."
   (org-info-follow-link path))
 
+(defun org-info-link-file-node (link)
+  "Extract file name and node from info LINK.
+
+Return list containing file name and node name or \"Top\".
+Components may be separated by \"::\" or by \"#\"."
+  (and
+   link
+   (or
+    (string-match "\\`\\([^:]*:\\)?\\(.*\\)\\(?:#\\|::\\)\\(.*\\)?\\'" link)
+    (string-match "\\`\\([^:]*:\\)?\\(.*\\)\\'" link))
+   (let ((scheme (match-string 1 link))
+         (file (match-string 2 link))
+         (node (match-string 3 link)))
+     (and
+      (or (not scheme) (string-equal "info:" scheme))
+      (org-string-nw-p file)
+      (list file (or (org-string-nw-p node) "Top"))))))
+
+(defun org-info-description-as-command (link desc)
+  "Info link description that can be pasted as command.
+
+For the folloing LINK
+
+    \"info:elisp::Non-ASCII in Strings\"
+
+the result is
+
+    info \"(elisp) Non-ASCII in Strings\"
+
+that may be executed as a shell command or evaluated by
+\\[eval-expression] (wrapped with parenthesis) to read the manual
+in Emacs.
+
+Calling convention is similar to `org-link-make-description-function'.
+DESC has higher priority and returned when it is not nil.
+If LINK is not an info link then DESC is returned."
+  (or (org-string-nw-p desc)
+      (let* ((file-node (org-info-link-file-node link))
+             (file (car file-node))
+             (node (cadr file-node)))
+        (cond
+         ((and node (not (string-equal "Top" node)))
+          (format "info \"(%s) %s\"" file node))
+         (file (format "info %s" file))
+         (t desc)))))
+
 
 (defun org-info-follow-link (name)
   "Follow an Info file and node link specified by NAME."
-  (if (or (string-match "\\(.*\\)\\(?:#\\|::\\)\\(.*\\)" name)
-          (string-match "\\(.*\\)" name))
-      (let ((filename (match-string 1 name))
-	    (nodename-or-index (or (match-string 2 name) "Top")))
-	(require 'info)
-	;; If nodename-or-index is invalid node name, then look it up
-	;; in the index.
-	(condition-case nil
-	    (Info-find-node filename nodename-or-index)
-	  (user-error (Info-find-node filename "Top")
-		      (condition-case nil
-			  (Info-index nodename-or-index)
-			(user-error "Could not find '%s' node or index entry"
-				    nodename-or-index)))))
-    (user-error "Could not open: %s" name)))
+  (let* ((file-node (org-info-link-file-node name))
+         (filename (car file-node))
+         (nodename-or-index (cadr file-node)))
+    (if (not filename)
+        (user-error "Could not open: %s" name)
+      (require 'info)
+      ;; If nodename-or-index is invalid node name, then look it up
+      ;; in the index.
+      (condition-case nil
+          (Info-find-node filename nodename-or-index)
+        (user-error (Info-find-node filename "Top")
+                    (condition-case nil
+                        (Info-index nodename-or-index)
+                      (user-error "Could not find '%s' node or index entry"
+                                  nodename-or-index)))))))
 
 (defconst org-info-emacs-documents
   '("ada-mode" "auth" "autotype" "bovine" "calc" "ccmode" "cl" "dbus" "dired-x"
@@ -129,9 +175,9 @@ See `org-info-emacs-documents' and `org-info-other-documents' for details."
 (defun org-info-export (path desc format)
   "Export an info link.
 See `org-link-parameters' for details about PATH, DESC and FORMAT."
-  (let* ((parts (split-string path "#\\|::"))
+  (let* ((parts (org-info-link-file-node path))
 	 (manual (car parts))
-	 (node (or (nth 1 parts) "Top")))
+	 (node (nth 1 parts)))
     (pcase format
       (`html
        (format "<a href=\"%s#%s\">%s</a>"
diff --git a/testing/lisp/test-ol-info.el b/testing/lisp/test-ol-info.el
new file mode 100644
index 000000000..aa5234260
--- /dev/null
+++ b/testing/lisp/test-ol-info.el
@@ -0,0 +1,87 @@
+;;; test-ol-info.el --- tests for ol-info.el                        -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2022 Free Software Foundation, Inc.
+
+;; Author:  Max Nikulin <manikulin@gmail.com>
+;; Keywords: org, texinfo
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Test some functions from ol-info.el.
+
+;;; Code:
+
+(unless (featurep 'ol-info)
+  (signal 'missing-test-dependency "Support for info links"))
+
+(ert-deftest test-org-link-info/link-file-node ()
+  "Test parse info links by `org-info-link-file-node'."
+  (should (equal '("success" "Double Colon Separator")
+                 (org-info-link-file-node "success::Double Colon Separator")))
+  (should (equal '("success" "Hash Separator")
+                 (org-info-link-file-node "success#Hash Separator")))
+  (should (equal '("nodeless" "Top")
+                 (org-info-link-file-node "nodeless")))
+  (should (equal '("trailing-hash" "Top")
+                 (org-info-link-file-node "trailing-hash#")))
+  (should (equal '("trailing-double-colon" "Top")
+                 (org-info-link-file-node "trailing-double-colon")))
+  (should (equal '("with-scheme" "Double Colon Separator")
+                 (org-info-link-file-node "info:with-scheme::Double Colon Separator")))
+  (should (equal '("with-scheme" "Hash Separator")
+                 (org-info-link-file-node "info:with-scheme#Hash Separator")))
+  (should (equal '("scheme-and-file" "Top")
+                 (org-info-link-file-node "info:scheme-and-file")))
+  (should (equal '("scheme-file-hash" "Top")
+                 (org-info-link-file-node "info:scheme-file-hash#")))
+  (should (equal '("scheme-file-double-colon" "Top")
+                 (org-info-link-file-node "info:scheme-file-double-colon")))
+  (should (eq nil (org-info-link-file-node nil)))
+  (should (eq nil (org-info-link-file-node "Info:broken#Wrong scheme case")))
+  (should (eq nil (org-info-link-file-node "http:broken::Not info scheme"))))
+
+(ert-deftest test-org-link-info/description-as-command ()
+  "Test `org-info-description-as-command'."
+  (let ((cases
+         '(("info file" "file")
+           ("info strip-top" "strip-top#Top")
+           ("info strip-top-hash" "info:strip-top-hash#Top")
+           ("info strip-top-double-colon" "strip-top-double-colon::Top")
+           ("info \"(pass) Hash\"" "pass#Hash")
+           ("info \"(pass) Double Colon\"" "info:pass#Double Colon")
+           (nil nil)
+           (nil "https://wrong.scheme"))))
+    (dolist (expectation-input cases)
+      (let ((expectation (car expectation-input))
+            (input (cadr expectation-input)))
+        (should (equal
+                 expectation
+                 (org-info-description-as-command input nil))))))
+  (let ((cases
+         '(("Override link" "ignored#Link" "Override link")
+           ("Fallback description" "http://not.info/link" "Fallback description")
+           ("Link is nil" nil "Link is nil")
+           (nil nil nil))))
+        (dolist (expectation-input-desc cases)
+      (let ((expectation (car expectation-input-desc))
+            (input (cadr expectation-input-desc))
+            (desc (nth 2 expectation-input-desc)))
+        (should (equal
+                 expectation
+                 (org-info-description-as-command input desc)))))))
+
+(provide 'test-ol-info)
+;;; test-ol-info.el ends here
-- 
2.25.1


[-- Attachment #3: 0002-ol-info-Enable-insert-description-feature.patch --]
[-- Type: text/x-patch, Size: 743 bytes --]

From f27a3b5bc01cdb915114797347f7b3491d8101dd Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sat, 30 Jul 2022 19:16:42 +0700
Subject: [PATCH 2/2] ol-info: Enable :insert-description feature

---
 lisp/ol-info.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/ol-info.el b/lisp/ol-info.el
index 008b4d34f..83e08ae18 100644
--- a/lisp/ol-info.el
+++ b/lisp/ol-info.el
@@ -43,7 +43,8 @@
 (org-link-set-parameters "info"
 			 :follow #'org-info-open
 			 :export #'org-info-export
-			 :store #'org-info-store-link)
+			 :store #'org-info-store-link
+                         :insert-description #'org-info-description-as-command)
 
 ;; Implementation
 (defun org-info-store-link ()
-- 
2.25.1

Re: [PATCH] ol-info: Enable :insert-description feature

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> To get impression of the new feature in action, I modified ol-info.el. 
> First patch does not depend on :insert-description and might be useful 
> per se. Second one just enables the feature.

Thanks! The patches look useful.
You should also add an entry to ORG-NEWS. Otherwise, I just have two
minor comments.

> +(defun org-info-description-as-command (link desc)
> +  "Info link description that can be pasted as command.
> +
> +For the folloing LINK

          ^following

> +
> +    \"info:elisp::Non-ASCII in Strings\"
> +
> +the result is
> +
> +    info \"(elisp) Non-ASCII in Strings\"
> +
> +that may be executed as a shell command or evaluated by
> +\\[eval-expression] (wrapped with parenthesis) to read the manual
> +in Emacs.
> +
> +Calling convention is similar to `org-link-make-description-function'.
> +DESC has higher priority and returned when it is not nil.
> +If LINK is not an info link then DESC is returned."
> +  (or (org-string-nw-p desc)
> +      (let* ((file-node (org-info-link-file-node link))
> +             (file (car file-node))
> +             (node (cadr file-node)))

pcase-let would be shorter here.


-- 
Ihor Radchenko,
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

[PATCH v2] ol-info: Define :insert-description function

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

On 06/08/2022 14:00, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> +If LINK is not an info link then DESC is returned."
>> +  (or (org-string-nw-p desc)
>> +      (let* ((file-node (org-info-link-file-node link))
>> +             (file (car file-node))
>> +             (node (cadr file-node)))
> 
> pcase-let would be shorter here.

I have rewritten the patch to use `pcase' and to fix allowed separators 
between file name and node.

I have realized that unlike other type specific functions, 
:insert-description receives link including "scheme:" prefix. However 
attempt to achieve consistency at this point may cause more problems.

[-- Attachment #2: v2-0001-ol-info-Define-insert-description-function.patch --]
[-- Type: text/x-patch, Size: 11156 bytes --]

From 7aeaa36ee1b30aa1c9711fe76e151ec956199f27 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sat, 30 Jul 2022 19:13:01 +0700
Subject: [PATCH v2] ol-info: Define :insert-description function

* lisp/ol-info.el (org-info--link-file-node): New helper to parse info
link info file (manual) name and node.
(org-info-follow-link, org-info-export): Use `org-info--link-file-node'.
(org-info-description-as-command): New function to create description
for info links that may executed to view the manual.
(org-link-parameters): Specify `org-info-description-as-command' as
`:insert-description' for info links.
(org-info-other-documents): Add URL of directory index.
* testing/lisp/test-org-info.el (test-org-info/export): Add cases for
texinfo export with link description.
(test-org-info/link-file-node, test-org-info/description-as-command):
New tests for new functions `org-info--link-file-node' and
`org-info-description-as-command'.

Use recently added :insert-description feature of `org-link'.
Alternative separators between file name and node ":", "::", "#:"
are preserved.  Added interpretation of empty path as dir index,
Org manual is assumed if file is not specified for given node.
---
 lisp/ol-info.el               | 85 ++++++++++++++++++++++++++---------
 testing/lisp/test-org-info.el | 82 ++++++++++++++++++++++++++++++++-
 2 files changed, 146 insertions(+), 21 deletions(-)

diff --git a/lisp/ol-info.el b/lisp/ol-info.el
index dc5f6d5ba..0584e6f8c 100644
--- a/lisp/ol-info.el
+++ b/lisp/ol-info.el
@@ -30,6 +30,7 @@
 
 ;;; Code:
 
+(require 'subr-x) ; `string-trim', `string-remove-prefix'
 (require 'ol)
 
 ;; Declare external functions and variables
@@ -43,7 +44,8 @@
 (org-link-set-parameters "info"
 			 :follow #'org-info-open
 			 :export #'org-info-export
-			 :store #'org-info-store-link)
+			 :store #'org-info-store-link
+                         :insert-description #'org-info-description-as-command)
 
 ;; Implementation
 (defun org-info-store-link ()
@@ -63,24 +65,68 @@
   "Follow an Info file and node link specified by PATH."
   (org-info-follow-link path))
 
+(defun org-info--link-file-node (path)
+  "Extract file name and node from info link PATH.
+
+Return cons consisting of file name and node name or \"Top\" if node
+part is not specified. Components may be separated by \":\" or by \"#\"."
+  (if (not path)
+      '("dir" . "Top")
+    (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
+    (let* ((node (match-string 2 path))
+           ;; `string-trim' modifies match
+           (file (string-trim (match-string 1 path)))
+           (has-file (org-string-nw-p file))
+           (has-node (org-string-nw-p node)))
+      (cons
+       ;; Fallback to "org" is an arbirtrary choice
+       ;; and added because "(dir)filename" does not work as "filename".
+       (if has-file file (if has-node "org" "dir"))
+       (if has-node (string-trim node) "Top")))))
+
+(defun org-info-description-as-command (link desc)
+  "Info link description that can be pasted as command.
+
+For the following LINK
+
+    \"info:elisp#Non-ASCII in Strings\"
+
+the result is
+
+    info \"(elisp) Non-ASCII in Strings\"
+
+that may be executed as shell command or evaluated by
+\\[eval-expression] (wrapped with parenthesis) to read the manual
+in Emacs.
+
+Calling convention is similar to `org-link-make-description-function'.
+DESC has higher priority and returned when it is not nil or empty string.
+If LINK is not an info link then DESC is returned."
+  (let* ((prefix "info:")
+         (need-file-node (and (not (org-string-nw-p desc))
+                              (string-prefix-p prefix link))))
+    (pcase (and need-file-node
+                (org-info--link-file-node (string-remove-prefix prefix link)))
+      ;; Unlike (info "dir"), "info dir" shell command opens "(coreutils)dir invocation"
+      (`("dir" . "Top") "info \"(dir)\"")
+      (`(,file . "Top") (format "info %s" file))
+      (`(,file . ,node) (format "info \"(%s) %s\"" file node))
+      (_ desc))))
 
 (defun org-info-follow-link (name)
   "Follow an Info file and node link specified by NAME."
-  (if (or (string-match "\\(.*\\)\\(?:#\\|::\\)\\(.*\\)" name)
-          (string-match "\\(.*\\)" name))
-      (let ((filename (match-string 1 name))
-	    (nodename-or-index (or (match-string 2 name) "Top")))
-	(require 'info)
-	;; If nodename-or-index is invalid node name, then look it up
-	;; in the index.
-	(condition-case nil
-	    (Info-find-node filename nodename-or-index)
-	  (user-error (Info-find-node filename "Top")
-		      (condition-case nil
-			  (Info-index nodename-or-index)
-			(user-error "Could not find '%s' node or index entry"
-				    nodename-or-index)))))
-    (user-error "Could not open: %s" name)))
+  (pcase-let ((`(,filename . ,nodename-or-index)
+	       (org-info--link-file-node name)))
+    (require 'info)
+    ;; If nodename-or-index is invalid node name, then look it up
+    ;; in the index.
+    (condition-case nil
+        (Info-find-node filename nodename-or-index)
+      (user-error (Info-find-node filename "Top")
+                  (condition-case nil
+                      (Info-index nodename-or-index)
+                    (user-error "Could not find '%s' node or index entry"
+                                nodename-or-index))))))
 
 (defconst org-info-emacs-documents
   '("ada-mode" "auth" "autotype" "bovine" "calc" "ccmode" "cl" "dbus" "dired-x"
@@ -95,7 +141,8 @@
 Taken from <https://www.gnu.org/software/emacs/manual/html_mono/.>")
 
 (defconst org-info-other-documents
-  '(("libc" . "https://www.gnu.org/software/libc/manual/html_mono/libc.html")
+  '(("dir" . "https://www.gnu.org/manual/manual.html") ; index
+    ("libc" . "https://www.gnu.org/software/libc/manual/html_mono/libc.html")
     ("make" . "https://www.gnu.org/software/make/manual/make.html"))
   "Alist of documents generated from Texinfo source.
 When converting info links to HTML, links to any one of these manuals are
@@ -129,9 +176,7 @@ See `org-info-emacs-documents' and `org-info-other-documents' for details."
 (defun org-info-export (path desc format)
   "Export an info link.
 See `org-link-parameters' for details about PATH, DESC and FORMAT."
-  (let* ((parts (split-string path "#\\|::"))
-	 (manual (car parts))
-	 (node (or (nth 1 parts) "Top")))
+  (pcase-let ((`(,manual . ,node) (org-info--link-file-node path)))
     (pcase format
       (`html
        (format "<a href=\"%s#%s\">%s</a>"
diff --git a/testing/lisp/test-org-info.el b/testing/lisp/test-org-info.el
index 94923169c..3b8f85a2b 100644
--- a/testing/lisp/test-org-info.el
+++ b/testing/lisp/test-org-info.el
@@ -28,6 +28,11 @@
   (should
    (equal (org-info-export "filename" nil 'html)
 	  "<a href=\"filename.html#Top\">filename</a>"))
+  ;; Directory index. Top anchor actually should not be added,
+  ;; but it should be rather rare case to add special code path
+  (should
+   (equal (org-info-export "dir" nil 'html)
+	  "<a href=\"https://www.gnu.org/manual/manual.html#Top\">dir</a>"))
   ;; When exporting to HTML, ensure node names are expanded according
   ;; to (info "(texinfo) HTML Xref Node Name Expansion").
   (should
@@ -56,9 +61,84 @@
 	  "@ref{Top,,,filename,}"))
   (should
    (equal (org-info-export "filename#node" nil 'texinfo)
-	  "@ref{node,,,filename,}")))
+	  "@ref{node,,,filename,}"))
+  ;; "Top" is preserved, "::" as node separator.
+  (should
+   (equal "@ref{Top,,,emacs,}"
+          (org-info-export "emacs::Top" nil 'texinfo)))
+
+  ;; Description.
+  (should
+   (equal "@ref{Top,Emacs,,emacs,}"
+          (org-info-export "emacs" "Emacs" 'texinfo)))
+  (should
+   (equal "@ref{Destructuring with pcase Patterns,pcase-let,,emacs,}"
+          (org-info-export "emacs#Destructuring with pcase Patterns"
+                           "pcase-let" 'texinfo))))
 
+(ert-deftest test-org-info/link-file-node ()
+  "Test parse info links by `org-info--link-file-node'."
+  (should (equal '("success" . "Hash Separator")
+                 (org-info--link-file-node "success#Hash Separator")))
+  ;; Other separators
+  (should (equal '("success" . "Single Colon Separator")
+                 (org-info--link-file-node "success:Single Colon Separator")))
+  (should (equal '("success" . "Double Colon Separator")
+                 (org-info--link-file-node "success::Double Colon Separator")))
+  (should (equal '("success" . "Hash Colon Separator")
+                 (org-info--link-file-node "success#:Hash Colon Separator")))
+  ;; Partial specification
+  (should (equal '("nodeless" . "Top")
+                 (org-info--link-file-node "nodeless")))
+  (should (equal '("dir" . "Top")
+                 (org-info--link-file-node "")))
+  (should (equal '("dir" . "Top")
+                 (org-info--link-file-node nil)))
+  (should (equal '("org" . "Introduction")
+                 (org-info--link-file-node "#Introduction")))
+  ;; Trailing separator
+  (should (equal '("trailing-hash" . "Top")
+                 (org-info--link-file-node "trailing-hash#")))
+  (should (equal '("trailing-single-colon" . "Top")
+                 (org-info--link-file-node "trailing-single-colon:")))
+  (should (equal '("trailing-double-colon" . "Top")
+                 (org-info--link-file-node "trailing-double-colon::")))
+  (should (equal '("trailing-hash-colon" . "Top")
+                 (org-info--link-file-node "trailing-hash-colon#:")))
+  ;; Trim spaces
+  (should (equal '("trim" . "Spaces")
+                 (org-info--link-file-node " trim # Spaces \t"))))
 
+(ert-deftest test-org-info/description-as-command ()
+  "Test `org-info-description-as-command'."
+  (let ((cases
+         '(("info file" "info:file")
+           ("info strip-top-hash" "info:strip-top-hash#Top")
+           ("info strip-top-single-colon" "info:strip-top-single-colon:Top")
+           ("info strip-top-double-colon" "info:strip-top-double-colon::Top")
+           ("info \"(pass) Hash\"" "info:pass#Hash")
+           ("info \"(pass) Double Colon\"" "info:pass:: Double Colon")
+           ("info \"(info) Advanced\"" "info:info:Advanced")
+           ("info \"(dir)\"" "info:")
+           ("info \"(org) Tables\"" "info::Tables")
+           (nil "http://orgmode.org/index.html#Not-info-link"))))
+    (dolist (expectation-input cases)
+      (let ((expectation (car expectation-input))
+            (input (cadr expectation-input)))
+        (should (equal
+                 expectation
+                 (org-info-description-as-command input nil))))))
+  (let ((cases
+         '(("Override link" "info:ignored#Link" "Override link")
+           ("Fallback description" "http://not.info/link" "Fallback description")
+           ("Link is nil" nil "Link is nil"))))
+        (dolist (expectation-input-desc cases)
+      (let ((expectation (car expectation-input-desc))
+            (input (cadr expectation-input-desc))
+            (desc (nth 2 expectation-input-desc)))
+        (should (equal
+                 expectation
+                 (org-info-description-as-command input desc)))))))
 
 (provide 'test-org-info)
 ;;; test-org-info.el ends here
-- 
2.25.1

Re: [PATCH v2] ol-info: Define :insert-description function

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> I have rewritten the patch to use `pcase' and to fix allowed separators 
> between file name and node.

Thanks!

> +(defun org-info--link-file-node (path)
> +  "Extract file name and node from info link PATH.
> +
> +Return cons consisting of file name and node name or \"Top\" if node
> +part is not specified. Components may be separated by \":\" or by \"#\"."

It looks like the docstring does not match what the function actually
returns.

> +  (if (not path)
> +      '("dir" . "Top")

"dir" is not a file. Also, it is not very clear what "dir" is referring
to. Maybe you can add a comment pointing to `org-info-other-documents'?

> +    (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
> +    (let* ((node (match-string 2 path))
> +           ;; `string-trim' modifies match

Here and is several other places, including docstrings, please make sure
that the sentences end with "." and are separated with "  ".

> +      (cons
> +       ;; Fallback to "org" is an arbirtrary choice
> +       ;; and added because "(dir)filename" does not work as "filename".

Should this be documented? Or at least mentioned that the behaviour is
undefined. And if it is undefined you should not test it in the tests.

-- 
Ihor Radchenko,
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

Re: [PATCH v2] ol-info: Define :insert-description function

[Max Nikulin]

On 19/08/2022 11:28, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> +(defun org-info--link-file-node (path)
>> +  "Extract file name and node from info link PATH.
>> +
>> +Return cons consisting of file name and node name or \"Top\" if node
>> +part is not specified. Components may be separated by \":\" or by \"#\"."
> 
> It looks like the docstring does not match what the function actually
> returns.

It returns a cons, doesn't it? Is it confusing that separator for 
components is related to the argument?

>> +  (if (not path)
>> +      '("dir" . "Top")
> 
> "dir" is not a file. Also, it is not very clear what "dir" is referring
> to. Maybe you can add a comment pointing to `org-info-other-documents'?

Try M-x info RET when you do not have *info* buffer and you get "(dir) 
Top" node. It is the directory of info files. If you run "info" without 
argument in shell you will get the same.

>> +    (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
>> +    (let* ((node (match-string 2 path))
>> +           ;; `string-trim' modifies match
> 
> Here and is several other places, including docstrings, please make sure
> that the sentences end with "." and are separated with "  ".

It was supposed to be a brief phrase rather than complete sentence.

>> +      (cons
>> +       ;; Fallback to "org" is an arbirtrary choice
>> +       ;; and added because "(dir)filename" does not work as "filename".
> 
> Should this be documented? Or at least mentioned that the behaviour is
> undefined. And if it is undefined you should not test it in the tests.

The purpose of test is to check that it does not signal some obscure 
error. I am unsure how to handle corner cases, so I am open to 
suggestions. Some considerations
- `org-info--link-file-node' may return nil when link path is incomplete 
or some value that may help to avoid error handling code paths in callers.
- <info:> does not look like a valid link but it may be handled like 
shell "info" command without argument, so I chose "(dir)" node. Elisp 
(info) without arguments however may display existing buffer.
- <info:dir> certainly should be handled as (info "(dir)")
- <info:dir#elisp> is invalid. Maybe (info "elisp") should be used instead.
- <info:#Tables> I am unsure in my choice to open (info "(org) Tables"). 
Maybe it is better to treat it as "(dir) Tables" and so as "(dir) Top" 
node since "(dir) Top" is quite reasonable for <info:> with empty path.

Thanks for the review.

Re: [PATCH v2] ol-info: Define :insert-description function

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 19/08/2022 11:28, Ihor Radchenko wrote:
>> Max Nikulin writes:
>> 
>>> +(defun org-info--link-file-node (path)
>>> +  "Extract file name and node from info link PATH.
>>> +
>>> +Return cons consisting of file name and node name or \"Top\" if node
>>> +part is not specified. Components may be separated by \":\" or by \"#\"."
>> 
>> It looks like the docstring does not match what the function actually
>> returns.
>
> It returns a cons, doesn't it? Is it confusing that separator for 
> components is related to the argument?

It is confusing that cons consist of _file name_ and node name.
However, the function may return non-file as car of the cons.

>>> +  (if (not path)
>>> +      '("dir" . "Top")
>> 
>> "dir" is not a file. Also, it is not very clear what "dir" is referring
>> to. Maybe you can add a comment pointing to `org-info-other-documents'?
>
> Try M-x info RET when you do not have *info* buffer and you get "(dir) 
> Top" node. It is the directory of info files. If you run "info" without 
> argument in shell you will get the same.

Sure. I know this. But see the above. Not every person reading the Org
source is familiar with info conventions.

>>> +    (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
>>> +    (let* ((node (match-string 2 path))
>>> +           ;; `string-trim' modifies match
>> 
>> Here and is several other places, including docstrings, please make sure
>> that the sentences end with "." and are separated with "  ".
>
> It was supposed to be a brief phrase rather than complete sentence.

What about other places like the docstring?
Or

+      ;; Unlike (info "dir"), "info dir" shell command opens "(coreutils)dir invocation"

(missing ".")

probably some more places.

(Of course, this is minor, and I could do it myself; just pointed this
together with other more important comments)

>>> +      (cons
>>> +       ;; Fallback to "org" is an arbirtrary choice
>>> +       ;; and added because "(dir)filename" does not work as "filename".
>> 
>> Should this be documented? Or at least mentioned that the behaviour is
>> undefined. And if it is undefined you should not test it in the tests.
>
> The purpose of test is to check that it does not signal some obscure 
> error. I am unsure how to handle corner cases, so I am open to 
> suggestions. Some considerations
> - `org-info--link-file-node' may return nil when link path is incomplete 
> or some value that may help to avoid error handling code paths in callers.
> - <info:> does not look like a valid link but it may be handled like 
> shell "info" command without argument, so I chose "(dir)" node. Elisp 
> (info) without arguments however may display existing buffer.
> - <info:dir> certainly should be handled as (info "(dir)")
> - <info:dir#elisp> is invalid. Maybe (info "elisp") should be used instead.
> - <info:#Tables> I am unsure in my choice to open (info "(org) Tables"). 
> Maybe it is better to treat it as "(dir) Tables" and so as "(dir) Top" 
> node since "(dir) Top" is quite reasonable for <info:> with empty path.

I am not sure what would be the best here. The main point - whatever
fallback mechanism you choose, please document it in the docstring.

In general, I favour having a fallback more compared to not having any.

-- 
Ihor Radchenko,
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

Re: [PATCH v2] ol-info: Define :insert-description function

[Max Nikulin]

On 20/08/2022 14:29, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> On 19/08/2022 11:28, Ihor Radchenko wrote:
>>> Max Nikulin writes:
>>>
>>>> +(defun org-info--link-file-node (path)
>>>> +  "Extract file name and node from info link PATH.
>>>> +
>>>> +Return cons consisting of file name and node name or \"Top\" if node
>>>> +part is not specified. Components may be separated by \":\" or by \"#\"."
>>>
>>> It looks like the docstring does not match what the function actually
>>> returns.
>>
>> It returns a cons, doesn't it? Is it confusing that separator for
>> components is related to the argument?
> 
> It is confusing that cons consist of _file name_ and node name.
> However, the function may return non-file as car of the cons.

"dir" should not appear in well-formed links <info:file#node>. If a user 
adds "info:" links then it is unlikely that their has never used info 
and has never seen directory index. Moreover I do not see real problem 
since it is considered as a virtual info file:

https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/info.el#n2344
(add-to-list 'Info-virtual-files
	     '("\\`dir\\'" ; ...

After all, it is an internal function and I am still unsure that I have 
chosen the best variant. I believed that underspecified behavior here 
may give less harm than making some 3rd party code relying on the 
proposed variant.

I can drop "(dir)" special case to consider incomplete links as invalid 
ones. The only problem is test cases for escaping of various characters 
since they use links without file name as the input, but they may be 
easily adjusted by adding some file name.

There is huge room for improvements in "ol-info.el", e.g. other virtual 
files such as "(*History*)" may signal an error during HTML export.

Re: [PATCH v2] ol-info: Define :insert-description function

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>>> It returns a cons, doesn't it? Is it confusing that separator for
>>> components is related to the argument?
>> 
>> It is confusing that cons consist of _file name_ and node name.
>> However, the function may return non-file as car of the cons.
>
> "dir" should not appear in well-formed links <info:file#node>. If a user 
> adds "info:" links then it is unlikely that their has never used info 
> and has never seen directory index. Moreover I do not see real problem 
> since it is considered as a virtual info file:
>
> https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/info.el#n2344
> (add-to-list 'Info-virtual-files
> 	     '("\\`dir\\'" ; ...

I do not see ("dir" . "etc") as a problematic return value.
The only issue is that docstring is a bit confusing.

Maybe, in addition to "Return cons consisting of file name and node name",
you can put something like "File name may also be a virtual file name
(see `Info-virtual-files')."

> After all, it is an internal function and I am still unsure that I have 
> chosen the best variant. I believed that underspecified behavior here 
> may give less harm than making some 3rd party code relying on the 
> proposed variant.
>
> I can drop "(dir)" special case to consider incomplete links as invalid 
> ones. The only problem is test cases for escaping of various characters 
> since they use links without file name as the input, but they may be 
> easily adjusted by adding some file name.

I am ok with current behavior. It is only the docstring that I find
slightly confusing.

> There is huge room for improvements in "ol-info.el", e.g. other virtual 
> files such as "(*History*)" may signal an error during HTML export.

May you open a separate bug report about this?

-- 
Ihor Radchenko,
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

Re: [PATCH v3] ol-info: Define :insert-description function

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

On 22/08/2022 11:10, Ihor Radchenko wrote:
> 
> Maybe, in addition to "Return cons consisting of file name and node name",
> you can put something like "File name may also be a virtual file name
> (see `Info-virtual-files')."

I have tried to fix docstrings.

> I am ok with current behavior. It is only the docstring that I find
> slightly confusing.

I am still unsure how to treat links with missed filename, so I changed 
it from "(org) Something" to "(dir) Something". The latter is not valid, 
but it is more consistent with "(dir) Top" for "info:" (empty path) links.

>> There is huge room for improvements in "ol-info.el", e.g. other virtual
>> files such as "(*History*)" may signal an error during HTML export.
> 
> May you open a separate bug report about this?

I do not think it deserves an entry on updates.orgmode.org, especially 
taking into account that it might be tricky to delete it.

[-- Attachment #2: v3-0001-ol-info-Define-insert-description-function.patch --]
[-- Type: text/x-patch, Size: 11188 bytes --]

From 59b154e089788f49158b465c5b177c99889f2e06 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sat, 30 Jul 2022 19:13:01 +0700
Subject: [PATCH v3] ol-info: Define :insert-description function

* lisp/ol-info.el (org-info--link-file-node): New helper to parse info
link info file (manual) name and node.
(org-info-follow-link, org-info-export): Use `org-info--link-file-node'.
(org-info-description-as-command): New function to create description
for info links that may executed to view the manual.
(org-link-parameters): Specify `org-info-description-as-command' as
`:insert-description' for info links.
(org-info-other-documents): Add URL of directory index.
* testing/lisp/test-org-info.el (test-org-info/export): Add cases for
texinfo export with link description.
(test-org-info/link-file-node, test-org-info/description-as-command):
New tests for new functions `org-info--link-file-node' and
`org-info-description-as-command'.

Use recently added :insert-description feature of `org-link'.
Alternative separators between file name and node ":", "::", "#:"
are preserved.  Added interpretation of empty path or omitted
file name as info dir index.
---
 lisp/ol-info.el               | 82 ++++++++++++++++++++++++---------
 testing/lisp/test-org-info.el | 85 ++++++++++++++++++++++++++++++++++-
 2 files changed, 146 insertions(+), 21 deletions(-)

diff --git a/lisp/ol-info.el b/lisp/ol-info.el
index dc5f6d5ba..7be63b3e1 100644
--- a/lisp/ol-info.el
+++ b/lisp/ol-info.el
@@ -30,6 +30,7 @@
 
 ;;; Code:
 
+(require 'subr-x) ; `string-trim', `string-remove-prefix'
 (require 'ol)
 
 ;; Declare external functions and variables
@@ -43,7 +44,8 @@
 (org-link-set-parameters "info"
 			 :follow #'org-info-open
 			 :export #'org-info-export
-			 :store #'org-info-store-link)
+			 :store #'org-info-store-link
+                         :insert-description #'org-info-description-as-command)
 
 ;; Implementation
 (defun org-info-store-link ()
@@ -63,24 +65,65 @@
   "Follow an Info file and node link specified by PATH."
   (org-info-follow-link path))
 
+(defun org-info--link-file-node (path)
+  "Extract file name and node from info link PATH.
+
+Return cons consisting of file name and node name or \"Top\" if node
+part is not specified.  Components may be separated by \":\" or by \"#\".
+File may be a virtual one, see `Info-virtual-files'."
+  (if (not path)
+      '("dir" . "Top")
+    (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
+    (let* ((node (match-string 2 path))
+           ;; Do not reorder, `string-trim' modifies match.
+           (file (string-trim (match-string 1 path))))
+      (cons
+       (if (org-string-nw-p file) file "dir")
+       (if (org-string-nw-p node) (string-trim node) "Top")))))
+
+(defun org-info-description-as-command (link desc)
+  "Info link description that can be pasted as command.
+
+For the following LINK
+
+    \"info:elisp#Non-ASCII in Strings\"
+
+the result is
+
+    info \"(elisp) Non-ASCII in Strings\"
+
+that may be executed as shell command or evaluated by
+\\[eval-expression] (wrapped with parenthesis) to read the manual
+in Emacs.
+
+Calling convention is similar to `org-link-make-description-function'.
+DESC has higher priority and returned when it is not nil or empty string.
+If LINK is not an info link then DESC is returned."
+  (let* ((prefix "info:")
+         (need-file-node (and (not (org-string-nw-p desc))
+                              (string-prefix-p prefix link))))
+    (pcase (and need-file-node
+                (org-info--link-file-node (string-remove-prefix prefix link)))
+      ;; Unlike (info "dir"), "info dir" shell command opens "(coreutils)dir invocation".
+      (`("dir" . "Top") "info \"(dir)\"")
+      (`(,file . "Top") (format "info %s" file))
+      (`(,file . ,node) (format "info \"(%s) %s\"" file node))
+      (_ desc))))
 
 (defun org-info-follow-link (name)
   "Follow an Info file and node link specified by NAME."
-  (if (or (string-match "\\(.*\\)\\(?:#\\|::\\)\\(.*\\)" name)
-          (string-match "\\(.*\\)" name))
-      (let ((filename (match-string 1 name))
-	    (nodename-or-index (or (match-string 2 name) "Top")))
-	(require 'info)
-	;; If nodename-or-index is invalid node name, then look it up
-	;; in the index.
-	(condition-case nil
-	    (Info-find-node filename nodename-or-index)
-	  (user-error (Info-find-node filename "Top")
-		      (condition-case nil
-			  (Info-index nodename-or-index)
-			(user-error "Could not find '%s' node or index entry"
-				    nodename-or-index)))))
-    (user-error "Could not open: %s" name)))
+  (pcase-let ((`(,filename . ,nodename-or-index)
+	       (org-info--link-file-node name)))
+    (require 'info)
+    ;; If nodename-or-index is invalid node name, then look it up
+    ;; in the index.
+    (condition-case nil
+        (Info-find-node filename nodename-or-index)
+      (user-error (Info-find-node filename "Top")
+                  (condition-case nil
+                      (Info-index nodename-or-index)
+                    (user-error "Could not find '%s' node or index entry"
+                                nodename-or-index))))))
 
 (defconst org-info-emacs-documents
   '("ada-mode" "auth" "autotype" "bovine" "calc" "ccmode" "cl" "dbus" "dired-x"
@@ -95,7 +138,8 @@
 Taken from <https://www.gnu.org/software/emacs/manual/html_mono/.>")
 
 (defconst org-info-other-documents
-  '(("libc" . "https://www.gnu.org/software/libc/manual/html_mono/libc.html")
+  '(("dir" . "https://www.gnu.org/manual/manual.html") ; index
+    ("libc" . "https://www.gnu.org/software/libc/manual/html_mono/libc.html")
     ("make" . "https://www.gnu.org/software/make/manual/make.html"))
   "Alist of documents generated from Texinfo source.
 When converting info links to HTML, links to any one of these manuals are
@@ -129,9 +173,7 @@ See `org-info-emacs-documents' and `org-info-other-documents' for details."
 (defun org-info-export (path desc format)
   "Export an info link.
 See `org-link-parameters' for details about PATH, DESC and FORMAT."
-  (let* ((parts (split-string path "#\\|::"))
-	 (manual (car parts))
-	 (node (or (nth 1 parts) "Top")))
+  (pcase-let ((`(,manual . ,node) (org-info--link-file-node path)))
     (pcase format
       (`html
        (format "<a href=\"%s#%s\">%s</a>"
diff --git a/testing/lisp/test-org-info.el b/testing/lisp/test-org-info.el
index 94923169c..1ca2aca2e 100644
--- a/testing/lisp/test-org-info.el
+++ b/testing/lisp/test-org-info.el
@@ -28,6 +28,11 @@
   (should
    (equal (org-info-export "filename" nil 'html)
 	  "<a href=\"filename.html#Top\">filename</a>"))
+  ;; Directory index. Top anchor actually should not be added,
+  ;; but it should be rather rare case to add special code path.
+  (should
+   (equal (org-info-export "dir" nil 'html)
+	  "<a href=\"https://www.gnu.org/manual/manual.html#Top\">dir</a>"))
   ;; When exporting to HTML, ensure node names are expanded according
   ;; to (info "(texinfo) HTML Xref Node Name Expansion").
   (should
@@ -56,9 +61,87 @@
 	  "@ref{Top,,,filename,}"))
   (should
    (equal (org-info-export "filename#node" nil 'texinfo)
-	  "@ref{node,,,filename,}")))
+	  "@ref{node,,,filename,}"))
+  ;; "Top" is preserved, "::" as node separator.
+  (should
+   (equal "@ref{Top,,,emacs,}"
+          (org-info-export "emacs::Top" nil 'texinfo)))
+
+  ;; Description.
+  (should
+   (equal "@ref{Top,Emacs,,emacs,}"
+          (org-info-export "emacs" "Emacs" 'texinfo)))
+  (should
+   (equal "@ref{Destructuring with pcase Patterns,pcase-let,,emacs,}"
+          (org-info-export "emacs#Destructuring with pcase Patterns"
+                           "pcase-let" 'texinfo))))
 
+(ert-deftest test-org-info/link-file-node ()
+  "Test parse info links by `org-info--link-file-node'."
+  (should (equal '("success" . "Hash Separator")
+                 (org-info--link-file-node "success#Hash Separator")))
+  ;; Other separators.
+  (should (equal '("success" . "Single Colon Separator")
+                 (org-info--link-file-node "success:Single Colon Separator")))
+  (should (equal '("success" . "Double Colon Separator")
+                 (org-info--link-file-node "success::Double Colon Separator")))
+  (should (equal '("success" . "Hash Colon Separator")
+                 (org-info--link-file-node "success#:Hash Colon Separator")))
+  ;; Partial specification.
+  (should (equal '("nodeless" . "Top")
+                 (org-info--link-file-node "nodeless")))
+  (should (equal '("dir" . "Top")
+                 (org-info--link-file-node "")))
+  (should (equal '("dir" . "Top")
+                 (org-info--link-file-node nil)))
+  ;; Feel free to change behavior of underspecified links,
+  ;; the case is added to check that it does not signal some error.
+  (should (equal '("dir" . "broken")
+                 (org-info--link-file-node "#broken")))
+  ;; Trailing separator.
+  (should (equal '("trailing-hash" . "Top")
+                 (org-info--link-file-node "trailing-hash#")))
+  (should (equal '("trailing-single-colon" . "Top")
+                 (org-info--link-file-node "trailing-single-colon:")))
+  (should (equal '("trailing-double-colon" . "Top")
+                 (org-info--link-file-node "trailing-double-colon::")))
+  (should (equal '("trailing-hash-colon" . "Top")
+                 (org-info--link-file-node "trailing-hash-colon#:")))
+  ;; Trim spaces.
+  (should (equal '("trim" . "Spaces")
+                 (org-info--link-file-node " trim # Spaces \t"))))
 
+(ert-deftest test-org-info/description-as-command ()
+  "Test `org-info-description-as-command'."
+  (let ((cases
+         '(("info file" "info:file")
+           ("info strip-top-hash" "info:strip-top-hash#Top")
+           ("info strip-top-single-colon" "info:strip-top-single-colon:Top")
+           ("info strip-top-double-colon" "info:strip-top-double-colon::Top")
+           ("info \"(pass) Hash\"" "info:pass#Hash")
+           ("info \"(pass) Double Colon\"" "info:pass:: Double Colon")
+           ("info \"(info) Advanced\"" "info:info:Advanced")
+           ("info \"(dir)\"" "info:")
+           ;; It actually works as "(dir) Top", test that no errors is signalled.
+           ("info \"(dir) Invalid\"" "info::Invalid")
+           (nil "http://orgmode.org/index.html#Not-info-link"))))
+    (dolist (expectation-input cases)
+      (let ((expectation (car expectation-input))
+            (input (cadr expectation-input)))
+        (should (equal
+                 expectation
+                 (org-info-description-as-command input nil))))))
+  (let ((cases
+         '(("Override link" "info:ignored#Link" "Override link")
+           ("Fallback description" "http://not.info/link" "Fallback description")
+           ("Link is nil" nil "Link is nil"))))
+        (dolist (expectation-input-desc cases)
+      (let ((expectation (car expectation-input-desc))
+            (input (cadr expectation-input-desc))
+            (desc (nth 2 expectation-input-desc)))
+        (should (equal
+                 expectation
+                 (org-info-description-as-command input desc)))))))
 
 (provide 'test-org-info)
 ;;; test-org-info.el ends here
-- 
2.25.1

Re: [PATCH v3] ol-info: Define :insert-description function

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> On 22/08/2022 11:10, Ihor Radchenko wrote:
>> 
>> Maybe, in addition to "Return cons consisting of file name and node name",
>> you can put something like "File name may also be a virtual file name
>> (see `Info-virtual-files')."
>
> I have tried to fix docstrings.

Thanks! LGTM now.
Applied onto main via 372788a18.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=372788a18933b9ed38b747e5f5429ea39d71106a

>> I am ok with current behavior. It is only the docstring that I find
>> slightly confusing.
>
> I am still unsure how to treat links with missed filename, so I changed 
> it from "(org) Something" to "(dir) Something". The latter is not valid, 
> but it is more consistent with "(dir) Top" for "info:" (empty path) links.

Let's not overthink edge cases. Unless we can reasonably expect some
specific invalid links in the wild, there is no reason to be too smart
here.

-- 
Ihor Radchenko,
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

Re: [PATCH] ol-info: Enable :insert-description feature

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On 06/08/2022 14:00, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> To get impression of the new feature in action, I modified ol-info.el.
>> First patch does not depend on :insert-description and might be useful
>> per se. Second one just enables the feature.
> 
> Thanks! The patches look useful.
> You should also add an entry to ORG-NEWS. Otherwise, I just have two
> minor comments.

I have realized that I did not address this comment. See the first 
attachment.

I have noticed `org-trim' function, so second attached patch replaces 
`string-trim' in the newly added code to this function native to Org. 
Unfortunately `org-unbracket-string' can not handle empty suffix, but 
the fix is rather simple, so `string-remove-prefix' becomes unnecessary 
as well. The only reason of this patch is that earlier subr-x was 
avoided, see e.g. https://list.orgmode.org/87sg0onfrw.fsf@nicolasgoaziou.fr/

Patches are independent and neither of them is strictly necessary, I do 
not mind if they would be ignored.

[-- Attachment #2: 0001-ORG-NEWS-org-info-description-as-command.patch --]
[-- Type: text/x-patch, Size: 1430 bytes --]

From 7fffacdda6015cee895e8eddb554bce1b82da6d2 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Wed, 31 Aug 2022 09:15:17 +0700
Subject: [PATCH 1/2] ORG-NEWS: `org-info-description-as-command'

* etc/ORG-NEWS: Mention that `org-info' uses `:insert-description',
a new feature of `org-link-parameters'.
---
 etc/ORG-NEWS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index d6d99a64b..713a850f6 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -455,6 +455,17 @@ prompting for a link description.  It can be a string (used as-is) or
 a function (called with the same arguments as
 ~org-make-link-description-function~ to return a string to use).
 
+An example of a such function for =info:= links is
+~org-info-description-as-command~.  To access a manual section outside
+of Org, description may be pasted to shell prompt or evaluated withing
+Emacs using =M-:= (wrapped into parenthesis).  For example,
+description of the =info:org#Tags= link is =info "(org) Tags"=.  To
+restore earlier behavior add to your Emacs init file the following:
+#+begin_src elisp :results silent :eval never-export
+  (with-eval-after-load 'ol-info
+    (org-link-set-parameters "info" :insert-description nil))
+#+end_src
+
 *** New list of languages for LaTeX export: ~org-latex-language-alist~ 
 
 ~org-latex-language-alist~ unifies into a single list the old language
-- 
2.25.1


[-- Attachment #3: 0002-ol-info-Use-org-function-instead-of-subr-x.patch --]
[-- Type: text/x-patch, Size: 2710 bytes --]

From ef29e143545b4c1806abfb8ca072f7c98842fff2 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Wed, 31 Aug 2022 09:21:47 +0700
Subject: [PATCH 2/2] ol-info: Use org function instead of subr-x

* lisp/org-macs.el (org-unbracket-string): Handle empty suffix string.
* lisp/ol-info.el (org-info--link-file-node):
(org-info-description-as-command): Use `org-trim' and
`org-unbracket-string' instead of `string-trim' and
`string-remove-prefix' from the subr-x package.
---
 lisp/ol-info.el  | 9 ++++-----
 lisp/org-macs.el | 3 ++-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/ol-info.el b/lisp/ol-info.el
index e54fedc06..d332b2837 100644
--- a/lisp/ol-info.el
+++ b/lisp/ol-info.el
@@ -30,7 +30,6 @@
 
 ;;; Code:
 
-(require 'subr-x) ; `string-trim', `string-remove-prefix'
 (require 'org-macs)
 (org-assert-version)
 
@@ -78,11 +77,11 @@ File may be a virtual one, see `Info-virtual-files'."
       '("dir" . "Top")
     (string-match "\\`\\([^#:]*\\)\\(?:[#:]:?\\(.*\\)\\)?\\'" path)
     (let* ((node (match-string 2 path))
-           ;; Do not reorder, `string-trim' modifies match.
-           (file (string-trim (match-string 1 path))))
+           ;; Do not reorder, `org-trim' modifies match.
+           (file (org-trim (match-string 1 path))))
       (cons
        (if (org-string-nw-p file) file "dir")
-       (if (org-string-nw-p node) (string-trim node) "Top")))))
+       (if (org-string-nw-p node) (org-trim node) "Top")))))
 
 (defun org-info-description-as-command (link desc)
   "Info link description that can be pasted as command.
@@ -106,7 +105,7 @@ If LINK is not an info link then DESC is returned."
          (need-file-node (and (not (org-string-nw-p desc))
                               (string-prefix-p prefix link))))
     (pcase (and need-file-node
-                (org-info--link-file-node (string-remove-prefix prefix link)))
+                (org-info--link-file-node (org-unbracket-string prefix "" link)))
       ;; Unlike (info "dir"), "info dir" shell command opens "(coreutils)dir invocation".
       (`("dir" . "Top") "info \"(dir)\"")
       (`(,file . "Top") (format "info %s" file))
diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 13d872a82..2e4590e93 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -1101,7 +1101,8 @@ removed.  Return the new string.  If STRING is nil, return nil."
   (and string
        (if (and (string-prefix-p pre string)
 		(string-suffix-p post string))
-	   (substring string (length pre) (- (length post)))
+	   (substring string (length pre)
+                      (and (not (string-equal "" post)) (- (length post))))
 	 string)))
 
 (defun org-strip-quotes (string)
-- 
2.25.1

Re: [PATCH] ol-info: Enable :insert-description feature

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Patches are independent and neither of them is strictly necessary, I do 
> not mind if they would be ignored.

They do add value.
Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=1b647b00d08354d1c2824d0140149a39b271fab7
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=76a5f300345fabce7c0eb4908eee217c58b663d0

-- 
Ihor Radchenko,
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

Re: [PATCH v9] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

> Hugo
> From 039c2131462f5454f2804e809e085a055b5bf552 Mon Sep 17 00:00:00 2001
> From: Hugo Heagren <hugo@heagren.com>
> Date: Mon, 28 Mar 2022 23:18:45 +0100
> Subject: [PATCH 1/2] ol.el: add description format parameter to
>  org-link-parameters
> ...
> From 708916f8d4f31af1e786e87154b5a4479d0ed1b8 Mon Sep 17 00:00:00 2001
> From: Hugo Heagren <hugo@heagren.com>
> Date: Sat, 16 Jul 2022 19:50:15 +0100
> Subject: [PATCH 2/2] test-ol: tests for insert-description param when
>  inserting links

I have addressed the further comments (mine and Max's) myself after applying your patch.
Applied onto main via 2bbb92a72 and e3a05d09b.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=e3a05d09b7398b46e8ef724ae7609eeb8a35346e
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=2bbb92a72d4a1be8e56acd41a9c94f5f0023fb28

You are now officially a contributor ;) 🎇

-- 
Ihor Radchenko,
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

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Ihor Radchenko]

Hugo Heagren <hugo@heagren.com> writes:

> On 18/07/2022 17:55, Max Nikulin wrote:
>
>> I am sorry if I am wrong, but I do not see you among known Org, 
>> contributors. You patch is likely greater than it is allowed for
>> TINYCHANGE, so before you patch can be committed, copyright assignment
>> should be signed, see
>> <https://orgmode.org/worg/org-contribute.html#copyright> for details.
>
> I've already assigned copyright to the FSF for work on Emacs (see
> attached certificate). Do I need to do anything else for org-mode in
> particular?

Bastien, can you please confirm that Hugo is listed in the FSF records
and then add him to the contributors.org?

Best,
Ihor

Re: [PATCH v7] ol.el: add description format parameter to org-link-parameters

[Bastien Guerry]

Hi Ihor and Hugo,

Ihor Radchenko <yantar92@gmail.com> writes:

> Bastien, can you please confirm that Hugo is listed in the FSF
> records

Yes I do!

> and then add him to the contributors.org?

Done.

-- 
 Bastien

Re: [PATCH v4] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

On 08/07/2022 02:57, Hugo Heagren wrote:
> Since the last version of this patch, I have:

Thank you, this version should be more reliable.

> tl;dr The question is: what is the Good Behaviour when
> :default-description is set to something, which is meant to return a
> string and returns 'nil instead? Should it be treated like an empty
> string, or as an error?
Just an idea: if the :default-description function returns "" then use 
empty description, if it returns nil then try 
`org-link-make-description-function'.

Unsure if it is better but I would consider `or' instead of `cond':

(or description
     (let ((make (org-link-get-parameter type :default-description)))
      (and make (condition-case ; ...
     )))
     (and org-link-make-description-function
          (condition-case ; ...
     ))
     desc)

So it becomes a kind of responsibility chain and nil becomes a valid and 
useful value.

I am in doubts concerning "default-description" as the parameter name. I 
would consider either more specific "insert-description" or shorter 
"description". Concerning the former, sometimes I do not mind to have 
default description for export shared by most of backends without 
necessity to explicitly write :export function handling all backends. 
E.g. for <info:org#Protocols> generate 
https://orgmode.org/manual/Protocols.html target and 'info "(org) 
Protocols"' description that is suitable for LaTeX/PDF, HTML, Markdown. 
If something like this were implemented, default-description would 
become ambiguous if it is related to insert or to export.

> +(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
[...]
> +  ;; Copy all keys in `parameters' and their original values to
> +  ;; `orig-parameters'. For `parity': nil = odd, non-nill = even
> +  `(let (parity orig-parameters)
> +     (dolist (p ',parameters)

Have I missed something or the whole macro may be simplified to just 
copy `org-link-parameters'?

   `(let ((org-link-parameters (copy-tree org-link-parameters)))
      (org-link-set-parameters ,type ,@parameters)
      ,@body))

Otherwise `gensym'-generated name should be used instead of hardcoded 
rtn to avoid accidental modification of the variable by the body code:

> +         (let ((_ (org-link-set-parameters ,type ,@parameters))
> +               ;; Do body
> +               (rtn (progn ,@body)))

In addition, `declare' form should be added to `defmacro' to specify 
which argument is considered as its body.

> +    (setq type
> +          (cond

My opinion is that it should be inside
    (let ((initial-input ...
and maybe even be a sibling of
    (def (org-link-get-parameter type
to keep related code more local.

> -	      ((not org-link-make-description-function) desc)
> +              (desc)
> +              ((org-link-get-parameter type :default-description)
> +               (let ((def (org-link-get-parameter type :default-description)))

I have not tested, so I may be wrong in respect to the following. It 
seems, you rises priority of desc value that earlier was a fallback. The 
reason why I am in doubts, is that it seems, desc is initialized from 
current link description when point is withing an existing link and in 
such cases desc likely should be preserved. I can not say that I like 
that it is responsibility of all description functions to return the 
desc argument if it is supplied.

>  	      (t (condition-case nil
>  		     (funcall org-link-make-description-function link desc)

Notice that before you modification `funcall' was guarded by "(not 
org-link-make-description-function)" test.

I like the idea of description specific to link type and it is sour that 
previous attempts to implement the feature was not completed.

Re: [PATCH v3] ol.el: add description format parameter to org-link-parameters

[Max Nikulin]

On 21/06/2022 19:03, Hugo Heagren wrote:
> +(defmacro test-ol-with-link-parameters-as (type parameters &rest body)
[...]
> +  `(progn
> +     (setq orig-parameters org-link-parameters)

I can easily miss something, but wouldn't it be enough to use let-binding

`(let ((org-link-parameters org-link-parameters))
    ,@body)

Otherwise it is better to use something like `condition-case' to restore 
original state even when some error is signaled.

> +     (org-link-set-parameters ,type ,@parameters)
> +     (let ((rtn (progn ,@body)))
> +       (setq org-link-parameters orig-parameters)
> +       rtn)))