From: Max Nikulin <manikulin@gmail.com>
To: Org Mode List <emacs-orgmode@gnu.org>
Cc: Hugo Heagren <hugo@heagren.com>
Subject: Re: [PATCH v5] ol.el: add description format parameter to org-link-parameters
Date: Sun, 17 Jul 2022 13:11:11 +0700 [thread overview]
Message-ID: <e77616e4-bda9-87d2-aad2-d78a1e73679f@gmail.com> (raw)
In-Reply-To: <87tu7gkb4l.fsf@heagren.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.
next prev parent reply other threads:[~2022-07-17 6:31 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-28 23:15 ol.el: add description format parameter to org-link-parameters Hugo Heagren
2022-03-28 23:15 ` [PATCH] " Hugo Heagren
2022-04-04 9:49 ` Ihor Radchenko
2022-04-05 19:29 ` [PATCH v2] " Hugo Heagren
2022-04-07 5:13 ` Ihor Radchenko
2022-06-21 12:03 ` [PATCH v3] " Hugo Heagren
2022-06-21 13:41 ` Robert Pluim
2022-07-07 19:57 ` [PATCH v4] " Hugo Heagren
2022-07-09 3:31 ` Ihor Radchenko
2022-07-14 13:08 ` [PATCH v5] " Hugo Heagren
2022-07-16 9:09 ` Ihor Radchenko
2022-07-16 21:20 ` Hugo Heagren
2022-07-17 6:11 ` Max Nikulin [this message]
2022-07-17 10:27 ` Ihor Radchenko
2022-07-17 10:18 ` Ihor Radchenko
2022-07-17 20:59 ` [PATCH v6] " Hugo Heagren
2022-07-18 10:55 ` Max Nikulin
2022-07-23 7:48 ` [PATCH v7] " Hugo Heagren
2022-07-23 7:59 ` Max Nikulin
2022-07-23 13:06 ` Ihor Radchenko
2022-07-23 15:46 ` Max Nikulin
2022-07-24 10:34 ` Max Nikulin
2022-07-24 13:15 ` Ihor Radchenko
2022-07-25 11:55 ` [PATCH v8] " Hugo Heagren
2022-07-29 12:54 ` Max Nikulin
2022-07-29 19:05 ` [PATCH v9] " Hugo Heagren
2022-07-30 6:29 ` Ihor Radchenko
[not found] ` <87tu6zf2o1.fsf@heagren.com>
2022-07-30 8:02 ` Ihor Radchenko
2022-07-30 12:34 ` [PATCH] ol-info: Enable :insert-description feature Max Nikulin
2022-08-06 7:00 ` Ihor Radchenko
2022-08-14 16:41 ` [PATCH v2] ol-info: Define :insert-description function Max Nikulin
2022-08-19 4:28 ` Ihor Radchenko
2022-08-19 12:26 ` Max Nikulin
2022-08-20 7:29 ` Ihor Radchenko
2022-08-21 14:49 ` Max Nikulin
2022-08-22 4:10 ` Ihor Radchenko
2022-08-24 14:37 ` [PATCH v3] " Max Nikulin
2022-08-26 13:15 ` Ihor Radchenko
2022-09-04 15:05 ` [PATCH] ol-info: Enable :insert-description feature Max Nikulin
2022-09-05 6:36 ` Ihor Radchenko
2022-08-06 6:06 ` [PATCH v9] ol.el: add description format parameter to org-link-parameters Ihor Radchenko
2022-07-29 1:47 ` [PATCH v7] " Ihor Radchenko
2022-07-29 7:05 ` Bastien Guerry
2022-07-10 10:26 ` [PATCH v4] " Max Nikulin
2022-06-21 15:01 ` [PATCH v3] " Max Nikulin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e77616e4-bda9-87d2-aad2-d78a1e73679f@gmail.com \
--to=manikulin@gmail.com \
--cc=emacs-orgmode@gnu.org \
--cc=hugo@heagren.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).