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