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

[Max Nikulin <manikulin@gmail.com>]

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.