[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Improvement proposals for `completing-read'

From: Daniel Mendler
Subject: Re: Improvement proposals for `completing-read'
Date: Wed, 7 Apr 2021 21:46:43 +0200

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)?


> 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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]