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

[Hugo Heagren <hugo@heagren.com>]

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