Improvement proposals for `completing-read'

Improvement proposals for `completing-read'

[Daniel Mendler]

While working on a set of packages around completion ([Selectrum],
[Consult], [Marginalia], [Embark], [Orderless], [Vertico], ...) it
became clear that there are a few spots where improvements to the
`completing-read' API can be made. After submitting my Vertico package
to GNU ELPA two days ago there has been the follow-up discussion in the
thread "Stepping Back: A Wealth Of Completion systems", where the
potential introduction of a `selecting-read' function has been proposed
by Philip Kaludercic. I want to respond to that and echo the opinion
stated by Stefan Monnier:

       I kind of agree, yet at the same time, the difference
       between the two is very small in most cases. So I think it
       might be worthwhile to look at it instead as making it
       possible for the caller to give a *hint* about what kind of
       UI would be best.

       It might deserve a completely new function to replace
       `completing-read', but I think it would be good to make this
       new function such that it makes `completing-read' obsolete.

The cost of introduction of a new function is not to be underestimated
in particular if it sits at a central place. Introducing another
function which only differs slightly from the existing `completing-read'
function could do more harm than good if it increases inconsistency and
the effort for the implementors of completion UIs. If a replacement can
be made which supersedes `completing-read' fully, this cost is reduced,
but such a change is still very impactful given that many packages use
`completing-read'.

I see `completing-read' as a comparatively complicated API, which is
hard use and hard to implement in its full extent. For the end-user it
helps to wrap the function in a more friendly API. Eric Abrahamsen has
used such an approach in his Gnorb package, see
`gnorb-select-from-list'. I have done the same in my Consult package,
see `consult--read'. Maybe moving to a more comfortable (but mostly
`completing-read' compatible) API would mitigate the usability issues
people are observing?

For now I want to be a bit more concrete and look at `completing-read'
and `minibuffer.el' directly and propose a few moderate improvements.
The improvements can help package authors in the short term, since they
address issues with the current state of the API. I am willing to
follow-up with patches, which implement the proposals.

The proposals have been inspired by the work on the aforementioned
packages and came to light in discussions with the respective authors of
the packages: Clemens Radermacher of Selectrum, Omar Antolín Camarena of
Embark and Orderless, and also Dmitry Gutov and Protesilaos Stavrou.

I am very much interested in your opinion regarding the following
proposals.

Daniel Mendler


[Selectrum] <https://github.com/raxod502/selectrum>

[Consult] <https://github.com/minad/consult>

[Marginalia] <https://github.com/minad/marginalia>

[Embark] <https://github.com/oantolin/embark>

[Orderless] <https://github.com/oantolin/orderless>

[Vertico] <https://github.com/minad/vertico>


Improvement proposals for `completing-read'
===========================================

   .. Proposal 1: Disabling the history
   .. Proposal 2: More efficient sorting
   .. Proposal 3: Sort file names by history
   .. Proposal 4: Add support for `group-function'
   .. Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
   .. Proposal 6: Return text properties from `completing-read'
   .. Proposal 7: Allow duplicates and retain object identity
   .. Proposal 8: Completion style optimization of filtering and 
highlighting


Proposal 1: Disabling the history
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   I propose to allow the symbol `t' as `HIST' argument to
   `completing-read' in order to disable the history. This aligns
   `completing-read' with `read-from-minibuffer'. This change requires a
   modification of the function `completion-all-sorted-completions',
   which sorts the candidates by history position and currently assumes
   that the `minibuffer-history-variable' is a variable symbol.

   Example:
   ,----
   | (completing-read "No History: " '(first second third) nil nil nil t)
   `----


Proposal 2: More efficient sorting
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   Currently candidate sorting is `O(m*n*log(n))' with history length m
   and candidate list length n. This leads to a noticeable slowdown for
   large values of `history-length'. I propose to improve the speed of
   `completion-all-sorted-completions' by using a hash table. The Vertico
   package provides an optimized sorting function, see `vertico--sort'.


Proposal 3: Sort file names by history
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   Currently `completion-all-sorted-completions' does not sort file
   candidates based on history, when the candidates are file names and
   the history elements are paths. The Vertico package contains the
   function `vertico--sort' which special cases file names. This approach
   could be adopted by `completion-all-sorted-completions'.

   Unfortunately `vertico--sort' does not sort file names by history if
   the file names are completed using `partial-completion' and I have not
   found an efficient way to do that. More generally, one may want to
   sort completion candidates if `completion-boundaries' are used by the
   `minibuffer-completion-table'. However since sorting files already
   requires special casing and files are most important, I did not
   implement this in `vertico--sort'.


Proposal 4: Add support for `group-function'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   Completion tables can provide the functions `display-sort-function',
   `cycle-sort-function', `annotation-function' and `affixation-function'
   as completion metadata. I am proposing the addition of a
   `group-function', which receives a list of candidates and returns a
   list of groups `((group-title . group-candidates) ...)'. The group
   function should retain the present order of the candidates within the
   groups.

   This function is used in two ways. After sorting the candidates, the
   group function is called with the candidate list in order to produce a
   grouped list. Then the completion UI can call the group function a
   second time when displaying the candidate groups in order to determine
   the group titles. This is useful for example if candidates originate
   from different sources. Grouping is popular in Helm, for example as
   seen in [helm-m-x].

   For now, I implemented `group-function' support under the name
   `x-group-function' in the Vertico package. My Consult package uses
   this functionality for candidates originating from [multiple sources].

   Grouping can be added to the default completion system by modifying
   `completion-all-sorted-completions' and `completion--insert-strings'.
   A proof of concept can be found on the [Consult wiki].

   Furthermore support for `x-group-function' has been added to
   [Selectrum] and the external [Icomplete-vertical] package, which is to
   be distinguished from the Icomplete-vertical patches proposed on
   emacs-devel.


[helm-m-x] <https://tuhdo.github.io/static/part3/helm-m-x.gif>

[multiple sources] <https://github.com/minad/consult#multiple-sources>

[Consult wiki] <https://github.com/minad/consult/wiki/Grouping-support>

[Selectrum] <https://github.com/raxod502/selectrum>

[Icomplete-vertical] <https://github.com/oantolin/icomplete-vertical>


Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   If `completing-read' is passed `t' as argument to `REQUIRE-MATCH', the
   completion must match a candidate from the completion table. However
   there is also the possibility to exit the minibuffer with a null
   completion. I propose to disallow this possibility. This change makes
   it easier for users of `completing-read' since they can rely on
   setting `REQUIRE-MATCH' to `t' and do not have to handle the null
   completion specially.

   Note that null completion cannot occur if a default value is passed to
   `completing-read'. Therefore null completion can be avoided by a
   wrapper, which passes a special default argument. Unfortunately this
   leads to flicker.

   ,----
   | (defun completing-read-non-null (prompt table)
   |   (let ((ret))
   |     (while (eq (setq ret (completing-read prompt table nil t nil nil
   |                                           'null-completion))
   |                'null-completion))
   |     ret))
   | (completing-read-non-null "Prompt: " '(first second third))
   `----

   If it is desired to avoid a backward incompatible change one could
   consider adding a new special value for `REQUIRE-MATCH', for example
   `'require-candidate', which explicitly forbids null completion.


Proposal 6: Return text properties from `completing-read'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   As of now, `completing-read' strips text properties from the result
   string. This behavior is reasonable for strings, which are not present
   in the collection if `REQUIRE-MATCH/=t'. However if a candidate string
   is selected, which is a member of the collection, the text properties
   can be retained. If this is implemented and `REQUIRE-MATCH=t', the
   user of `completing-read' can always rely on text properties to attach
   arbitrary data to the candidates. When storing the selected candidate
   in the history, the text properties should still be stripped to avoid
   serialization problems. There is still null completion, see Proposal
   5. As of now the user has to look up the associated data in an an
   association list, but there is the more serious issue of duplicate
   candidates as addressed in the next section.

   Example:
   ,----
   | (completing-read "Text properties: "
   |                  (list (propertize "first" 'id 0)
   |                        (propertize "second" 'id 1)
   |                        (propertize "third" 'id 2))
   |                  nil t)
   `----


Proposal 7: Allow duplicates and retain object identity
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   If a completion table contains duplicates, these duplicates should not
   be removed. There are not many completion tables which generate
   duplicate candidates and there exist multiple completion systems which
   do not perform deduplication at all.

   It is reasonable to handle the deduplication on a case by case basis
   in each completion table. Furthermore empty strings are removed by
   default completion for some reason. I rather consider this a bug in a
   completion table if it returns undesired empty candidates.

   Allowing duplicates is slightly more efficient and allows to retain
   the object identity of the candidates. If a candidate is selected
   which is part of the collection, this exact candidate should be
   returned. This subsumes Proposal 6 and allows to use text properties
   for disambiguation of candidates.

   Note that this proposal is useful mainly for completion tables which
   disable sorting by setting `display/cycle-sort-function' to the
   identity function. Currently if identical candidate strings are used
   for completion, these strings must be manually disambiguated by adding
   some unique prefixes or suffixes. I had this issue when implementing
   the `consult-line' command, which is a Swiper-like command based on
   `completing-read'. Furthermore the disambiguation issue occurs when
   mixing candidates from different candidate sources.

   Example:
   ,----
   | (completing-read "Duplicates: "
   |                  (list (propertize "dup" 'id 0)
   |                        (propertize "dup" 'id 1)
   |                        (propertize "dup" 'id 2))
   |                  nil t)
   `----


Proposal 8: Completion style optimization of filtering and highlighting
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   While working on Selectrum and Vertico it has been found that
   highlighting has a significant cost. This matters if the number of
   displayed candidates differs greatly from the number of filtered
   candidates. Therefore it would be good to have a possibility to
   separate highlighting and filtering. In the Orderless
   completion-style, there is a variable `orderless-skip-highlighting'
   which can be set to `t' or to a predicate function. Depending on the
   value of this variable highlighting is applied or not applied by
   `completion-all-completions'.

   In the first step, all the candidates can be computed and filtered
   with `orderless-skip-highlighting=t', see
   `vertico--recompute-candidates'. Afterwards, when displaying the
   candidates, the completion UI has to pass the small subset of
   candidates once again through `completion-all-completions', this time
   with `orderless-skip-highlighting=nil', see `vertico--highlight'.

   I propose to generalize this Orderless feature by introducing a
   variable `completion-skip-highlighting', which behaves the same as
   `orderless-skip-highlighting' and is implemented for all built-in
   completion styles. In Orderless, the filtering and highlighting is
   already separate internally, therefore skipping the highlighting
   turned out to be a natural decision in Orderless. The situation could
   be different for the built-in styles.

   As an alternative, one can introduce a third function
   `completion-highlight-completions', which is specified in
   `completion-styles-alist'. But I suspect that the introduction of an
   extra function requires more effort.

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

>       It might deserve a completely new function to replace
>       `completing-read', but I think it would be good to make this
>       new function such that it makes `completing-read' obsolete.
>
> The cost of introduction of a new function is not to be underestimated
> in particular if it sits at a central place. Introducing another
> function which only differs slightly from the existing `completing-read'
> function could do more harm than good if it increases inconsistency and

All I meant is that the design could start from a clean slate, and
afterwards look at the result and see whether/how it can be retrofitted
into `completing-read`.

> Proposal 1: Disabling the history
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   I propose to allow the symbol `t' as `HIST' argument to
>   `completing-read' in order to disable the history. This aligns
>   `completing-read' with `read-from-minibuffer'. This change requires a
>   modification of the function `completion-all-sorted-completions',
>   which sorts the candidates by history position and currently assumes
>   that the `minibuffer-history-variable' is a variable symbol.
>
>   Example:
>   ,----
>   | (completing-read "No History: " '(first second third) nil nil nil t)
>   `----

[ I'm not sure there's a great benefit here, since in the situation
  where simplicity is what matters, using nil seems like a much better
  choice, but: ]
Good idea.

> Proposal 2: More efficient sorting
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   Currently candidate sorting is `O(m*n*log(n))' with history length m
>   and candidate list length n. This leads to a noticeable slowdown for
>   large values of `history-length'. I propose to improve the speed of
>   `completion-all-sorted-completions' by using a hash table. The Vertico
>   package provides an optimized sorting function, see `vertico--sort'.

You're asking whether we should fix a performance bug, the answer is: yes.

> Proposal 3: Sort file names by history
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   Currently `completion-all-sorted-completions' does not sort file
>   candidates based on history, when the candidates are file names and
>   the history elements are paths.

I think this is just a bug in that we don't pay attention to boundaries
when sorting.  It's probably just a matter of prepending the prefix
removed from the all-completions output when looking for an element in
the list (the end of this prefix is returned in the last `cdr` of
`completion-all-completions`).  Similarly, we may want to compare with
`string-prefix-p` rather than `equal`.

> Proposal 4: Add support for `group-function'
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   Completion tables can provide the functions `display-sort-function',
>   `cycle-sort-function', `annotation-function' and `affixation-function'
>   as completion metadata. I am proposing the addition of a
>   `group-function', which receives a list of candidates and returns a
>   list of groups `((group-title . group-candidates) ...)'. The group
>   function should retain the present order of the candidates within the
>   groups.

Sounds fine to me, yes.

[ This touches a delicate issue, of course: since some completion styles
  have their idea of sorting (e.g. `flex`), some UIs have other ideas,
  and then some completion tables have yet more ideas.

  I'm not super-happy with all those functions, to be honest, but it's
  not clear what an API should look like to better decouple the UIs,
  styles, and backends in this respect, so for now the best is probably
  to keep piling on stuff until a clearer picture emerges.  ]

>   This function is used in two ways. After sorting the candidates, the
>   group function is called with the candidate list in order to produce a
>   grouped list. Then the completion UI can call the group function a
>   second time when displaying the candidate groups in order to determine
>   the group titles.

I'd have expected the "list of lists" result to already include the
group titles.

>   This is useful for example if candidates originate from different
>   sources. Grouping is popular in Helm, for example as seen in
>   [helm-m-x].

Of course, it may be difficult for your function to recover the origin
of a candidate if the same candidate can come from two or more sources.

> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   If `completing-read' is passed `t' as argument to `REQUIRE-MATCH', the
>   completion must match a candidate from the completion table. However
>   there is also the possibility to exit the minibuffer with a null
>   completion. I propose to disallow this possibility. This change makes
>   it easier for users of `completing-read' since they can rely on
>   setting `REQUIRE-MATCH' to `t' and do not have to handle the null
>   completion specially.
[...]
>   If it is desired to avoid a backward incompatible change one could
>   consider adding a new special value for `REQUIRE-MATCH', for example
>   `'require-candidate', which explicitly forbids null completion.

I generally like the idea (I've also been bothered by this possibility
to return an empty string), but I haven't looked into fixing the
problem.  There's already an obvious way to tell `completing-read`
whether the null string is acceptable (by making the completion-table's
`test-completion` return non-nil for it), so maybe we don't need
a special new value.  But I don't have a good sense of the scale of the
potential compatibility breakage [I suspect that such a change might
also fix some code which doesn't bother to check for an empty output. ]
IOW, this deserves a bit for `grep`ping through all the ELisp code you
can get your hands on and try to see how/if it would be affected by such
a change.

As you say, in the worst case we can introduce a new value for
REQUIRE-MATCH, so as to be backward-compatibility-safe.  But I'd be
interested to see if there are cases where the current empty-string
"(mis)feature" is actually beneficial.

> Proposal 6: Return text properties from `completing-read'
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   As of now, `completing-read' strips text properties from the result
>   string. This behavior is reasonable for strings, which are not present
>   in the collection if `REQUIRE-MATCH/=t'. However if a candidate string
>   is selected, which is a member of the collection, the text properties
>   can be retained. If this is implemented and `REQUIRE-MATCH=t', the
>   user of `completing-read' can always rely on text properties to attach
>   arbitrary data to the candidates.

I think what you mean is that the text properties should be stripped
from the text taken from the minibuffer, but should be preserved from
the text taken from the completion tables's candidates (and then
together with that, you'd want to guarantee that when require-match is
non-nil the string object returned is actually one of those candidates
returned by the completion table)?

This is one of the important differences between "completion" and
"selection" in that completion is about using a completion-table as
a helper to write a chunk of text (but the resulting text in the end
should not be affected by how the user used the completion table to
come up with the final text), whereas selection is about choosing among
a set of candidates and the result is expected to be `eq` to one of
the candidates.

I agree that it would be good to move towards making `completing-read`
better support the "selection" use case.

> Proposal 7: Allow duplicates and retain object identity
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   If a completion table contains duplicates, these duplicates should not
>   be removed. There are not many completion tables which generate
>   duplicate candidates and there exist multiple completion systems which
>   do not perform deduplication at all.

[ I've been resisting this for quite some years, as Drew can testify.  ]

While I agree with preserving object identity, the issue of duplicates
is more problematic, because it means the user can't choose between them
using self-insert-command + RET.  IOW such completion-tables are
fundamentally making assumptions about the kind of UI with which they
can be used.

> Proposal 8: Completion style optimization of filtering and highlighting
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   While working on Selectrum and Vertico it has been found that
>   highlighting has a significant cost. This matters if the number of
>   displayed candidates differs greatly from the number of filtered
>   candidates. Therefore it would be good to have a possibility to
>   separate highlighting and filtering.

Agreed.  It doesn't seem easy to retro fit this into the current API, tho.
[ The redesign of the completion API which I had started back in 2019
  (see scratch/completion-api for a very rough sketch of the general
  direction) also aimed to fix this.  ]

>   In the Orderless completion-style, there is a variable
>   `orderless-skip-highlighting' which can be set to `t' or to
>   a predicate function. Depending on the value of this variable
>   highlighting is applied or not applied by
>   `completion-all-completions'.

That's not a great solution, but as a temporary patch until we have
a better API it's OK.

>   In the first step, all the candidates can be computed and filtered
>   with `orderless-skip-highlighting=t', see
>   `vertico--recompute-candidates'. Afterwards, when displaying the
>   candidates, the completion UI has to pass the small subset of
>   candidates once again through `completion-all-completions', this time
>   with `orderless-skip-highlighting=nil', see `vertico--highlight'.

I suspect "pass the small subset of candidates once again through
`completion-all-completions'" can be tricky to do (e.g. for things like
file-name completion with `partial-completion`).

>   I propose to generalize this Orderless feature by introducing a
>   variable `completion-skip-highlighting', which behaves the same as
>   `orderless-skip-highlighting' and is implemented for all built-in
>   completion styles. In Orderless, the filtering and highlighting is
>   already separate internally, therefore skipping the highlighting
>   turned out to be a natural decision in Orderless. The situation could
>   be different for the built-in styles.

I think in all styles I can think of highlighting is done as a separate step.

>   As an alternative, one can introduce a third function
>   `completion-highlight-completions', which is specified in
>   `completion-styles-alist'. But I suspect that the introduction of an
>   extra function requires more effort.

I'd rather not go there, indeed.


        Stefan

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

BTW, rereading the below, I see I'm sounding a bit too authoritative.
I'm the original author of the minibuffer.el completion code, but
remember that what I wrote is just my opinion: you'll want to
convince the actual maintainers for some of those changes ;-)


        Stefan


Stefan Monnier [2021-04-07 13:11:39] wrote:

>>       It might deserve a completely new function to replace
>>       `completing-read', but I think it would be good to make this
>>       new function such that it makes `completing-read' obsolete.
>>
>> The cost of introduction of a new function is not to be underestimated
>> in particular if it sits at a central place. Introducing another
>> function which only differs slightly from the existing `completing-read'
>> function could do more harm than good if it increases inconsistency and
>
> All I meant is that the design could start from a clean slate, and
> afterwards look at the result and see whether/how it can be retrofitted
> into `completing-read`.
>
>> Proposal 1: Disabling the history
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   I propose to allow the symbol `t' as `HIST' argument to
>>   `completing-read' in order to disable the history. This aligns
>>   `completing-read' with `read-from-minibuffer'. This change requires a
>>   modification of the function `completion-all-sorted-completions',
>>   which sorts the candidates by history position and currently assumes
>>   that the `minibuffer-history-variable' is a variable symbol.
>>
>>   Example:
>>   ,----
>>   | (completing-read "No History: " '(first second third) nil nil nil t)
>>   `----
>
> [ I'm not sure there's a great benefit here, since in the situation
>   where simplicity is what matters, using nil seems like a much better
>   choice, but: ]
> Good idea.
>
>> Proposal 2: More efficient sorting
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   Currently candidate sorting is `O(m*n*log(n))' with history length m
>>   and candidate list length n. This leads to a noticeable slowdown for
>>   large values of `history-length'. I propose to improve the speed of
>>   `completion-all-sorted-completions' by using a hash table. The Vertico
>>   package provides an optimized sorting function, see `vertico--sort'.
>
> You're asking whether we should fix a performance bug, the answer is: yes.
>
>> Proposal 3: Sort file names by history
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   Currently `completion-all-sorted-completions' does not sort file
>>   candidates based on history, when the candidates are file names and
>>   the history elements are paths.
>
> I think this is just a bug in that we don't pay attention to boundaries
> when sorting.  It's probably just a matter of prepending the prefix
> removed from the all-completions output when looking for an element in
> the list (the end of this prefix is returned in the last `cdr` of
> `completion-all-completions`).  Similarly, we may want to compare with
> `string-prefix-p` rather than `equal`.
>
>> Proposal 4: Add support for `group-function'
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   Completion tables can provide the functions `display-sort-function',
>>   `cycle-sort-function', `annotation-function' and `affixation-function'
>>   as completion metadata. I am proposing the addition of a
>>   `group-function', which receives a list of candidates and returns a
>>   list of groups `((group-title . group-candidates) ...)'. The group
>>   function should retain the present order of the candidates within the
>>   groups.
>
> Sounds fine to me, yes.
>
> [ This touches a delicate issue, of course: since some completion styles
>   have their idea of sorting (e.g. `flex`), some UIs have other ideas,
>   and then some completion tables have yet more ideas.
>
>   I'm not super-happy with all those functions, to be honest, but it's
>   not clear what an API should look like to better decouple the UIs,
>   styles, and backends in this respect, so for now the best is probably
>   to keep piling on stuff until a clearer picture emerges.  ]
>
>>   This function is used in two ways. After sorting the candidates, the
>>   group function is called with the candidate list in order to produce a
>>   grouped list. Then the completion UI can call the group function a
>>   second time when displaying the candidate groups in order to determine
>>   the group titles.
>
> I'd have expected the "list of lists" result to already include the
> group titles.
>
>>   This is useful for example if candidates originate from different
>>   sources. Grouping is popular in Helm, for example as seen in
>>   [helm-m-x].
>
> Of course, it may be difficult for your function to recover the origin
> of a candidate if the same candidate can come from two or more sources.
>
>> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   If `completing-read' is passed `t' as argument to `REQUIRE-MATCH', the
>>   completion must match a candidate from the completion table. However
>>   there is also the possibility to exit the minibuffer with a null
>>   completion. I propose to disallow this possibility. This change makes
>>   it easier for users of `completing-read' since they can rely on
>>   setting `REQUIRE-MATCH' to `t' and do not have to handle the null
>>   completion specially.
> [...]
>>   If it is desired to avoid a backward incompatible change one could
>>   consider adding a new special value for `REQUIRE-MATCH', for example
>>   `'require-candidate', which explicitly forbids null completion.
>
> I generally like the idea (I've also been bothered by this possibility
> to return an empty string), but I haven't looked into fixing the
> problem.  There's already an obvious way to tell `completing-read`
> whether the null string is acceptable (by making the completion-table's
> `test-completion` return non-nil for it), so maybe we don't need
> a special new value.  But I don't have a good sense of the scale of the
> potential compatibility breakage [I suspect that such a change might
> also fix some code which doesn't bother to check for an empty output. ]
> IOW, this deserves a bit for `grep`ping through all the ELisp code you
> can get your hands on and try to see how/if it would be affected by such
> a change.
>
> As you say, in the worst case we can introduce a new value for
> REQUIRE-MATCH, so as to be backward-compatibility-safe.  But I'd be
> interested to see if there are cases where the current empty-string
> "(mis)feature" is actually beneficial.
>
>> Proposal 6: Return text properties from `completing-read'
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   As of now, `completing-read' strips text properties from the result
>>   string. This behavior is reasonable for strings, which are not present
>>   in the collection if `REQUIRE-MATCH/=t'. However if a candidate string
>>   is selected, which is a member of the collection, the text properties
>>   can be retained. If this is implemented and `REQUIRE-MATCH=t', the
>>   user of `completing-read' can always rely on text properties to attach
>>   arbitrary data to the candidates.
>
> I think what you mean is that the text properties should be stripped
> from the text taken from the minibuffer, but should be preserved from
> the text taken from the completion tables's candidates (and then
> together with that, you'd want to guarantee that when require-match is
> non-nil the string object returned is actually one of those candidates
> returned by the completion table)?
>
> This is one of the important differences between "completion" and
> "selection" in that completion is about using a completion-table as
> a helper to write a chunk of text (but the resulting text in the end
> should not be affected by how the user used the completion table to
> come up with the final text), whereas selection is about choosing among
> a set of candidates and the result is expected to be `eq` to one of
> the candidates.
>
> I agree that it would be good to move towards making `completing-read`
> better support the "selection" use case.
>
>> Proposal 7: Allow duplicates and retain object identity
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   If a completion table contains duplicates, these duplicates should not
>>   be removed. There are not many completion tables which generate
>>   duplicate candidates and there exist multiple completion systems which
>>   do not perform deduplication at all.
>
> [ I've been resisting this for quite some years, as Drew can testify.  ]
>
> While I agree with preserving object identity, the issue of duplicates
> is more problematic, because it means the user can't choose between them
> using self-insert-command + RET.  IOW such completion-tables are
> fundamentally making assumptions about the kind of UI with which they
> can be used.
>
>> Proposal 8: Completion style optimization of filtering and highlighting
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>   While working on Selectrum and Vertico it has been found that
>>   highlighting has a significant cost. This matters if the number of
>>   displayed candidates differs greatly from the number of filtered
>>   candidates. Therefore it would be good to have a possibility to
>>   separate highlighting and filtering.
>
> Agreed.  It doesn't seem easy to retro fit this into the current API, tho.
> [ The redesign of the completion API which I had started back in 2019
>   (see scratch/completion-api for a very rough sketch of the general
>   direction) also aimed to fix this.  ]
>
>>   In the Orderless completion-style, there is a variable
>>   `orderless-skip-highlighting' which can be set to `t' or to
>>   a predicate function. Depending on the value of this variable
>>   highlighting is applied or not applied by
>>   `completion-all-completions'.
>
> That's not a great solution, but as a temporary patch until we have
> a better API it's OK.
>
>>   In the first step, all the candidates can be computed and filtered
>>   with `orderless-skip-highlighting=t', see
>>   `vertico--recompute-candidates'. Afterwards, when displaying the
>>   candidates, the completion UI has to pass the small subset of
>>   candidates once again through `completion-all-completions', this time
>>   with `orderless-skip-highlighting=nil', see `vertico--highlight'.
>
> I suspect "pass the small subset of candidates once again through
> `completion-all-completions'" can be tricky to do (e.g. for things like
> file-name completion with `partial-completion`).
>
>>   I propose to generalize this Orderless feature by introducing a
>>   variable `completion-skip-highlighting', which behaves the same as
>>   `orderless-skip-highlighting' and is implemented for all built-in
>>   completion styles. In Orderless, the filtering and highlighting is
>>   already separate internally, therefore skipping the highlighting
>>   turned out to be a natural decision in Orderless. The situation could
>>   be different for the built-in styles.
>
> I think in all styles I can think of highlighting is done as a separate step.
>
>>   As an alternative, one can introduce a third function
>>   `completion-highlight-completions', which is specified in
>>   `completion-styles-alist'. But I suspect that the introduction of an
>>   extra function requires more effort.
>
> I'd rather not go there, indeed.
>
>
>         Stefan

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/7/21 7:20 PM, Stefan Monnier wrote:
> BTW, rereading the below, I see I'm sounding a bit too authoritative.
> I'm the original author of the minibuffer.el completion code, but
> remember that what I wrote is just my opinion: you'll want to
> convince the actual maintainers for some of those changes ;-)

I am aware that you are the original author of this code. Thank you for 
your response and your comments. Actually I felt your response sounded 
more like "I mostly agree with some of the proposals, let's see patches, 
then we talk!" ;) I hope we will also hear some opinions by the current 
maintainers.

I respond to the points in your previous mail:

 >> Proposal 1: Disabling the history
 > [ I'm not sure there's a great benefit here, since in the situation
 >   where simplicity is what matters, using nil seems like a much better
 >   choice, but: ]

I agree that the choice is not great. I would have either chosen `nil` 
or some special value like `'disable-history` instead. But this is what 
we got from `read-from-minibuffer`. Therefore I would go with `t` for 
consistency.

 >> Proposal 3: Sort file names by history
 > I think this is just a bug in that we don't pay attention to boundaries
 > when sorting.  It's probably just a matter of prepending the prefix
 > removed from the all-completions output when looking for an element in
 > the list (the end of this prefix is returned in the last `cdr` of
 > `completion-all-completions`).  Similarly, we may want to compare with
 > `string-prefix-p` rather than `equal`.

I think for files you cannot get away with looking up the 
prefix+candidate as obtained from the boundaries. It will work mostly 
but you may want to implement a bit more path normalization to get 
better results.

There are two different ways one can implement this: (1) Either lookup 
the prefix+candidate in the history or (2) normalize the history first 
and lookup the plain candidates. Option (2) will usually require fewer 
allocations.

 >> Proposal 4: Add support for `group-function'
 >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 >
 > Sounds fine to me, yes.
 >
 > [ This touches a delicate issue, of course: since some completion styles
 >  have their idea of sorting (e.g. `flex`), some UIs have other ideas,
 >  and then some completion tables have yet more ideas.
 >  I'm not super-happy with all those functions, to be honest, but it's
 >  not clear what an API should look like to better decouple the UIs,
 >  styles, and backends in this respect, so for now the best is probably
 >  to keep piling on stuff until a clearer picture emerges.  ]

The ad-hoc sorting of flex is something I am actually not happy with. Is 
it really necessary to have this? My problem with this is two-fold:

- There is this metadata adjustment which somehow unexpectedly 
adjusts/mutates the metadata
- Why does a completion style sort? I am mostly happy with styles which 
don't do that. When I was working on my Consult package I was actually 
confused by the "disorder" introduced by `flex` and `fido-mode` even 
with `display/cycle-sort-function` set to the identity.

I should probably read a bit up on the history of this feature, then I 
will know better. But when I first saw this flex adjustment I got confused.

 > I'd have expected the "list of lists" result to already include the
 > group titles.

Yes, the group function is supposed to return an alist of group titles 
and group candidates. I only want to emphasize that the completion UI 
will use the function twice, first to group/sort the candidates, while 
retaining the original sorting order. And second the function can be 
used to obtain the titles again for the candidates which are actually 
displayed, which may not be many, e.g., only 10 in the case of Ivy and 
friends for example.

 >> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
 >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 >
 > I generally like the idea (I've also been bothered by this possibility
 > to return an empty string), but I haven't looked into fixing the
 > problem.  There's already an obvious way to tell `completing-read`
 > whether the null string is acceptable (by making the completion-table's
 > `test-completion` return non-nil for it), so maybe we don't need
 > a special new value.  But I don't have a good sense of the scale of the
 > potential compatibility breakage [I suspect that such a change might
 > also fix some code which doesn't bother to check for an empty output. ]
 > IOW, this deserves a bit for `grep`ping through all the ELisp code you
 > can get your hands on and try to see how/if it would be affected by such
 > a change.

Okay, I think it is reasonable to gather some data regarding the extent 
of the problem. But then I would strongly prefer to just fix the issue. 
That being said, it was not obvious to me that you can actually force 
require match by writing a proper completion table which properly 
handles the `test-completion` action. However this is a very indirect 
way to fix the issue and I think I've observed multiple commands in the 
wild which choked up with null completion.

 >> Proposal 6: Return text properties from `completing-read'
 >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 >
 > I think what you mean is that the text properties should be stripped
 > from the text taken from the minibuffer, but should be preserved from
 > the text taken from the completion tables's candidates (and then
 > together with that, you'd want to guarantee that when require-match is
 > non-nil the string object returned is actually one of those candidates
 > returned by the completion table)?

Yes.

 > This is one of the important differences between "completion" and
 > "selection" in that completion is about using a completion-table as
 > a helper to write a chunk of text (but the resulting text in the end
 > should not be affected by how the user used the completion table to
 > come up with the final text), whereas selection is about choosing among
 > a set of candidates and the result is expected to be `eq` to one of
 > the candidates.

Yes, I agree. I am glad for the discussion regarding the difference of 
selection/completion (Thanks, Philip K!). I hope we can adjust the API 
such that it caters for the selection use case a bit better.

 >> Proposal 7: Allow duplicates and retain object identity
 >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 >>
 > [I've been resisting this for quite some years, as Drew can testify.]
 >
 > While I agree with preserving object identity, the issue of duplicates
 > is more problematic, because it means the user can't choose between them
 > using self-insert-command + RET.  IOW such completion-tables are
 > fundamentally making assumptions about the kind of UI with which they
 > can be used.

Yes, that is right. If you care about the actual candidate you get from 
a set of duplicates, you must use a completion UI which can handle 
duplicates. If you use an UI which cannot handle this (which rather 
completes) you cannot get the distinction.

I think there is a real need for this if you look at the Swiper command 
and you want to navigate across equal lines. Then there is the 
ivy-occur/embark-export/embark-collect command which allows to export 
the lines to an occur buffer where you can again navigate over the 
duplicate items. Given how everything is implemented internally (list of 
candidates, filtering via completion-all-completions etc), it feels like 
an unnecessary restriction to remove duplicates. It would be more 
natural to just allow them.

If one actually implements proposal 6 it may be even more natural then, 
since completion UIs which supports deduplication (and an index based 
selection) will then just do the right thing automatically. These UIs 
would have to say - Hah, I got a duplicate let's scramble that and just 
return the first or a random one of the duplicates.

Using deduplication prefixes is a really ugly solution to work around 
the duplicate issue currently. I am using such prefixes and to make 
matters worse, these prefixes break the basic completion style.

 >> Proposal 8: Completion style optimization of filtering and highlighting
 >
 > That's not a great solution, but as a temporary patch until we have
 > a better API it's OK.

Yes, it was just the first thing we came up with. It may be worth to 
think about better alternatives. But as a data point if one uses the 
Orderless completion style which supports this extension it works really 
well with many candidates and gives a noticeable performance boost.

 > I suspect "pass the small subset of candidates once again through
 > `completion-all-completions'" can be tricky to do (e.g. for things like
 > file-name completion with `partial-completion`).

In `vertico--highlight` I am checking the length of the result. If it is 
not equal to the original length, the UI simply displays the original 
candidates. This is an ugly hack. Maybe it would be better to have a 
`completion-style-operation` variable which can be either `'both`, 
`'filter` or `'highlight`?

Daniel Mendler

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

>>> Proposal 1: Disabling the history
>> [ I'm not sure there's a great benefit here, since in the situation
>>   where simplicity is what matters, using nil seems like a much better
>>   choice, but: ]
> I agree that the choice is not great. I would have either chosen `nil` or
> some special value like `'disable-history` instead. But this is what we got
> from `read-from-minibuffer`. Therefore I would go with `t` for consistency.

What I meant is that I can't think of many situations where
we specifically want no history (which is where t is used) as opposed to
the many situations where we simply don't care about history (in which
case nil works just as well).

IIRC the t case for read-from-minibuffer was added for the needs of
`read-passwd` where we really don't want the history, for security reasons.
I don't know of any other use case yet.

>>> Proposal 3: Sort file names by history
>> I think this is just a bug in that we don't pay attention to boundaries
>> when sorting.  It's probably just a matter of prepending the prefix
>> removed from the all-completions output when looking for an element in
>> the list (the end of this prefix is returned in the last `cdr` of
>> `completion-all-completions`).  Similarly, we may want to compare with
>> `string-prefix-p` rather than `equal`.
> I think for files you cannot get away with looking up the prefix+candidate
> as obtained from the boundaries. It will work mostly but you may want to
> implement a bit more path normalization to get better results.

Currently we're not even close to that.  So I think the first step is to
fix the code to pay attention to boundaries.  My comment was just
pointing out that this can be done right now a simple bug fix without
any extra information.

We can later consider adding ad-hoc canonicalization functions to
provide a more "semantic" notion of equivalence rather than strict
textual equality if we think it matters enough, but maybe we'll decide
that it's not needed, or that something completely different is needed
(or that we can use the `cycle-sort-function` for that).

So far the use of the history for sorting has been treated as a "quick
hack" (I basically added it so that `minibuffer-force-complete` guesses
right for me in most cases in M-x).

> The ad-hoc sorting of flex is something I am actually not happy with.
> Is it really necessary to have this? My problem with this is two-fold:
> - There is this metadata adjustment which somehow unexpectedly
>   adjusts/mutates the metadata
> - Why does a completion style sort? I am mostly happy with styles which
>   don't do that. When I was working on my Consult package I was actually
>   confused by the "disorder" introduced by `flex` and `fido-mode` even with
>  `display/cycle-sort-function` set to the identity.

`flex` without sorting is *much* less usable, in my experience.
The direction of `flex` is actually to replace filtering with sorting.
E.g. I have a local completion style which I call "forgiving" because it
tries to accommodate typos in the input, so basically every member of
the completion table always matches, only its sorting position is
affected by the input text.  So if you removing sorting, there's
nothing left.

I agree with you that the current way style-based sorting works is
problematic, but I think the principle of style-based sorting is sound
because we don't want this to be specific to a UI nor to a backend.

>> I'd have expected the "list of lists" result to already include the
>> group titles.
> Yes, the group function is supposed to return an alist of group titles and
> group candidates. I only want to emphasize that the completion UI will use
> the function twice, first to group/sort the candidates, while retaining the
> original sorting order. And second the function can be used to obtain the
> titles again for the candidates which are actually displayed, which may not
> be many, e.g., only 10 in the case of Ivy and friends for example.

If you say so.  I think I don't understand enough of what you're
proposing to understand what that means.  But I don't think it matters
too much at this point.

>>> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> I generally like the idea (I've also been bothered by this possibility
>> to return an empty string), but I haven't looked into fixing the
>> problem.  There's already an obvious way to tell `completing-read`
>> whether the null string is acceptable (by making the completion-table's
>> `test-completion` return non-nil for it), so maybe we don't need
>> a special new value.  But I don't have a good sense of the scale of the
>> potential compatibility breakage [I suspect that such a change might
>> also fix some code which doesn't bother to check for an empty output. ]
>> IOW, this deserves a bit for `grep`ping through all the ELisp code you
>> can get your hands on and try to see how/if it would be affected by such
>> a change.
>
> Okay, I think it is reasonable to gather some data regarding the extent of
> the problem. But then I would strongly prefer to just fix the issue. That
> being said, it was not obvious to me that you can actually force require
> match by writing a proper completion table which properly handles the
> `test-completion` action.

No, you can't force require match this way.  What you can do this way is
to re-loosen the match so as to accept the empty-string again like in
the old (i.e. current) code.

> Yes, that is right. If you care about the actual candidate you get from
> a set of duplicates, you must use a completion UI which can handle
> duplicates. If you use an UI which cannot handle this (which rather
> completes) you cannot get the distinction.
>
> I think there is a real need for this if you look at the Swiper command and
> you want to navigate across equal lines. Then there is the
> ivy-occur/embark-export/embark-collect command which allows to export the
> lines to an occur buffer where you can again navigate over the duplicate
> items. Given how everything is implemented internally (list of candidates,
> filtering via completion-all-completions etc), it feels like an unnecessary
> restriction to remove duplicates. It would be more natural to just
> allow them.

As long as we can offer some way (that's not too cumbersome) to select
between the different cases using things like self-insert-command + RET,
I'm fine with it.  IOW I think it require reifying the "subtle
differences" as some kind of optional extra text.

>> I suspect "pass the small subset of candidates once again through
>> `completion-all-completions'" can be tricky to do (e.g. for things like
>> file-name completion with `partial-completion`).
> In `vertico--highlight` I am checking the length of the result. If it is not
> equal to the original length, the UI simply displays the original
> candidates. This is an ugly hack. Maybe it would be better to have
> a `completion-style-operation` variable which can be either `'both`,
> `'filter` or `'highlight`?

I'm not really interested in hacking a slightly better solution, so
I think your current hack is good enough: the way I see it, the
replacement of `completion-all-completions` should return
non-highlighted candidates along with some extra opaque data, and then
a separate new function can take one of those candidates, together with
that opaque data (plus some additional optional args) to add highlight
one candidate at a time.

This way, the highlighting in *Completions* could be applied
incrementally via jit-lock, and the "additional optional args" should
make it possible for the UI to have control over the color scheme
being used.


        Stefan

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/7/21 11:26 PM, Stefan Monnier wrote:
> What I meant is that I can't think of many situations where
> we specifically want no history (which is where t is used) as opposed to
> the many situations where we simply don't care about history (in which
> case nil works just as well).
> 
> IIRC the t case for read-from-minibuffer was added for the needs of
> `read-passwd` where we really don't want the history, for security reasons.
> I don't know of any other use case yet.

There is one use case I found a bit useful - if you are browsing a 
history itself with `completing-read` then you may want to disable 
writing to the given or another history at the same time, such that the 
completion you are doing is not side-effecting.

I would also like if `completing-read` is consistent with 
`read-from-minibuffer` in this case, since this lead to actual 
confusion, where the false assumption has been made that 
`completing-read` also accepts `t`.

>>>> Proposal 3: Sort file names by history
> Currently we're not even close to that.  So I think the first step is to
> fix the code to pay attention to boundaries.  My comment was just
> pointing out that this can be done right now a simple bug fix without
> any extra information.

Okay, I also have the code for that approach. It is a simple bug fix as 
you say. I dropped it from Vertico for my file name sorting for 
performance reasons and since I wanted to do better in the special 
important case of files. But no problem to prepare patches for both and 
fix the boundaries case first.

> `flex` without sorting is *much* less usable, in my experience.
> The direction of `flex` is actually to replace filtering with sorting.
> E.g. I have a local completion style which I call "forgiving" because it
> tries to accommodate typos in the input, so basically every member of
> the completion table always matches, only its sorting position is
> affected by the input text.  So if you removing sorting, there's
> nothing left.
> 
> I agree with you that the current way style-based sorting works is
> problematic, but I think the principle of style-based sorting is sound
> because we don't want this to be specific to a UI nor to a backend.

Okay, I see. I am actually not using the flex style since I find it too 
slow and generating too many matches. But in the Emacs completion 
implementation it will only be triggered if the earlier styles do not 
match so maybe that is okay.

I am not using flex since I am using something else - the orderless 
style. "regexp1 regexp2 regexp3" matches the regexps in any order. 
Furthermore I am using "style dispatchers", "regexp flex~ !not 
literal=", where the "flex~" token is used to generate a flex regexp, 
etc. Maybe we will see the orderless package in ELPA at some point.

>>>> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Okay, I think it is reasonable to gather some data regarding the extent of
>> the problem. But then I would strongly prefer to just fix the issue. That
>> being said, it was not obvious to me that you can actually force require
>> match by writing a proper completion table which properly handles the
>> `test-completion` action.
> 
> No, you can't force require match this way.  What you can do this way is
> to re-loosen the match so as to accept the empty-string again like in
> the old (i.e. current) code.

Thanks for clarifying.

>> Yes, that is right. If you care about the actual candidate you get from
>> a set of duplicates, you must use a completion UI which can handle
>> duplicates. If you use an UI which cannot handle this (which rather
>> completes) you cannot get the distinction.
>>
>> I think there is a real need for this if you look at the Swiper command and
>> you want to navigate across equal lines. Then there is the
>> ivy-occur/embark-export/embark-collect command which allows to export the
>> lines to an occur buffer where you can again navigate over the duplicate
>> items. Given how everything is implemented internally (list of candidates,
>> filtering via completion-all-completions etc), it feels like an unnecessary
>> restriction to remove duplicates. It would be more natural to just
>> allow them.
> 
> As long as we can offer some way (that's not too cumbersome) to select
> between the different cases using things like self-insert-command + RET,
> I'm fine with it.  IOW I think it require reifying the "subtle
> differences" as some kind of optional extra text.

Okay, maybe I can come up with something. This will require 
experimentation. The duplicates could be deduplicated at the UI level by 
appending some index for example "candidate (1)", "candidate (2)", ...

>>> I suspect "pass the small subset of candidates once again through
>>> `completion-all-completions'" can be tricky to do (e.g. for things like
>>> file-name completion with `partial-completion`).
>> In `vertico--highlight` I am checking the length of the result. If it is not
>> equal to the original length, the UI simply displays the original
>> candidates. This is an ugly hack. Maybe it would be better to have
>> a `completion-style-operation` variable which can be either `'both`,
>> `'filter` or `'highlight`?
> 
> I'm not really interested in hacking a slightly better solution, so
> I think your current hack is good enough: the way I see it, the
> replacement of `completion-all-completions` should return
> non-highlighted candidates along with some extra opaque data, and then
> a separate new function can take one of those candidates, together with
> that opaque data (plus some additional optional args) to add highlight
> one candidate at a time.
> 
> This way, the highlighting in *Completions* could be applied
> incrementally via jit-lock, and the "additional optional args" should
> make it possible for the UI to have control over the color scheme
> being used.

One thing to consider - maybe returning the highlights as some extra 
data is not actually faster than simply propertizing the strings? I 
guess you only save the allocations of the strings? The idea of the 
proposed hack is really to avoid the entire work of computing the 
highlights, since it is mostly unnecessary and slow if you filter many 
candidates and display only a small subset. I would therefore go with 
the simple solution first since this provable solves the issue. But 
using jit-lock in buffers has appeal.

Daniel Mendler

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

> I would also like if `completing-read` is consistent with
> `read-from-minibuffer` in this case, since this lead to actual
> confusion, where the false assumption has been made that
> `completing-read` also accepts `t`.

No objection from me (the discussion was more because I was curious ab
out the motivation for the change).

> Maybe we will see the orderless package in ELPA at some point.

It's already in ELPA (more specifically in the MELPA archive).
But I hope it will appear in GNU ELPA, indeed.

>> As long as we can offer some way (that's not too cumbersome) to select
>> between the different cases using things like self-insert-command + RET,
>> I'm fine with it.  IOW I think it require reifying the "subtle
>> differences" as some kind of optional extra text.
> Okay, maybe I can come up with something.  This will require
> experimentation. The duplicates could be deduplicated at the UI level by
> appending some index for example "candidate (1)", "candidate (2)", ...

I think we should arrange for the completion table to provide that extra
text, so it can be more meaningful than an arbitrary number.  Also the
user needs to be able to know "which is which", so it needs to be
connected to something that's displayed in *Completions* that
distinguishes them.

E.g. maybe we could arrange for the minibuffer text to be
matched against the *annotated* candidates in some cases.  I think this
requires more thought, together with concrete examples.

> One thing to consider - maybe returning the highlights as some extra data is
> not actually faster than simply propertizing the strings?

The extra opaque data is cheap/free to compute.  It would not contain
"the highlights", but just the side information that might be needed to
compute the highlights: e.g. you can't do the highlighting correctly for
`partial-completion` without having access to the "pattern" against
which it was matches.

> I would therefore go with the simple solution first since this
> provable solves the issue.

Yes, the "skip-highlights" approach is a good solution for now.  The one
I discuss above requires a whole new API, so it's just hotair right now.


        Stefan

Re: Improvement proposals for `completing-read'

[Stefan Kangas]

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

>> Maybe we will see the orderless package in ELPA at some point.
>
> It's already in ELPA (more specifically in the MELPA archive).
> But I hope it will appear in GNU ELPA, indeed.

Did someone reach out to ask them about this?

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 5:26 PM, Stefan Kangas wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>>> Maybe we will see the orderless package in ELPA at some point.
>>
>> It's already in ELPA (more specifically in the MELPA archive).
>> But I hope it will appear in GNU ELPA, indeed.
> 
> Did someone reach out to ask them about this?

Let's just ask Omar (@oantolin) directly (CCed him). As far as I know, 
Omar has not done the copyright assignment yet, but I've heard he may be 
willing to do it for Orderless and some of his other packages. I 
contributed a bit to the Orderless package and there are a few other 
contributors, see https://github.com/oantolin/orderless/graphs/contributors.

Daniel

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

>> It's already in ELPA (more specifically in the MELPA archive).
>> But I hope it will appear in GNU ELPA, indeed.
> Did someone reach out to ask them about this?

I did, yes, we're tracking down paperwork.


        Stefan

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 4:44 PM, Stefan Monnier wrote:
>>> As long as we can offer some way (that's not too cumbersome) to select
>>> between the different cases using things like self-insert-command + RET,
>>> I'm fine with it.  IOW I think it require reifying the "subtle
>>> differences" as some kind of optional extra text.
>> Okay, maybe I can come up with something.  This will require
>> experimentation. The duplicates could be deduplicated at the UI level by
>> appending some index for example "candidate (1)", "candidate (2)", ...
> 
> I think we should arrange for the completion table to provide that extra
> text, so it can be more meaningful than an arbitrary number.  Also the
> user needs to be able to know "which is which", so it needs to be
> connected to something that's displayed in *Completions* that
> distinguishes them.

Yes, that would be the best possibility. There are many ways to provide 
that data: Via a text property, via an extra metadata function, etc. In 
the case of Swiper the extra data could simply be the line number. 
However a general solution should also automatically generate indices, 
since it is probably better to not require the completion table to 
provide the information in any case. I would like some graceful degradation.

> E.g. maybe we could arrange for the minibuffer text to be
> matched against the *annotated* candidates in some cases.  I think this
> requires more thought, together with concrete examples.

Generally matching against annotations functions is problematic. As I 
interpret things, the point of annotations is that they are not 
matchable. Maybe they are more expensive to compute, so this should 
happen lazily for the displayed candidates. But using it only for 
disambiguation should be okay. Then one would only compute the 
annotation when doing the actual completion?

>> One thing to consider - maybe returning the highlights as some extra data is
>> not actually faster than simply propertizing the strings?
> 
> The extra opaque data is cheap/free to compute.  It would not contain
> "the highlights", but just the side information that might be needed to
> compute the highlights: e.g. you can't do the highlighting correctly for
> `partial-completion` without having access to the "pattern" against
> which it was matches.

I don't think this is true. At least not for the orderless style. There 
the pattern compiler compiles the input to the `completion-regexp-list`. 
This is fast for filtering but there is no way to obtain the highlights 
directly as far as I see it. Then in order to compute the highlights the 
highlighter once again has to receive the input pattern, generate the 
regexps and match them one by one, adding some faces to the strings.

Daniel

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> Generally matching against annotations functions is problematic. As I
> interpret things, the point of annotations is that they are not
> matchable. Maybe they are more expensive to compute, so this should
> happen lazily for the displayed candidates.

Yes.

> But using it only for
> disambiguation should be okay. Then one would only compute the
> annotation when doing the actual completion?

No, annotations should not be used as the, or even as
a, means of disambiguating candidates.  That would be
an ugly and limiting hack.

Annotations are user-visible.  They have their own
use cases.  Making them take on the role of
disambiguating candidates - and especially making
them be _the_ means of disambiguating - would be
misguided, IMO.

> > you can't do the highlighting correctly for
> > `partial-completion` without having access to
> > the "pattern" against which it was matches.
> 
> I don't think this is true. At least not for the orderless style. There
> the pattern compiler compiles the input to the `completion-regexp-list`.

That's a simple case.  Things get more complicated
for other kinds of matching.

> This is fast for filtering but there is no way to obtain the highlights
> directly as far as I see it. Then in order to compute the highlights the
> highlighter once again has to receive the input pattern, generate the
> regexps and match them one by one, adding some faces to the strings.

Why "once again"?  Such highlighting should only be
done when displaying candidates (e.g. in *Completions*).

You have the generated composite regexp.  Why generate
it again?  You have it and you can use it - just
`string-match' candidates to highlight the matched parts.

Re: [External] : Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 08.04.2021 20:22, Drew Adams wrote:
>> But using it only for
>> disambiguation should be okay. Then one would only compute the
>> annotation when doing the actual completion?
> No, annotations should not be used as the, or even as
> a, means of disambiguating candidates.  That would be
> an ugly and limiting hack.
> 
> Annotations are user-visible.  They have their own
> use cases.  Making them take on the role of
> disambiguating candidates - and especially making
> them be_the_  means of disambiguating - would be
> misguided, IMO.

That's exactly why they are a good means of disambiguating completions: 
completions with different annotations have something for the user to 
distinguish them with.

I'm not sure about annotation auto-generation, though.

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> >> But using it only for disambiguation should be okay.
> >> Then one would only compute the annotation when
> >> doing the actual completion?
> >
> > No, annotations should not be used as the, or even as
> > a, means of disambiguating candidates.  That would be
> > an ugly and limiting hack.
> >
> > Annotations are user-visible.  They have their own
> > use cases.  Making them take on the role of
> > disambiguating candidates - and especially making
> > them be _the_  means of disambiguating - would be
> > misguided, IMO.
> 
> That's exactly why they are a good means of disambiguating completions:
> completions with different annotations have something for the user to
> distinguish them with.

That's backward.  Users can distinguish them (when
the annotations differ).  It doesn't follow that
that's the best or only way to distinguish candidates.

There can be _lots_ of info that's part of a full
candidate which we do _not_ expose in the UI, so
users don't necessarily see a difference in
*Completions*.

We can nevertheless make any such info visible in
other ways, e.g. on demand.

Regardless, nothing says that some of the info we
use to distinguish candidates (for whatever reasons,
including maybe performance) needs to also be
user-visible.

And nothing says that the best or only way to make
some such info visible to users is using annotations.

Annotations are just one tool.  Not everything is a
nail.

There's a difference between making something visible
and making it matchable.  It's good to have something
that can serve as an unmatchable annotation.  We have
that.

What's useful is to add a new feature: letting
candidates themselves have additional matchable parts,
which can be, but need not be, displayed.

A (full) candidate, itself, already and necessarily
contains whatever info is needed to distinguish it -
makes it unique.  An annotations is (intentionally)
not part of a candidate.

Re: [External] : Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 08.04.2021 21:48, Drew Adams wrote:
> There can be_lots_  of info that's part of a full
> candidate which we do_not_  expose in the UI, so
> users don't necessarily see a difference in
> *Completions*.

If the users can't make a conscious choice between two completions based 
on info available to them, such completions shouldn't both be present in 
the completion results at the same time.

> We can nevertheless make any such info visible in
> other ways, e.g. on demand.

Any such attributes can be accounted for the duplicates-removing logic.

If they aren't visible in *Completions* by default, that doesn't seem 
like great UX, though.

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> > There can be_lots_  of info that's part of a full
> > candidate which we do_not_  expose in the UI, so
> > users don't necessarily see a difference in
> > *Completions*.
> 
> If the users can't make a conscious choice between two completions based
> on info available to them, such completions shouldn't both be present in
> the completion results at the same time.

Read again, please.  "in *Completions*"

And I explicitly mentioned that some such info could
be made available to users in other ways, and that
could even be done on demand.

Lots of things are possible, depending on what's
thought to be most useful for users in the particular
context.

And even the position of a candidate in the displayed
list can, in some cases, be used to disambiguate it
from other similar-looking candidates in the list.

And narrowing can filter on <whatever>, including
data that's not necessarily displayed.  So narrowing
also can distinguish similarly named candidates.

Lots and lots and lots of things are possible.

No one is saying that we should never prefix or
suffix candidates with text that disambiguates them,
or even that just makes them quick to access.

Think Avy.  It shows disambiguating text next to
the current search hits, which you can use for easy
access.  That text is shown on demand, and only for
the current hits.

That kind of thing could be done for otherwise
ambiguous completion candidates - when you cycle to
a `foo' candidate all `foo' candidates show text
that you can use for choosing.  That's just one
possibility (and not necessarily a good one) that
comes to mind in the moment.

Tools, tools; not tool.  Not everything is a nail.

> > We can nevertheless make any such info visible
> > in other ways, e.g. on demand.
> 
> Any such attributes can be accounted for the
> duplicates-removing logic.

Sorry, hand-waving phrases don't mean much to me.

> If they aren't visible in *Completions* by
> default, that doesn't seem like great UX, though.

You can't define what's a great UI in an abstract,
blanket way.  Not usefully.

You're being dismissive, and yet it's likely that
the UI innovations of most of the completion UIs
that are found to be so useful today introduced UI
features that could have been summarily dismissed
by someone not able to see past the veil of their
current hammer.

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

> Yes, that would be the best possibility. There are many ways to provide that
> data: Via a text property, via an extra metadata function, etc. In the case
> of Swiper the extra data could simply be the line number. However a general
> solution should also automatically generate indices, since it is probably
> better to not require the completion table to provide the information in any
> case. I would like some graceful degradation.

There's also the issue that we want the system to work "by default"
(e.g. even if your UI doesn't take advantage of all the latest extra
metadata features).

So maybe rather than having a way to add extra data for disambiguation,
we should require the completion candidates to be unique (as is the case
currently), and then add an extra feature that can hide some part of
the text.

E.g. for a completion table of something like Swiper, every entry would
contain the line's text plus a suffix containing the line number.
And then the metadata would tell the UI how it can hide the suffix if it
doesn't need textual uniqueness.

>>> One thing to consider - maybe returning the highlights as some extra data is
>>> not actually faster than simply propertizing the strings?
>> The extra opaque data is cheap/free to compute.  It would not contain
>> "the highlights", but just the side information that might be needed to
>> compute the highlights: e.g. you can't do the highlighting correctly for
>> `partial-completion` without having access to the "pattern" against
>> which it was matched.
> I don't think this is true.
[...]

I don't see anything in your description that disagrees with what
I said, so it looks like we're in "violent agreement" ;-)


        Stefan

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 7:30 PM, Stefan Monnier wrote:
>> Yes, that would be the best possibility. There are many ways to provide that
>> data: Via a text property, via an extra metadata function, etc. In the case
>> of Swiper the extra data could simply be the line number. However a general
>> solution should also automatically generate indices, since it is probably
>> better to not require the completion table to provide the information in any
>> case. I would like some graceful degradation.
> E.g. for a completion table of something like Swiper, every entry would
> contain the line's text plus a suffix containing the line number.
> And then the metadata would tell the UI how it can hide the suffix if it
> doesn't need textual uniqueness.

This is similar to what I am doing in my `consult-line` command, which 
is a Swiper equivalent. There I encode the line number in Unicode 
characters of some private plane and hide them with 'invisible or 
'display. At least it is unlikely that we don't get unintentional 
matches on those prefixes. But is still a very ugly hack.

If I could mark that part of the string as 'unmatchable I would be happy 
to! Maybe that's the cleaner solution? The nice advantage is that it 
would be backward compatible. I could just start to mark my prefixes as 
'unmatchable and the basic completion style would suddenly start to 
work. On older Emacs versions everything would continue to work as is; 
the user is required to use a completion style which matches substrings, 
e.g., substring, orderless, flex, etc.

>>>> One thing to consider - maybe returning the highlights as some extra data is
>>>> not actually faster than simply propertizing the strings?
>>> The extra opaque data is cheap/free to compute.  It would not contain
>>> "the highlights", but just the side information that might be needed to
>>> compute the highlights: e.g. you can't do the highlighting correctly for
>>> `partial-completion` without having access to the "pattern" against
>>> which it was matched.
>> I don't think this is true.
> [...]
> 
> I don't see anything in your description that disagrees with what
> I said, so it looks like we're in "violent agreement" ;-)

Ok, I didn't get it. But I violently agree :)

Daniel

Re: Improvement proposals for `completing-read'

[Stefan Monnier]

> This is similar to what I am doing in my `consult-line` command, which is
> a Swiper equivalent. There I encode the line number in Unicode characters of
> some private plane and hide them with 'invisible or 'display.

I can understand the `invisible` but I would have used plain `%s` or
`number-to-string` rather than funny unicode chars.

> If I could mark that part of the string as 'unmatchable I would be happy to!

Fun fact: I use a locally-hacked isearch.el that lets me do `C-s NNNN`
to go to line NNNN.

> Maybe that's the cleaner solution? The nice advantage is that it would be
> backward compatible. I could just start to mark my prefixes as 'unmatchable

Why put it in the prefix rather than the suffix?

> and the basic completion style would suddenly start to work. On older Emacs
> versions everything would continue to work as is; the user is required to
> use a completion style which matches substrings,

If you put the line numbers in the suffix, then they won't bother
anyone, even with the most basic completion scheme.


        Stefan

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 8:13 PM, Stefan Monnier wrote:
>> This is similar to what I am doing in my `consult-line` command, which is
>> a Swiper equivalent. There I encode the line number in Unicode characters of
>> some private plane and hide them with 'invisible or 'display.
> 
> I can understand the `invisible` but I would have used plain `%s` or
> `number-to-string` rather than funny unicode chars.

I am using funny unicode tofus in order to prevent accidental matching 
when searching for numbers. The idea is that the numbers should be 
unmatchable!

>> Maybe that's the cleaner solution? The nice advantage is that it would be
>> backward compatible. I could just start to mark my prefixes as 'unmatchable
> 
> Why put it in the prefix rather than the suffix?
> 
>> and the basic completion style would suddenly start to work. On older Emacs
>> versions everything would continue to work as is; the user is required to
>> use a completion style which matches substrings,
> 
> If you put the line numbers in the suffix, then they won't bother
> anyone, even with the most basic completion scheme.

Yes, then it would work with basic completion. The main reason was that 
the candidates are already prefixed with line numbers in the 'display 
property. The real string is the tofu-encoded number hidden behind that.

Maybe it would be better to implement this differently. Use an 
affixation function prefix for the line numbers and append the 
'invisible tofu-encoded line number as suffix. But I think I implemented 
`consult-line` before the affixation function came into existence. I 
have to consider this.

Daniel

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 08.04.2021 18:37, Daniel Mendler wrote:

>> I think we should arrange for the completion table to provide that extra
>> text, so it can be more meaningful than an arbitrary number.  Also the
>> user needs to be able to know "which is which", so it needs to be
>> connected to something that's displayed in *Completions* that
>> distinguishes them.
> 
> Yes, that would be the best possibility. There are many ways to provide 
> that data: Via a text property, via an extra metadata function, etc. In 
> the case of Swiper the extra data could simply be the line number. 
> However a general solution should also automatically generate indices, 
> since it is probably better to not require the completion table to 
> provide the information in any case. I would like some graceful 
> degradation.

Another approach would be to add a new "extra property" or metadata 
element which will say "don't remove duplicates".

I do think it's questionable, though, to include completions that are 
hard to distinguish.

>> E.g. maybe we could arrange for the minibuffer text to be
>> matched against the *annotated* candidates in some cases.  I think this
>> requires more thought, together with concrete examples.
> 
> Generally matching against annotations functions is problematic. As I 
> interpret things, the point of annotations is that they are not 
> matchable. Maybe they are more expensive to compute, so this should 
> happen lazily for the displayed candidates. But using it only for 
> disambiguation should be okay. Then one would only compute the 
> annotation when doing the actual completion?

Matching on annotations is indeed problematic. And if such conflicts are 
rare, perhaps we could rely on the user making the final choice using 
the *Completions* buffer?

Further, that's only important if the choice between them is actually 
important (and we weren't using them just to display, say, all available 
overloads of a method in the completion popup).

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> Another approach would be to add a new "extra property" 
> or metadata element which will say "don't remove duplicates".

I mentioned variable `icicle-transform-function', whose
most common values are `icicle-remove-duplicates' and
nil (which means don't remove dups).

(Like such a variable, completion metadata is an
aggregate operation: it applies to a set of completions,
not to just this or that completion.)

`icicle-transform-function' is applied to all candidates,
and it can do anything.

> I do think it's questionable, though, to include
> completions that are hard to distinguish.

Don't confuse looking different in *Completions*, at the
outset and always, with easy to distinguish.  There are
other ways to make them appear different - from mode-line
to ephemeral appearance changes in *Completions*.  There
is more than one way to skin this cat.

> Matching on annotations is indeed problematic. And if such conflicts are
> rare, perhaps we could rely on the user making the final choice using
> the *Completions* buffer?

Don't rely on any one such thing.  Don't force UI
interactions into one channel.  Provide a general
mechanism (or more than one such).

> Further, that's only important if the choice between them is actually
> important (and we weren't using them just to display, say, all available
> overloads of a method in the completion popup).

Yes.

Re: [External] : Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

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

On 08.04.2021 22:46, Drew Adams wrote:
>> Another approach would be to add a new "extra property"
>> or metadata element which will say "don't remove duplicates".
> 
> I mentioned variable `icicle-transform-function', whose
> most common values are `icicle-remove-duplicates' and
> nil (which means don't remove dups).
> 
> (Like such a variable, completion metadata is an
> aggregate operation: it applies to a set of completions,
> not to just this or that completion.)
> 
> `icicle-transform-function' is applied to all candidates,
> and it can do anything.

A general completion table does specify any `icicle-transform-function`, 
and it wouldn't know how.

Some standard, cross-frontend approach would be required.

>> I do think it's questionable, though, to include
>> completions that are hard to distinguish.
> 
> Don't confuse looking different in *Completions*, at the
> outset and always, with easy to distinguish.  There are
> other ways to make them appear different - from mode-line
> to ephemeral appearance changes in *Completions*.  There
> is more than one way to skin this cat.

Ephemeral appearance changes could be good enough. Or icons, like on the 
attached video.


[-- Attachment #2: Screencast_08.04.2021_22:59:36.webm --]
[-- Type: video/webm, Size: 1028475 bytes --]

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> > I mentioned variable `icicle-transform-function', whose
> > most common values are `icicle-remove-duplicates' and
> > nil (which means don't remove dups).
> >
> > (Like such a variable, completion metadata is an
> > aggregate operation: it applies to a set of completions,
> > not to just this or that completion.)
> >
> > `icicle-transform-function' is applied to all candidates,
> > and it can do anything.
> 
> A general completion table does specify any,
> `icicle-transform-function` and it wouldn't know how.

(Maybe you meant "does not"?)

I described `icicle-transform-function' - see that.
The point is about the behavior provided: transforming
the current completion set.

The current completion table is not the only thing that
can be general.  Everything need not be encoded in it.

Binding a variable allows for use of the same "table"
to behave somewhat differently.  And the same transform
function can be used with multiple, and very different,
"tables".

Both of those are important - as opposed to having to
encode the transformation in each completion table (e.g.
function) itself.

(Of course a completion table can itself remove dups
(or just not create any).  That's already available.)

An alist is a completion table.  Nothing says that the
general completion machinery can't, tomorrow, apply a
function to that alist (a map, a reduce, a whatever)
to produce a different alist.

And there are other ways, besides binding a function
variable or encapsulating in a completion table, to
affect the behavior of completion over a candidate set.

> Some standard, cross-frontend approach would be required.

Sure.  And one such way is to allow for a function that
acts on the set of current candidates (e.g. those matching
input pattern and satisfying filter predicates), possibly
transforming it.

One such possible (and common) transformation is to remove
duplicates.

My points here were two, the first being the more important:

1. Allow for more general behavior possibilities than just
   removing dupicates - however that's done.  (And yes, of
   course we're talking in this thread about what would be
   a common approach.)

2. Binding a function variable is one way that can be done.

Do I think the way chosen should necessarily be part of the
completion table?  A priori, no.  No need to limit thought
to only that possibility now.  And see above, about some
advantages of not doing that.

Am I wedded to binding a function variable as the only way
to do it?  No.  It's certainly a simple way to do it, not
involving any surgery or redefinition of "completion tables".

But the main point was #1: transformation in general, not
just dup removal.  Why not?

> >> I do think it's questionable, though, to include
> >> completions that are hard to distinguish.
> >
> > Don't confuse looking different in *Completions*, at the
> > outset and always, with easy to distinguish.  There are
> > other ways to make them appear different - from mode-line
> > to ephemeral appearance changes in *Completions*.  There
> > is more than one way to skin this cat.
> 
> Ephemeral appearance changes could be good enough. Or icons,
> like on the attached video.

I noticed only one use of `completing-read' in that video -
for `M-x electric-pair-mode'), and no icons in that case (no
*Completions* buffer display at all).

And I didn't notice any cases of duplicate candidates,
whether or not distinguished by their icons.

But I guess you just meant to show icons next to completion
candidates.  Sure, why not?  Any kind of apparent distinction
(face, label, sound,...) is an apparent distinction.

Re: [External] : Re: Improvement proposals for `completing-read'

[Stefan Kangas]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Ephemeral appearance changes could be good enough. Or icons, like on the
> attached video.

We could probably make good use of icons in several places in Emacs.

They look really good in that video, FWIW, and they immediately convey
meaning in a way that you simply can't reproduce with mere coloring.

BTW, what do you use to get those icons?  Are they available OOTB in
company-mode?

Re: [External] : Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 09.04.2021 01:37, Stefan Kangas wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Ephemeral appearance changes could be good enough. Or icons, like on the
>> attached video.
> 
> We could probably make good use of icons in several places in Emacs.

It would be nice to have a set of general purpose icons available in 
ELPA, or ever in the core. For this and other purposes.

For now I'm using vscode's ones (under CC BY 4.0), but I'm guessing it 
won't be good for Emacs's image to migrate to that wholesale.

> They look really good in that video, FWIW, and they immediately convey
> meaning in a way that you simply can't reproduce with mere coloring.
> 
> BTW, what do you use to get those icons?  Are they available OOTB in
> company-mode?

Yes, in the development version. Though disabled by default, for now.

company-format-margin-function is the relevant variable.

You also need a fairly recent Emacs built from master, for 
elisp-completion-at-point integration. With SVG support enabled, too.

Using more and/or better icons in Emacs

[Stefan Kangas]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> It would be nice to have a set of general purpose icons available in
> ELPA, or ever in the core. For this and other purposes.

Yup.  Have a look at the attached quick mock-up of how things could look
if we would use Material icons for the tool-bar as an example.

> For now I'm using vscode's ones (under CC BY 4.0), but I'm guessing it
> won't be good for Emacs's image to migrate to that wholesale.

True.  all-the-icons.el has some links to other freely licensed sets.
Could one of them be a suitable replacement?

[-- Attachment #2: 0001-Use-Material-icons-for-tool-bar.patch --]
[-- Type: text/x-diff, Size: 11201 bytes --]

From e4a10177fadbfbe0a7743b4d7e4c09c45209dc2a Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Fri, 9 Apr 2021 03:42:14 +0200
Subject: [PATCH] Use Material icons for tool-bar

---
 etc/images/material/action/ic_search_24px.svg |  1 +
 .../material/content/ic_content_copy_24px.svg |  1 +
 .../material/content/ic_content_cut_24px.svg  |  1 +
 .../content/ic_content_paste_24px.svg         |  1 +
 .../material/content/ic_create_24px.svg       |  1 +
 etc/images/material/content/ic_redo_24px.svg  |  1 +
 etc/images/material/content/ic_save_24px.svg  |  1 +
 etc/images/material/content/ic_undo_24px.svg  |  1 +
 .../content/inventory_2_black_24dp.svg        |  1 +
 etc/images/material/file/ic_folder_24px.svg   |  1 +
 .../material/navigation/ic_close_24px.svg     |  1 +
 lisp/tool-bar.el                              | 31 ++++++++++---------
 12 files changed, 27 insertions(+), 15 deletions(-)
 create mode 100644 etc/images/material/action/ic_search_24px.svg
 create mode 100644 etc/images/material/content/ic_content_copy_24px.svg
 create mode 100644 etc/images/material/content/ic_content_cut_24px.svg
 create mode 100644 etc/images/material/content/ic_content_paste_24px.svg
 create mode 100644 etc/images/material/content/ic_create_24px.svg
 create mode 100644 etc/images/material/content/ic_redo_24px.svg
 create mode 100644 etc/images/material/content/ic_save_24px.svg
 create mode 100644 etc/images/material/content/ic_undo_24px.svg
 create mode 100644 etc/images/material/content/inventory_2_black_24dp.svg
 create mode 100644 etc/images/material/file/ic_folder_24px.svg
 create mode 100644 etc/images/material/navigation/ic_close_24px.svg

diff --git a/etc/images/material/action/ic_search_24px.svg b/etc/images/material/action/ic_search_24px.svg
new file mode 100644
index 0000000000..12440059b6
--- /dev/null
+++ b/etc/images/material/action/ic_search_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M15.5 14h-.79l-.28-.27C15.41 12.59 16 11.11 16 9.5 16 5.91 13.09 3 9.5 3S3 5.91 3 9.5 5.91 16 9.5 16c1.61 0 3.09-.59 4.23-1.57l.27.28v.79l5 4.99L20.49 19l-4.99-5zm-6 0C7.01 14 5 11.99 5 9.5S7.01 5 9.5 5 14 7.01 14 9.5 11.99 14 9.5 14z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_content_copy_24px.svg b/etc/images/material/content/ic_content_copy_24px.svg
new file mode 100644
index 0000000000..7c6b60aeec
--- /dev/null
+++ b/etc/images/material/content/ic_content_copy_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M16 1H4c-1.1 0-2 .9-2 2v14h2V3h12V1zm3 4H8c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h11c1.1 0 2-.9 2-2V7c0-1.1-.9-2-2-2zm0 16H8V7h11v14z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_content_cut_24px.svg b/etc/images/material/content/ic_content_cut_24px.svg
new file mode 100644
index 0000000000..b89a0d03c0
--- /dev/null
+++ b/etc/images/material/content/ic_content_cut_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M9.64 7.64c.23-.5.36-1.05.36-1.64 0-2.21-1.79-4-4-4S2 3.79 2 6s1.79 4 4 4c.59 0 1.14-.13 1.64-.36L10 12l-2.36 2.36C7.14 14.13 6.59 14 6 14c-2.21 0-4 1.79-4 4s1.79 4 4 4 4-1.79 4-4c0-.59-.13-1.14-.36-1.64L12 14l7 7h3v-1L9.64 7.64zM6 8c-1.1 0-2-.89-2-2s.9-2 2-2 2 .89 2 2-.9 2-2 2zm0 12c-1.1 0-2-.89-2-2s.9-2 2-2 2 .89 2 2-.9 2-2 2zm6-7.5c-.28 0-.5-.22-.5-.5s.22-.5.5-.5.5.22.5.5-.22.5-.5.5zM19 3l-6 6 2 2 7-7V3z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_content_paste_24px.svg b/etc/images/material/content/ic_content_paste_24px.svg
new file mode 100644
index 0000000000..af63a642cc
--- /dev/null
+++ b/etc/images/material/content/ic_content_paste_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M19 2h-4.18C14.4.84 13.3 0 12 0c-1.3 0-2.4.84-2.82 2H5c-1.1 0-2 .9-2 2v16c0 1.1.9 2 2 2h14c1.1 0 2-.9 2-2V4c0-1.1-.9-2-2-2zm-7 0c.55 0 1 .45 1 1s-.45 1-1 1-1-.45-1-1 .45-1 1-1zm7 18H5V4h2v3h10V4h2v16z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_create_24px.svg b/etc/images/material/content/ic_create_24px.svg
new file mode 100644
index 0000000000..f5ddfe1942
--- /dev/null
+++ b/etc/images/material/content/ic_create_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M3 17.25V21h3.75L17.81 9.94l-3.75-3.75L3 17.25zM20.71 7.04c.39-.39.39-1.02 0-1.41l-2.34-2.34c-.39-.39-1.02-.39-1.41 0l-1.83 1.83 3.75 3.75 1.83-1.83z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_redo_24px.svg b/etc/images/material/content/ic_redo_24px.svg
new file mode 100644
index 0000000000..764ab7789b
--- /dev/null
+++ b/etc/images/material/content/ic_redo_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M18.4 10.6C16.55 8.99 14.15 8 11.5 8c-4.65 0-8.58 3.03-9.96 7.22L3.9 16c1.05-3.19 4.05-5.5 7.6-5.5 1.95 0 3.73.72 5.12 1.88L13 16h9V7l-3.6 3.6z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_save_24px.svg b/etc/images/material/content/ic_save_24px.svg
new file mode 100644
index 0000000000..1d3c3005a2
--- /dev/null
+++ b/etc/images/material/content/ic_save_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M17 3H5c-1.11 0-2 .9-2 2v14c0 1.1.89 2 2 2h14c1.1 0 2-.9 2-2V7l-4-4zm-5 16c-1.66 0-3-1.34-3-3s1.34-3 3-3 3 1.34 3 3-1.34 3-3 3zm3-10H5V5h10v4z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/ic_undo_24px.svg b/etc/images/material/content/ic_undo_24px.svg
new file mode 100644
index 0000000000..c37cf40136
--- /dev/null
+++ b/etc/images/material/content/ic_undo_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M12.5 8c-2.65 0-5.05.99-6.9 2.6L2 7v9h9l-3.62-3.62c1.39-1.16 3.16-1.88 5.12-1.88 3.54 0 6.55 2.31 7.6 5.5l2.37-.78C21.08 11.03 17.15 8 12.5 8z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/content/inventory_2_black_24dp.svg b/etc/images/material/content/inventory_2_black_24dp.svg
new file mode 100644
index 0000000000..c35ba104ee
--- /dev/null
+++ b/etc/images/material/content/inventory_2_black_24dp.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" enable-background="new 0 0 24 24" height="24px" viewBox="0 0 24 24" width="24px" fill="#000000"><g><rect fill="none" height="24" width="24"/></g><g><path d="M20,2H4C3,2,2,2.9,2,4v3.01C2,7.73,2.43,8.35,3,8.7V20c0,1.1,1.1,2,2,2h14c0.9,0,2-0.9,2-2V8.7c0.57-0.35,1-0.97,1-1.69V4 C22,2.9,21,2,20,2z M15,14H9v-2h6V14z M20,7H4V4h16V7z"/></g></svg>
\ No newline at end of file
diff --git a/etc/images/material/file/ic_folder_24px.svg b/etc/images/material/file/ic_folder_24px.svg
new file mode 100644
index 0000000000..e70b01ce41
--- /dev/null
+++ b/etc/images/material/file/ic_folder_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M10 4H4c-1.1 0-1.99.9-1.99 2L2 18c0 1.1.9 2 2 2h16c1.1 0 2-.9 2-2V8c0-1.1-.9-2-2-2h-8l-2-2z"/></svg>
\ No newline at end of file
diff --git a/etc/images/material/navigation/ic_close_24px.svg b/etc/images/material/navigation/ic_close_24px.svg
new file mode 100644
index 0000000000..865788b755
--- /dev/null
+++ b/etc/images/material/navigation/ic_close_24px.svg
@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12z"/></svg>
\ No newline at end of file
diff --git a/lisp/tool-bar.el b/lisp/tool-bar.el
index 6da401187b..2c29aae4af 100644
--- a/lisp/tool-bar.el
+++ b/lisp/tool-bar.el
@@ -153,13 +153,15 @@ tool-bar--image-expression
 	 (pbm-spec (append (list :type 'pbm :file
                                  (concat icon ".pbm")) colors))
 	 (xbm-spec (append (list :type 'xbm :file
-                                 (concat icon ".xbm")) colors)))
+                                 (concat icon ".xbm")) colors))
+         (svg-spec (append (list :type 'svg :file
+                                 (concat icon ".svg")) colors)))
     `(find-image (cond ((not (display-color-p))
 			',(list pbm-spec xbm-spec xpm-lo-spec xpm-spec))
 		       ((< (display-color-cells) 256)
-			',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
-		       (t
-			',(list xpm-spec pbm-spec xbm-spec)))
+                        ',(list xpm-lo-spec xpm-spec pbm-spec xbm-spec))
+                       (t
+                        ',(list svg-spec xpm-spec pbm-spec xbm-spec)))
                  t)))
 
 ;;;###autoload
@@ -250,25 +252,24 @@ tool-bar-local-item-from-menu
 (defun tool-bar-setup ()
   (setq tool-bar-separator-image-expression
 	(tool-bar--image-expression "separator"))
-  (tool-bar-add-item-from-menu 'find-file "new" nil :label "New File"
+  (tool-bar-add-item-from-menu 'find-file "material/content/ic_create_24px" nil :label "New File"
 			       :vert-only t)
-  (tool-bar-add-item-from-menu 'menu-find-file-existing "open" nil
+  (tool-bar-add-item-from-menu 'menu-find-file-existing "material/file/ic_folder_24px" nil
 			       :label "Open" :vert-only t)
-  (tool-bar-add-item-from-menu 'dired "diropen" nil :vert-only t)
-  (tool-bar-add-item-from-menu 'kill-this-buffer "close" nil :vert-only t)
-  (tool-bar-add-item-from-menu 'save-buffer "save" nil
-			       :label "Save")
+  (tool-bar-add-item-from-menu 'dired "material/content/inventory_2_black_24dp" nil :vert-only t)
+  (tool-bar-add-item-from-menu 'kill-this-buffer "material/navigation/ic_close_24px" nil :vert-only t)
+  (tool-bar-add-item-from-menu 'save-buffer "material/content/ic_save_24px" nil :vert-only t)
   (define-key-after (default-value 'tool-bar-map) [separator-1] menu-bar-separator)
-  (tool-bar-add-item-from-menu 'undo "undo" nil)
+  (tool-bar-add-item-from-menu 'undo "material/content/ic_undo_24px" nil :vert-only t)
   (define-key-after (default-value 'tool-bar-map) [separator-2] menu-bar-separator)
   (tool-bar-add-item-from-menu (lookup-key menu-bar-edit-menu [cut])
-			       "cut" nil :vert-only t)
+                               "material/content/ic_content_cut_24px" nil :vert-only t)
   (tool-bar-add-item-from-menu (lookup-key menu-bar-edit-menu [copy])
-			       "copy" nil :vert-only t)
+                               "material/content/ic_content_copy_24px" nil :vert-only t)
   (tool-bar-add-item-from-menu (lookup-key menu-bar-edit-menu [paste])
-			       "paste" nil :vert-only t)
+                               "material/content/ic_content_paste_24px" nil :vert-only t)
   (define-key-after (default-value 'tool-bar-map) [separator-3] menu-bar-separator)
-  (tool-bar-add-item-from-menu 'isearch-forward "search"
+  (tool-bar-add-item-from-menu 'isearch-forward "material/action/ic_search_24px"
 			       nil :label "Search" :vert-only t)
   ;;(tool-bar-add-item-from-menu 'ispell-buffer "spell")
 
-- 
2.30.2


[-- Attachment #3: material-toolbar.png --]
[-- Type: image/png, Size: 37190 bytes --]

Re: Using more and/or better icons in Emacs

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Yup.  Have a look at the attached quick mock-up of how things could look
> if we would use Material icons for the tool-bar as an example.

Looks good.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Using more and/or better icons in Emacs

[chad]

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

On Thu, Apr 8, 2021 at 7:10 PM Stefan Kangas <stefan@marxist.se> wrote:

>
> True.  all-the-icons.el has some links to other freely licensed sets.
> Could one of them be a suitable replacement?
>

All-the-icons uses fonts to "bundle" the icons, which I recall some people
dislike, but we could extract them. Here's a link to the details of those
fonts:
  https://github.com/domtronn/all-the-icons.el#resource-fonts

That takes you to Github, so here's the relevant data. (I tried to use the
eww render, but it lost the links.)

All of the fonts provided in this packages as resources come with either
the SIL Open Font License *(OFL)* or an MIT License, below I will link to
each of the fonts Sources and their Licenses.
Font NameFontLicense
file-icons.ttf Atom File Icons Plugin <https://atom.io/packages/file-icons> MIT
LICENSE <https://github.com/DanBrooker/file-icons/blob/master/LICENSE.md>
fontawesome.ttf FontAwesome Icons <http://fontawesome.io/> SIL OFL LICENSE
<https://github.com/FortAwesome/Font-Awesome#license>
ocitcons.ttf GitHub OctIcons <http://octicons.github.com/> SIL OFL LICENSE
<https://github.com/primer/octicons/blob/master/LICENSE>
weathericons.ttf Weather Icons
<https://erikflowers.github.io/weather-icons/> SIL OFL LICENSE
<https://github.com/primer/octicons/blob/master/LICENSE>
material-design-icons.ttf Material Icons
<http://google.github.io/material-design-icons/> APACHE LICENSE v2.0
<http://www.apache.org/licenses/LICENSE-2.0.txt>
all-the-icons.ttf Custom Made Font MIT LICENSE
Hope that helps,
~Chad

[-- Attachment #2: Type: text/html, Size: 6392 bytes --]

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

chad <yandros@gmail.com> writes:

> All-the-icons uses fonts to "bundle" the icons, which I recall some people
> dislike, but we could extract them.

AFAIU, there is no need to extract anything.  all-the-icons have simply
packed various freely licensed item sets into ttf fonts, so we just need
to copy the original SVG files.

They also have a custom made font.  Perhaps we would need to extract the
glyphs from that one (I don't know), but it doesn't seem to be a very
large set.

Re: Using more and/or better icons in Emacs

[Yuri Khan]

On Fri, 9 Apr 2021 at 09:10, Stefan Kangas <stefan@marxist.se> wrote:

> Yup.  Have a look at the attached quick mock-up of how things could look
> if we would use Material icons for the tool-bar as an example.

The proper way to achieve that look is to design a GTK icon theme with
those icons, then install and activate it in your environment. It’s
not a good idea to override GTK icons with application-bundled icons,
it leads to inconsistency over the desktop.

Re: Using more and/or better icons in Emacs

[tomas]

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

On Fri, Apr 09, 2021 at 02:41:04PM +0700, Yuri Khan wrote:
> On Fri, 9 Apr 2021 at 09:10, Stefan Kangas <stefan@marxist.se> wrote:
> 
> > Yup.  Have a look at the attached quick mock-up of how things could look
> > if we would use Material icons for the tool-bar as an example.
> 
> The proper way to achieve that look is to design a GTK icon theme with
> those icons, then install and activate it in your environment. It’s
> not a good idea to override GTK icons with application-bundled icons,
> it leads to inconsistency over the desktop.

This is actually a conflict being sorted out currently (although
many participants therein don't even know they've been coopted to
that).

Oversimplifying a fair bit, it reduces to whether the application
or the sysadmin (aka "operating system in a very broad sense") is
boss.

A conflict perhaps nearly as old as computing is, but with the
upcoming of  huge mammoths facing operating systems they don't
control, the push is now in the direction of "application is boss"
(it isn't a coincidence that Google Chrome wants to do client-side
decorations: "I'm the operating system, deal with it"; it's not
invited to my computer for that reason ;-)

Cheers
 - t

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Using more and/or better icons in Emacs

[Dmitry Gutov]

On 09.04.2021 12:59, tomas@tuxteam.de wrote:
> This is actually a conflict being sorted out currently (although
> many participants therein don't even know they've been coopted to
> that).

Where?

Re: Using more and/or better icons in Emacs

[Dmitry Gutov]

On 09.04.2021 05:09, Stefan Kangas wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> It would be nice to have a set of general purpose icons available in
>> ELPA, or ever in the core. For this and other purposes.
> 
> Yup.  Have a look at the attached quick mock-up of how things could look
> if we would use Material icons for the tool-bar as an example.

I like it!

My Emacs already shows nice icons in the toolbar, though, thanks to GTK3 
theme integration. And weren't we talking about removing the toolbar?

You have my support for replacing the default set, of course.

>> For now I'm using vscode's ones (under CC BY 4.0), but I'm guessing it
>> won't be good for Emacs's image to migrate to that wholesale.
> 
> True.  all-the-icons.el has some links to other freely licensed sets.
> Could one of them be a suitable replacement?

To be clear, I'm using SVG icons there in the video. And I think the 
discussions around emacs-devel showed certain rejection of the ideal of 
using of fonts for icons.

Would be nice otherwise, though. If perhaps a tad limiting.

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Dmitry Gutov <dgutov@yandex.ru> writes:

> My Emacs already shows nice icons in the toolbar, though, thanks to GTK3
> theme integration. And weren't we talking about removing the toolbar?

Yes, removing it would be better.

But while it is still there we could at least make it look better.

> You have my support for replacing the default set, of course.

Thanks.

>> True.  all-the-icons.el has some links to other freely licensed sets.
>> Could one of them be a suitable replacement?
>
> To be clear, I'm using SVG icons there in the video. And I think the
> discussions around emacs-devel showed certain rejection of the ideal of
> using of fonts for icons.

all-the-icons.el does distribute TTF fonts, but IIUC that's simply
packaged up versions of free sets that were originally distributed in
SVG.

So what I meant was that we could look at what sets they are linking to,
get the original SVG files and then use those.  That's what I did for
the Material icon toolbar mock-up.

Re: Using more and/or better icons in Emacs

[Dmitry Gutov]

On 09.04.2021 15:22, Stefan Kangas wrote:
> all-the-icons.el does distribute TTF fonts, but IIUC that's simply
> packaged up versions of free sets that were originally distributed in
> SVG.
> 
> So what I meant was that we could look at what sets they are linking to,
> get the original SVG files and then use those.  That's what I did for
> the Material icon toolbar mock-up.

Ah, that's a very good idea.

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 09.04.2021 15:22, Stefan Kangas wrote:
>> all-the-icons.el does distribute TTF fonts, but IIUC that's simply
>> packaged up versions of free sets that were originally distributed in
>> SVG.
>>
>> So what I meant was that we could look at what sets they are linking to,
>> get the original SVG files and then use those.  That's what I did for
>> the Material icon toolbar mock-up.
>
> Ah, that's a very good idea.

For this to work, we obviously need to insert SVG files that are the
same height as the current face.

Experimenting with this now, using my patch,

(insert-image (find-image '((:type svg
                             :file "material/file/ic_folder_24px.svg"))))

I'm getting an SVG in the buffer that is actually taller than the line.
When we added new SVG icons to customize recently, Alan Third fiddled
with the height and viewport in the SVG file to force it to be the same
height as the line.

But is there any way to do this directly from Emacs?  It seems like a
pain to have to edit the SVG file for every icon (and my attempts at
doing so has failed so far...).

In a web browser, I think you would just set the CSS width and height
to whatever you want and the browser would resize the image to fit.

Re: Using more and/or better icons in Emacs

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 9 Apr 2021 12:46:57 -0500
> 
> Experimenting with this now, using my patch,
> 
> (insert-image (find-image '((:type svg
>                              :file "material/file/ic_folder_24px.svg"))))
> 
> I'm getting an SVG in the buffer that is actually taller than the line.
> When we added new SVG icons to customize recently, Alan Third fiddled
> with the height and viewport in the SVG file to force it to be the same
> height as the line.
> 
> But is there any way to do this directly from Emacs?

The :scale attribute is supposed to be the way to do that, AFAIU.

Re: Using more and/or better icons in Emacs

[Alan Third]

On Fri, Apr 09, 2021 at 12:46:57PM -0500, Stefan Kangas wrote:
> For this to work, we obviously need to insert SVG files that are the
> same height as the current face.
> 
> Experimenting with this now, using my patch,
> 
> (insert-image (find-image '((:type svg
>                              :file "material/file/ic_folder_24px.svg"))))
> 
> I'm getting an SVG in the buffer that is actually taller than the line.
> When we added new SVG icons to customize recently, Alan Third fiddled
> with the height and viewport in the SVG file to force it to be the same
> height as the line.
> 
> But is there any way to do this directly from Emacs?  It seems like a
> pain to have to edit the SVG file for every icon (and my attempts at
> doing so has failed so far...).

Something like

    (insert-image (create-image "my/svg/file.svg" 'svg nil
                   :height (line-pixel-height)
                   :ascent 'center))

Although it won't tie in quite as neatly as having the actual SVG
height set to 1em or whatever.

-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Alan Third]

On Fri, Apr 09, 2021 at 08:30:14PM +0100, Alan Third wrote:
> Something like
> 
>     (insert-image (create-image "my/svg/file.svg" 'svg nil
>                    :height (line-pixel-height)
>                    :ascent 'center))
> 
> Although it won't tie in quite as neatly as having the actual SVG
> height set to 1em or whatever.

... and if you want the actual text height instead of line height it's
more complex, have a look at scale and height in this:

    https://gist.github.com/alanthird/7b86dc66df1ed3b9006bcd3fddd7350f

although there's probably a better way of doing it.
-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Dmitry Gutov]

On 09.04.2021 22:40, Alan Third wrote:
> ... and if you want the actual text height instead of line height it's
> more complex, have a look at scale and height in this:
> 
>      https://gist.github.com/alanthird/7b86dc66df1ed3b9006bcd3fddd7350f
> 
> although there's probably a better way of doing it.

Thanks, Alan.

I also found that line height is too much for an icon (it looks much 
bigger than a text character this way).

This algorithm gives a nice result, not far from the size I already 
picked myself.

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Alan Third <alan@idiocy.org> writes:

> On Fri, Apr 09, 2021 at 08:30:14PM +0100, Alan Third wrote:
>> Something like
>>
>>     (insert-image (create-image "my/svg/file.svg" 'svg nil
>>                    :height (line-pixel-height)
>>                    :ascent 'center))
>>
>> Although it won't tie in quite as neatly as having the actual SVG
>> height set to 1em or whatever.
>
> ... and if you want the actual text height instead of line height it's
> more complex, have a look at scale and height in this:
>
>     https://gist.github.com/alanthird/7b86dc66df1ed3b9006bcd3fddd7350f
>
> although there's probably a better way of doing it.

Thanks!  That is really useful.

Basing myself on that I have ended up with the following fragment:

(let* ((scale (cadr (assoc :height (assoc 'default face-remapping-alist))))
       (family (face-attribute 'default :family))
       (height (* (aref (font-info family) 2) (if scale scale 1)))
       (ascent (* (aref (font-info family) 8) (if scale scale 1))))
  (insert-image (create-image "material/action/ic_search_24px.svg" 'svg nil
                              :height height
                              :ascent 'center)))

Unfortunately, the SVG image doesn't change size when I use
`text-scale-adjust'.  But in our recent work on `customize' that is not
the case, so I feel like I must be overlooking something here.

> BTW, looking at those icons all you should have to do is set the width
> and height to "1em". I had to insert the viewBox on the other ones
> because they didn't have one to begin with, just the width and height.

The above fragment seems to work the same way whether or not I use 24px
or 1em as width and height in the SVG file.  IOW, it doesn't make a
difference.

Is that expected?  I can't understand why it wouldn't matter.

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> Basing myself on that I have ended up with the following fragment:
>
> (let* ((scale (cadr (assoc :height (assoc 'default face-remapping-alist))))
>        (family (face-attribute 'default :family))
>        (height (* (aref (font-info family) 2) (if scale scale 1)))
>        (ascent (* (aref (font-info family) 8) (if scale scale 1))))
>   (insert-image (create-image "material/action/ic_search_24px.svg" 'svg nil
>                               :height height
>                               :ascent 'center)))

Hmm, I'm now testing simply using this, with width and height set to 1em
in the SVG file:

(insert-image (create-image "material/action/ic_search_24px.svg" 'svg nil
                            :ascent 'center))

And it seems to do precisely the correct thing, as well as work with
`text-scale-adjust'.  So perhaps that's the winner?

Re: Using more and/or better icons in Emacs

[Alan Third]

On Fri, Apr 09, 2021 at 08:43:56PM -0500, Stefan Kangas wrote:
> Stefan Kangas <stefan@marxist.se> writes:
> 
> > Basing myself on that I have ended up with the following fragment:
> >
> > (let* ((scale (cadr (assoc :height (assoc 'default face-remapping-alist))))
> >        (family (face-attribute 'default :family))
> >        (height (* (aref (font-info family) 2) (if scale scale 1)))
> >        (ascent (* (aref (font-info family) 8) (if scale scale 1))))
> >   (insert-image (create-image "material/action/ic_search_24px.svg" 'svg nil
> >                               :height height
> >                               :ascent 'center)))
> 
> Hmm, I'm now testing simply using this, with width and height set to 1em
> in the SVG file:
> 
> (insert-image (create-image "material/action/ic_search_24px.svg" 'svg nil
>                             :ascent 'center))
> 
> And it seems to do precisely the correct thing, as well as work with
> `text-scale-adjust'.  So perhaps that's the winner?

Yes, sorry, I seem to have caused some confusion. I thought you were
asking how to do the same thing without modifying the files. I don't
know how to do the exact same thing, because as you say they don't
scale with the text on the fly, but it's as near as I've been able to
get.

So modifying the files seems like the best solution to me. It works
the best and doesn't require special code when inserting the image.
-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Alan Third <alan@idiocy.org> writes:

>> Hmm, I'm now testing simply using this, with width and height set to 1em
>> in the SVG file:
>>
>> (insert-image (create-image "material/action/ic_search_24px.svg" 'svg nil
>>                             :ascent 'center))
>>
>> And it seems to do precisely the correct thing, as well as work with
>> `text-scale-adjust'.  So perhaps that's the winner?
>
> Yes, sorry, I seem to have caused some confusion. I thought you were
> asking how to do the same thing without modifying the files. I don't
> know how to do the exact same thing, because as you say they don't
> scale with the text on the fly, but it's as near as I've been able to
> get.

Well, I might be the cause for that as I was going back and forth a bit
as I was experimenting.  It is useful here to know that to get the best
results there is no substitution for editing the actual files.  So I'll
continue experimenting in that direction for now.

But is there anything lacking in Emacs' SVG and/or image support?  For
example, it would be very nice to be able to just do something similar
to what you would do with CSS in a browser: give `:height "1em" and have
it work automatically.  Is it worth filing a feature request about this,
or are we happy with the state of things?

> So modifying the files seems like the best solution to me. It works
> the best and doesn't require special code when inserting the image.

Yup.  Thanks a lot for your help!

Re: Using more and/or better icons in Emacs

[Alan Third]

On Sat, Apr 10, 2021 at 05:56:32AM -0500, Stefan Kangas wrote:
> But is there anything lacking in Emacs' SVG and/or image support?  For
> example, it would be very nice to be able to just do something similar
> to what you would do with CSS in a browser: give `:height "1em" and have
> it work automatically.  Is it worth filing a feature request about this,
> or are we happy with the state of things?

I don't see any reason why we couldn't support CSS style units. At the
point that we're creating the image we have access to the complete
face information so it would be fairly simple to convert things like
'1em' to a number of pixels.

Although maybe we'd like to make it a bit more lispy and do something
like ":height '(1 . em)". It's easier to parse in C. ;)
-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Alan Third]

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

On Sat, Apr 10, 2021 at 02:48:56PM +0100, Alan Third wrote:
> On Sat, Apr 10, 2021 at 05:56:32AM -0500, Stefan Kangas wrote:
> > But is there anything lacking in Emacs' SVG and/or image support?  For
> > example, it would be very nice to be able to just do something similar
> > to what you would do with CSS in a browser: give `:height "1em" and have
> > it work automatically.  Is it worth filing a feature request about this,
> > or are we happy with the state of things?
> 
> I don't see any reason why we couldn't support CSS style units. At the
> point that we're creating the image we have access to the complete
> face information so it would be fairly simple to convert things like
> '1em' to a number of pixels.
> 
> Although maybe we'd like to make it a bit more lispy and do something
> like ":height '(1 . em)". It's easier to parse in C. ;)

See attached.

I've only put in "em", as I don't know what other units might be of
use, or are easy to get our hands on.
-- 
Alan Third

[-- Attachment #2: 0001-Allow-use-of-em-in-image-spec-sizes.patch --]
[-- Type: text/plain, Size: 4750 bytes --]

From 63260127f5f4aeed7843e6c17a82ea74eedc9e8e Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 12 Apr 2021 20:30:12 +0100
Subject: [PATCH] Allow use of em in image spec sizes

* src/image.c (image_get_length): New function.
(compute_image_size): Use image_get_length to set the sizes, and pass
in the image struct instead of just the spec.
(image_set_transform):
(imagemagick_load_image):
(svg_load_image): Use the image instead of hte spec in compute_image_size.
(syms_of_image): Add 'em' as a symbol.
---
 src/image.c | 53 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/src/image.c b/src/image.c
index 6fe0b23f73..575b4c7200 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1987,46 +1987,63 @@ scale_image_size (int size, size_t divisor, size_t multiplier)
   return INT_MAX;
 }
 
+/* Return a size in pixels either directly from the value specified by
+   SYMBOL, or from a CONS of the form (VALUE . UNITS).  If the value
+   doesn't exist in the image spec, or is invalid, return -1.  */
+static int
+image_get_length (struct image *img, Lisp_Object symbol)
+{
+  Lisp_Object value = image_spec_value (img->spec, symbol, NULL);
+
+  if (FIXNATP (value))
+    return min (XFIXNAT (value), INT_MAX);
+  if (CONSP (value) && NUMBERP (CAR (value)) && EQ (Qem, CDR (value)))
+    return min (img->face_font_size * XFLOATINT (CAR (value)), INT_MAX);
+
+  return -1;
+}
+
 /* Compute the desired size of an image with native size WIDTH x HEIGHT.
    Use SPEC to deduce the size.  Store the desired size into
    *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
 static void
 compute_image_size (size_t width, size_t height,
-		    Lisp_Object spec,
+		    struct image *img,
 		    int *d_width, int *d_height)
 {
   Lisp_Object value;
+  int int_value;
   int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
   double scale = 1;
 
-  value = image_spec_value (spec, QCscale, NULL);
+  value = image_spec_value (img->spec, QCscale, NULL);
   if (NUMBERP (value))
     scale = XFLOATINT (value);
 
-  value = image_spec_value (spec, QCmax_width, NULL);
-  if (FIXNATP (value))
-    max_width = min (XFIXNAT (value), INT_MAX);
+  int_value = image_get_length (img, QCmax_width);
+  if (int_value >= 0)
+    max_width = int_value;
 
-  value = image_spec_value (spec, QCmax_height, NULL);
-  if (FIXNATP (value))
-    max_height = min (XFIXNAT (value), INT_MAX);
+  int_value = image_get_length (img, QCmax_height);
+  if (int_value >= 0)
+    max_height = int_value;
 
   /* If width and/or height is set in the display spec assume we want
      to scale to those values.  If either h or w is unspecified, the
      unspecified should be calculated from the specified to preserve
      aspect ratio.  */
-  value = image_spec_value (spec, QCwidth, NULL);
-  if (FIXNATP (value))
+  int_value = image_get_length (img, QCwidth);
+  if (int_value >= 0)
     {
-      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
+      desired_width = int_value;
       /* :width overrides :max-width. */
       max_width = -1;
     }
 
-  value = image_spec_value (spec, QCheight, NULL);
-  if (FIXNATP (value))
+  int_value = image_get_length (img, QCheight);
+  if (int_value >= 0)
     {
-      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
+      desired_height = int_value;
       /* :height overrides :max-height. */
       max_height = -1;
     }
@@ -2216,7 +2233,7 @@ image_set_transform (struct frame *f, struct image *img)
     }
   else
 #endif
-    compute_image_size (img->width, img->height, img->spec, &width, &height);
+    compute_image_size (img->width, img->height, img, &width, &height);
 
   /* Determine rotation.  */
   double rotation = 0.0;
@@ -9210,7 +9227,7 @@ imagemagick_load_image (struct frame *f, struct image *img,
 
   compute_image_size (MagickGetImageWidth (image_wand),
 		      MagickGetImageHeight (image_wand),
-		      img->spec, &desired_width, &desired_height);
+		      img, &desired_width, &desired_height);
 
   if (desired_width != -1 && desired_height != -1)
     {
@@ -10068,7 +10085,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
     viewbox_height = dimension_data.height;
   }
 
-  compute_image_size (viewbox_width, viewbox_height, img->spec,
+  compute_image_size (viewbox_width, viewbox_height, img,
                       &width, &height);
 
   width *= FRAME_SCALE_FACTOR (f);
@@ -10777,6 +10794,8 @@ syms_of_image (void)
   DEFSYM (QCmax_width, ":max-width");
   DEFSYM (QCmax_height, ":max-height");
 
+  DEFSYM (Qem, "em");
+
 #ifdef HAVE_NATIVE_TRANSFORMS
   DEFSYM (Qscale, "scale");
   DEFSYM (Qrotate, "rotate");
-- 
2.29.2

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Alan Third <alan@idiocy.org> writes:

>> I don't see any reason why we couldn't support CSS style units. At the
>> point that we're creating the image we have access to the complete
>> face information so it would be fairly simple to convert things like
>> '1em' to a number of pixels.
>>
>> Although maybe we'd like to make it a bit more lispy and do something
>> like ":height '(1 . em)". It's easier to parse in C. ;)
>
> See attached.

Wow, this is very useful.  It works great.  Thanks!

The only issue I see is that, unless the original SVG also has height
and width 1em, it doesn't seem to automatically resize images on
`text-scale-adjust'.

Re: Using more and/or better icons in Emacs

[Alan Third]

On Mon, Apr 12, 2021 at 08:25:44PM -0500, Stefan Kangas wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> >> I don't see any reason why we couldn't support CSS style units. At the
> >> point that we're creating the image we have access to the complete
> >> face information so it would be fairly simple to convert things like
> >> '1em' to a number of pixels.
> >>
> >> Although maybe we'd like to make it a bit more lispy and do something
> >> like ":height '(1 . em)". It's easier to parse in C. ;)
> >
> > See attached.
> 
> Wow, this is very useful.  It works great.  Thanks!
> 
> The only issue I see is that, unless the original SVG also has height
> and width 1em, it doesn't seem to automatically resize images on
> `text-scale-adjust'.

Hmm, it works fine here... Even with XPMs...

Can you show me what exactly you're doing?
-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Alan Third <alan@idiocy.org> writes:

>> The only issue I see is that, unless the original SVG also has height
>> and width 1em, it doesn't seem to automatically resize images on
>> `text-scale-adjust'.
>
> Hmm, it works fine here... Even with XPMs...
>
> Can you show me what exactly you're doing?

Hmm, I'm not sure anymore.  It seems to work perfectly, so I must have
confused myself somehow.  Sorry about the noise.

Your patch LGTM, but I guess it would need documentation and a NEWS entry.
Thanks again for working on this!

Re: Using more and/or better icons in Emacs

[Alan Third]

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

On Tue, Apr 13, 2021 at 05:39:55AM -0500, Stefan Kangas wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> >> The only issue I see is that, unless the original SVG also has height
> >> and width 1em, it doesn't seem to automatically resize images on
> >> `text-scale-adjust'.
> >
> > Hmm, it works fine here... Even with XPMs...
> >
> > Can you show me what exactly you're doing?
> 
> Hmm, I'm not sure anymore.  It seems to work perfectly, so I must have
> confused myself somehow.  Sorry about the noise.
> 
> Your patch LGTM, but I guess it would need documentation and a NEWS entry.
> Thanks again for working on this!

Thanks. I've attached a version with documentation. I think it's good
enough but would appreciate someone else double checking it makes sense.
-- 
Alan Third

[-- Attachment #2: 0001-Allow-use-of-em-in-image-spec-sizes.patch --]
[-- Type: text/plain, Size: 6224 bytes --]

From 8719118481d673ef8a5269ac186aa132526830fa Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 12 Apr 2021 20:30:12 +0100
Subject: [PATCH] Allow use of em in image spec sizes

* src/image.c (image_get_length): New function.
(compute_image_size): Use image_get_length to set the sizes, and pass
in the image struct instead of just the spec.
(image_set_transform):
(imagemagick_load_image):
(svg_load_image): Use the image instead of the spec in compute_image_size.
(syms_of_image): Add 'em' as a symbol.
---
 doc/lispref/display.texi |  7 ++++++
 etc/NEWS                 |  7 ++++++
 src/image.c              | 53 +++++++++++++++++++++++++++-------------
 3 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 68d7e827d2..2f478471a9 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5289,6 +5289,13 @@ Image Descriptors
 and values, including at least the pair @code{:type @var{type}} that
 specifies the image type.
 
+  Image descriptors which define image dimensions, i.e. @code{:width},
+@code{:height}, @code{:max-width} and @code{:max-height}, may take
+either a number, which represents the dimension in pixels, or a pair
+@code{(@var{value} . 'em)}, where @var{value} is the dimension's
+length in ``ems''.  One ``em'' is equivalent to the height of the
+font.
+
   The following is a list of properties that are meaningful for all
 image types (there are also properties which are meaningful only for
 certain image types, as documented in the following subsections):
diff --git a/etc/NEWS b/etc/NEWS
index d3a8748ded..c005b59553 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1532,6 +1532,13 @@ will match the font size in use where it is embedded.
 
 This feature relies on librsvg 2.48 or above being available.
 
++++
+*** Image properties support 'em' sizes.
+Size image properties, for example ':height', ':max-height', etc., can
+be given a cons of the form '(SIZE . em)', where SIZE is a number
+which is multiplied by the font size to calculate the image size, and
+'em' is a symbol.
+
 ** EWW
 
 +++
diff --git a/src/image.c b/src/image.c
index 6fe0b23f73..575b4c7200 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1987,46 +1987,63 @@ scale_image_size (int size, size_t divisor, size_t multiplier)
   return INT_MAX;
 }
 
+/* Return a size in pixels either directly from the value specified by
+   SYMBOL, or from a CONS of the form (VALUE . UNITS).  If the value
+   doesn't exist in the image spec, or is invalid, return -1.  */
+static int
+image_get_length (struct image *img, Lisp_Object symbol)
+{
+  Lisp_Object value = image_spec_value (img->spec, symbol, NULL);
+
+  if (FIXNATP (value))
+    return min (XFIXNAT (value), INT_MAX);
+  if (CONSP (value) && NUMBERP (CAR (value)) && EQ (Qem, CDR (value)))
+    return min (img->face_font_size * XFLOATINT (CAR (value)), INT_MAX);
+
+  return -1;
+}
+
 /* Compute the desired size of an image with native size WIDTH x HEIGHT.
    Use SPEC to deduce the size.  Store the desired size into
    *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
 static void
 compute_image_size (size_t width, size_t height,
-		    Lisp_Object spec,
+		    struct image *img,
 		    int *d_width, int *d_height)
 {
   Lisp_Object value;
+  int int_value;
   int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
   double scale = 1;
 
-  value = image_spec_value (spec, QCscale, NULL);
+  value = image_spec_value (img->spec, QCscale, NULL);
   if (NUMBERP (value))
     scale = XFLOATINT (value);
 
-  value = image_spec_value (spec, QCmax_width, NULL);
-  if (FIXNATP (value))
-    max_width = min (XFIXNAT (value), INT_MAX);
+  int_value = image_get_length (img, QCmax_width);
+  if (int_value >= 0)
+    max_width = int_value;
 
-  value = image_spec_value (spec, QCmax_height, NULL);
-  if (FIXNATP (value))
-    max_height = min (XFIXNAT (value), INT_MAX);
+  int_value = image_get_length (img, QCmax_height);
+  if (int_value >= 0)
+    max_height = int_value;
 
   /* If width and/or height is set in the display spec assume we want
      to scale to those values.  If either h or w is unspecified, the
      unspecified should be calculated from the specified to preserve
      aspect ratio.  */
-  value = image_spec_value (spec, QCwidth, NULL);
-  if (FIXNATP (value))
+  int_value = image_get_length (img, QCwidth);
+  if (int_value >= 0)
     {
-      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
+      desired_width = int_value;
       /* :width overrides :max-width. */
       max_width = -1;
     }
 
-  value = image_spec_value (spec, QCheight, NULL);
-  if (FIXNATP (value))
+  int_value = image_get_length (img, QCheight);
+  if (int_value >= 0)
     {
-      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
+      desired_height = int_value;
       /* :height overrides :max-height. */
       max_height = -1;
     }
@@ -2216,7 +2233,7 @@ image_set_transform (struct frame *f, struct image *img)
     }
   else
 #endif
-    compute_image_size (img->width, img->height, img->spec, &width, &height);
+    compute_image_size (img->width, img->height, img, &width, &height);
 
   /* Determine rotation.  */
   double rotation = 0.0;
@@ -9210,7 +9227,7 @@ imagemagick_load_image (struct frame *f, struct image *img,
 
   compute_image_size (MagickGetImageWidth (image_wand),
 		      MagickGetImageHeight (image_wand),
-		      img->spec, &desired_width, &desired_height);
+		      img, &desired_width, &desired_height);
 
   if (desired_width != -1 && desired_height != -1)
     {
@@ -10068,7 +10085,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
     viewbox_height = dimension_data.height;
   }
 
-  compute_image_size (viewbox_width, viewbox_height, img->spec,
+  compute_image_size (viewbox_width, viewbox_height, img,
                       &width, &height);
 
   width *= FRAME_SCALE_FACTOR (f);
@@ -10777,6 +10794,8 @@ syms_of_image (void)
   DEFSYM (QCmax_width, ":max-width");
   DEFSYM (QCmax_height, ":max-height");
 
+  DEFSYM (Qem, "em");
+
 #ifdef HAVE_NATIVE_TRANSFORMS
   DEFSYM (Qscale, "scale");
   DEFSYM (Qrotate, "rotate");
-- 
2.29.2

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Alan Third <alan@idiocy.org> writes:

> Thanks. I've attached a version with documentation. I think it's good
> enough but would appreciate someone else double checking it makes sense.

LGTM with some minor nits below.

> +  Image descriptors which define image dimensions, i.e. @code{:width},

The "i.e." could perhaps more clearly be written as "such as".

> +@code{:height}, @code{:max-width} and @code{:max-height}, may take
> +either a number, which represents the dimension in pixels, or a pair
> +@code{(@var{value} . 'em)}, where @var{value} is the dimension's
> +length in ``ems''.  One ``em'' is equivalent to the height of the
> +font.

Is it worth clarifying that `value' can be a float?

> ++++
> +*** Image properties support 'em' sizes.
> +Size image properties, for example ':height', ':max-height', etc., can
> +be given a cons of the form '(SIZE . em)', where SIZE is a number
> +which is multiplied by the font size to calculate the image size, and
> +'em' is a symbol.

Same question here.

Re: Using more and/or better icons in Emacs

[Eli Zaretskii]

> Date: Tue, 13 Apr 2021 20:50:12 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org, Dmitry Gutov <dgutov@yandex.ru>
> 
> > Your patch LGTM, but I guess it would need documentation and a NEWS entry.
> > Thanks again for working on this!
> 
> Thanks. I've attached a version with documentation. I think it's good
> enough but would appreciate someone else double checking it makes sense.

Thanks, a few minor nits below.

> --- a/doc/lispref/display.texi
> +++ b/doc/lispref/display.texi
> @@ -5289,6 +5289,13 @@ Image Descriptors
>  and values, including at least the pair @code{:type @var{type}} that
>  specifies the image type.
>  
> +  Image descriptors which define image dimensions, i.e. @code{:width},
> +@code{:height}, @code{:max-width} and @code{:max-height}, may take
> +either a number, which represents the dimension in pixels, or a pair
> +@code{(@var{value} . 'em)}, where @var{value} is the dimension's
> +length in ``ems''.  One ``em'' is equivalent to the height of the
> +font.

I would use @dfn{em} the first time, and thereafter just em, without
quotes.  It would also be good to have a @footnote explaining the
source of the name "em".

Also, VALUE can be a float, and that should be documented.

> +/* Return a size in pixels either directly from the value specified by
> +   SYMBOL, or from a CONS of the form (VALUE . UNITS).  If the value
> +   doesn't exist in the image spec, or is invalid, return -1.  */
> +static int
> +image_get_length (struct image *img, Lisp_Object symbol)

The "length" part of the name defeats its mnemonic value.  I suggest
image_get_dimension instead.

Also, I think the commentary is inaccurate or at least confusing (is
it from some earlier version of the code, perhaps?).

Re: Using more and/or better icons in Emacs

[Alan Third]

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

On Wed, Apr 14, 2021 at 09:46:38AM +0300, Eli Zaretskii wrote:
> > Date: Tue, 13 Apr 2021 20:50:12 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org, Dmitry Gutov <dgutov@yandex.ru>
> > 
> > > Your patch LGTM, but I guess it would need documentation and a NEWS entry.
> > > Thanks again for working on this!
> > 
> > Thanks. I've attached a version with documentation. I think it's good
> > enough but would appreciate someone else double checking it makes sense.
> 
> Thanks, a few minor nits below.

Thanks. Hopefully the attached solves the problems, and Stefan's
comments too.

-- 
Alan Third

[-- Attachment #2: v3-0001-Allow-use-of-em-in-image-spec-sizes.patch --]
[-- Type: text/plain, Size: 6608 bytes --]

From 362ad64a9ab04709993d330bee120eeb04b7d767 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Mon, 12 Apr 2021 20:30:12 +0100
Subject: [PATCH v3] Allow use of em in image spec sizes

* src/image.c (image_get_dimension): New function.
(compute_image_size): Use image_get_dimension to set the sizes, and
pass in the image struct instead of just the spec.
(image_set_transform):
(imagemagick_load_image):
(svg_load_image): Use the image instead of the spec in compute_image_size.
(syms_of_image): Add 'em' as a symbol.
---
 doc/lispref/display.texi | 10 +++++++
 etc/NEWS                 |  7 +++++
 src/image.c              | 58 ++++++++++++++++++++++++++++------------
 3 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 68d7e827d2..4082bb50d9 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5289,6 +5289,16 @@ Image Descriptors
 and values, including at least the pair @code{:type @var{type}} that
 specifies the image type.
 
+  Image descriptors which define image dimensions, @code{:width},
+@code{:height}, @code{:max-width} and @code{:max-height}, may take
+either an integer, which represents the dimension in pixels, or a pair
+@code{(@var{value} . 'em)}, where @var{value} is the dimension's
+length in @dfn{ems}@footnote{In typography an em is a distance
+equivalent to the height of the type.  For example when using 12 point
+type 1 em is equal to 12 points.  Its use ensures distances and type
+remain proportional.}.  One em is equivalent to the height of the font
+and @var{value} may be an integer or a float.
+
   The following is a list of properties that are meaningful for all
 image types (there are also properties which are meaningful only for
 certain image types, as documented in the following subsections):
diff --git a/etc/NEWS b/etc/NEWS
index d3a8748ded..88863e1a55 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1532,6 +1532,13 @@ will match the font size in use where it is embedded.
 
 This feature relies on librsvg 2.48 or above being available.
 
++++
+*** Image properties support 'em' sizes.  Size image properties, for
+example ':height', ':max-height', etc., can be given a cons of the
+form '(SIZE . em)', where SIZE is an integer or float which is
+multiplied by the font size to calculate the image size, and em is a
+symbol.
+
 ** EWW
 
 +++
diff --git a/src/image.c b/src/image.c
index 6fe0b23f73..1619886f5a 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1987,46 +1987,68 @@ scale_image_size (int size, size_t divisor, size_t multiplier)
   return INT_MAX;
 }
 
+/* Return a size, in pixels, from the value specified by SYMBOL, which
+   may be an integer or a pair of the form (VALUE . 'em) where VALUE
+   is a float that is multiplied by the font size to get the final
+   dimension.
+
+   If the value doesn't exist in the image spec, or is invalid, return
+   -1.
+*/
+static int
+image_get_dimension (struct image *img, Lisp_Object symbol)
+{
+  Lisp_Object value = image_spec_value (img->spec, symbol, NULL);
+
+  if (FIXNATP (value))
+    return min (XFIXNAT (value), INT_MAX);
+  if (CONSP (value) && NUMBERP (CAR (value)) && EQ (Qem, CDR (value)))
+    return min (img->face_font_size * XFLOATINT (CAR (value)), INT_MAX);
+
+  return -1;
+}
+
 /* Compute the desired size of an image with native size WIDTH x HEIGHT.
    Use SPEC to deduce the size.  Store the desired size into
    *D_WIDTH x *D_HEIGHT.  Store -1 x -1 if the native size is OK.  */
 static void
 compute_image_size (size_t width, size_t height,
-		    Lisp_Object spec,
+		    struct image *img,
 		    int *d_width, int *d_height)
 {
   Lisp_Object value;
+  int int_value;
   int desired_width = -1, desired_height = -1, max_width = -1, max_height = -1;
   double scale = 1;
 
-  value = image_spec_value (spec, QCscale, NULL);
+  value = image_spec_value (img->spec, QCscale, NULL);
   if (NUMBERP (value))
     scale = XFLOATINT (value);
 
-  value = image_spec_value (spec, QCmax_width, NULL);
-  if (FIXNATP (value))
-    max_width = min (XFIXNAT (value), INT_MAX);
+  int_value = image_get_dimension (img, QCmax_width);
+  if (int_value >= 0)
+    max_width = int_value;
 
-  value = image_spec_value (spec, QCmax_height, NULL);
-  if (FIXNATP (value))
-    max_height = min (XFIXNAT (value), INT_MAX);
+  int_value = image_get_dimension (img, QCmax_height);
+  if (int_value >= 0)
+    max_height = int_value;
 
   /* If width and/or height is set in the display spec assume we want
      to scale to those values.  If either h or w is unspecified, the
      unspecified should be calculated from the specified to preserve
      aspect ratio.  */
-  value = image_spec_value (spec, QCwidth, NULL);
-  if (FIXNATP (value))
+  int_value = image_get_dimension (img, QCwidth);
+  if (int_value >= 0)
     {
-      desired_width = min (XFIXNAT (value) * scale, INT_MAX);
+      desired_width = int_value;
       /* :width overrides :max-width. */
       max_width = -1;
     }
 
-  value = image_spec_value (spec, QCheight, NULL);
-  if (FIXNATP (value))
+  int_value = image_get_dimension (img, QCheight);
+  if (int_value >= 0)
     {
-      desired_height = min (XFIXNAT (value) * scale, INT_MAX);
+      desired_height = int_value;
       /* :height overrides :max-height. */
       max_height = -1;
     }
@@ -2216,7 +2238,7 @@ image_set_transform (struct frame *f, struct image *img)
     }
   else
 #endif
-    compute_image_size (img->width, img->height, img->spec, &width, &height);
+    compute_image_size (img->width, img->height, img, &width, &height);
 
   /* Determine rotation.  */
   double rotation = 0.0;
@@ -9210,7 +9232,7 @@ imagemagick_load_image (struct frame *f, struct image *img,
 
   compute_image_size (MagickGetImageWidth (image_wand),
 		      MagickGetImageHeight (image_wand),
-		      img->spec, &desired_width, &desired_height);
+		      img, &desired_width, &desired_height);
 
   if (desired_width != -1 && desired_height != -1)
     {
@@ -10068,7 +10090,7 @@ svg_load_image (struct frame *f, struct image *img, char *contents,
     viewbox_height = dimension_data.height;
   }
 
-  compute_image_size (viewbox_width, viewbox_height, img->spec,
+  compute_image_size (viewbox_width, viewbox_height, img,
                       &width, &height);
 
   width *= FRAME_SCALE_FACTOR (f);
@@ -10777,6 +10799,8 @@ syms_of_image (void)
   DEFSYM (QCmax_width, ":max-width");
   DEFSYM (QCmax_height, ":max-height");
 
+  DEFSYM (Qem, "em");
+
 #ifdef HAVE_NATIVE_TRANSFORMS
   DEFSYM (Qscale, "scale");
   DEFSYM (Qrotate, "rotate");
-- 
2.29.2

Re: Using more and/or better icons in Emacs

[Eli Zaretskii]

> Date: Thu, 15 Apr 2021 16:37:43 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: stefan@marxist.se, dgutov@yandex.ru, emacs-devel@gnu.org
> 
> +  Image descriptors which define image dimensions, @code{:width},
> +@code{:height}, @code{:max-width} and @code{:max-height}, may take
> +either an integer, which represents the dimension in pixels, or a pair
> +@code{(@var{value} . 'em)}, where @var{value} is the dimension's
                        ^^^
Doe we really need to quote em here?

> ++++
> +*** Image properties support 'em' sizes.  Size image properties, for
                                           ^^
Break the line after the period, so that the heading is a single
sentence.

Thanks.

Re: Using more and/or better icons in Emacs

[Alan Third]

On Thu, Apr 15, 2021 at 06:50:37PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 15 Apr 2021 16:37:43 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: stefan@marxist.se, dgutov@yandex.ru, emacs-devel@gnu.org
> > 
> > +  Image descriptors which define image dimensions, @code{:width},
> > +@code{:height}, @code{:max-width} and @code{:max-height}, may take
> > +either an integer, which represents the dimension in pixels, or a pair
> > +@code{(@var{value} . 'em)}, where @var{value} is the dimension's
>                         ^^^
> Doe we really need to quote em here?
> 
> > ++++
> > +*** Image properties support 'em' sizes.  Size image properties, for
>                                            ^^
> Break the line after the period, so that the heading is a single
> sentence.

I've fixed these and, as far as I can see, not introduced any other
stupid mistakes. So I've pushed it to master.

Thanks!
-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Dmitry Gutov]

On 09.04.2021 22:40, Alan Third wrote:
> ... and if you want the actual text height instead of line height it's
> more complex, have a look at scale and height in this:
> 
>      https://gist.github.com/alanthird/7b86dc66df1ed3b9006bcd3fddd7350f
> 
> although there's probably a better way of doing it.

After testing it a bit more, I've noticed that it doesn't take into 
account the :height attribute of the default face.

This seems to return a more correct result:

   (let ((default-font (face-font 'default)))
       (* (aref (font-info default-font) 2)))

(Adapted from default-font-height.)

Re: Using more and/or better icons in Emacs

[Alan Third]

On Fri, Apr 09, 2021 at 12:46:57PM -0500, Stefan Kangas wrote:
> I'm getting an SVG in the buffer that is actually taller than the line.
> When we added new SVG icons to customize recently, Alan Third fiddled
> with the height and viewport in the SVG file to force it to be the same
> height as the line.
> 
> But is there any way to do this directly from Emacs?  It seems like a
> pain to have to edit the SVG file for every icon (and my attempts at
> doing so has failed so far...).

BTW, looking at those icons all you should have to do is set the width
and height to "1em". I had to insert the viewBox on the other ones
because they didn't have one to begin with, just the width and height.

We'd also probably want to remove most of the colour information (or
replace it with 'currentColor'). At least for simple two colour
images.
-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Stefan Kangas]

Alan Third <alan@idiocy.org> writes:

> BTW, looking at those icons all you should have to do is set the width
> and height to "1em". I had to insert the viewBox on the other ones
> because they didn't have one to begin with, just the width and height.

[Please see my other email about how to insert these into a buffer.]

> We'd also probably want to remove most of the colour information (or
> replace it with 'currentColor'). At least for simple two colour
> images.

Hmm, if we are going to use these we should probably write a script to
import them for our use.  It might be nice to do it in ELisp, or we
could use a Python or Shell script or something.

Do we have any ELisp code to manipulate SVG image files?

Otherwise, we could perhaps get away with some hacky regexps for this
(to avoid having to do a proper job with XML, which is rarely fun IME).

I welcome any opinions on how to go about doing this without
overly compromising my sanity.

Re: Using more and/or better icons in Emacs

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 9 Apr 2021 19:55:52 -0500
> Cc: Dmitry Gutov <dgutov@yandex.ru>, emacs-devel@gnu.org
> 
> Hmm, if we are going to use these we should probably write a script to
> import them for our use.  It might be nice to do it in ELisp, or we
> could use a Python or Shell script or something.

ELisp is preferable, of course.

> Do we have any ELisp code to manipulate SVG image files?

svg.el?

Re: Using more and/or better icons in Emacs

[Alan Third]

On Sat, Apr 10, 2021 at 10:23:49AM +0300, Eli Zaretskii wrote:
> > From: Stefan Kangas <stefan@marxist.se>
> > Date: Fri, 9 Apr 2021 19:55:52 -0500
> > Cc: Dmitry Gutov <dgutov@yandex.ru>, emacs-devel@gnu.org
> > 
> > Hmm, if we are going to use these we should probably write a script to
> > import them for our use.  It might be nice to do it in ELisp, or we
> > could use a Python or Shell script or something.
> 
> ELisp is preferable, of course.
> 
> > Do we have any ELisp code to manipulate SVG image files?
> 
> svg.el?

I suspect svg.el doesn't provide the functionality we need, but it's
built on dom.el which might.

-- 
Alan Third

Re: Using more and/or better icons in Emacs

[Eli Zaretskii]

> Date: Sat, 10 Apr 2021 10:17:42 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: Stefan Kangas <stefan@marxist.se>, dgutov@yandex.ru,
> 	emacs-devel@gnu.org
> 
> > ELisp is preferable, of course.
> > 
> > > Do we have any ELisp code to manipulate SVG image files?
> > 
> > svg.el?
> 
> I suspect svg.el doesn't provide the functionality we need, but it's
> built on dom.el which might.

Then I guess we need to extend svg.el so it does support what's needed
here?

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> maybe we could arrange for the minibuffer text to be
> matched against the *annotated* candidates in some cases.
> I think this requires more thought, together with concrete
> examples.

In Icicles there's a distinction:

1. Annotations are _not_ matchable (intentionally).

2. Completion candidates can be multi-part.  Each
   part can be matched separately.  Use of any part
   by a user is optional.

So if a command wants to make what might normally
be considered an annotation matchable it just uses
multi-completion, that is, it gives candidates two
parts, the second being what would have been the
annotation.

It's also possible for particular parts not to be
displayed in *Completions*.  You can nevertheless
complete against them.

An example is choosing a file or buffer name.  The
commands for this use multi-completion.  The first
part of a candidate is the file or buffer name.
The second part is the file or buffer content.
The content part is _not_ shown in *Completions*.

So you can choose a file or buffer by matching its
name or its content or both.

Users can completely ignore the content part.  They
can even not know it's available to match against.
(It's created only on demand, so no performance
penalty when you don't match content.)

Doing this is fairly straightforward.  It would be
even more so if vanilla `completing-read-default'
took care of it.  As it is, I need to fudge the
parts together for `completing-read-default', and
tease them apart when needed.

As a candidate for `(icicle-)completing-read', a
multi-completion just has a list of strings as its
car instead of a single string.  That possibility
is currently unexploited (aka available) by vanilla.

(Multi-completions are thus possible only for alist
collections.  That restriction could be removed if
underlying code such as `completing-read-default'
took care of them.)

From the doc of `icicle-completing-read':

  The car of an alist entry can also be a list of
  strings.  In this case, the completion candidate
  is a multi-completion.

  The strings are joined pairwise with
  `icicle-list-join-string' to form the completion
  candidate seen by the user.

  You can use variable `icicle-candidate-properties-alist'
  to control the appearance of multi-completions in
  buffer `*Completions*'.

  You can use variables `icicle-list-use-nth-parts'
  and `icicle-list-nth-parts-join-string' to control
  the minibuffer behavior of multi-completions.

See https://www.emacswiki.org/emacs/Icicles_-_Programming_Multi-Completions.

> you can't do the highlighting correctly for
> `partial-completion` without having access to
> the "pattern" against which it was matches.

Exactly.  The pattern and the kind of matching.  E.g.,
for regexp patterns `string-match' is relevant for
finding the matched part of a candidate.

Highlighting candidate match parts (and their common
expansion, if there is one) should be done only in
the function that displays the candidates (e.g. in
*Completions*).

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

I wrote:

> 2. Completion candidates can be multi-part.  Each
>    part can be matched separately.  Use of any part
>    by a user is optional.

I meant to also mention that the matching can be
different for each part - even totally different.

> An example is choosing a file or buffer name.  The
> commands for this use multi-completion.  The first
> part of a candidate is the file or buffer name.
> The second part is the file or buffer content.
> The content part is _not_ shown in *Completions*.

And clearly those are examples of using different
kinds of matching for those parts.

This is quite different from just appending some
string bit to each candidate, to distinguish it,
and possibly hiding that to prevent matching etc.,
which you are discussing as a way to fulfill the
need of disambiguating "duplicates".

There are multiple ways to disambiguate duplicates.
I mentioned this one, and I mentioned giving each
string candidate the "full" candidate as a text
property.

What you're describing (leveraging annotations) is
pretty limited in terms of what it allows for
completion behavior (for users).

RE: [External] : Re: Improvement proposals for `completing-read'

[Drew Adams]

> - There is this metadata adjustment which somehow unexpectedly
> adjusts/mutates the metadata
> - Why does a completion style sort? I am mostly happy with styles which
> don't do that. When I was working on my Consult package I was actually
> confused by the "disorder" introduced by `flex` and `fido-mode` even
> with `display/cycle-sort-function` set to the identity.

In most situations, sorting can and should be
separate from filtering, i.e., from determining
what the current set of candidates is.

However, some kinds of filtering naturally lend
themselves to some sort orders.  This is true of
certain kinds of "fuzzy" matching, for example.

Even in those cases it can sometimes make sense
for users to be able to turn off such "natural"
sorting or to impose another sort order.

In general (nearly always) it makes sense to
provide users with the ability to _choose_ a sort
order.  And preferably the ability to do that on
the fly, during completion/matching.

I don't know if another completion "system"
besides Icicles offers that possibility.  (Well,
I also offer it in my small `sortie.el', which
is based on what Icicles does for such sorting.)

I'm not aware of any, but I wouldn't be surprised,
and would be glad, if that feature too has been
picked by other completion frameworks.  IMO, it's
super-important to other things that "completion"
can offer.

In particular, choosing - or even seeing first -
candidates based on a given sort order makes a
huge difference in how you interact with the
overall set of candidates.

Some completion regimes, such as Ido, have even
hard-coded a sort order they suppose to be most
handy (based on use frequency or recency or some
such).  (Maybe Ido offers more possibilities now;
dunno.)

[FWIW, I've also never seen a good reason why
vanilla Emacs separates `display-sort-function'
and `cycle-sort-function'.  There's no doubt a
use case for using only one of them, or using
different functions for them, but I haven't
seen it.]

Vanilla Emacs does offer convenient access to
previous inputs, in reverse chronological order,
so at least that order has always been recognized
as useful.  But previous inputs matched by your
current pattern aren't sorted specially as
candidates (for cycle order or display order).

> I should probably read a bit up on the history of
> this feature, then I will know better. But when I
> first saw this flex adjustment I got confused.

[IMO, such fuzzy-match methods and attendant sort
orders have limited applicability for completion
contexts.  They're useful for some things, not
useful for most things.  Just one opinion.  And
yet, Icicles offers them.  It offers 7 kinds of
fuzzy matching, but only a few of them are coupled
with sort orders.]

Re: Improvement proposals for `completing-read'

[Eli Zaretskii]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Wed, 7 Apr 2021 21:46:43 +0200
> Cc: emacs-devel@gnu.org
> 
> On 4/7/21 7:20 PM, Stefan Monnier wrote:
> > BTW, rereading the below, I see I'm sounding a bit too authoritative.
> > I'm the original author of the minibuffer.el completion code, but
> > remember that what I wrote is just my opinion: you'll want to
> > convince the actual maintainers for some of those changes ;-)
> 
> I am aware that you are the original author of this code. Thank you for 
> your response and your comments. Actually I felt your response sounded 
> more like "I mostly agree with some of the proposals, let's see patches, 
> then we talk!" ;) I hope we will also hear some opinions by the current 
> maintainers.

My opinion is also "sounds good, let's see the patches".  TIA.

Re: Improvement proposals for `completing-read'

[Jean Louis]

* Daniel Mendler <mail@daniel-mendler.de> [2021-04-07 14:18]:
> While working on a set of packages around completion ([Selectrum],
> [Consult], [Marginalia], [Embark], [Orderless], [Vertico], ...) it

Your analysis is appreciated. I have not found anything in the
description that would disturb my workflow with various completion
packages, an I found few new of them to try out.

I like when it works out of the box. Ivy works most of the time,
sometimes will not show completions. Selectrum requires more packages
to work similar to Ivy and Helm.

I hope that new patch to icomplete as vertical is soon adopted, as
best is when it is built-in into Emacs, though I don't like mode line
jumping up when completing, I like new split window opened.

-- 
Jean

Take action in Free Software Foundation campaigns:
https://www.fsf.org/campaigns

Sign an open letter in support of Richard M. Stallman
https://rms-support-letter.github.io/

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/7/21 8:39 PM, Jean Louis wrote:
 > Your analysis is appreciated. I have not found anything in the
 > description that would disturb my workflow with various completion
 > packages, an I found few new of them to try out.

Thank you for reading through the proposals.

 > I hope that new patch to icomplete as vertical is soon adopted, as
 > best is when it is built-in into Emacs, though I don't like mode line
 > jumping up when completing, I like new split window opened.

One could certainly add some display action which shows the results in a 
buffer instead. Selectrum supports this. But then I don't like that the 
mode line is below the buffer and my eyes have to focus on the mode line 
and the top of the buffer at the same time.

Daniel Mendler

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 07.04.2021 14:16, Daniel Mendler wrote:
>    .. Proposal 1: Disabling the history
>    .. Proposal 2: More efficient sorting
>    .. Proposal 3: Sort file names by history
>    .. Proposal 4: Add support for `group-function'
>    .. Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
>    .. Proposal 6: Return text properties from `completing-read'
>    .. Proposal 7: Allow duplicates and retain object identity
>    .. Proposal 8: Completion style optimization of filtering and 
> highlighting

If you allow me to offer some brief comments and present my own wishlist.

5. would indeed be very nice to have. You can see my current attempt to 
hack around it in project--completing-read-strict.

6. would indeed make sense, and I'm not sure why we wouldn't want to 
have it in the non-selection case. Whatever come makes use of the 
completed value, could do the stripping of properties.

Often, completing-read's caller can ensure the properties are there, by 
using something like (assoc-default completed-string collection) at the 
end. But that only works if the caller is also the provider of the 
completion table (otherwise it's an "opaque" data structure).

...

9. completion tables need to be able to delegate all matching logic to 
an external process, both filtering and sorting. That's an important 
case for code completion, where we can take advantage of existing code 
and its "fuzzy matching" implementations.

10. support for delayed/asynchronous fetching of completions which 
doesn't interrupt user's typing (it would generally abort the request if 
user input is detected, but there might be other approaches to that). 
Again, that's helpful when completions are produces by an external process.

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 12:16 AM, Dmitry Gutov wrote:
> On 07.04.2021 14:16, Daniel Mendler wrote:
>>    .. Proposal 1: Disabling the history
>>    .. Proposal 2: More efficient sorting
>>    .. Proposal 3: Sort file names by history
>>    .. Proposal 4: Add support for `group-function'
>>    .. Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
>>    .. Proposal 6: Return text properties from `completing-read'
>>    .. Proposal 7: Allow duplicates and retain object identity
>>    .. Proposal 8: Completion style optimization of filtering and 
>> highlighting
> 6. would indeed make sense, and I'm not sure why we wouldn't want to 
> have it in the non-selection case. Whatever come makes use of the 
> completed value, could do the stripping of properties.
> 
> Often, completing-read's caller can ensure the properties are there, by 
> using something like (assoc-default completed-string collection) at the 
> end. But that only works if the caller is also the provider of the 
> completion table (otherwise it's an "opaque" data structure).

Yes, you can use an alist and this is also what I use in my Consult 
package when I want to obtain the associated data. But allowing 
text-properties could simplify some scenarios and it would be more 
natural when you are doing selection. If you consider proposal 7 
(identity/deduplication), retaining the text properties is an automatic 
outcome.

> 9. completion tables need to be able to delegate all matching logic to 
> an external process, both filtering and sorting. That's an important 
> case for code completion, where we can take advantage of existing code 
> and its "fuzzy matching" implementations.

This would be neat, but it requires a lot of restructuring of the 
completion logic. For this reason I am not fond of this idea. But you 
can achieve something like this with your proposal 10. See what I 
describe there, regarding Consult async.

I tried to integrate `fzf` once with Consult async, like generating a 
list outside Emacs, pushing it through `fzf` for fuzzy-filtering and 
presenting it to the user via completion. But it turned out that most of 
the external implementations are not good enough for this use case. They 
don't have an option to open a pipe to update the filtering input for 
example. I could write my own fuzzy matcher external backend which would 
work perfectly with async completion. However then I can also just wait 
for gccemacs :)

> 10. support for delayed/asynchronous fetching of completions which 
> doesn't interrupt user's typing (it would generally abort the request if 
> user input is detected, but there might be other approaches to that). 
> Again, that's helpful when completions are produces by an external process.

You may want to take a look at my Consult package, specifically the 
async functionality. I believe that this functionality can easily be 
provided on top of the current infrastructure, and actually in a nice way.

In Consult I am using closures which hold the asynchronously acquired 
data. The closure function must accept a single argument, it can either 
be a string (the new input) or it can be a list of newly obtained 
candidates.

(There are also a few more arguments which are accepted 'flush and one 
can potentially extend this and compose the closure functions. But this 
is a detail.)

If it is a list the closure function must append the new candidates to 
the already existing ones and return the full list of candidates. The 
completion table then calls the async closure with either the input or 
nil when doing all-completions.

Now a single problem remains - if new data is incoming the async data 
source must somehow inform completion that new candidates are available. 
In order to do this the async source must trigger the UI for example via 
icomplete-exhibit/selectrum-exhibit and so on. It would be good to have 
a common "completion-refresh" entry point for that. In Consult I have to 
write a tiny bit of integration code for each supported completion system.

But since this proposal is much more complicated than the ones I didn't 
add something like this here. Small steps first.

Daniel Mendler

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 08.04.2021 11:37, Daniel Mendler wrote:

>> 6. would indeed make sense, and I'm not sure why we wouldn't want to 
>> have it in the non-selection case. Whatever come makes use of the 
>> completed value, could do the stripping of properties.
>>
>> Often, completing-read's caller can ensure the properties are there, 
>> by using something like (assoc-default completed-string collection) at 
>> the end. But that only works if the caller is also the provider of the 
>> completion table (otherwise it's an "opaque" data structure).
> 
> Yes, you can use an alist and this is also what I use in my Consult 
> package when I want to obtain the associated data. But allowing 
> text-properties could simplify some scenarios and it would be more 
> natural when you are doing selection. If you consider proposal 7 
> (identity/deduplication), retaining the text properties is an automatic 
> outcome.

Perhaps. We might disable deduplication only optionally, though, OTOH 
retaining text properties seems to be always useful (even if caller 
wouldn't be able to always rely on it).

>> 9. completion tables need to be able to delegate all matching logic to 
>> an external process, both filtering and sorting. That's an important 
>> case for code completion, where we can take advantage of existing code 
>> and its "fuzzy matching" implementations.
> 
> This would be neat, but it requires a lot of restructuring of the 
> completion logic. For this reason I am not fond of this idea. But you 
> can achieve something like this with your proposal 10. See what I 
> describe there, regarding Consult async.

AFAIK, being able to do this is essential to reach best performance in a 
number of important use cases.

> I tried to integrate `fzf` once with Consult async, like generating a 
> list outside Emacs, pushing it through `fzf` for fuzzy-filtering and 
> presenting it to the user via completion. But it turned out that most of 
> the external implementations are not good enough for this use case. They 
> don't have an option to open a pipe to update the filtering input for 
> example. I could write my own fuzzy matcher external backend which would 
> work perfectly with async completion. However then I can also just wait 
> for gccemacs :)

I was thinking more about interactions over network, with HTTP requests 
sent and received asynchronously. Mainly the cases where one uses the 
LSP protocol or similar.

>> 10. support for delayed/asynchronous fetching of completions which 
>> doesn't interrupt user's typing (it would generally abort the request 
>> if user input is detected, but there might be other approaches to 
>> that). Again, that's helpful when completions are produces by an 
>> external process.
> 
> You may want to take a look at my Consult package, specifically the 
> async functionality. I believe that this functionality can easily be 
> provided on top of the current infrastructure, and actually in a nice way.

You can check out Company's asynchronous convention for backends:

https://github.com/company-mode/company-mode/blob/f3aacd77d0135c09227400fef45c54b717d33f2e/company.el#L456-L467

It's a very simple lambda-based future-like value. It can be updated to 
use a named type, and with other features too. I think it's a clean and 
simple base to build on, though.

> In Consult I am using closures which hold the asynchronously acquired 
> data. The closure function must accept a single argument, it can either 
> be a string (the new input) or it can be a list of newly obtained 
> candidates.

I'm not sure where to look, sorry.

> If it is a list the closure function must append the new candidates to 
> the already existing ones and return the full list of candidates. The 
> completion table then calls the async closure with either the input or 
> nil when doing all-completions.

I'm not 100% clear, but it sounds like chunked iteration. Which is a 
good feature to have. Though perhaps not always applicable to every UI 
(blinking with new results in a completion popup might be not 
user-friendly).

> Now a single problem remains - if new data is incoming the async data 
> source must somehow inform completion that new candidates are available. 
> In order to do this the async source must trigger the UI for example via 
> icomplete-exhibit/selectrum-exhibit and so on. It would be good to have 
> a common "completion-refresh" entry point for that. In Consult I have to 
> write a tiny bit of integration code for each supported completion system.

See my link, perhaps.

Or in general, a Future/Promise API has a way to subscribe to the 
value's update(s) (and the completion frontend can do that).

Having to use a global variable seems pretty inelegant in comparison.

> But since this proposal is much more complicated than the ones I didn't 
> add something like this here. Small steps first.

No hurry at all. Sometimes, though, a big feature like that can inform 
the whole design from the outset.

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 10:44 PM, Dmitry Gutov wrote:
>> I tried to integrate `fzf` once with Consult async, like generating a 
>> list outside Emacs, pushing it through `fzf` for fuzzy-filtering and 
>> presenting it to the user via completion. But it turned out that most 
>> of the external implementations are not good enough for this use case. 
>> They don't have an option to open a pipe to update the filtering input 
>> for example. I could write my own fuzzy matcher external backend which 
>> would work perfectly with async completion. However then I can also 
>> just wait for gccemacs :)
> 
> I was thinking more about interactions over network, with HTTP requests 
> sent and received asynchronously. Mainly the cases where one uses the 
> LSP protocol or similar.

Yes, this is all possible with async completion tables in Consult. There 
is a consult-spotify package which queries some web api and there is 
also consult-lsp in the works which accesses the lsp api 
(https://github.com/minad/consult/issues/263).

>> You may want to take a look at my Consult package, specifically the 
>> async functionality. I believe that this functionality can easily be 
>> provided on top of the current infrastructure, and actually in a nice 
>> way.
> 
> You can check out Company's asynchronous convention for backends:
> 
> https://github.com/company-mode/company-mode/blob/f3aacd77d0135c09227400fef45c54b717d33f2e/company.el#L456-L467 
> 
> It's a very simple lambda-based future-like value. It can be updated to 
> use a named type, and with other features too. I think it's a clean and 
> simple base to build on, though.

Yes, this looks very simple. I actually prefer the functional style in 
contrast to some named types as you have it in Company. So how is this 
used? When completing the fetcher is called and as soon as it returns 
via the callback the results are displayed? But chunking is not possible 
and probably also not desired? See below for my response regarding 
chunking in Consult.

>> In Consult I am using closures which hold the asynchronously acquired 
>> data. The closure function must accept a single argument, it can 
>> either be a string (the new input) or it can be a list of newly 
>> obtained candidates.
>
> I'm not sure where to look, sorry.

Take a look at `consult--async-sink` at
https://github.com/minad/consult/blob/3121b34e207222b2db6ac96a655d68c0edf1a449/consult.el#L1264-L1297.

These `consult--async-*` functions can be chained together to produce an 
async pipeline. The goal here was to have reusable functions which I can 
glue together to create different async backends. See for example the 
pipeline for asynchronous commands: 
https://github.com/minad/consult/blob/3121b34e207222b2db6ac96a655d68c0edf1a449/consult.el#L1505-L1513.

> I'm not 100% clear, but it sounds like chunked iteration. Which is a 
> good feature to have. Though perhaps not always applicable to every UI 
> (blinking with new results in a completion popup might be not 
> user-friendly).

Indeed, the UI receives the candidates as soon as they come in. One has 
to ensure that this does not freeze the UI with some timer. Then there 
is also throttling when the user changes the input. It works very well 
for the `consult-grep` style commands. You may want to try those. They 
are similar to `counsel-grep` but additionally allow to filter using the 
Emacs completion style. Take a look here, in case you are interested
https://github.com/minad/consult#asynchronous-search. Note that you 
don't have to use chunking necessarily. You can use a single chunk if 
the whole async result arrives in bulk.

>> Now a single problem remains - if new data is incoming the async data 
>> source must somehow inform completion that new candidates are 
>> available. In order to do this the async source must trigger the UI 
>> for example via icomplete-exhibit/selectrum-exhibit and so on. It 
>> would be good to have a common "completion-refresh" entry point for 
>> that. In Consult I have to write a tiny bit of integration code for 
>> each supported completion system.
> 
> See my link, perhaps.
> 
> Or in general, a Future/Promise API has a way to subscribe to the 
> value's update(s) (and the completion frontend can do that).
> 
> Having to use a global variable seems pretty inelegant in comparison.

It is not a global variable but a function. But for sure, one could also 
design the async future such that it receives a callback argument which 
should be called when new candidates arrive. The way I wrote it in 
Consult is that the `consult-async-sink` handles a 'refresh action, 
which then informs the completion UI.

> No hurry at all. Sometimes, though, a big feature like that can inform 
> the whole design from the outset.

Yes, sure. When planning to do a big overhaul you are certainly right. 
But currently I am more focused on fixing a few smaller pain points with 
the API, like retaining text properties and so on.

Daniel Mendler

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 09.04.2021 00:30, Daniel Mendler wrote:
> On 4/8/21 10:44 PM, Dmitry Gutov wrote:

>> I was thinking more about interactions over network, with HTTP 
>> requests sent and received asynchronously. Mainly the cases where one 
>> uses the LSP protocol or similar.
> 
> Yes, this is all possible with async completion tables in Consult. There 
> is a consult-spotify package which queries some web api and there is 
> also consult-lsp in the works which accesses the lsp api 
> (https://github.com/minad/consult/issues/263).

Very good.

>>> You may want to take a look at my Consult package, specifically the 
>>> async functionality. I believe that this functionality can easily be 
>>> provided on top of the current infrastructure, and actually in a nice 
>>> way.
>>
>> You can check out Company's asynchronous convention for backends:
>>
>> https://github.com/company-mode/company-mode/blob/f3aacd77d0135c09227400fef45c54b717d33f2e/company.el#L456-L467 
>>
>> It's a very simple lambda-based future-like value. It can be updated 
>> to use a named type, and with other features too. I think it's a clean 
>> and simple base to build on, though.
> 
> Yes, this looks very simple. I actually prefer the functional style in 
> contrast to some named types as you have it in Company. So how is this 
> used? When completing the fetcher is called and as soon as it returns 
> via the callback the results are displayed?

Pretty much. If the user has not typed the next char before that, of course.

> But chunking is not possible 
> and probably also not desired? See below for my response regarding 
> chunking in Consult.

It would be easy to extend to allow chunking, say, with an additional 
arg called MORE-TO-COME. Just like in UPDATE-FUNCTION argument of the VC 
action 'dir-status-files.

It just doesn't seem very useful for code completion so far. Not sure 
what to do with it in the UI, and there are no backends interested in it 
currently (the new version of the LSP protocol has added a feature like 
that, but apparently it's better for other kinds of queries).

>>> In Consult I am using closures which hold the asynchronously acquired 
>>> data. The closure function must accept a single argument, it can 
>>> either be a string (the new input) or it can be a list of newly 
>>> obtained candidates.
>>
>> I'm not sure where to look, sorry.
> 
> Take a look at `consult--async-sink` at
> https://github.com/minad/consult/blob/3121b34e207222b2db6ac96a655d68c0edf1a449/consult.el#L1264-L1297. 

Got it, thank you.

> These `consult--async-*` functions can be chained together to produce an 
> async pipeline. The goal here was to have reusable functions which I can 
> glue together to create different async backends. See for example the 
> pipeline for asynchronous commands: 
> https://github.com/minad/consult/blob/3121b34e207222b2db6ac96a655d68c0edf1a449/consult.el#L1505-L1513. 

I also like that idea.

How are the backtraces in case of error? Whether they can be made 
readable enough, was one of the sticking points in the (quite heated) 
discussion of bug#41531.

>> I'm not 100% clear, but it sounds like chunked iteration. Which is a 
>> good feature to have. Though perhaps not always applicable to every UI 
>> (blinking with new results in a completion popup might be not 
>> user-friendly).
> 
> Indeed, the UI receives the candidates as soon as they come in. One has 
> to ensure that this does not freeze the UI with some timer. Then there 
> is also throttling when the user changes the input. It works very well 
> for the `consult-grep` style commands. You may want to try those. They 
> are similar to `counsel-grep` but additionally allow to filter using the 
> Emacs completion style. Take a look here, in case you are interested
> https://github.com/minad/consult#asynchronous-search. Note that you 
> don't have to use chunking necessarily. You can use a single chunk if 
> the whole async result arrives in bulk.

Yeah, I figured those commands would be the the main beneficiary.

I haven't seriously tried Consult yet, but IIRC Helm touted those kind 
of searches as one of the selling points back in the day.

>>> Now a single problem remains - if new data is incoming the async data 
>>> source must somehow inform completion that new candidates are 
>>> available. In order to do this the async source must trigger the UI 
>>> for example via icomplete-exhibit/selectrum-exhibit and so on. It 
>>> would be good to have a common "completion-refresh" entry point for 
>>> that. In Consult I have to write a tiny bit of integration code for 
>>> each supported completion system.
>>
>> See my link, perhaps.
>>
>> Or in general, a Future/Promise API has a way to subscribe to the 
>> value's update(s) (and the completion frontend can do that).
>>
>> Having to use a global variable seems pretty inelegant in comparison.
> 
> It is not a global variable but a function.

That function would have to work with (and notify) different frontends, 
so that probably requires a hook variable of some sort where they would 
register themselves. And that is a global variable.

And/or there would need to be added some tracking of which frontend to 
send the results to.

> But for sure, one could also 
> design the async future such that it receives a callback argument which 
> should be called when new candidates arrive. The way I wrote it in 
> Consult is that the `consult-async-sink` handles a 'refresh action, 
> which then informs the completion UI.

Yup, that sounds better.

I would probably say that a UI should itself know better when to refresh 
or not, but I'm guessing you have good counter-examples.

>> No hurry at all. Sometimes, though, a big feature like that can inform 
>> the whole design from the outset.
> 
> Yes, sure. When planning to do a big overhaul you are certainly right. 
> But currently I am more focused on fixing a few smaller pain points with 
> the API, like retaining text properties and so on.

Sounds good. I just wanted to add some context for completeness, in case 
the work turns into the direction of the "next completing-read".

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/10/21 4:21 AM, Dmitry Gutov wrote:
>> These `consult--async-*` functions can be chained together to produce 
>> an async pipeline. The goal here was to have reusable functions which 
>> I can glue together to create different async backends. See for 
>> example the pipeline for asynchronous commands: 
>> https://github.com/minad/consult/blob/3121b34e207222b2db6ac96a655d68c0edf1a449/consult.el#L1505-L1513. 
> 
> I also like that idea.
> 
> How are the backtraces in case of error? Whether they can be made 
> readable enough, was one of the sticking points in the (quite heated) 
> discussion of bug#41531.

Backtraces are rather opaque. But having such issues on the language 
level should not be a road block. I think the proper fix would be to 
improve the debugging infrastructure slightly.

Instead of printing bytecodes - make this more accessible. Either show 
some disassembled string or show location information, which should be 
attached to the lambdas. Elisp retains all the information due to its 
dynamic nature. So all the information is still present, we can print 
all objects in a nice way.

I believe the state of introspectability of lambdas is a result of the 
late introduction of lexical binding. I am so glad and thankful we got 
this - I just saw that Stefan Monnier pushed the last few patches 
switching over files from dynamic to lexical binding. But not that you 
get me wrong - the dynamic/lexical combination is an excellent feature 
for Emacs and its extensibility.

Then there were a few other issue with lambdas, I think the interpreter 
captures too much in the closures which can leads to large closures, 
which is bad for debugability. The bytecode compiler in contrast seems 
to perform an analysis. Is this right - please correct me if I am wrong? 
I wonder why there is even the actual interpreter left - why is it not 
possible to pass everything through bytecode? I guess this is a legacy 
issue and also a bootstrapping issue since the bytecode compiler is 
written in elisp itself.

Furthermore I had another issue with lambdas - if you create large 
closures, which I am doing with Consult async, which capture the whole 
candidates set, then you end up with memory problems if you register 
these closures as hooks. The problem is that `add-hook/remove-hook` 
compares using `equal` and this uses hash tables internally, which can 
get very expensive. See bug#46326, bug#46407 and bug#46414.
>> It is not a global variable but a function.
> 
> That function would have to work with (and notify) different frontends, 
> so that probably requires a hook variable of some sort where they would 
> register themselves. And that is a global variable.
> 
> And/or there would need to be added some tracking of which frontend to 
> send the results to.

Yes, in Consult I am using a hook which performs the refreshing 
`consult--completion-refresh-hook`. Each backend registers itself there 
and refreshes if it is currently active in the relevant minibuffer.

> I would probably say that a UI should itself know better when to refresh 
> or not, but I'm guessing you have good counter-examples.

One could update the UI using some timer if an async source is used 
(polling). However since I am setting this on top of the 
`completing-read' infrastructure I felt it to be better to do it the 
other way round, since the table is only queried when the user enters 
new input. I guess for fast sources polling will be as good, but for 
slow sources, notifying the UI is better.

>>> No hurry at all. Sometimes, though, a big feature like that can 
>>> inform the whole design from the outset.
>>
>> Yes, sure. When planning to do a big overhaul you are certainly right. 
>> But currently I am more focused on fixing a few smaller pain points 
>> with the API, like retaining text properties and so on.
> 
> Sounds good. I just wanted to add some context for completeness, in case 
> the work turns into the direction of the "next completing-read".

Yes, it seems the discussion already went a bit in that direction. I 
agree that it is good to keep all these points in the head if one 
designs a new `completing-read'. However from my work on Consult I am 
actually not that unhappy with `completing-read' as is. With the handful 
of small proposals I made in my original mail the state will be improved 
where I had issues. If you look at my `consult--read` wrapper, it has to 
do some special enhancements (preview, narrowing, async, ...), but I 
think one can work reasonably well with the `completing-read' API. For 
now I prefer to work with what exists than throwing everything out. At 
least the Consult/Embark package show that one can implement more 
advanced completion features based on top of the existing 
infrastructure, with only a small amount of advices/hacks. And it is 
already somehow nice, since one can swap out the UI and use Consult with 
Selectrum, Vertico, Icomplete, default completion and maybe more in the 
future.

Daniel Mendler

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 10.04.2021 12:18, Daniel Mendler wrote:
> On 4/10/21 4:21 AM, Dmitry Gutov wrote:
>>> These `consult--async-*` functions can be chained together to produce 
>>> an async pipeline. The goal here was to have reusable functions which 
>>> I can glue together to create different async backends. See for 
>>> example the pipeline for asynchronous commands: 
>>> https://github.com/minad/consult/blob/3121b34e207222b2db6ac96a655d68c0edf1a449/consult.el#L1505-L1513. 
>>
>>
>> I also like that idea.
>>
>> How are the backtraces in case of error? Whether they can be made 
>> readable enough, was one of the sticking points in the (quite heated) 
>> discussion of bug#41531.
> 
> Backtraces are rather opaque. But having such issues on the language 
> level should not be a road block. I think the proper fix would be to 
> improve the debugging infrastructure slightly.
> 
> Instead of printing bytecodes - make this more accessible. Either show 
> some disassembled string or show location information, which should be 
> attached to the lambdas. Elisp retains all the information due to its 
> dynamic nature. So all the information is still present, we can print 
> all objects in a nice way.

If you just (load ...) the package, there will be no bytecode in the 
output. I was more concerned about simply how it reads (whether it's 
easy enough to diagnose a problem by reading the backtrace). Or at least 
whether it's not considerably worse than the alternatives.

Regarding navigation, though, precise symbol locations are an old Emacs 
problem. IIRC Alan posted about making some progress on it in the recent 
months, and lambdas could indeed similarly be annotated with that solution.

> Then there were a few other issue with lambdas, I think the interpreter 
> captures too much in the closures which can leads to large closures, 
> which is bad for debugability. The bytecode compiler in contrast seems 
> to perform an analysis. Is this right - please correct me if I am wrong? 
> I wonder why there is even the actual interpreter left - why is it not 
> possible to pass everything through bytecode? I guess this is a legacy 
> issue and also a bootstrapping issue since the bytecode compiler is 
> written in elisp itself.

Speed, probably (all the JS VMs include an interpreter stage, IIRC)?

And if we always worked with byte code directly, stuff like edebug, 
backtrace printer, would need to be repurposed.

There are others here better qualified to answer, anyway.

> Furthermore I had another issue with lambdas - if you create large 
> closures, which I am doing with Consult async, which capture the whole 
> candidates set, then you end up with memory problems if you register 
> these closures as hooks. The problem is that `add-hook/remove-hook` 
> compares using `equal` and this uses hash tables internally, which can 
> get very expensive. See bug#46326, bug#46407 and bug#46414.

Perhaps that's another reason not to use hook for this, and instead to 
attach frontend updater callbacks to the "future" values directly? With 
lexically scoped callbacks, for example.

>> I would probably say that a UI should itself know better when to 
>> refresh or not, but I'm guessing you have good counter-examples.
> 
> One could update the UI using some timer if an async source is used 
> (polling). However since I am setting this on top of the 
> `completing-read' infrastructure I felt it to be better to do it the 
> other way round, since the table is only queried when the user enters 
> new input. I guess for fast sources polling will be as good, but for 
> slow sources, notifying the UI is better.

Perhaps we're just talking about the same thing, differently.

What I meant, the source should invoke the callback when it gets new 
data, and the callback should store the result somewhere, and it can 
notify the UI about the possibility of refreshing (the frontend 
implements the callback, so it knows whether and how to do it). But 
whether to refresh right away, or wait, debounce, or simply discard the 
output, should be the frontend's choice.

>>>> No hurry at all. Sometimes, though, a big feature like that can 
>>>> inform the whole design from the outset.
>>>
>>> Yes, sure. When planning to do a big overhaul you are certainly 
>>> right. But currently I am more focused on fixing a few smaller pain 
>>> points with the API, like retaining text properties and so on.
>>
>> Sounds good. I just wanted to add some context for completeness, in 
>> case the work turns into the direction of the "next completing-read".
> 
> Yes, it seems the discussion already went a bit in that direction. I 
> agree that it is good to keep all these points in the head if one 
> designs a new `completing-read'. However from my work on Consult I am 
> actually not that unhappy with `completing-read' as is. With the handful 
> of small proposals I made in my original mail the state will be improved 
> where I had issues. If you look at my `consult--read` wrapper, it has to 
> do some special enhancements (preview, narrowing, async, ...), but I 
> think one can work reasonably well with the `completing-read' API. For 
> now I prefer to work with what exists than throwing everything out. At 
> least the Consult/Embark package show that one can implement more 
> advanced completion features based on top of the existing 
> infrastructure, with only a small amount of advices/hacks.

I've read it briefly, thanks.

Sounds like you have what is needed to propose an "async" extension to 
the standard completion tables API?

;-)

(No pressure.)

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/11/21 2:51 AM, Dmitry Gutov wrote:>>> I would probably say that a 
UI should itself know better when to
>>> refresh or not, but I'm guessing you have good counter-examples.
>>
>> One could update the UI using some timer if an async source is used 
>> (polling). However since I am setting this on top of the 
>> `completing-read' infrastructure I felt it to be better to do it the 
>> other way round, since the table is only queried when the user enters 
>> new input. I guess for fast sources polling will be as good, but for 
>> slow sources, notifying the UI is better.
> 
> Perhaps we're just talking about the same thing, differently.
> 
> What I meant, the source should invoke the callback when it gets new 
> data, and the callback should store the result somewhere, and it can 
> notify the UI about the possibility of refreshing (the frontend 
> implements the callback, so it knows whether and how to do it). But 
> whether to refresh right away, or wait, debounce, or simply discard the 
> output, should be the frontend's choice.

Yes, I think we are mostly on the same page. If you look at my Consult 
async pipelines there are functions which do debouncing/waiting etc.

1. sink      ^
2. wait      | candidates, refresh, flush requests etc
3. debounce  |
4. source    |

The incoming candidates are sent from the source (a process or web query 
source) to the sink and then the sink also performs the refreshing by 
talking directly to the UI. Then the UI can also make its own decisions 
what to do with the refreshing requests.

> Sounds like you have what is needed to propose an "async" extension to 
> the standard completion tables API?

Yes, one could consider it as that. But maybe it it is sufficient to 
only provide a general refresh hook which I proposed initially. This way 
the core API would stay simple and everything could be implemented on 
top without constraining everything directly.

Daniel

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 11.04.2021 16:08, Daniel Mendler wrote:

> Yes, I think we are mostly on the same page. If you look at my Consult 
> async pipelines there are functions which do debouncing/waiting etc.
> 
> 1. sink      ^
> 2. wait      | candidates, refresh, flush requests etc
> 3. debounce  |
> 4. source    |
> 
> The incoming candidates are sent from the source (a process or web query 
> source) to the sink and then the sink also performs the refreshing by 
> talking directly to the UI. Then the UI can also make its own decisions 
> what to do with the refreshing requests.

Sounds good.

>> Sounds like you have what is needed to propose an "async" extension to 
>> the standard completion tables API?
> 
> Yes, one could consider it as that. But maybe it it is sufficient to 
> only provide a general refresh hook which I proposed initially. This way 
> the core API would stay simple and everything could be implemented on 
> top without constraining everything directly.

I'm not sure if that would work for code completion. Aside from tracking 
which value belongs to which frontend, and the associated bugs with 
hooks you mentioned, how to you invalidate the results?

For instance, there is a request, it has been made a while ago, and the 
context has changed (the buffer has changed, position has changed, etc). 
The request has taken a long time but finally the response has arrived. 
How do you reliably ignore it? Or, ideally, are able abort it (if it's a 
one-time external process, perhaps killing the search process)?

If the result is some kind of "future" value, the handler code can 
either change some variable in the lexical scope once its execution 
reaches the point of "we don't need that computation anymore", or could 
even call some generic method like future-abort.

How does the general refresh hook solve these problems?

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/12/21 2:32 AM, Dmitry Gutov wrote:
> I'm not sure if that would work for code completion. Aside from tracking 
> which value belongs to which frontend, and the associated bugs with 
> hooks you mentioned, how to you invalidate the results?
> 
> For instance, there is a request, it has been made a while ago, and the 
> context has changed (the buffer has changed, position has changed, etc). 
> The request has taken a long time but finally the response has arrived. 
> How do you reliably ignore it? Or, ideally, are able abort it (if it's a 
> one-time external process, perhaps killing the search process)?
> 
> If the result is some kind of "future" value, the handler code can 
> either change some variable in the lexical scope once its execution 
> reaches the point of "we don't need that computation anymore", or could 
> even call some generic method like future-abort.
> 
> How does the general refresh hook solve these problems?

I don't know how it looks in the context of code completion. But in the 
context of the minibuffer completion, the state is minibuffer-local. The 
hook is executed within the minibuffer, which belongs to the async 
operation. With Consult async you can have multiple recursive 
minibuffers where each performs an async operation. Does this answer 
your question?

Daniel

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 12.04.2021 03:40, Daniel Mendler wrote:
> I don't know how it looks in the context of code completion. But in the 
> context of the minibuffer completion, the state is minibuffer-local. The 
> hook is executed within the minibuffer, which belongs to the async 
> operation. With Consult async you can have multiple recursive 
> minibuffers where each performs an async operation. Does this answer 
> your question?

What happens if you have an anomalously long running request in one 
nested minibuffer, which you exit, so it arrives in the "parent" one 
that's still managed by Consult, but shows the results for a different 
command/input/etc?

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/12/21 12:47 PM, Dmitry Gutov wrote:
> What happens if you have an anomalously long running request in one 
> nested minibuffer, which you exit, so it arrives in the "parent" one 
> that's still managed by Consult, but shows the results for a different 
> command/input/etc?

This cannot happen. If you exit the minibuffer, the running request will 
be terminated. The parent minibuffer, maybe also running a Consult 
command (or something else) is isolated from the child minibuffer and no 
disruption should happen.

I just tested running nested `consult-ripgrep` and that should work, but 
for some reason there is a problem 
(https://github.com/minad/consult/issues/272). I have to check if this 
can be fixed. So what I described to you is how everything should work 
in theory, since the state is captured in the closures.

The `consult--async-sink` contains this code which performs the refreshing:

      ;; Refresh the UI when the current minibuffer window belongs
      ;; to the current asynchronous completion session.
      (when-let (win (active-minibuffer-window))
        (when (eq (window-buffer win) buffer)
          (with-selected-window win
            (run-hooks 'consult--completion-refresh-hook)))))

Daniel

Re: Improvement proposals for `completing-read'

[Dmitry Gutov]

On 12.04.2021 14:04, Daniel Mendler wrote:
> On 4/12/21 12:47 PM, Dmitry Gutov wrote:
>> What happens if you have an anomalously long running request in one 
>> nested minibuffer, which you exit, so it arrives in the "parent" one 
>> that's still managed by Consult, but shows the results for a different 
>> command/input/etc?
> 
> This cannot happen. If you exit the minibuffer, the running request will 
> be terminated.

Because the process belongs to the buffer?

> The parent minibuffer, maybe also running a Consult 
> command (or something else) is isolated from the child minibuffer and no 
> disruption should happen.
> 
> I just tested running nested `consult-ripgrep` and that should work, but 
> for some reason there is a problem 
> (https://github.com/minad/consult/issues/272). I have to check if this 
> can be fixed. So what I described to you is how everything should work 
> in theory, since the state is captured in the closures.
> 
> The `consult--async-sink` contains this code which performs the refreshing:
> 
>       ;; Refresh the UI when the current minibuffer window belongs
>       ;; to the current asynchronous completion session.
>       (when-let (win (active-minibuffer-window))
>         (when (eq (window-buffer win) buffer)
>           (with-selected-window win
>             (run-hooks 'consult--completion-refresh-hook)))))

OK, so it seems like the sink saves the "context" which it was called 
from, and then compares it against the current one upon completion, and 
does nothing when they don't match.

Sounds good for minibuffers, but not so great for code completion, where 
context is more complex. And since completing-read shares the notion of 
completion table with completion-at-point-functions, I think any async 
support we add should work for both purposes.

Re: Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/14/21 2:00 AM, Dmitry Gutov wrote:
> On 12.04.2021 14:04, Daniel Mendler wrote:
>> The `consult--async-sink` contains this code which performs the 
>> refreshing:
>>
>>       ;; Refresh the UI when the current minibuffer window belongs
>>       ;; to the current asynchronous completion session.
>>       (when-let (win (active-minibuffer-window))
>>         (when (eq (window-buffer win) buffer)
>>           (with-selected-window win
>>             (run-hooks 'consult--completion-refresh-hook)))))
>
> Sounds good for minibuffers, but not so great for code completion, where 
> context is more complex. And since completing-read shares the notion of 
> completion table with completion-at-point-functions, I think any async 
> support we add should work for both purposes.

Yes, that would be good. In order to generalize the Consult async 
protocol one would have to pass some refresh/notify function during 
setup, e.g. instead of passing only the 'setup argument, also pass a 
notify function argument. It seems to me that would be sufficient.

RE: [External] : Improvement proposals for `completing-read'

[Drew Adams]

> I see `completing-read' as a comparatively complicated API, which is
> hard use and hard to implement in its full extent.

You see that.  OK.  Specifics?  Complicated, sure,
maybe.  Like Emacs.  But hard to use and hard to
implement?  How so?

> For the end-user it helps to wrap the function in a more friendly API.

How does it help?  What's more friendly about it?
What's the problem you it tries to solve?

> Eric Abrahamsen has used such an approach in his Gnorb package, see
> `gnorb-select-from-list'. I have done the same in my Consult package,
> see `consult--read'. Maybe moving to a more comfortable (but mostly
> `completing-read' compatible) API would mitigate the usability issues
> people are observing?

More "comfortable" in what way?

What usability issues?  (This sounds like the
classic, "When did you stop beating your wife?")

> For now I want to be a bit more concrete

Thank goodness.

> and look at `completing-read'
> and `minibuffer.el' directly and propose a few moderate improvements.
> The improvements can help package authors in the short term, since they
> address issues with the current state of the API.

Please specify the issues - "a bit more concrete",
please.

> The proposals have been inspired by the work on the aforementioned
> packages and came to light in discussions with the respective authors of
> the packages: Clemens Radermacher of Selectrum, Omar Antolín Camarena of
> Embark and Orderless, and also Dmitry Gutov and Protesilaos Stavrou.

How about specifying whatever's relevant that you
feel came out of those discussions?  Or should we
just accept that the Summit Meeting has decided?

> I am very much interested in your opinion regarding
> the following proposals.

I'd like to hear about the problems the proposals
are meant to solve.

> Improvement proposals for `completing-read'
> ===========================================
> 
> Proposal 1: Disabling the history
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    I propose to allow the symbol `t' as `HIST' argument to
>    `completing-read' in order to disable the history. This aligns
>    `completing-read' with `read-from-minibuffer'. This change requires a
>    modification of the function `completion-all-sorted-completions',
>    which sorts the candidates by history position and currently assumes
>    that the `minibuffer-history-variable' is a variable symbol.

What's the problem?  How does it hurt for users to
have the default history, `minibuffer-history',
available?  They don't have to use it, right?

Is your complaint about the time to sort it (see next)?

I'm not saying I'm opposed to your proposed solution.
I'm saying I haven't seen the problem specified that
you're trying to solve.

> Proposal 2: More efficient sorting
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    Currently candidate sorting is `O(m*n*log(n))' with history length m
>    and candidate list length n. This leads to a noticeable slowdown for
>    large values of `history-length'. I propose to improve the speed of
>    `completion-all-sorted-completions' by using a hash table. The Vertico
>    package provides an optimized sorting function, see `vertico--sort'.

I'm all for more sort orders and faster sort orders.
Who wouldn't be?

But I'd propose to not sort in any hard-coded way, and
to let _users_ control what sort orders are used where.

A given command (or category of completion candidates,
or...) can provide a given set of sort orders for
users to choose from.

In Icicles that's the case.  And users can change
the possible sort orders and groups of orders.  They
can even customize the sort orders used for choosing
among sort orders (that too is on the fly with
completion).

> Proposal 3: Sort file names by history
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    Currently `completion-all-sorted-completions' does not sort file
>    candidates based on history, when the candidates are file names and
>    the history elements are paths. The Vertico package contains the
>    function `vertico--sort' which special cases file names. This approach
>    could be adopted by `completion-all-sorted-completions'.

See above.  Like other kinds of candidates, there are
umpteen interesting/useful ways to sort file-name
candidates.

These are the sort orders Icicles provides by default
for file-name candidates.

 alphabetical
 by 2nd parts alphabetically
 by directories first 
 by directories last 
 by file type
 by flx score
 by last file
 access time 
 by last file modification time
 by last use
 by last use as input
 by last use, dirs first 
 by previous use alphabetically
 case insensitive
 extra candidates first
 special candidates first 
 turned OFF

[`by 2nd parts alphabetically' is generic for commands
 that let you input two or more patterns, to complete
 against two or more "parts" of completion candidates
 (each part is optional).

 In the case of files, the two parts are (1) file name
 and (2) file contents.  So sorting `by 2nd parts
 alphabetically' for file-name candidates sorts first
 by the matches against file content, alphabetically.]

And it's trivial to define new sort orders, using
macro `icicle-define-sort-command'.

To be clear: this is not an advertisement for Icicles.
It's an argument that there are many useful ways to
sort, and users should be able to choose, on the fly.

> Proposal 4: Add support for `group-function'
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    Grouping can be added to the default completion system by modifying
>    `completion-all-sorted-completions' and `completion--insert-strings'.

So the overall problem you say you want to solve is
that `completing-read' is too complex, hard to use,
and hard to implement.  Yet you want to add something
like this, to make it even more complex and hard to
implement? ;-)

Don't get me wrong.  I'm actually inclined toward
some such grouping behavior, FWIW.

My main concern about further complicating the code
implementing `completing-read' is that things will
end up hard-coded - and hence difficult to use by
users and by implementers of 3rd-party code.

One person's brilliant once-and-for-all optimization
can be another person's hard-coded ball & chain.

> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    If `completing-read' is passed `t' as argument to `REQUIRE-MATCH', the
>    completion must match a candidate from the completion table. However
>    there is also the possibility to exit the minibuffer with a null
>    completion. I propose to disallow this possibility. This change makes
>    it easier for users of `completing-read' since they can rely on
>    setting `REQUIRE-MATCH' to `t' and do not have to handle the null
>    completion specially.

When you say users, here, you mean people who write
code that uses `completing-read', not users of such
code, right?

Why is this a problem for these coders?  I've never
encountered a problem in this regard.  Can you specify
a problem here?

Empty completion returning a special value (namely "")
lets code know that the user provided empty completion.
That's a _feature_.

Code can do _anything_ it wants in that case.  Why
would we hard-code the behavior, by disallowing
`completing-read' callers from knowing that a user
provided empty input?

Again, maybe you have a good reason or two why this
is needed.  But so far you've mentioned only the
difficulty for coders - and you've shown no example
of even that difficulty.

You propose to take away knowledge by code of what
a user input action was.  Why?  What real problem
is this meant to solve?

>    Note that null completion cannot occur if a default value is passed to
>    `completing-read'. Therefore null completion can be avoided by a
>    wrapper, which passes a special default argument. Unfortunately this
>    leads to flicker.

Most of what you write here (90%) seems to be about
solutions so far, without the problems having been
presented yet.  I think I see more about what you
think are handicaps for implementers and less about
what might benefit users.  That seems backwards, as
an Emacs priority.

>    If it is desired to avoid a backward incompatible change one could
>    consider adding a new special value for `REQUIRE-MATCH', for example
>    `'require-candidate', which explicitly forbids null completion.

Much better!  But I still wonder what the use case
(problem) is.

> Proposal 6: Return text properties from `completing-read'
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

100% agreement.  I proposed this a zillion years ago
(and added it to Icicles).  Rejected for Emacs.  But
this feature should only be _possible_, not imposed.

I did finally get vanilla Emacs to not strip text
properties by default for `read-from-minibuffer' and
`read-string'.  But this was refused for "any of the
functions that do minibuffer input with completion;
they always discard text properties."  (`C-h v
minibuffer-allow-text-properties')

In Icicles this is governed by option
`icicle-unpropertize-completion-result-flag':

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

  Remember that you can use multi-command `icicle-toggle-option'
  anytime (`M-i M-i' during completion) to toggle an option
  value.

> Proposal 7: Allow duplicates and retain object identity
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    If a completion table contains duplicates, these
>    duplicates should not be removed.

Absolutely.  This mania was perhaps a side effect of
those enamoured by the introduction of hash tables to
Emacs.  Such no-duplicate-keys maps are great; they
have their place.  But it doesn't follow that there
are no use cases for completion where the candidates
you choose among are duplicates.

It's a great thing (though admittedly sometimes a bit
of a bother) that for Emacs a completion candidate
that you choose is a string but the "full candidate"
for the underlying program can be an alist entry, a
hash-table key+value pair, etc.

(Even cases where both the key and value are equal -
in whatever sense - are possible.  But typically only
the keys are duplicates.)

This feature is used often in my code.  And in many
cases I put the "full candidate" thingy on the string
that gets displayed to users as a text property, for
unambiguous retrieval of the full value from the
returned chosen string.

(Of course, if the returned string doesn't result
from choosing a candidate but from user text input,
that changes possibilities - it won't carry the full
candidate on its shoulder.)

>    There are not many completion tables which generate
>    duplicate candidates

Do you mean not many _kinds_ of completion tables,
or not many completion tables (in practice)?  If
you mean the latter, I'd like to know why you think
that, as I doubt it's the case.  (No, I don't have
numbers either.)

>    and there exist multiple completion systems
>    which do not perform deduplication at all.

Icicles among them.

Or, rather, this is under program control.  And
even under user control.  [You may be sensing a
pattern, here...  More user control, less
deciding at design time (aka hard-coding).]

Variable `icicle-transform-function':

  Function used to transform the list of completion candidates.
  This is applied to the list of initial candidates.
  If this is nil, then no transformation takes place.

  You can toggle this at any time from the minibuffer using `C-$,'.

  The value is changed by program locally, for use in particular
  contexts.  E.g., when you use `C-c C-`' (`icicle-search-generic')
  in a `*shell*' buffer, Icicles uses this variable with a value of
  `icicle-remove-duplicates', to remove duplicate shell commands
  from your input history list.

  You can use this variable in your Lisp code to transform the list
  of candidates any way you like.  A typical use is to remove
  duplicates, by binding it to `icicle-remove-duplicates' or
  `icicle-remove-dups-if-extras'.

Interactively, `C-$' can behave differently,
depending on the current command (actually, the
current use of `completing-read').

>    It is reasonable to handle the deduplication on a
>    case by case basis in each completion table. 

Amen.

>    Allowing duplicates is slightly more efficient and
>    allows to retain the object identity of the candidates.

The latter is the reason to retain this feature.
It's important.  The former isn't important, IMO.

>    If a candidate is selected
>    which is part of the collection, this exact candidate should be
>    returned. This subsumes Proposal 6 and allows to use text properties
>    for disambiguation of candidates.
> 
>    Note that this proposal is useful mainly for completion tables which
>    disable sorting by setting `display/cycle-sort-function' to the
>    identity function.

Why?  (I'm pretty sure I disagree.)

But maybe you just mean that if you sort the
candidates by taking their "object identity" into
account then that can help determine just which
duplicate a user chose.

If so, that's just implementation.  And it can
mean jumping through additional hoops.

>    Currently if identical candidate strings are used for
>    completion, these strings must be manually disambiguated

Yes.  And relying on a sort order to disambiguate
them is yet another roundabout hack.  Même combat.

>    Furthermore the disambiguation issue occurs when
>    mixing candidates from different candidate sources.

Indeed.

> Proposal 8: Completion style optimization of filtering and highlighting
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>    While working on Selectrum and Vertico it has been found that
>    highlighting has a significant cost. 

Again, please explain just what's been found,
instead of reporting what you've decided.  Details
matter.  What kind of highlighting?  When and how
was highlighting done, and to what?  What was tried?
In what contexts?  Just what problems were found?

>    This matters if the number of
>    displayed candidates differs greatly from the number of filtered
>    candidates. Therefore it would be good to have a possibility to
>    separate highlighting and filtering. 

If you're critiquing the highlighting done by
vanilla `completing-read', then I agree that such
highlighting isn't great.  Too eager (too early),
and too blanket (hard-coded).

As Stefan said recently, that old highlighting
was OK for simple prefix matching.  It just gets
in the way for other kinds of matching.

Icicles just tosses that highlighting
(both `completions-common-part' and
`completions-first-difference') out the window
and uses its own.

(defun icicle-undo-std-completion-faces ()
  "Get rid of standard completion-root highlighting in `*Completions*'."
  (when (fboundp 'face-spec-reset-face)
    (when (facep 'completions-common-part)
      (face-spec-reset-face 'completions-common-part)
      (set-face-attribute 'completions-common-part nil :inherit nil))
    (when (facep 'completions-first-difference)
      (face-spec-reset-face 'completions-first-difference)
      (set-face-attribute 'completions-first-difference nil :inherit nil))))

Icicles does the following kind of common-match
expansion (by default), and yes, highlighting can
be hard/problematic for some kinds of matching.

https://www.emacswiki.org/emacs/Icicles_-_Expanded-Common-Match_Completion

>    In the Orderless
>    completion-style, there is a variable `orderless-skip-highlighting'
>    which can be set to `t' or to a predicate function. Depending on the
>    value of this variable highlighting is applied or not applied by
>    `completion-all-completions'.
> 
>    In the first step, all the candidates can be computed and filtered
>    with `orderless-skip-highlighting=t', see
>    `vertico--recompute-candidates'. Afterwards, when displaying the
>    candidates, the completion UI has to pass the small subset of
>    candidates once again through `completion-all-completions', this time
>    with `orderless-skip-highlighting=nil', see `vertico--highlight'.
> 
>    I propose to generalize this Orderless feature by introducing a
>    variable `completion-skip-highlighting', which behaves the same as
>    `orderless-skip-highlighting' and is implemented for all built-in
>    completion styles. In Orderless, the filtering and highlighting is
>    already separate internally, therefore skipping the highlighting
>    turned out to be a natural decision in Orderless. The situation could
>    be different for the built-in styles.
> 
>    As an alternative, one can introduce a third function
>    `completion-highlight-completions', which is specified in
>    `completion-styles-alist'. But I suspect that the introduction of an
>    extra function requires more effort.

I do it in (I think) a simpler way in Icicles.
`completion-all-completions' shouldn't be involved
at all, IMO.  Icicles does it only in the function
that displays the _current_ set of candidates,
`icicle-display-candidates-in-Completions'.

And users can in rare cases control whether such
highlighting is done.  For Info commands where
completion candidates are node names, you can
optionally have candidates for already-visited nodes
be highlighted differently in *Completions*.  But
that's costly.  You can have this off by default and
just highlight them on demand with `C-x C-M-l'.
_____


General critique: Please present _one_ proposal per
thread, for consideration.  This thing about a Yalta
meeting having taken place and someone descending
from the summit with stone tables presenting the
IX Proposals isn't so helpful, IMO.

Admittedly, there are some relations among some of
them.  But still.

I propose that each proposal be submitted as a
separate enhancement request: `M-x report-emacs-bug'.
It could still be discussed more widely here, though.

And I'm glad people are thinking about, discussing,
and implementing such things.  Hooray!  So please
don't take my comments here too negatively.
Contradiction ~> progress.

Re: [External] : Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/8/21 1:49 AM, Drew Adams wrote:
>> Proposal 1: Disabling the history
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> What's the problem?  How does it hurt for users to
> have the default history, `minibuffer-history',
> available?  They don't have to use it, right?
> 
> I'm not saying I'm opposed to your proposed solution.
> I'm saying I haven't seen the problem specified that
> you're trying to solve.

As an author of completion commands I like the ability to disable the 
history if I think it is useful. One such case is when you browse the 
history itself using `completing-read`, but this is certainly a very 
narrow case. But `read-from-minibuffer` already accepts the `t` 
argument, so it will be less confusing if `completing-read` is aligned 
with it.

>> Proposal 4: Add support for `group-function'
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> So the overall problem you say you want to solve is
> that `completing-read' is too complex, hard to use,
> and hard to implement.  Yet you want to add something
> like this, to make it even more complex and hard to
> implement? ;-)

Haha, yes :) But adding metadata functions is a small cost and small 
addition in complexity.

> My main concern about further complicating the code
> implementing `completing-read' is that things will
> end up hard-coded - and hence difficult to use by
> users and by implementers of 3rd-party code.

In this case it is actually an optional extension. The UI does not have 
to use it, but can use it to offer an enhanced display. If the 
completion command does not use it nothing is lost. The proposal is 
fully forward/backward compatible. It is a pure enhancement, like 
annotation/affixation functions.

>> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Empty completion returning a special value (namely "")
> lets code know that the user provided empty completion.
> That's a _feature_.

I'd say it is a bug. Stefan Monnier seems to agree to some extent if I 
understood correctly. Unfortunately the bug is widespread so one has to 
look a bit around if there will bad effects due to fixing the bug.

>> Proposal 6: Return text properties from `completing-read'
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 100% agreement.  I proposed this a zillion years ago
> (and added it to Icicles).  Rejected for Emacs.  But
> this feature should only be _possible_, not imposed.

I hope that patches which allow text properties will be accepted. The 
initial responses I received sounded positive. There seems to be some 
agreement that it is useful to improve the support of the selection use 
case of `completing-read`.

>> Proposal 7: Allow duplicates and retain object identity
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

>>     There are not many completion tables which generate
>>     duplicate candidates
> Do you mean not many _kinds_ of completion tables,
> or not many completion tables (in practice)?  If
> you mean the latter, I'd like to know why you think
> that, as I doubt it's the case.  (No, I don't have
> numbers either.)

I am just saying that I have not observed many duplicates while using 
Emacs with completion systems, which do not deduplicate. I think 
`find-library` or `load-library` shows duplicate candidates?

>>     and there exist multiple completion systems
>>     which do not perform deduplication at all.
> 
> Icicles among them.

And Selectrum and Vertico :)

>>     Note that this proposal is useful mainly for completion tables which
>>     disable sorting by setting `display/cycle-sort-function' to the
>>     identity function.
> 
> Why?  (I'm pretty sure I disagree.)

I assume a Swiper-like command. If you don't sort then you can simply 
scroll through the duplicate candidates, which retain their original 
order. I am sure one can jump through many hoops to make this nice in 
any case. Stefan Monnier requested that default completion should also 
disambiguate the duplicates on the UI level, such that one can still 
chose between them ("reifying the object identity").

>> Proposal 8: Completion style optimization of filtering and highlighting
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Again, please explain just what's been found,
> instead of reporting what you've decided.  Details
> matter.  What kind of highlighting?  When and how
> was highlighting done, and to what?  What was tried?
> In what contexts?  Just what problems were found?

No, I am making concrete proposals here. I am not reporting issues.

> General critique: Please present _one_ proposal per
> thread, for consideration.  This thing about a Yalta
> meeting having taken place and someone descending
> from the summit with stone tables presenting the
> IX Proposals isn't so helpful, IMO.

I considered this but decided against it due to the relations and to 
keep the discussion at one place. As you see from the current responses 
the discussion seems to be productive. This includes your mail. Thank 
you for your responses!

Daniel Mendler

RE: [External] : Improvement proposals for `completing-read'

[Drew Adams]

> >> Proposal 1: Disabling the history
> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > What's the problem?  How does it hurt for users to
> > have the default history, `minibuffer-history',
> > available?  They don't have to use it, right?
> >
> > I'm not saying I'm opposed to your proposed solution.
> > I'm saying I haven't seen the problem specified that
> > you're trying to solve.
> 
> As an author of completion commands I like the ability to
> disable the history if I think it is useful.

That justification just repeats "I want it."  Why?
What problem does the proposed solution try to solve?

I'm not saying you don't need it or you shouldn't
have it.  I'm asking why.

> One such case is when you browse the history itself
> using `completing-read`, but this is certainly a very
> narrow case.

Why is it even a use case?  Why disable history when
`completing-read' against the history?  Maybe you
have a good reason - what is it?

> But `read-from-minibuffer` already accepts the `t`
> argument, so it will be less confusing if
> `completing-read` is aligned with it.

That hardly seems like a good argument.  The
behavior of those functions is quite different,
including their use of arguments that might seem
similar because of their names.

Consistency slapped on with a broad brush is
generally not a good guideline.  Local consistency
can be a good guideline - one among many.

And is this perhaps another case of favoring users
of such Elisp functions over users of the code
that makes use of those functions?

Why/when is it useful for _users_ to not be able
to access the history of their inputs?  What if
they just want to explore that history (without
using some command explicitly designed for that)?

Again:

 I'm not saying I'm opposed to your proposed solution.
 I'm saying I haven't seen the problem specified that
 you're trying to solve.

> >> Proposal 5: Forbid the null completions for `REQUIRE-MATCH=t'
> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Empty completion returning a special value (namely "")
> > lets code know that the user provided empty completion.
> > That's a _feature_.
> 
> I'd say it is a bug.

Why?  Reasons, please.  I gave reasons why it's
a feature.  Why do you think it's a bug?

> Stefan Monnier seems to agree to some extent if
> I understood correctly.

Appeal to authority or popularity isn't a reason.

Explain how else code can know whether the user
just hit RET (empty input).  Or explain why we
would want to remove the possibility to know that.

> Unfortunately the bug is widespread so one has to
> look a bit around if there will bad effects due
> to fixing the bug.

We're not there yet.  I'm asking why this feature
should be removed.

> There seems to be some agreement that it is useful to
> improve the support of the selection use case of `completing-read`.

I don't know what that means.  What's "the selec..."?

> >> Proposal 7: Allow duplicates and retain object identity
> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> >>     There are not many completion tables which generate
> >>     duplicate candidates
> >
> > Do you mean not many _kinds_ of completion tables,
> > or not many completion tables (in practice)?  If
> > you mean the latter, I'd like to know why you think
> > that, as I doubt it's the case.  (No, I don't have
> > numbers either.)
> 
> I am just saying that I have not observed many duplicates while using
> Emacs with completion systems, which do not deduplicate. I think
> `find-library` or `load-library` shows duplicate candidates?

You haven't seen many is different from "there
are not many."

I'd agree that there are likely more uses of
completion where duplicates are removed than
uses where they are not.

I'd even say there are likely more uses where
dups _should_ be removed.  But more doesn't
imply that the less case is few.

I expect there are many use cases for retaining
duplicates.  And even that if there were only a
few there might be a lot of use of those few.

> >>     Note that this proposal is useful mainly for
> >>     completion tables which disable sorting by
> >>     setting `display/cycle-sort-function' to the
> >>     identity function.
> >
> > Why?  (I'm pretty sure I disagree.)
> 
> I assume a Swiper-like command. If you don't sort then you can simply
> scroll through the duplicate candidates, which retain their original
> order.

It sounds like you're assuming that the order is,
say, the order of things (text bits, markers,...)
in a buffer.

As long as the "duplicates" retain, internally,
their complete identities (unique objects), the
UI can, if it likes, make use of that, and even
make it known in various ways to users.

There's no reason that arbitrary sort orders
can't be useful, just because the displayed
candidates might look like duplicates.

> I am sure one can jump through many hoops to make this nice in
> any case. Stefan Monnier requested that default completion should also
> disambiguate the duplicates on the UI level, such that one can still
> chose between them ("reifying the object identity").

Why the "default completion"?  We already have
that possibility (e.g. using text properties).

It should be up to a specific use of completion
(e.g. of `completing-read') whether to (1) allow
seeming duplicates and (2) distinguish them during
completion.

> >> Proposal 8: Completion style optimization of
> >> filtering and highlighting
> >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Again, please explain just what's been found,
> > instead of reporting what you've decided.  Details
> > matter.  What kind of highlighting?  When and how
> > was highlighting done, and to what?  What was tried?
> > In what contexts?  Just what problems were found?
> 
> No, I am making concrete proposals here. I am not
> reporting issues.

Proposals require some presentation of the utility
or need.  Raison d'être is perhaps the most important
part of a proposal - any proposal.

(And no, those proposals are not very concrete.)

> > General critique: Please present _one_ proposal per
> > thread, for consideration.  This thing about a Yalta
> > meeting having taken place and someone descending
> > from the summit with stone tables presenting the
> > IX Proposals isn't so helpful, IMO.
> 
> I considered this but decided against it due to the relations and to
> keep the discussion at one place. As you see from the current responses
> the discussion seems to be productive. This includes your mail. Thank
> you for your responses!

Such a discussion can be productive, but it can only
go so far, as things get more real.  The devil is in
the details.

Re: [External] : Improvement proposals for `completing-read'

[Jean Louis]

* Drew Adams <drew.adams@oracle.com> [2021-04-08 20:21]:
> > One such case is when you browse the history itself
> > using `completing-read`, but this is certainly a very
> > narrow case.
> 
> Why is it even a use case?  Why disable history when
> `completing-read' against the history?  Maybe you
> have a good reason - what is it?

This way, I am browsing ANY history from minibuffer:
(completing-read "Choose: " '("Jane" "Joe") nil nil nil nil)

This way, though the documentation does not say something about
it, but I do not see any history and I do not need to have
history variable, but function thinks there is history:
(completing-read "Choose: " '("Jane" "Joe") nil nil nil t)

So it is effectively "disabled".

Maybe it was meant here to disable the general minibuffer
history, but that is obviously easy to do. Other way to disable
would be to encompass `completing-read' with one `let':

(let ((history))
  (completing-read "Choose: " '("Jane" "Joe") nil nil nil 'history))

Or simply to disable history variable in the program when necessary.


-- 
Jean

Take action in Free Software Foundation campaigns:
https://www.fsf.org/campaigns

Sign an open letter in support of Richard M. Stallman
https://rms-support-letter.github.io/

Re: [External] : Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/9/21 1:19 PM, Jean Louis wrote:
> * Drew Adams <drew.adams@oracle.com> [2021-04-08 20:21]:
>> Why is it even a use case?  Why disable history when
>> `completing-read' against the history?  Maybe you
>> have a good reason - what is it?
>
> This way, though the documentation does not say something about
> it, but I do not see any history and I do not need to have
> history variable, but function thinks there is history:
> (completing-read "Choose: " '("Jane" "Joe") nil nil nil t)
> 
> So it is effectively "disabled".

This is actually illegal and will break 
`completion-all-sorted-completions`. My proposal is to explicitly allow 
this, the value `t` for the HIST argument. Please take a look at my 
original mail.

> Maybe it was meant here to disable the general minibuffer
> history, but that is obviously easy to do. Other way to disable
> would be to encompass `completing-read' with one `let':
> 
> (let ((history))
>    (completing-read "Choose: " '("Jane" "Joe") nil nil nil 'history))

Sure there are workarounds. Note that the history variable must be 
dynamically bound. Then the history variable may also end up in the 
savehist. It is better to fix this properly.

Daniel Mendler

RE: [External] : Improvement proposals for `completing-read'

[Drew Adams]

> > > One such case is when you 
> > > browse the history itself using `completing-read`,
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > but this is certainly a very narrow case.
> >
> > Why is it even a use case?  Why disable history when
> > `completing-read' against the history?  Maybe you
> > have a good reason - what is it?
> 
> This way, I am browsing ANY history from minibuffer:
> (completing-read "Choose: " '("Jane" "Joe") nil nil nil nil)
> 
> This way, though the documentation does not say something about
> it, but I do not see any history and I do not need to have
> history variable, but function thinks there is history:
> (completing-read "Choose: " '("Jane" "Joe") nil nil nil t)
> 
> So it is effectively "disabled".
> 
> Maybe it was meant here to disable the general minibuffer
> history, but that is obviously easy to do. Other way to disable
> would be to encompass `completing-read' with one `let':
> 
> (let ((history))
>   (completing-read "Choose: " '("Jane" "Joe") nil nil nil 'history))
> 
> Or simply to disable history variable in the program when necessary.

None of what you wrote has to do with what Daniel
posited: use of `completing-read' with a history
list providing the completion candidates.  (That's
at least what I understood him to mean.)  E.g.,

(completing-read "Choose from file-name input history: "
                 file-name-history)

(completing-read "Choose from regexp input history: "
                 regexp-history)

(completing-read "Choose from input history: "
                 minibuffer-history)

I think Daniel was suggesting that it makes sense
in all such cases to use `t' for the HIST arg.  E.g.:

(completing-read "Choose from input history: "
                 minibuffer-history nil nil nil t)

I'm guessing his point would be that without `t'
traversing the history (e.g. with `M-p' or `M-r')
would be a redundant way to get to a completion
candidate, which it would be.

But an alternative way to get to a candidate isn't
a bad thing.

And if the REQUIRE arg is nil then it can even be
useful to use a _different_ history for the HISTORY
arg from that used for the COLLECTION arg.

(Even if REQUIRE is non-nil that might sometimes
be useful - it lets you use `M-p' or `M-r' to get
to candidates that are in both histories.)

Re: [External] : Improvement proposals for `completing-read'

[Daniel Mendler]

On 4/9/21 7:22 PM, Drew Adams wrote:
> None of what you wrote has to do with what Daniel
> posited: use of `completing-read' with a history
> list providing the completion candidates.  (That's
> at least what I understood him to mean.)  E.g.,
> 
> (completing-read "Choose from file-name input history: "
>                   file-name-history)
> 
> (completing-read "Choose from regexp input history: "
>                   regexp-history)
> 
> (completing-read "Choose from input history: "
>                   minibuffer-history)
> 
> I think Daniel was suggesting that it makes sense
> in all such cases to use `t' for the HIST arg.  E.g.:
> 
> (completing-read "Choose from input history: "
>                   minibuffer-history nil nil nil t)
> 
> I'm guessing his point would be that without `t'
> traversing the history (e.g. with `M-p' or `M-r')
> would be a redundant way to get to a completion
> candidate, which it would be.

Yes, you understood me correctly. Note also that if you specify a 
variable, the value of the history variable is modified. When browsing a 
history the desire to avoid modifying the history itself is not 
unreasonable. This means that in any case you would want to use a 
different history variable than the one you are accessing, the history 
of your history browsing.

> And if the REQUIRE arg is nil then it can even be
> useful to use a _different_ history for the HISTORY
> arg from that used for the COLLECTION arg.

Sure you can do that. You can have a history of your history browsing 
and then a history of your history of your history browsing.

All I am proposing is the removal of the unnecessary restriction of 
disallowing `t` for the HIST argument of `completing-read`. If you look 
at `completing-read-default`, it calls `read-from-minibuffer` directly, 
which accepts this. If you use a different completion system, the 
situation may be different.

Daniel Mendler