From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: Improvement proposals for `completing-read' Date: Wed, 07 Apr 2021 17:26:50 -0400 Message-ID: References: <0342c2d5-02dd-ad9e-5b8e-dfe52f6469c6@daniel-mendler.de> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="31634"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Daniel Mendler Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed Apr 07 23:28:07 2021 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lUFiY-00086x-Cf for ged-emacs-devel@m.gmane-mx.org; Wed, 07 Apr 2021 23:28:06 +0200 Original-Received: from localhost ([::1]:56854 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lUFiX-0003tB-BS for ged-emacs-devel@m.gmane-mx.org; Wed, 07 Apr 2021 17:28:05 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:46080) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lUFhT-0001b8-2P for emacs-devel@gnu.org; Wed, 07 Apr 2021 17:26:59 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:10609) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lUFhP-0001Tt-TD for emacs-devel@gnu.org; Wed, 07 Apr 2021 17:26:58 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 0695C441682; Wed, 7 Apr 2021 17:26:54 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id ADB9544167E; Wed, 7 Apr 2021 17:26:51 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1617830811; bh=axGC17/pIxEoCZuZVb8Qv4paqcBUhDceRnxgdCHBA6w=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=D2d3kuwIABDhNZqCnyuyZ/65UH23FZQxh6sJpkG5B+lRFGLtj7Kj7AA6MdbypL9Eh mdXysJjmfHtLJINRKMQCLYCjSeWp5PX3GniT54618+q548Y5JrXnor54UVbYqreROx 45Yvk+mMXZqSdYUVEUWsAYyuA47dGj3o9uvMxvJN/fbGYEefjLu7ozHkmlNFUcavmd w2ew0Tlhljqn1BXz96v2BsFwNH601Oabe9u3c5je6XmXb9T9LRwqcSwyDiXj83VCxx aaSO3tFipEzjNVMi6npn8Z4t4u2FDIcUAyje1YGOAJ0t0kpXI2MMEhwbHCgyQ8ycin c3Ni5N0OZyuYg== Original-Received: from alfajor (104-222-126-84.cpe.teksavvy.com [104.222.126.84]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 2F955120176; Wed, 7 Apr 2021 17:26:51 -0400 (EDT) In-Reply-To: <0342c2d5-02dd-ad9e-5b8e-dfe52f6469c6@daniel-mendler.de> (Daniel Mendler's message of "Wed, 7 Apr 2021 21:46:43 +0200") Received-SPF: pass client-ip=132.204.25.50; envelope-from=monnier@iro.umontreal.ca; helo=mailscanner.iro.umontreal.ca X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:267577 Archived-At: >>> 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