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: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>> 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.

I'm afraid imenu-flatten=annotation has one more hurdle to overcome.
Consider:

1. emacs -Q
2. (setq imenu-flatten 'annotation)
2. C-x C-f .../lisp/imenu.el
3. M-g i
4. M-<down> M-<down> M-<down> M-<down> ...
5. Now the selected candidate in *Completions* is e.g. imenu--cleanup,
   this is also the minibuffer contents, so far so good.
6. Type M-RET to jump to this candidate.
7. Land on imenu instead of imenu--cleanup.

No matter how far down you go with M-<down>, if you go through the entry
for imenu, that's where you'll land when you hit M-RET.  This is because
imenu-flatten=annotation tries to identify candidates by their text
properties, but completion--replace retains common parts while replacing
minibuffer text, along with the text properties of these common parts.

This affects imenu-flatten=group too.

It might be possible to resolve this by setting a different
completion-list-insert-choice-function that circumvents replaces the
whole minibuffer contents, unlike completion--replace, which tries hard
to preserve markers etc.


Best,

Eshel

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

[Juri Linkov]

> I'm afraid imenu-flatten=annotation has one more hurdle to overcome.
> Consider:
>
> 1. emacs -Q
> 2. (setq imenu-flatten 'annotation)
> 2. C-x C-f .../lisp/imenu.el
> 3. M-g i
> 4. M-<down> M-<down> M-<down> M-<down> ...
> 5. Now the selected candidate in *Completions* is e.g. imenu--cleanup,
>    this is also the minibuffer contents, so far so good.
> 6. Type M-RET to jump to this candidate.
> 7. Land on imenu instead of imenu--cleanup.
>
> No matter how far down you go with M-<down>, if you go through the entry
> for imenu, that's where you'll land when you hit M-RET.  This is because
> imenu-flatten=annotation tries to identify candidates by their text
> properties, but completion--replace retains common parts while replacing
> minibuffer text, along with the text properties of these common parts.
>
> This affects imenu-flatten=group too.
>
> It might be possible to resolve this by setting a different
> completion-list-insert-choice-function that circumvents replaces the
> whole minibuffer contents, unlike completion--replace, which tries hard
> to preserve markers etc.

Maybe then imenu--completion-buffer should try to get the text property
from the end of completion?  Provided that completion--replace will keep
some properties at the end instead of using insert-and-inherit.

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

[Eshel Yaron]

Juri Linkov <juri@linkov.net> writes:

>> I'm afraid imenu-flatten=annotation has one more hurdle to overcome.
>> Consider:
>>
>> 1. emacs -Q
>> 2. (setq imenu-flatten 'annotation)
>> 2. C-x C-f .../lisp/imenu.el
>> 3. M-g i
>> 4. M-<down> M-<down> M-<down> M-<down> ...
>> 5. Now the selected candidate in *Completions* is e.g. imenu--cleanup,
>>    this is also the minibuffer contents, so far so good.
>> 6. Type M-RET to jump to this candidate.
>> 7. Land on imenu instead of imenu--cleanup.
>>
>> No matter how far down you go with M-<down>, if you go through the entry
>> for imenu, that's where you'll land when you hit M-RET.  This is because
>> imenu-flatten=annotation tries to identify candidates by their text
>> properties, but completion--replace retains common parts while replacing
>> minibuffer text, along with the text properties of these common parts.
>>
>> This affects imenu-flatten=group too.
>>
>> It might be possible to resolve this by setting a different
>> completion-list-insert-choice-function that circumvents replaces the
>> whole minibuffer contents, unlike completion--replace, which tries hard
>> to preserve markers etc.
>
> Maybe then imenu--completion-buffer should try to get the text property
> from the end of completion?  Provided that completion--replace will keep
> some properties at the end instead of using insert-and-inherit.

I'm not sure, I think that might work in some cases, but since
completion--replace also keeps the common suffix it may still suffer
from the same issue when going from candidate "foo-baz" to "bar-baz".

Then there's the problem of what happens when you simply type the
candidate you want to jump to, without completion.  For example
M-g i imenu RET goes to the imenu function when imenu-flatten=nil,
but if imenu-flatten=annotation then it goes to the defgroup.

What I would suggest is to ensure completion candidates are unambiguous
as strings rather than relying on text properties.  Namely:
- Restore imenu-flatten to a boolean option: either flat or nested.
- If imenu-flatten is non-nil, prepend the full prefix to each candidate,
  like imenu-flatten=prefix does currently.  This takes care of
  disambiguating the candidate strings.
- If completions-group is also non-nil, then group candidates according
  to their prefix and trim the prefix in the group-function when
  transforming candidates for display.


Best,

Eshel

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

[Juri Linkov]

>>> I'm afraid imenu-flatten=annotation has one more hurdle to overcome.
>>> Consider:
>>>
>>> 1. emacs -Q
>>> 2. (setq imenu-flatten 'annotation)
>>> 2. C-x C-f .../lisp/imenu.el
>>> 3. M-g i
>>> 4. M-<down> M-<down> M-<down> M-<down> ...
>>> 5. Now the selected candidate in *Completions* is e.g. imenu--cleanup,
>>>    this is also the minibuffer contents, so far so good.
>>> 6. Type M-RET to jump to this candidate.
>>> 7. Land on imenu instead of imenu--cleanup.
>>>
>>> No matter how far down you go with M-<down>, if you go through the entry
>>> for imenu, that's where you'll land when you hit M-RET.  This is because
>>> imenu-flatten=annotation tries to identify candidates by their text
>>> properties, but completion--replace retains common parts while replacing
>>> minibuffer text, along with the text properties of these common parts.
>>>
>>> This affects imenu-flatten=group too.
>>>
>>> It might be possible to resolve this by setting a different
>>> completion-list-insert-choice-function that circumvents replaces the
>>> whole minibuffer contents, unlike completion--replace, which tries hard
>>> to preserve markers etc.
>>
>> Maybe then imenu--completion-buffer should try to get the text property
>> from the end of completion?  Provided that completion--replace will keep
>> some properties at the end instead of using insert-and-inherit.
>
> I'm not sure, I think that might work in some cases, but since
> completion--replace also keeps the common suffix it may still suffer
> from the same issue when going from candidate "foo-baz" to "bar-baz".
>
> Then there's the problem of what happens when you simply type the
> candidate you want to jump to, without completion.  For example
> M-g i imenu RET goes to the imenu function when imenu-flatten=nil,
> but if imenu-flatten=annotation then it goes to the defgroup.
>
> What I would suggest is to ensure completion candidates are unambiguous
> as strings rather than relying on text properties.  Namely:

Unfortunately, that'd be a step back.  Maybe at least we could document
limitations that annotations work only by selection.

> - Restore imenu-flatten to a boolean option: either flat or nested.
> - If imenu-flatten is non-nil, prepend the full prefix to each candidate,
>   like imenu-flatten=prefix does currently.  This takes care of
>   disambiguating the candidate strings.

It can't be boolean, because a more reliable replacement for annotations
would be appending the suffix, i.e. by the new value 'suffix'.

> - If completions-group is also non-nil, then group candidates according
>   to their prefix and trim the prefix in the group-function when
>   transforming candidates for display.

The disadvantage of completions-group is its too wide coverage.
We should strive for more specific options where possible.

Or better: let's enable groups by category.  I don't know why we have
such a glaring omission that groups still can't be enabled by category.
This should be simple to implement:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index d8df001159f..f26a49a019f 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1809,7 +1809,8 @@ completion-all-sorted-completions
                                            minibuffer-completion-table
                                            minibuffer-completion-predicate))
              (sort-fun (completion-metadata-get all-md 'cycle-sort-function))
-             (group-fun (completion-metadata-get all-md 'group-function)))
+             (group-fun (completion-metadata-get all-md 'group-function))
+             (group (completion-metadata-get all-md 'group)))
         (when last
           (setcdr last nil)
 
@@ -1821,7 +1822,7 @@ completion-all-sorted-completions
 
           (cond
            (sort-fun (setq all (funcall sort-fun all)))
-           ((and completions-group group-fun)
+           ((and (or completions-group group) group-fun)
             ;; TODO: experiment with re-grouping here.  Might be slow
             ;; if the group-fun (given by the table and out of our
             ;; control) is slow and/or allocates too much.

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

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>>> Maybe then imenu--completion-buffer should try to get the text property
>>> from the end of completion?  Provided that completion--replace will keep
>>> some properties at the end instead of using insert-and-inherit.
>>
>> I'm not sure, I think that might work in some cases, but since
>> completion--replace also keeps the common suffix it may still suffer
>> from the same issue when going from candidate "foo-baz" to "bar-baz".
>>
>> Then there's the problem of what happens when you simply type the
>> candidate you want to jump to, without completion.  For example
>> M-g i imenu RET goes to the imenu function when imenu-flatten=nil,
>> but if imenu-flatten=annotation then it goes to the defgroup.

I forgot to mention that this also affects using previous minibuffer
inputs with M-p: the text properties aren't recorded in the history
list, so you get an ambiguous input that doesn't always take you to
where you went when you previously used that input.

>> What I would suggest is to ensure completion candidates are unambiguous
>> as strings rather than relying on text properties.  Namely:
>
> Unfortunately, that'd be a step back.  Maybe at least we could document
> limitations that annotations work only by selection.

Indeed my suggestion was to take a step back of sorts.  If it's too late
for that, I agree it's a good idea to document the known issues.
>
>> - Restore imenu-flatten to a boolean option: either flat or nested.
>> - If imenu-flatten is non-nil, prepend the full prefix to each candidate,
>>   like imenu-flatten=prefix does currently.  This takes care of
>>   disambiguating the candidate strings.
>
> It can't be boolean, because a more reliable replacement for annotations
> would be appending the suffix, i.e. by the new value 'suffix'.

That makes sense.

>> - If completions-group is also non-nil, then group candidates according
>>   to their prefix and trim the prefix in the group-function when
>>   transforming candidates for display.
>
> The disadvantage of completions-group is its too wide coverage.
> We should strive for more specific options where possible.

Right.  The advantage, on the other hand, is that you can toggle
completions-group on the fly in the minibuffer, either with
toggle-option or with a dedicated command.  The wide coverage of
completions-group means there's just one variable to toggle, always.

> Or better: let's enable groups by category.  I don't know why we have
> such a glaring omission that groups still can't be enabled by
> category.  This should be simple to implement:

Doesn't the group-function metadata entry give enough control already?
If a category or a specific completion table wants to disable grouping,
it can simply avoid providing a group-function.  If it wants to enable
grouping, then it does provide a group-function, and now it's up to the
user's preferences which they express by setting completions-group.
Maybe your suggestion yields more flexibility in some cases?


Best,

Eshel

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

[Juri Linkov]

> I forgot to mention that this also affects using previous minibuffer
> inputs with M-p: the text properties aren't recorded in the history
> list, so you get an ambiguous input that doesn't always take you to
> where you went when you previously used that input.

Do you mean that text properties are not saved between sessions?
Because in the same session text properties are preserved in the history.
(This assumes that you select candidates by completion selection UI.)

>> Unfortunately, that'd be a step back.  Maybe at least we could document
>> limitations that annotations work only by selection.
>
> Indeed my suggestion was to take a step back of sorts.  If it's too late
> for that, I agree it's a good idea to document the known issues.

Ok, so here is a patch for emacs-30:

diff --git a/lisp/imenu.el b/lisp/imenu.el
index 708a39a0f71..ae030e0104a 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -158,6 +158,9 @@ imenu-flatten
 with a suffix that is the section name to which it belongs.
 If the value is `group', split completion candidates into groups
 according to the sections.
+Since the values `annotation' and `group' rely on text properties,
+you can use them only by selecting candidates from the completions
+buffer, not by typing in the minibuffer.
 Any other value is treated as `prefix'.

>> It can't be boolean, because a more reliable replacement for annotations
>> would be appending the suffix, i.e. by the new value 'suffix'.
>
> That makes sense.

Ok, here is the implementation for emacs-30 where documentation could be
added later to not cause merge conflicts with the documentation changes above:

diff --git a/lisp/imenu.el b/lisp/imenu.el
index 708a39a0f71..31937d3f333 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -827,6 +830,9 @@ imenu--flatten-index-alist
                        ('group (propertize name
                                            'imenu-section (or prefix "*")
                                            'imenu-choice item))
+                       ('suffix (if prefix
+				    (concat name imenu-level-separator prefix)
+			          name))
                        (_ new-prefix))
 		     pos)))

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

[Eshel Yaron]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>> I forgot to mention that this also affects using previous minibuffer
>> inputs with M-p: the text properties aren't recorded in the history
>> list, so you get an ambiguous input that doesn't always take you to
>> where you went when you previously used that input.
>
> Do you mean that text properties are not saved between sessions?
> Because in the same session text properties are preserved in the history.
> (This assumes that you select candidates by completion selection UI.)

Hmm right, I probably didn't select the candidate from the completions
UI when I tested this scenario.

>>> Unfortunately, that'd be a step back.  Maybe at least we could document
>>> limitations that annotations work only by selection.
>>
>> Indeed my suggestion was to take a step back of sorts.  If it's too late
>> for that, I agree it's a good idea to document the known issues.
>
> Ok, so here is a patch for emacs-30:
>
> diff --git a/lisp/imenu.el b/lisp/imenu.el
> index 708a39a0f71..ae030e0104a 100644
> --- a/lisp/imenu.el
> +++ b/lisp/imenu.el
> @@ -158,6 +158,9 @@ imenu-flatten
>  with a suffix that is the section name to which it belongs.
>  If the value is `group', split completion candidates into groups
>  according to the sections.
> +Since the values `annotation' and `group' rely on text properties,
> +you can use them only by selecting candidates from the completions
> +buffer, not by typing in the minibuffer.
>  Any other value is treated as `prefix'.
>
>>> It can't be boolean, because a more reliable replacement for annotations
>>> would be appending the suffix, i.e. by the new value 'suffix'.
>>
>> That makes sense.
>
> Ok, here is the implementation for emacs-30 where documentation could be
> added later to not cause merge conflicts with the documentation changes above:
>
> diff --git a/lisp/imenu.el b/lisp/imenu.el
> index 708a39a0f71..31937d3f333 100644
> --- a/lisp/imenu.el
> +++ b/lisp/imenu.el
> @@ -827,6 +830,9 @@ imenu--flatten-index-alist
>                         ('group (propertize name
>                                             'imenu-section (or prefix "*")
>                                             'imenu-choice item))
> +                       ('suffix (if prefix
> +				    (concat name imenu-level-separator prefix)
> +			          name))
>                         (_ new-prefix))
>  		     pos)))

Both of these changes look like nice improvements to me :)


Cheers,

Eshel

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

[Juri Linkov]

>> @@ -827,6 +830,9 @@ imenu--flatten-index-alist
>>                         ('group (propertize name
>>                                             'imenu-section (or prefix "*")
>>                                             'imenu-choice item))
>> +                       ('suffix (if prefix
>> +				    (concat name imenu-level-separator prefix)
>> +			          name))
>>                         (_ new-prefix))
>>  		     pos)))
>
> Both of these changes look like nice improvements to me :)

Now the documentation change is pushed to emacs-30,
but implementation of the 'suffix' value is problematic
since I remembered it's ambiguous:
https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01224.html
So probably need two separate suffix values.

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

[Juri Linkov]

>> Both of these changes look like nice improvements to me :)
>
> Now the documentation change is pushed to emacs-30,
> but implementation of the 'suffix' value is problematic
> since I remembered it's ambiguous:
> https://lists.gnu.org/archive/html/emacs-devel/2024-05/msg01224.html
> So probably need two separate suffix values.

Ok, here are two new suffix values for both variants:

  "<IDENT> <SUB-CATEGORY-TOP> <SUB-CATEGORY-BOTTOM>"
and
  "<IDENT> <SUB-CATEGORY-BOTTOM> <SUB-CATEGORY-TOP>"

diff --git a/lisp/imenu.el b/lisp/imenu.el
index 8f1b1f22a67..99d9d4a5582 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -154,6 +154,10 @@ imenu-flatten
 If the value is `prefix', pop up the completion buffer with a
 flattened menu where section names are prepended to completion
 candidates as prefixes.
+If the value is `suffix', use section names appended to completion
+candidates as suffixes.
+The value `suffix-reverse' is like `suffix', but the section names
+are in reverse order.
 If the value is `annotation', annotate each completion candidate
 with a suffix that is the section name to which it belongs.
 If the value is `group', split completion candidates into groups
@@ -168,6 +172,8 @@ imenu-flatten
 names of completion candidates."
   :type '(choice (const :tag "Show nested list" nil)
                  (const :tag "Flat list with sections as prefix" prefix)
+                 (const :tag "Flat list with sections as suffix" suffix)
+                 (const :tag "Flat list with reverse sections as suffix" suffix-reverse)
                  (const :tag "Flat list annotated with sections" annotation)
                  (const :tag "Flat list grouped by sections" group))
   :version "30.1")
@@ -816,7 +822,9 @@ imenu--flatten-index-alist
 	    (pos (cdr item))
 	    (new-prefix (and concat-names
 			     (if prefix
-				 (concat prefix imenu-level-separator name)
+				 (if (eq imenu-flatten 'suffix-reverse)
+                                     (concat name imenu-level-separator prefix)
+                                   (concat prefix imenu-level-separator name))
 			       name))))
        (cond
 	((not (imenu--subalist-p item))
@@ -830,6 +838,9 @@ imenu--flatten-index-alist
                        ('group (propertize name
                                            'imenu-section (or prefix "*")
                                            'imenu-choice item))
+                       ('suffix (if prefix
+				    (concat name imenu-level-separator prefix)
+			          name))
                        (_ 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>,  Eli Zaretskii
>  <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 09 Aug 2024 09:59:40 +0300
> 
>    :type '(choice (const :tag "Show nested list" nil)
>                   (const :tag "Flat list with sections as prefix" prefix)
> +                 (const :tag "Flat list with sections as suffix" suffix)
> +                 (const :tag "Flat list with reverse sections as suffix" suffix-reverse)

"Reverse sections"?  Can we make this tag text more clear?

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

[Juri Linkov]

>>    :type '(choice (const :tag "Show nested list" nil)
>>                   (const :tag "Flat list with sections as prefix" prefix)
>> +                 (const :tag "Flat list with sections as suffix" suffix)
>> +                 (const :tag "Flat list with reverse sections as suffix" suffix-reverse)
>
> "Reverse sections"?  Can we make this tag text more clear?

I have no idea.  Another problem is that the tag is too long.
This will make it shorter: "Flat list with reverse suffix".
An unclear tag is not a problem since it's explained in the docstring.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: me@eshelyaron.com,  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Fri, 09 Aug 2024 19:10:09 +0300
> 
> >>    :type '(choice (const :tag "Show nested list" nil)
> >>                   (const :tag "Flat list with sections as prefix" prefix)
> >> +                 (const :tag "Flat list with sections as suffix" suffix)
> >> +                 (const :tag "Flat list with reverse sections as suffix" suffix-reverse)
> >
> > "Reverse sections"?  Can we make this tag text more clear?
> 
> I have no idea.

How about "Flat list, sections as suffix, in reverse order of sections."

> Another problem is that the tag is too long.

That usually means the option is over-engineered.

> This will make it shorter: "Flat list with reverse suffix".

I don't understand what us "reverse suffix".

AFAIU, the "reverse" part refers to order, not to the suffixes.

> An unclear tag is not a problem since it's explained in the docstring.

I disagree.  If an unclear tag would not be a problem, we could just
make the tag be the name of the corresponding symbol.

Completions group metadata [was: master 431f8ff1e38: * lisp/imenu.el: Support more values for imenu-flatten (bug#70846)]

[Juri Linkov]

>>> - If completions-group is also non-nil, then group candidates according
>>>   to their prefix and trim the prefix in the group-function when
>>>   transforming candidates for display.
>>
>> The disadvantage of completions-group is its too wide coverage.
>> We should strive for more specific options where possible.
>
> Right.  The advantage, on the other hand, is that you can toggle
> completions-group on the fly in the minibuffer, either with
> toggle-option or with a dedicated command.  The wide coverage of
> completions-group means there's just one variable to toggle, always.

Interesting.  Then this requires such precedence (from high to low):

buffer-local value of completions-group -> metadata -> default-value
of completions-group

>> Or better: let's enable groups by category.  I don't know why we have
>> such a glaring omission that groups still can't be enabled by
>> category.  This should be simple to implement:
>
> Doesn't the group-function metadata entry give enough control already?

It's not easy for users to customize group-function metadata
by writing own group function.  Whereas with a boolean its easy
to toggle it in the Customization UI for completion-category-overrides.

> If a category or a specific completion table wants to disable grouping,
> it can simply avoid providing a group-function.  If it wants to enable
> grouping, then it does provide a group-function, and now it's up to the
> user's preferences which they express by setting completions-group.
> Maybe your suggestion yields more flexibility in some cases?

It makes sense to provide a group-function disabled by default
to help users just to enable it in completion-category-overrides
instead of writing it from scratch.

Re: Completions group metadata [

[Juri Linkov]

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

>>>> - If completions-group is also non-nil, then group candidates according
>>>>   to their prefix and trim the prefix in the group-function when
>>>>   transforming candidates for display.
>>>
>>> The disadvantage of completions-group is its too wide coverage.
>>> We should strive for more specific options where possible.
>>
>> Right.  The advantage, on the other hand, is that you can toggle
>> completions-group on the fly in the minibuffer, either with
>> toggle-option or with a dedicated command.  The wide coverage of
>> completions-group means there's just one variable to toggle, always.
>
> Interesting.  Then this requires such precedence (from high to low):
>
> buffer-local value of completions-group -> metadata -> default-value
> of completions-group

The patch below doesn't yet implement this precedence since
I can't find a dedicated command that toggles completions-group
on the fly.  Such a command doesn't exist even in your branch
'feature/minibuffer-completion-enhancements'.  And anyway
this should be improved at the same time with introducing
a support for toggling the sorting order, layout, etc.

>>> Or better: let's enable groups by category.  I don't know why we have
>>> such a glaring omission that groups still can't be enabled by
>>> category.  This should be simple to implement:
>>
>> Doesn't the group-function metadata entry give enough control already?
>
> It's not easy for users to customize group-function metadata
> by writing own group function.  Whereas with a boolean its easy
> to toggle it in the Customization UI for completion-category-overrides.

Regarding the value 'group' of imenu-flatten, it can't be removed
since it's used in imenu--flatten-index-alist, so the patch
hard-codes ':group-p t' for the value 'group'.

>> If a category or a specific completion table wants to disable grouping,
>> it can simply avoid providing a group-function.  If it wants to enable
>> grouping, then it does provide a group-function, and now it's up to the
>> user's preferences which they express by setting completions-group.
>> Maybe your suggestion yields more flexibility in some cases?
>
> It makes sense to provide a group-function disabled by default
> to help users just to enable it in completion-category-overrides
> instead of writing it from scratch.

Here is the patch that supports

 (setopt completion-category-overrides '((unicode-name (group-p . t))))

without enabling the global 'completions-group'.


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

diff --git a/lisp/international/mule-cmds.el b/lisp/international/mule-cmds.el
index 7d784ef3b1b..e0c282a54e5 100644
--- a/lisp/international/mule-cmds.el
+++ b/lisp/international/mule-cmds.el
@@ -3260,8 +3260,7 @@ read-char-by-name
 		   (affixation-function
 		    . ,#'mule--ucs-names-affixation)
 		   (group-function
-		    . ,(when completions-group
-			 #'mule--ucs-names-group))
+		    . ,#'mule--ucs-names-group)
 		   (category . unicode-name))
 	       (complete-with-action action (ucs-names) string pred)))))
 	 (char
diff --git a/lisp/imenu.el b/lisp/imenu.el
index 8f1b1f22a67..6efa2d55966 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -770,11 +776,12 @@ imenu--completion-buffer
                                ,(lambda (s) (get-text-property
                                              0 'imenu-section s))))
                          ,@(when (eq imenu-flatten 'group)
-                             `(:group-function
-                               ,(lambda (s transform)
-                                  (if transform s
-                                    (get-text-property
-                                     0 'imenu-section s)))))))
+                             `( :group-p t
+                                :group-function
+                                ,(lambda (s transform)
+                                   (if transform s
+                                     (get-text-property
+                                      0 'imenu-section s)))))))
           (unless imenu-eager-completion-buffer
             (minibuffer-completion-help)))
       (setq name (completing-read prompt
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index e8e0f169197..1269bab0d40 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1231,6 +1231,10 @@ completion-category-overrides
                          (const :tag "Historical sorting"
                                 minibuffer-sort-by-history)
                          (function :tag "Custom function")))
+           (cons :tag "Use Groups"
+                 (const :tag "Enable groups."
+                        group-p)
+                 (boolean))
            (cons :tag "Completion Groups"
                  (const :tag "Select one value from the menu."
                         group-function)
@@ -1809,7 +1813,8 @@ completion-all-sorted-completions
                                            minibuffer-completion-table
                                            minibuffer-completion-predicate))
              (sort-fun (completion-metadata-get all-md 'cycle-sort-function))
-             (group-fun (completion-metadata-get all-md 'group-function)))
+             (group-fun (completion-metadata-get all-md 'group-function))
+             (group-p (completion-metadata-get all-md 'group-p)))
         (when last
           (setcdr last nil)
 
@@ -1821,7 +1826,7 @@ completion-all-sorted-completions
 
           (cond
            (sort-fun (setq all (funcall sort-fun all)))
-           ((and completions-group group-fun)
+           ((and (or completions-group group-p) group-fun)
             ;; TODO: experiment with re-grouping here.  Might be slow
             ;; if the group-fun (given by the table and out of our
             ;; control) is slow and/or allocates too much.
@@ -2610,7 +2615,9 @@ minibuffer-completion-help
              (ann-fun (completion-metadata-get all-md 'annotation-function))
              (aff-fun (completion-metadata-get all-md 'affixation-function))
              (sort-fun (completion-metadata-get all-md 'display-sort-function))
-             (group-fun (completion-metadata-get all-md 'group-function))
+             (group-p (completion-metadata-get all-md 'group-p))
+             (group-fun (when (or completions-group group-p)
+                          (completion-metadata-get all-md 'group-function)))
              (mainbuf (current-buffer))
              ;; If the *Completions* buffer is shown in a new
              ;; window, mark it as softly-dedicated, so bury-buffer in
diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 2ea5e36fa88..07b8ec83cf9 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -791,8 +791,7 @@ icomplete--augment
 by `group-function''s second \"transformation\" protocol."
   (let* ((aff-fun (completion-metadata-get md 'affixation-function))
          (ann-fun (completion-metadata-get md 'annotation-function))
-         (grp-fun (and completions-group
-                       (completion-metadata-get md 'group-function)))
+         (grp-fun (completion-metadata-get md 'group-function))
          (annotated
           (cond (aff-fun
            (funcall aff-fun prospects))

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

[Stefan Monnier]

>> What I would suggest is to ensure completion candidates are unambiguous
>> as strings rather than relying on text properties.  Namely:
> Unfortunately, that'd be a step back.  Maybe at least we could document
> limitations that annotations work only by selection.

How about we make them unambiguous by adding text *at the end* (and
only if there's ambiguity)?

The completion code assumes in all kinds of places that the same text
means the same thing, so using the same text to mean different
completion choices will get you in trouble in all kinds of corner (and
not so corner) cases.


        Stefan

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

[Juri Linkov]

>>> What I would suggest is to ensure completion candidates are unambiguous
>>> as strings rather than relying on text properties.  Namely:
>> Unfortunately, that'd be a step back.  Maybe at least we could document
>> limitations that annotations work only by selection.
>
> How about we make them unambiguous by adding text *at the end* (and
> only if there's ambiguity)?
>
> The completion code assumes in all kinds of places that the same text
> means the same thing, so using the same text to mean different
> completion choices will get you in trouble in all kinds of corner (and
> not so corner) cases.

Ok, this patch adds the suffix only to ambiguous items.

At least it helps to get the right item for the case when not selecting
candidates from the completions list.

OTOH, it still doesn't solve the original issue of using `M-<down> M-<down> M-RET`
where `completion--replace` keeps properties of previous completion strings
by using `insert-and-inherit`.

```
diff --git a/lisp/imenu.el b/lisp/imenu.el
index 8f1b1f22a67..e151a685ce5 100644
--- a/lisp/imenu.el
+++ b/lisp/imenu.el
@@ -740,6 +740,24 @@ imenu--completion-buffer
 
 Return one of the entries in index-alist or nil."
   ;; Create a list for this buffer only when needed.
+  (when (eq imenu-flatten 'annotation)
+    (let ((hash (make-hash-table :test 'equal)))
+      (dolist (item index-alist)
+        (puthash (car item) (1+ (gethash (car item) hash 0)) hash))
+      (setq index-alist
+            (mapcar (lambda (item)
+                      (if (> (gethash (car item) hash) 1)
+                          (cons (let ((ann (get-text-property
+                                            0 'imenu-section (car item))))
+                                  (if ann
+                                      (concat (propertize (car item)
+                                                          'imenu-section nil)
+                                              imenu-level-separator
+                                              (substring ann 1))
+                                    (car item)))
+                                (cdr item))
+                        item))
+                    index-alist))))
   (let ((name (thing-at-point 'symbol))
 	choice
 	(prepared-index-alist
```

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.