ido, org-insert-link, and completion based on link description

ido, org-insert-link, and completion based on link description

[Max Nikulin]

Hi,

Does anyone have an idea why it was necessary to drop completion of 
stored links based on their description for the sake of ido?

I am not an ido user, so I am surprised that such feature negatively 
affected usability. I mean the commit

7f096ad37 2012-10-12 14:39:53 +1100 Tony Day: org-insert-link: Use ido 
when inserting links
https://list.orgmode.org/04D0E787-A8A1-4246-8DD2-D607E38D61BA@gmail.com/T/#u

> -		      (mapcar 'cadr org-stored-links))

If I read the code correctly, Bastien added this line to include link 
descriptions to the completion list in response to

Yagnesh Raghava Yakkala. #+LABEL and CUSTOM_ID with reftex. Mon, 21 May 
2012 04:45:29 +0900
https://list.orgmode.org/877gw6ocva.fsf@okhotsk19.lowtem.hokudai.ac.jp/T/#u

https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=1e34c5d34
2012-08-03 14:08:20 +0200 Bastien Guerry: org.el: Fontify links to 
current buffer when inserting a link.

The only issue I suspect is that `org-store-link' may add to the list of 
stored links entries with identical path and description causing 
duplicated completion options, but it may be solved in another way.

 From my point of view, currently the code of `org-insert-link' related 
to `auto-desc' is completely confusing. It was added to allow 
description completion, but it was not removed in the commit related to 
ido. It is rather inconsistent, so it may be unintentional.


P.S. My question is related to the following threads:
- Carlos Pita. Adding target and custom id links doesn't ask for 
description. Tue, 2 Aug 2022 14:44:58 -0300. 
https://list.orgmode.org/D99A712C-18D1-4A4F-8093-35A0BFB469C4@gmail.com
- Max Nikulin. Re: Bug: org-store-link uses CUSTOM_ID instead of target 
point. Sat, 6 Nov 2021 19:51:29 +0700. 
https://list.orgmode.org/e2c807a7-1924-6f08-9e63-4f70aee9d3b5@gmail.com

I decided to start a new thread to concentrate on ido, link completion 
by their description, and the `auto-desc' variable.

[PATCH] ol.el: Restore complete by description for insert link

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

On 06/09/2022 21:34, Max Nikulin wrote:
> 
> Does anyone have an idea why it was necessary to drop completion of 
> stored links based on their description for the sake of ido?

I have no idea what is the proper way to enable ido for 
`org-insert-link'. Functions and variables specific to ido were removed 
from Org.
(ido-everywhere) and (ido-mode) are not enough.

I tried

     (setq-local completing-read-function #'ido-completing-read)

and the command broke completing read completely.

     (add-function :override completing-read-function #'ido-completing-read)

inspired by `ido-everywhere' code broke M-x, but it enabled ido for 
`org-insert-link'.

I believe that descriptions as completion options were removed because 
ido signals an error when nil is passed inside completion list. I 
consider it as a bug in ido (at least in Emacs-27), but even when 
`completing-read-default' is used, it causes appearance of undesired 
"nil" option. No description is a frequent case for links.

So I am attaching a patch to restore completion of link targets by their 
description, nil descriptions are filtered out.

The change is caused by the auto-desc local variable in 
`org-insert-link', its usage is rather strange and confusing currently. 
  Despite with this patch descriptions are restored, I believe that 
logic related to auto-desc should be removed, anyway it was broken for 
10 years. I am unsure in which thread the next change should be discussed.

> P.S. My question is related to the following threads:
> - Carlos Pita. Adding target and custom id links doesn't ask for 
> description. Tue, 2 Aug 2022 14:44:58 -0300. 
> https://list.orgmode.org/D99A712C-18D1-4A4F-8093-35A0BFB469C4@gmail.com
> - Max Nikulin. Re: Bug: org-store-link uses CUSTOM_ID instead of target 
> point. Sat, 6 Nov 2021 19:51:29 +0700. 
> https://list.orgmode.org/e2c807a7-1924-6f08-9e63-4f70aee9d3b5@gmail.com

[-- Attachment #2: 0001-ol.el-Restore-complete-by-description-for-insert-lin.patch --]
[-- Type: text/x-patch, Size: 2814 bytes --]

From 92e36ec13e84d2af1b367e4a3cac34288d1e8c27 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Sat, 10 Sep 2022 17:23:13 +0700
Subject: [PATCH] ol.el: Restore complete by description for insert link

* lisp/ol.el (org-insert-link): Allow completion of link target by its
description.  Almost certainly the feature was removed unintentionally.

Link descriptions were added to completion options in
the commit 1e34c5d34 Bastien Guerry,  "org.el: Fontify links to current
buffer when inserting a link", 2012-08-03 14:08:20 +0200
in response to
https://list.orgmode.org/877gw6ocva.fsf@okhotsk19.lowtem.hokudai.ac.jp/T/#u
Yagnesh Raghava Yakkala, "#+LABEL and CUSTOM_ID with reftex",
Mon, 21 May 2012 04:45:29 +0900

List of description was removed from completion options
likely because `ido-completing-read' signals an error in the case of nil
variant (that is not uncommon for links with no description), see the
commit 7f096ad37 Tony Day, "org-insert-link: Use ido when inserting
links", 2012-10-12 14:39:53 +1100 and the discussion of the patch
- https://list.orgmode.org/04D0E787-A8A1-4246-8DD2-D607E38D61BA@gmail.com/T/#u
  tony day.  [PATCH] * org-insert-link: use ido when inserting links.
  Fri, 12 Oct 2012 14:58:29 +1100
- https://list.orgmode.org/5CE03302-7C87-44BE-B4AF-A6A92C96C803@gmail.com/T/#u
  tony day.  [PATCH] org-insert-link: allow ido usage when inserting
  links.  Fri, 14 Sep 2012 19:21:50 +1000
- https://list.orgmode.org/0CADA13B-8A22-4F34-91B1-2232997C1F04@gmail.com/T/#u
  tony day.  [PATCH] org-insert-link: allow ido usage when inserting
  links. Fri, 12 Oct 2012 14:56:10 +1100
- https://list.orgmode.org/97F9790D-3C7F-490B-BE9B-1A652BB9F187@gmail.com/
  tony day.  PATCH: using ido when inserting links.
  Fri, 14 Sep 2012 18:58:43 +1000

Since auto-desc variable added by first commit was not removed
by second one, I assume that disabling the feature was a side effect
rather than the purpose.
---
 lisp/ol.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/ol.el b/lisp/ol.el
index 7e5398b22..20b085682 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -1878,7 +1878,12 @@ Use TAB to complete link prefixes, then RET for type-specific completion support
 		   "Link: "
 		   (append
 		    (mapcar (lambda (x) (concat x ":")) all-prefixes)
-		    (mapcar #'car org-stored-links))
+		    (mapcar #'car org-stored-links)
+                    ;; Allow description completion.  Avoid "nil" option
+                    ;; in the case of `completing-read-default' and
+                    ;; an error in `ido-completing-read' when some links
+                    ;; have no description.
+                    (delq nil (mapcar 'cadr org-stored-links)))
 		   nil nil nil
 		   'org-link--history
 		   (caar org-stored-links)))
-- 
2.25.1

Re: [PATCH] ol.el: Restore complete by description for insert link

[Tim Cross]

Max Nikulin <manikulin@gmail.com> writes:

> On 06/09/2022 21:34, Max Nikulin wrote:
>> Does anyone have an idea why it was necessary to drop completion of stored links based
>> on their description for the sake of ido?
>
> I have no idea what is the proper way to enable ido for `org-insert-link'. Functions and
> variables specific to ido were removed from Org.
> (ido-everywhere) and (ido-mode) are not enough.
>
> I tried
>
>     (setq-local completing-read-function #'ido-completing-read)
>
> and the command broke completing read completely.
>
>     (add-function :override completing-read-function #'ido-completing-read)
>
> inspired by `ido-everywhere' code broke M-x, but it enabled ido for `org-insert-link'.
>
> I believe that descriptions as completion options were removed because ido signals an
> error when nil is passed inside completion list. I consider it as a bug in ido (at least
> in Emacs-27), but even when `completing-read-default' is used, it causes appearance of
> undesired "nil" option. No description is a frequent case for links.
>
> So I am attaching a patch to restore completion of link targets by their description, nil
> descriptions are filtered out.
>
> The change is caused by the auto-desc local variable in `org-insert-link', its usage is
> rather strange and confusing currently.   Despite with this patch descriptions are
> restored, I believe that logic related to auto-desc should be removed, anyway it was
> broken for 10 years. I am unsure in which thread the next change should be discussed.
>
>> P.S. My question is related to the following threads:
>> - Carlos Pita. Adding target and custom id links doesn't ask for description. Tue, 2 Aug
>> 2022 14:44:58
>> -0300. https://list.orgmode.org/D99A712C-18D1-4A4F-8093-35A0BFB469C4@gmail.com
>> - Max Nikulin. Re: Bug: org-store-link uses CUSTOM_ID instead of target point. Sat, 6
>> Nov 2021 19:51:29
>> +0700. https://list.orgmode.org/e2c807a7-1924-6f08-9e63-4f70aee9d3b5@gmail.com
>
> [2. text/x-patch; 0001-ol.el-Restore-complete-by-description-for-insert-lin.patch]...

You don't appear to be getting a lot of feedback on this. However, I
think it is important work your doing. I suspect the lack of feedback is
partially due to Emacs' completion infrastructure being somewhat
confusing, combined with references to ido, which I suspect is one of
the less popular completion frameworks these days. .

My take on this, which might be completely wrong, is that org-mode
should not cater for or support any specific completion
framework. Things like ido, icomplete, fido, vertico, corfu, et. al. are
something which should be supported in a generic and abstract manner
i.e. we just provide minimal necessary code to generate the candidates
these systems use. From your description, I think this is what your
doing. Perhaps the requirements might become clearer if you also tried
other completion frameworks, like fido, icomplete and vertico.

I think there has been a fair bit of work in this area in recent
versions of Emacs (i.e. Emacs 28). I also quite like packages like
vertico, corfu and embark because unlike other frameworks, they work
hard to be based on just core Emacs features/functionality and avoid
adding new libraries or re-implementing existing functionality in a new
library. THis tends to make the smaller and with better core Emacs
compatibility (main reason I don't like helm and to a lesser extent,
ivy). 

Re: [PATCH] ol.el: Restore complete by description for insert link

[Ihor Radchenko]

Tim Cross <theophilusx@gmail.com> writes:

> You don't appear to be getting a lot of feedback on this. However, I
> think it is important work your doing. I suspect the lack of feedback is
> partially due to Emacs' completion infrastructure being somewhat
> confusing, combined with references to ido, which I suspect is one of
> the less popular completion frameworks these days. .

Not necessarily Emacs infrastructure. Org completion functions are
fairly confusing as well, which makes it difficult to provide
constructive feedback. We should really work towards (1) documenting the
expected features in Org completion functions; (2) unifying and
de-duplicating completion code across Org, like in
https://orgmode.org/list/87zgisvuu5.fsf@localhost

> My take on this, which might be completely wrong, is that org-mode
> should not cater for or support any specific completion
> framework. Things like ido, icomplete, fido, vertico, corfu, et. al. are
> something which should be supported in a generic and abstract manner
> i.e. we just provide minimal necessary code to generate the candidates
> these systems use. From your description, I think this is what your
> doing. Perhaps the requirements might become clearer if you also tried
> other completion frameworks, like fido, icomplete and vertico.

I'd say that we may still do it. At least, for the most common completion
frameworks. However, we should do everything to reduce the maintenance
overheads of such support.

If some non-standard completion framework offer extra functionality
compared to built-in, we can provide Org's framework that extends
completion-read to support the extra functionality.

What I have in mind is something like

(org-completing-read normal-args extra-args)

with extra-args only playing out when more featureful completion
framework is active.

However, it is critical not to require the normal Org code to account
for non-standard frameworks. All the frameword-specific handling should
be done in a separate localized function/library.

Feel free to refute.

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92

Re: [PATCH] ol.el: Restore complete by description for insert link

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> I believe that descriptions as completion options were removed because 
> ido signals an error when nil is passed inside completion list. I 
> consider it as a bug in ido (at least in Emacs-27), but even when 
> `completing-read-default' is used, it causes appearance of undesired 
> "nil" option. No description is a frequent case for links.

This sounds reasonable.

In general, there should be no need to care which completion backend is
used by Emacs - they should all plug into `completing-read' without
anything special to be done on Org side.

The only exception is possible not-yet-fixed bugs in earlier Emacs
versions. However, workaround for such issues should be clearly marked
by a comment in the code and removed once the problematic Emacs version
support is dropped.

> So I am attaching a patch to restore completion of link targets by their 
> description, nil descriptions are filtered out.

Thanks!
Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=0432f4fe6ba9b07c17ac555beab1527d8f844234

> The change is caused by the auto-desc local variable in 
> `org-insert-link', its usage is rather strange and confusing currently. 
>   Despite with this patch descriptions are restored, I believe that 
> logic related to auto-desc should be removed, anyway it was broken for 
> 10 years. I am unsure in which thread the next change should be discussed.

To be frank, org-insert-link should be eventually refactored or at least
better commented. This is one of the "old" functions in Org mode and its
code style is not ideal.

I think that the best approach to fix this and similar functions will be
discussing/documenting the overall design of the Org link
storage/completion. We can then work towards removing inconsistencies
and subtle bugs in the code.

Now, even not all the functionality of org-insert-link is described in
its docstring (like re-using selected region).

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92