[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Nikulin
Subject: Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters
Date: Sun, 17 Jul 2022 13:11:11 +0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0


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

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)

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)

(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

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]