From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Daniel Mendler Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] `completing-read` - allow history=t, sorting improvements Date: Mon, 19 Apr 2021 21:36:50 +0200 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="11075"; mail-complaints-to="usenet@ciao.gmane.io" Cc: "emacs-devel@gnu.org" To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Mon Apr 19 22:24:09 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 1lYaRF-0002ks-9q for ged-emacs-devel@m.gmane-mx.org; Mon, 19 Apr 2021 22:24:09 +0200 Original-Received: from localhost ([::1]:54366 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lYaRE-0008Iq-Bc for ged-emacs-devel@m.gmane-mx.org; Mon, 19 Apr 2021 16:24:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:52874) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lYZhe-0001Qv-N2 for emacs-devel@gnu.org; Mon, 19 Apr 2021 15:37:03 -0400 Original-Received: from server.qxqx.de ([2a01:4f8:121:346::180]:51251 helo=mail.qxqx.de) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lYZhc-0000VQ-9J for emacs-devel@gnu.org; Mon, 19 Apr 2021 15:37:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=qxqx.de; s=mail1392553390; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=DlFfDj+MSeVZL/oi37o2jhinqE8UwHGk6Cq4yOWtnm0=; b=O+WVJy7abaQWeg9m6oBOSnLbQ3 rcXdOtndnvIZShp6GwEpZhd91/ni4rB93ynJQARKekLArHYgFAhOCqqSdyF1XAXFtQcta2hiSC9KO Q2KEjL3fzpOr9AFkZwO2Q6heJ/dY1+QbHkGYdLsMIodqAUBQDUraIJy0q2ZIBIr/SJDQ=; In-Reply-To: Content-Language: en-US Received-SPF: pass client-ip=2a01:4f8:121:346::180; envelope-from=mail@daniel-mendler.de; helo=mail.qxqx.de X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_PASS=-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:268191 Archived-At: Hi Stefan, thank you for pushing. I will adjust the patches which are still relevant and resubmit them. On 4/19/21 9:14 PM, Stefan Monnier wrote: >> 1. Allow history=t to disable the `completing-read` history > Thanks, pushed. > I see that this was really just a bug fix: t already prevented adding > the result to "the history", there was just one place where the code > burped when encountering this special "history var". Yes, also the documentation of `minibuffer-variable` specifies the meaning of `t`. >> * minibuffer.el: Use completion--message consistently for the messages >> "Incomplete", "Sole completion" and "No completions". > I don't have a strong opinion on this patch, but I have the impression > that there might have been a good reason for the difference (i.e. the > above two cases could be considered "more serious" than those using > `completion--message`). > > I would personally gladly get rid of `completion-show-inline-help`, so > I'm not the right person to judge if the above patch is doing the right > thing or not. For me the point was mostly to get this clarified, therefore I included it here. Note that the messages "Sole completion" and "No completions" are already shown at some other places via `completion--done` which uses `completion--message` itself, but on a different code path. >> * lisp/minibuffer.el (completion-all-sorted-completions): When >> building the hash of history positions drop the prefix as determined >> by `completion-boundaries`. For file completions drop everything >> behind the first "/". > I think I'm fine with this patch, but at that point, this sorting code > *really* needs to be moved out into a separate function (already the > previous patch, which I just pushed, was borderline in this respect). > Also, I think this should come with a test case. Okay, I agree with splitting up the large function. I will add a test case. >> * lisp/minibuffer.el (completion-all-sorted-completions): Sort by >> history position, length and alphabetically. > I'd expect this will not make a big difference in most cases, but the > fact that it results in a more deterministic outcome is very welcome. > If you rebase this patch on top of the current minibuffer.el, I'll be > happy to push it. I will send an updated patch. >> From 23f306510c4a00b539607b9e57486c7fb72f37bf Mon Sep 17 00:00:00 2001 >> From: Daniel Mendler >> Date: Mon, 19 Apr 2021 13:17:23 +0200 >> Subject: [PATCH 5/6] completion-all-sorted-completions: Sort candidates in a >> single pass > Does this result in a measurable difference? > The assumptions about maximum size and length aren't ideal, so unless > there's a clear performance argument, I think we're better off sticking > to the multiple passes. >> * lisp/minibuffer.el (completion-all-sorted-completions): Sort by >> history position, length and alphabetically. > Here also: have you compared the performance of multiple calls to `sort` > vs a single call to sort with a more costly comparison function? > I'd expect the difference to be negligible (and having a separate > alphabetical sort at the beginning would save us from having to handle > it twice (i.e. in the two different branches)). Agree generally. It makes a difference if one uses car-less-than-car, since then the comparison function is fast. The difference is less pronounced if one includes the lambdas which sort alphabetically (this is on non-native, on native the picture could be different). The important change is really the quadratic one. However in my Vertico package (and in other continuously updating UIs), the big bottleneck of the UI still is the sorting for many candidates, even when including optimizations. Therefore I am using a vertico-sort-threshold there. Maybe there are potential improvements on a lower level? Daniel