Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@jurta.org> writes:

> branch: master
> commit 431f8ff1e38ca4367637c6b9fbc25d13d6f760a7
> Author: Juri Linkov <juri@linkov.net>
> Commit: Juri Linkov <juri@linkov.net>
>
>     * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)
>
>     (imenu-flatten): Change type boolean to choice
>     of more values for prefix/suffix section names.
>     (imenu--completion-buffer): Add :annotation-function if
>     'imenu-flatten' is 'annotation'.

This is a nice option to have, unfortunately it doesn't play well with
the limitations of completing-read --- if we have two or more tree
branches that end with the same leaf (string), imenu-flatten=annotation
produces the same completion candidate string for multiple imenu items,
and choosing any of them loses information about the prefix and always
jumps to the first one.

For example, setting imenu-flatten to annotation seems to make it
impossible to jump to the second "Foo" heading in an Org buffer with the
following contents:

--8<---------------cut here---------------start------------->8---
* Bar
** Foo
* Baz
** Foo
--8<---------------cut here---------------end--------------->8---

What do you think about disambiguating just the duplicates in such cases
by adding some part of the prefix to the completion candidate string?


Best,

Eshel

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

> This is a nice option to have, unfortunately it doesn't play well with
> the limitations of completing-read --- if we have two or more tree
> branches that end with the same leaf (string), imenu-flatten=annotation
> produces the same completion candidate string for multiple imenu items,
> and choosing any of them loses information about the prefix and always
> jumps to the first one.
>
> For example, setting imenu-flatten to annotation seems to make it
> impossible to jump to the second "Foo" heading in an Org buffer with the
> following contents:
>
> * Bar
> ** Foo
> * Baz
> ** Foo
>
> What do you think about disambiguating just the duplicates in such cases
> by adding some part of the prefix to the completion candidate string?

I'm aware of this limitation, I tested it on function/variable ambiguity.
One way to disambiguate them is to use text properties
on the completion candidate strings.

But unfortunately read-from-minibuffer doesn't obey
a non-nil value of minibuffer-allow-text-properties,
because choose-completion unconditionally discards all properties
by substring-no-properties.

Maybe choose-completion should use substring instead of
substring-no-properties when minibuffer-allow-text-properties
is non-nil?

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

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

> One way to disambiguate them is to use text properties
> on the completion candidate strings.
>
> But unfortunately read-from-minibuffer doesn't obey
> a non-nil value of minibuffer-allow-text-properties,
> because choose-completion unconditionally discards all properties
> by substring-no-properties.
>
> Maybe choose-completion should use substring instead of
> substring-no-properties when minibuffer-allow-text-properties
> is non-nil?

Here is a new variable that helps to disambiguate completions with
the same name but different annotations by using text properties:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completion-allow-text-properties.patch --]
[-- Type: text/x-diff, Size: 3289 bytes --]

diff --git a/lisp/imenu.el b/lisp/imenu.el
index 93a544ff550..af344e5e250 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -729,6 +732,8 @@ imenu--completion-buffer
   ;; Create a list for this buffer only when needed.
   (let ((name (thing-at-point 'symbol))
 	choice
+	(minibuffer-allow-text-properties t)
+	(completion-allow-text-properties t)
 	(prepared-index-alist
 	 (if (not imenu-space-replacement) index-alist
 	   (mapcar
@@ -762,10 +768,12 @@ imenu--completion-buffer
 				  nil t nil 'imenu--history-list name)))
 
     (when (stringp name)
-      (setq choice (assoc name prepared-index-alist))
-      (if (imenu--subalist-p choice)
-	  (imenu--completion-buffer (cdr choice) prompt)
-	choice))))
+      (or (get-text-property 0 'imenu-choice name)
+	  (progn
+	    (setq choice (assoc name prepared-index-alist))
+	    (if (imenu--subalist-p choice)
+		(imenu--completion-buffer (cdr choice) prompt)
+	      choice))))))
 
 (defun imenu--mouse-menu (index-alist event &optional title)
   "Let the user select from a buffer index from a mouse menu.
@@ -800,7 +808,8 @@ imenu--flatten-index-alist
 	((not (imenu--subalist-p item))
 	 (list (cons (if (and (eq imenu-flatten 'annotation) prefix)
 			 (propertize name 'imenu-section
-				     (format " (%s)" prefix))
+				     (format " (%s)" prefix)
+				     'imenu-choice item)
 		       new-prefix)
 		     pos)))
 	(t
diff --git a/lisp/simple.el b/lisp/simple.el
index deab52c4201..fa180323963 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10102,6 +10102,14 @@ choose-completion-deselect-if-after
 
 This makes `completions--deselect' effective.")
 
+(defcustom completion-allow-text-properties nil
+  "Non-nil means `choose-completion' should not discard text properties.
+This also affects `completing-read' and any of the functions that do
+minibuffer input with completion."
+  :type 'boolean
+  :version "30.1"
+  :group 'completion)
+
 (defun choose-completion (&optional event no-exit no-quit)
   "Choose the completion at point.
 If EVENT, use EVENT's position to determine the starting position.
@@ -10123,7 +10131,9 @@ choose-completion
           (choice
            (if choose-completion-deselect-if-after
                (if-let ((str (get-text-property (posn-point (event-start event)) 'completion--string)))
-                   (substring-no-properties str)
+                   (if completion-allow-text-properties
+                       (substring str)
+                     (substring-no-properties str))
                  (error "No completion here"))
            (save-excursion
              (goto-char (posn-point (event-start event)))
@@ -10139,8 +10149,11 @@ choose-completion
                (setq beg (or (previous-single-property-change
                               beg 'completion--string)
                              beg))
-               (substring-no-properties
-                (get-text-property beg 'completion--string)))))))
+               (if completion-allow-text-properties
+                   (substring
+                    (get-text-property beg 'completion--string))
+                 (substring-no-properties
+                  (get-text-property beg 'completion--string))))))))
 
       (unless (buffer-live-p buffer)
         (error "Destination buffer is dead"))

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: emacs-devel@gnu.org
> Date: Tue, 14 May 2024 09:08:01 +0300
> 
> +(defcustom completion-allow-text-properties nil
> +  "Non-nil means `choose-completion' should not discard text properties.
> +This also affects `completing-read' and any of the functions that do
> +minibuffer input with completion."

This new user option should be announced in NEWS.

I also wonder whether it should be a user option, i.e. is it
reasonable that a user would like all of his/her completions to
preserve text properties?  Stefan, WDYT?

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Stefan Monnier]

>> +(defcustom completion-allow-text-properties nil
>> +  "Non-nil means `choose-completion' should not discard text properties.
>> +This also affects `completing-read' and any of the functions that do
>> +minibuffer input with completion."
>
> This new user option should be announced in NEWS.
>
> I also wonder whether it should be a user option, i.e. is it
> reasonable that a user would like all of his/her completions to
> preserve text properties?  Stefan, WDYT?

I can't see how it can make sense as a user-option, no.  AFAICT it's
a hack for specific front-ends or specific backends (or combinations
thereof) to work around limitations in the completion API (or maybe
rather to allow abusing the completion API as a selection API 🙂).


        Stefan

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

>>> +(defcustom completion-allow-text-properties nil
>>> +  "Non-nil means `choose-completion' should not discard text properties.
>>> +This also affects `completing-read' and any of the functions that do
>>> +minibuffer input with completion."
>>
>> This new user option should be announced in NEWS.
>>
>> I also wonder whether it should be a user option, i.e. is it
>> reasonable that a user would like all of his/her completions to
>> preserve text properties?  Stefan, WDYT?
>
> I can't see how it can make sense as a user-option, no.  AFAICT it's
> a hack for specific front-ends or specific backends (or combinations
> thereof) to work around limitations in the completion API (or maybe

Sorry, it was a mistake, it was intended as a counterpart of
minibuffer-allow-text-properties that is not a user option.

> rather to allow abusing the completion API as a selection API 🙂).

Indeed, this value of imenu-flatten can be unambiguous only
in case of the Selection API.  So completion-allow-text-properties
would be useful for users who prefer Selection API
to select a candidate with annotation.  And for
the Completion API another value of imenu-flatten
could be added that will add a suffix, so the annotation
will be part of the completion candidate string.

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Daniel Mendler via Emacs development discussions.]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> +(defcustom completion-allow-text-properties nil
>>> +  "Non-nil means `choose-completion' should not discard text properties.
>>> +This also affects `completing-read' and any of the functions that do
>>> +minibuffer input with completion."
>>
>> This new user option should be announced in NEWS.
>>
>> I also wonder whether it should be a user option, i.e. is it
>> reasonable that a user would like all of his/her completions to
>> preserve text properties?  Stefan, WDYT?
>
> I can't see how it can make sense as a user-option, no.  AFAICT it's
> a hack for specific front-ends or specific backends (or combinations
> thereof) to work around limitations in the completion API (or maybe
> rather to allow abusing the completion API as a selection API 🙂).

As far as I can tell, both Corfu and Company preserve the text
properties if a candidate is selected explicitly. The same could work
for default completion when operating in "selection mode", i.e., when
the user explicitly selects a candidate explicitly in the *Completions*
buffer. However in the regular mode of operation, where the user
step-wise builds up input in the minibuffer, text properties wouldn't
and shouldn't be preserved.

Preserving text properties during selection can be useful when
distinguishing equal candidates, even if regular completion will
continue to work as is. For example Eglot provides both snippet and
identifier candidates (which may be equal). Then the user can explicitly
select among them. Similarly, Eglot may return equal candidates for
overloaded methods with argument list expansion like in Java.

However I am not sure if my proposal would help with the Imenu duplicate
candidate issue you are discussing here. When completing, the first
equal candidate would win, but the user could perhaps opt-in to select
another one explicitly.

Daniel

FW: [External] : Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Drew Adams]

Sending again.  For some reason, "Reply All"
didn't include emacs-devel@gnu.org.

-----Original Message-----
From: Drew Adams Sent: Tuesday, May 14, 2024 4:16 PM
To: 'Daniel Mendler'; Stefan Monnier
Cc: Eli Zaretskii; Juri Linkov; me@eshelyaron.com; Dmitry Gutov 

> However in the regular mode of operation, where the user
> step-wise builds up input in the minibuffer, text properties wouldn't
> and shouldn't be preserved.

Why shouldn't they?

Maybe I don't understand the case you describe,
but isn't the caller of completing-read or
whatever completion function ultimately getting
a completion candidate returned from that
completion function?

If so, and if the completions are propertized
strings, why "shouldn't" that be returned as
the result?  What does it matter how or whether
thematching input was "stepwise built up in the
minibuffer"?

It's OK if some particular completion apparatus
_can't_ return a propertized string for some
reason.  But why should such a limitation be
elevated to a proscription that completion in
general "shouldn't" return a propertized string?

> Preserving text properties during selection can be useful when
> distinguishing equal candidates, even if regular completion will
> continue to work as is.

Yes, of course.  That's a main use case for
propertizing candidates.  E.g., a property
that holds a buffer position or that holds
the complete alist-element value (a string
candidate that holds all the info of the
entire "real" candidate).

(Icicles has been exploiting this "gimmick"
for 16 years now.)

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

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

>> +(defcustom completion-allow-text-properties nil
>> +  "Non-nil means `choose-completion' should not discard text properties.
>> +This also affects `completing-read' and any of the functions that do
>> +minibuffer input with completion."
>
> This new user option should be announced in NEWS.
>
> I also wonder whether it should be a user option

So here it's a variable, that will later help to select Imenu
completion candidates with same names from different groups.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: completion-allow-text-properties.patch --]
[-- Type: text/x-diff, Size: 4075 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index 34052764f5f..0db85410ebe 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1784,6 +1784,13 @@ A major mode based on the tree-sitter library for editing Lua files.
 
 ** Minibuffer and Completions
 
+*** New variable 'completion-allow-text-properties'.
+Like non-nil 'minibuffer-allow-text-properties' that doesn't discard
+text properties, it does the same by keeping text properties
+on the selected completion candidate.  So when these two variables
+both are non-nil then 'completing-read' returns a selected completion
+with the initial text properties kept intact.
+
 +++
 *** New global minor mode 'minibuffer-regexp-mode'.
 This is a minor mode for editing regular expressions in the minibuffer.
diff --git a/lisp/simple.el b/lisp/simple.el
index cdbbd876e3b..8793e590733 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10102,6 +10102,11 @@ choose-completion-deselect-if-after
 
 This makes `completions--deselect' effective.")
 
+(defvar completion-allow-text-properties nil
+  "Non-nil means `choose-completion' should not discard text properties.
+This affects `completing-read' and any of the functions that do
+minibuffer input with completion.")
+
 (defun choose-completion (&optional event no-exit no-quit)
   "Choose the completion at point.
 If EVENT, use EVENT's position to determine the starting position.
@@ -10122,8 +10127,10 @@ choose-completion
           (completion-no-auto-exit (if no-exit t completion-no-auto-exit))
           (choice
            (if choose-completion-deselect-if-after
-               (if-let ((str (get-text-property (posn-point (event-start event)) 'completion--string)))
-                   (substring-no-properties str)
+               (if-let ((str (get-text-property (posn-point (event-start event))
+                                                'completion--string)))
+                   (if completion-allow-text-properties str
+                     (substring-no-properties str))
                  (error "No completion here"))
            (save-excursion
              (goto-char (posn-point (event-start event)))
@@ -10139,8 +10146,9 @@ choose-completion
                (setq beg (or (previous-single-property-change
                               beg 'completion--string)
                              beg))
-               (substring-no-properties
-                (get-text-property beg 'completion--string)))))))
+               (let ((str (get-text-property beg 'completion--string)))
+                 (if completion-allow-text-properties str
+                   (substring-no-properties str))))))))
 
       (unless (buffer-live-p buffer)
         (error "Destination buffer is dead"))
diff --git a/lisp/imenu.el b/lisp/imenu.el
index ea097f5da3a..952c3dc8969 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -732,6 +732,8 @@ imenu--completion-buffer
   ;; Create a list for this buffer only when needed.
   (let ((name (thing-at-point 'symbol))
 	choice
+	(minibuffer-allow-text-properties t)
+	(completion-allow-text-properties t)
 	(prepared-index-alist
 	 (if (not imenu-space-replacement) index-alist
 	   (mapcar
@@ -765,10 +767,12 @@ imenu--completion-buffer
 				  nil t nil 'imenu--history-list name)))
 
     (when (stringp name)
-      (setq choice (assoc name prepared-index-alist))
-      (if (imenu--subalist-p choice)
-	  (imenu--completion-buffer (cdr choice) prompt)
-	choice))))
+      (or (get-text-property 0 'imenu-choice name)
+	  (progn
+	    (setq choice (assoc name prepared-index-alist))
+	    (if (imenu--subalist-p choice)
+		(imenu--completion-buffer (cdr choice) prompt)
+	      choice))))))
 
 (defun imenu--mouse-menu (index-alist event &optional title)
   "Let the user select from a buffer index from a mouse menu.
@@ -803,7 +807,8 @@ imenu--flatten-index-alist
 	((not (imenu--subalist-p item))
 	 (list (cons (if (and (eq imenu-flatten 'annotation) prefix)
 			 (propertize name 'imenu-section
-				     (format " (%s)" prefix))
+				     (format " (%s)" prefix)
+				     'imenu-choice item)
 		       new-prefix)
 		     pos)))
 	(t

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  me@eshelyaron.com,
>   emacs-devel@gnu.org
> Date: Wed, 15 May 2024 19:51:25 +0300
> 
> > I also wonder whether it should be a user option
> 
> So here it's a variable, that will later help to select Imenu
> completion candidates with same names from different groups.

Thanks, LGTM.

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>>> +(defcustom completion-allow-text-properties nil
>>> +  "Non-nil means `choose-completion' should not discard text properties.
>>> +This also affects `completing-read' and any of the functions that do
>>> +minibuffer input with completion."
>>
>> This new user option should be announced in NEWS.
>>
>> I also wonder whether it should be a user option
>
> So here it's a variable, that will later help to select Imenu
> completion candidates with same names from different groups.
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 34052764f5f..0db85410ebe 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1784,6 +1784,13 @@ A major mode based on the tree-sitter library for editing Lua files.
>
>  ** Minibuffer and Completions
>
> +*** New variable 'completion-allow-text-properties'.
> +Like non-nil 'minibuffer-allow-text-properties' that doesn't discard
> +text properties, it does the same by keeping text properties
> +on the selected completion candidate.  So when these two variables
> +both are non-nil then 'completing-read' returns a selected completion
> +with the initial text properties kept intact.

Note that when minibuffer-allow-text-properties is non-nil, you can
already get the same original text properties from completing-read if
you "select" your candidate by cycling, since that doesn't go through
choose-completion which strips text properties.  It feels a bit
surprising to have this separate variable that affects one kind of
selection ("choosing") and not other kinds ("cycling", "expanding").
IMO, it'd be better, if possible, to just cease stripping text
properties in choose-completion altogether.  Note that choose-completion
calls completion--replace to do the actual insertion, and that function
already respects minibuffer-allow-text-properties.

> --- a/lisp/imenu.el
> +++ b/lisp/imenu.el
> @@ -732,6 +732,8 @@ imenu--completion-buffer
>    ;; Create a list for this buffer only when needed.
>    (let ((name (thing-at-point 'symbol))
>       choice
> +	(minibuffer-allow-text-properties t)
> +	(completion-allow-text-properties t)

IIUC, these let-bindings around completing-read will affect all
recursive minibuffers too, and even completion-at-point completions if
you start editing another buffer before exiting the minibuffer.  Perhaps
we can use buffer-local bindings in the minibuffer and propagate them to
the completions list buffer when populating it instead of let-binding?


Best,

Eshel

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

>> +*** New variable 'completion-allow-text-properties'.
>> +Like non-nil 'minibuffer-allow-text-properties' that doesn't discard
>> +text properties, it does the same by keeping text properties
>> +on the selected completion candidate.  So when these two variables
>> +both are non-nil then 'completing-read' returns a selected completion
>> +with the initial text properties kept intact.
>
> Note that when minibuffer-allow-text-properties is non-nil, you can
> already get the same original text properties from completing-read if
> you "select" your candidate by cycling, since that doesn't go through
> choose-completion which strips text properties.  It feels a bit
> surprising to have this separate variable that affects one kind of
> selection ("choosing") and not other kinds ("cycling", "expanding").
> IMO, it'd be better, if possible, to just cease stripping text
> properties in choose-completion altogether.  Note that choose-completion
> calls completion--replace to do the actual insertion, and that function
> already respects minibuffer-allow-text-properties.

I agree that a new variable is unnecessary, so it would be better just
to preserve text properties in choose-completion unconditionally.
Unless there are objections this looks like the right thing to do.

>>    (let ((name (thing-at-point 'symbol))
>>       choice
>> +	(minibuffer-allow-text-properties t)
>> +	(completion-allow-text-properties t)
>
> IIUC, these let-bindings around completing-read will affect all
> recursive minibuffers too, and even completion-at-point completions if
> you start editing another buffer before exiting the minibuffer.  Perhaps
> we can use buffer-local bindings in the minibuffer and propagate them to
> the completions list buffer when populating it instead of let-binding?

This problem could be solved much easier if we avoid adding a new variable
completion-allow-text-properties.  Then we can set only the existing variable
minibuffer-allow-text-properties in the minibuffer.

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Stefan Monnier
>  <monnier@iro.umontreal.ca>,  emacs-devel@gnu.org
> Date: Thu, 16 May 2024 09:08:21 +0300
> 
> >> +*** New variable 'completion-allow-text-properties'.
> >> +Like non-nil 'minibuffer-allow-text-properties' that doesn't discard
> >> +text properties, it does the same by keeping text properties
> >> +on the selected completion candidate.  So when these two variables
> >> +both are non-nil then 'completing-read' returns a selected completion
> >> +with the initial text properties kept intact.
> >
> > Note that when minibuffer-allow-text-properties is non-nil, you can
> > already get the same original text properties from completing-read if
> > you "select" your candidate by cycling, since that doesn't go through
> > choose-completion which strips text properties.  It feels a bit
> > surprising to have this separate variable that affects one kind of
> > selection ("choosing") and not other kinds ("cycling", "expanding").
> > IMO, it'd be better, if possible, to just cease stripping text
> > properties in choose-completion altogether.  Note that choose-completion
> > calls completion--replace to do the actual insertion, and that function
> > already respects minibuffer-allow-text-properties.
> 
> I agree that a new variable is unnecessary, so it would be better just
> to preserve text properties in choose-completion unconditionally.
> Unless there are objections this looks like the right thing to do.

Does that mean completion candidates will always appear with their
original text properties?  If so, I don't think it's TRT in all cases.
Whether it's TRT depends on the use case, so a variable definitely
seems like the way to go.

However, AFAIU Eshel didn't mean to say we should always preserve text
properties, he said we already have a variable to indicate whether
properties are to be preserved.  So the issue, AFAIU, is whether we
need _another_ variable, or should use a single one in both cases.

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

>> > Note that when minibuffer-allow-text-properties is non-nil, you can
>> > already get the same original text properties from completing-read if
>> > you "select" your candidate by cycling, since that doesn't go through
>> > choose-completion which strips text properties.  It feels a bit
>> > surprising to have this separate variable that affects one kind of
>> > selection ("choosing") and not other kinds ("cycling", "expanding").
>> > IMO, it'd be better, if possible, to just cease stripping text
>> > properties in choose-completion altogether.  Note that choose-completion
>> > calls completion--replace to do the actual insertion, and that function
>> > already respects minibuffer-allow-text-properties.
>>
>> I agree that a new variable is unnecessary, so it would be better just
>> to preserve text properties in choose-completion unconditionally.
>> Unless there are objections this looks like the right thing to do.
>
> Does that mean completion candidates will always appear with their
> original text properties?  If so, I don't think it's TRT in all cases.
> Whether it's TRT depends on the use case, so a variable definitely
> seems like the way to go.

Like Eshel noted, the text properties are already discarded from the completion
candidate by default since the default value of minibuffer-allow-text-properties
is nil.

> However, AFAIU Eshel didn't mean to say we should always preserve text
> properties, he said we already have a variable to indicate whether
> properties are to be preserved.  So the issue, AFAIU, is whether we
> need _another_ variable, or should use a single one in both cases.

Indeed, the existing variable minibuffer-allow-text-properties
can be used for both cases.  So when choose-completion will preserve
text properties, then completion--replace will decide whether to
discard them based on the value of minibuffer-allow-text-properties.

But there is another separate problem: Eshel asked to replace
let-bindings around completing-read with the minibuffer-local value
of minibuffer-allow-text-properties.  However, this is impossible to do
because read-from-minibuffer called from completing-read can't use
a minibuffer-local value of minibuffer-allow-text-properties.

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Stefan Monnier]

> But there is another separate problem: Eshel asked to replace
> let-bindings around completing-read with the minibuffer-local value
> of minibuffer-allow-text-properties.  However, this is impossible to do
> because read-from-minibuffer called from completing-read can't use
> a minibuffer-local value of minibuffer-allow-text-properties.

It could use something like `minibuffer-with-setup-hook`, no?
This might be a good opportunity to allow the INIT argument to be
a function (run within the fresh new minibuffer).


        Stefan

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

>> But there is another separate problem: Eshel asked to replace
>> let-bindings around completing-read with the minibuffer-local value
>> of minibuffer-allow-text-properties.  However, this is impossible to do
>> because read-from-minibuffer called from completing-read can't use
>> a minibuffer-local value of minibuffer-allow-text-properties.
>
> It could use something like `minibuffer-with-setup-hook`, no?
> This might be a good opportunity to allow the INIT argument to be
> a function (run within the fresh new minibuffer).

I see no way of doing that, this doesn't work:

  (minibuffer-with-setup-hook
      (lambda ()
        (setq-local minibuffer-allow-text-properties t))
    (completing-read "Prompt: " (list (propertize "foo1" 'prop t)
                                      (propertize "foo2" 'prop t))))

because when `completing-read-default` calls `read-from-minibuffer`,
it uses the value of `minibuffer-allow-text-properties` from
the original buffer:

  val = read_minibuf (keymap, initial_contents, prompt,
		      !NILP (read),
		      histvar, histpos, default_value,
		      minibuffer_allow_text_properties,
		      !NILP (inherit_input_method));

Then `read_minibuf` uses its argument `allow_props`:

  if (allow_props)
    val = Fminibuffer_contents ();
  else
    val = Fminibuffer_contents_no_properties ();

Maybe it could use `minibuffer-allow-text-properties` directly here?

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Stefan Monnier]

> because when `completing-read-default` calls `read-from-minibuffer`,
> it uses the value of `minibuffer-allow-text-properties` from
> the original buffer:
>
>   val = read_minibuf (keymap, initial_contents, prompt,
> 		      !NILP (read),
> 		      histvar, histpos, default_value,
> 		      minibuffer_allow_text_properties,
> 		      !NILP (inherit_input_method));

Damn!  This is too bad: a buffer-local setting of
`minibuffer_allow_text_properties` can basically never be used then,
because it's read from the wrong buffer.

> Then `read_minibuf` uses its argument `allow_props`:
>
>   if (allow_props)
>     val = Fminibuffer_contents ();
>   else
>     val = Fminibuffer_contents_no_properties ();
>
> Maybe it could use `minibuffer-allow-text-properties` directly here?

Indeed: since a buffer-local setting can't work, we know that all
callers must use a plain let-binding so the binding will be active
during the whole minibuffer session, so we may as well read it at the
end (in the minibuffer) rather than at the beginning (in the
`minibuffer--original-buffer`).


        Stefan


PS: Git shows that `minibuffer-allow-text-properties` was introduced
eons ago (and basically never touched since then, even the docstring is
mostly unchanged).  And Grep shows it's not used very often (and
several of those uses are around `completing-read`).
Funnily enough, one of the few uses in our tree binds it to nil,
I wonder why that was needed.

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

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

>> because when `completing-read-default` calls `read-from-minibuffer`,
>> it uses the value of `minibuffer-allow-text-properties` from
>> the original buffer:
>>
>>   val = read_minibuf (keymap, initial_contents, prompt,
>> 		      !NILP (read),
>> 		      histvar, histpos, default_value,
>> 		      minibuffer_allow_text_properties,
>> 		      !NILP (inherit_input_method));
>
> Damn!  This is too bad: a buffer-local setting of
> `minibuffer_allow_text_properties` can basically never be used then,
> because it's read from the wrong buffer.
>
>> Then `read_minibuf` uses its argument `allow_props`:
>>
>>   if (allow_props)
>>     val = Fminibuffer_contents ();
>>   else
>>     val = Fminibuffer_contents_no_properties ();
>>
>> Maybe it could use `minibuffer-allow-text-properties` directly here?
>
> Indeed: since a buffer-local setting can't work, we know that all
> callers must use a plain let-binding so the binding will be active
> during the whole minibuffer session, so we may as well read it at the
> end (in the minibuffer) rather than at the beginning (in the
> `minibuffer--original-buffer`).

Ok, then here is a complete patch with documentation updates
where many documentation changes are fixing the documentation
to describe the current behavior existed even before applying
these code changes.

> PS: Git shows that `minibuffer-allow-text-properties` was introduced
> eons ago (and basically never touched since then, even the docstring is
> mostly unchanged).  And Grep shows it's not used very often (and
> several of those uses are around `completing-read`).
> Funnily enough, one of the few uses in our tree binds it to nil,
> I wonder why that was needed.

Probably the nil let-binding was intended to negate the effect
of a non-nil let-binding higher in the stack of recursive minibuffers.
So this is another reason to prefer buffer-local settings.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: minibuffer_allow_text_properties.patch --]
[-- Type: text/x-diff, Size: 8774 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index 4e52d4dccb2..cefbc0eb938 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2033,6 +2033,17 @@ UTF-8 byte sequence, and the optional parameter MULTIBYTE of
 'dbus-string-to-byte-array' should be a regular Lisp string, not a
 unibyte string.
 
++++
+** 'minibuffer-allow-text-properties' now can be set buffer-local.
+'read-from-minibuffer' and functions that use it can take the
+buffer-local value from the minibuffer.
+
++++
+** 'minibuffer-allow-text-properties' also affects completions.
+When it has a non-nil value, then completion functions like
+'completing-read' don't discard text properties from the returned
+completion candidate.
+
 \f
 * Lisp Changes in Emacs 30.1
 
diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index 8f2d0d702f9..af5552851e2 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -185,7 +185,8 @@ Text from Minibuffer
 History}.
 
 If the variable @code{minibuffer-allow-text-properties} is
-non-@code{nil}, then the string that is returned includes whatever text
+non-@code{nil}, either let-bound or buffer-local in the minibuffer,
+then the string that is returned includes whatever text
 properties were present in the minibuffer.  Otherwise all the text
 properties are stripped when the value is returned.  (By default this
 variable is @code{nil}.)
@@ -352,28 +353,27 @@ Text from Minibuffer
 
 @defvar minibuffer-allow-text-properties
 If this variable is @code{nil}, the default, then
-@code{read-from-minibuffer} and @code{read-string} strip all text
+@code{read-from-minibuffer}, @code{read-string}, and all
+functions that do minibuffer input with completion strip all text
 properties from the minibuffer input before returning it.  However,
-@code{read-no-blanks-input} (see below), as well as
 @code{read-minibuffer} and related functions (@pxref{Object from
-Minibuffer,, Reading Lisp Objects With the Minibuffer}), and all
-functions that do minibuffer input with completion, remove the
+Minibuffer,, Reading Lisp Objects With the Minibuffer}), remove the
 @code{face} property unconditionally, regardless of the value of this
 variable.
 
-If this variable is non-@code{nil}, most text properties on strings
-from the completion table are preserved---but only on the part of the
-strings that were completed.
+If this variable is non-@code{nil}, either let-bound or buffer-local in
+the minibuffer, most text properties on strings from the completion
+table are preserved---but only on the part of the strings that were
+completed.
 
 @lisp
 (let ((minibuffer-allow-text-properties t))
   (completing-read "String: " (list (propertize "foobar" 'data 'zot))))
-=> #("foobar" 3 6 (data zot))
+=> #("foobar" 0 6 (data zot))
 @end lisp
 
 In this example, the user typed @samp{foo} and then hit the @kbd{TAB}
-key, so the text properties are only preserved on the last three
-characters.
+key, so the text properties are preserved on all characters.
 @end defvar
 
 @vindex minibuffer-mode-map
@@ -433,18 +433,6 @@ Text from Minibuffer
 keymap as the @var{keymap} argument for that function.  Since the keymap
 @code{minibuffer-local-ns-map} does not rebind @kbd{C-q}, it @emph{is}
 possible to put a space into the string, by quoting it.
-
-This function discards text properties, regardless of the value of
-@code{minibuffer-allow-text-properties}.
-
-@smallexample
-@group
-(read-no-blanks-input @var{prompt} @var{initial})
-@equiv{}
-(let (minibuffer-allow-text-properties)
-  (read-from-minibuffer @var{prompt} @var{initial} minibuffer-local-ns-map))
-@end group
-@end smallexample
 @end defun
 
 @c Slightly unfortunate name, suggesting it might be related to the
diff --git a/src/minibuf.c b/src/minibuf.c
index 9c1c86680d4..f2a76086361 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -563,7 +563,8 @@ DEFUN ("minibuffer-contents-no-properties", Fminibuffer_contents_no_properties,
 
    DEFALT specifies the default value for the sake of history commands.
 
-   If ALLOW_PROPS, do not throw away text properties.
+   If ALLOW_PROPS or `minibuffer-allow-text-properties' is non-nil,
+   do not throw away text properties.
 
    if INHERIT_INPUT_METHOD, the minibuffer inherits the
    current input method.  */
@@ -928,7 +929,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 
   /* Make minibuffer contents into a string.  */
   Fset_buffer (minibuffer);
-  if (allow_props)
+  if (allow_props || minibuffer_allow_text_properties)
     val = Fminibuffer_contents ();
   else
     val = Fminibuffer_contents_no_properties ();
@@ -1321,7 +1322,8 @@ DEFUN ("read-from-minibuffer", Fread_from_minibuffer,
 Seventh arg INHERIT-INPUT-METHOD, if non-nil, means the minibuffer inherits
  the current input method and the setting of `enable-multibyte-characters'.
 
-If the variable `minibuffer-allow-text-properties' is non-nil,
+If the variable `minibuffer-allow-text-properties' is non-nil
+ (either let-bound or buffer-local in the minibuffer),
  then the string which is returned includes whatever text properties
  were present in the minibuffer.  Otherwise the value has no text properties.
 
@@ -2464,9 +2466,10 @@ syms_of_minibuf (void)
   DEFVAR_BOOL ("minibuffer-allow-text-properties",
 	       minibuffer_allow_text_properties,
 	       doc: /* Non-nil means `read-from-minibuffer' should not discard text properties.
-This also affects `read-string', but it does not affect `read-minibuffer',
-`read-no-blanks-input', or any of the functions that do minibuffer input
-with completion; they always discard text properties.  */);
+The value could be let-bound or buffer-local in the minibuffer.
+This also affects `read-string', or any of the functions that do
+minibuffer input with completion, but it does not affect `read-minibuffer'
+that always discards text properties.  */);
   minibuffer_allow_text_properties = 0;
 
   DEFVAR_LISP ("minibuffer-prompt-properties", Vminibuffer_prompt_properties,
diff --git a/lisp/simple.el b/lisp/simple.el
index bcd26da13ed..8fbfa683e97 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10127,9 +10127,9 @@ choose-completion
           (completion-no-auto-exit (if no-exit t completion-no-auto-exit))
           (choice
            (if choose-completion-deselect-if-after
-               (if-let ((str (get-text-property (posn-point (event-start event)) 'completion--string)))
-                   (substring-no-properties str)
-                 (error "No completion here"))
+               (or (get-text-property (posn-point (event-start event))
+                                      'completion--string)
+                   (error "No completion here"))
            (save-excursion
              (goto-char (posn-point (event-start event)))
              (let (beg)
@@ -10144,8 +10144,7 @@ choose-completion
                (setq beg (or (previous-single-property-change
                               beg 'completion--string)
                              beg))
-               (substring-no-properties
-                (get-text-property beg 'completion--string)))))))
+               (get-text-property beg 'completion--string))))))
 
       (unless (buffer-live-p buffer)
         (error "Destination buffer is dead"))
diff --git a/lisp/imenu.el b/lisp/imenu.el
index ea097f5da3a..93d84106ec1 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -752,6 +752,7 @@ imenu--completion-buffer
     ;; Display the completion buffer.
     (minibuffer-with-setup-hook
         (lambda ()
+          (setq-local minibuffer-allow-text-properties t)
           (setq-local completion-extra-properties
                       `( :category imenu
                          ,@(when (eq imenu-flatten 'annotation)
@@ -765,10 +766,12 @@ imenu--completion-buffer
 				  nil t nil 'imenu--history-list name)))
 
     (when (stringp name)
-      (setq choice (assoc name prepared-index-alist))
-      (if (imenu--subalist-p choice)
-	  (imenu--completion-buffer (cdr choice) prompt)
-	choice))))
+      (or (get-text-property 0 'imenu-choice name)
+	  (progn
+	    (setq choice (assoc name prepared-index-alist))
+	    (if (imenu--subalist-p choice)
+		(imenu--completion-buffer (cdr choice) prompt)
+	      choice))))))
 
 (defun imenu--mouse-menu (index-alist event &optional title)
   "Let the user select from a buffer index from a mouse menu.
@@ -798,7 +801,9 @@ imenu--flatten-index-alist
 	    (new-prefix (and concat-names
 			     (if prefix
 				 (concat prefix imenu-level-separator name)
-			       name))))
+			       (if (eq imenu-flatten 'annotation)
+                                   (propertize name 'imenu-choice item)
+                                 name)))))
        (cond
 	((not (imenu--subalist-p item))
 	 (list (cons (if (and (eq imenu-flatten 'annotation) prefix)

Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Juri Linkov]

>>> Maybe it could use `minibuffer-allow-text-properties` directly here?
>>
>> Indeed: since a buffer-local setting can't work, we know that all
>> callers must use a plain let-binding so the binding will be active
>> during the whole minibuffer session, so we may as well read it at the
>> end (in the minibuffer) rather than at the beginning (in the
>> `minibuffer--original-buffer`).
>
> Ok, then here is a complete patch with documentation updates
> where many documentation changes are fixing the documentation
> to describe the current behavior existed even before applying
> these code changes.

So this patch is pushed now.

RE: [External] : Re: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Drew Adams]

> Here is a new variable that helps to disambiguate completions with
> the same name but different annotations by using text properties:

> (defcustom completion-allow-text-properties nil
>   "Non-nil means `choose-completion' should not
> discard text properties.
> This also affects `completing-read' and any of the
> functions that do
> minibuffer input with completion."
>  :type 'boolean :version "30.1" :group 'completion)
___

FWIW, since 2008 Icicles has had this feature.

I tried several times, unsuccessfully, to
persuade vanilla Emacs to add it, i.e., to
allow the result of completion to keep text
properties, unless a user option says to
strip them.

IOW, the default behavior in Icicles is
to _keep_, not to remove, text properties.

Emacs should do the same, in order to, as
you say, let code "disambiguate completions
with the same name".  Hard to believe this
hasn't even been possible till now, let
alone been done by default.
___

`icicle-unpropertize-completion-result-flag' is a variable
defined in `icicles-opt.el'.

Its value is nil

Documentation:

Non-nil means strip text properties from the completion result.
Set or bind this option to non-nil only if you need to ensure, for
some other library, that the string returned by `completing-read' and
`read-file-name' has no text properties.

Typically, you will not use a non-nil value.  Internal text properties
added by Icicles are always removed anyway.  A non-nil value lets you
also remove properties such as `face'.
___

As for whether a user option is appropriate: yes.
But I don't take the view that no code should
ever bind a user option.