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

[Hugo Heagren <hugo@heagren.com>]

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