Re: Improvement proposals for `completing-read'

[Daniel Mendler <mail@daniel-mendler.de>]

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