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: [PATCH] `completing-read` - allow history=t, sorting improvements Date: Mon, 19 Apr 2021 15:14:09 -0400 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36276"; 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 Mon Apr 19 21:17:44 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 1lYZOy-0009Le-EY for ged-emacs-devel@m.gmane-mx.org; Mon, 19 Apr 2021 21:17:44 +0200 Original-Received: from localhost ([::1]:48482 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lYZOx-0004yy-9Q for ged-emacs-devel@m.gmane-mx.org; Mon, 19 Apr 2021 15:17:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:47564) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lYZLi-0001e6-G1 for emacs-devel@gnu.org; Mon, 19 Apr 2021 15:14:23 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:58048) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lYZLc-0007Ts-1S for emacs-devel@gnu.org; Mon, 19 Apr 2021 15:14:20 -0400 Original-Received: from pmg2.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 1759D80A77; Mon, 19 Apr 2021 15:14:14 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg2.iro.umontreal.ca (Proxmox) with ESMTP id 0704B805D6; Mon, 19 Apr 2021 15:14:12 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1618859652; bh=dUpSMDBonJc5300ONvDisRUgE27WSp/pReRj1xS9tPg=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=P17/u9nxab2CRmazSGJ2tvmfVNXOsaSGd5X1S0+5DPuQAL2sEkhQzOMrWpk5MHrgq E0QP/6LCc2n3w0yGooA5WbaFf9ui0yPD/zfpJbF809bxmTrRweHzP2A3TbT2gGZz3v wSQxi9TDCLRWKuiY+QzlPS9HFi9E5b5DMMKNM+4x75N3WqPFV2g/B9ExJM2MyQXCkI AOgO3Lfk4QoWwTMKJstzL4Ov2t7+HvCfBl6GKT8ub5VMsxS5FdPoip6HnOpFWwX4fH qEwOVrcBC+RF6zZ9HTVI06qhGCgdaSobQJm7HiATNVb3VWiwdjiD0Cd1PJD7KW7H8S NZlMtU9s0Bv6g== Original-Received: from alfajor (104-222-126-84.cpe.teksavvy.com [104.222.126.84]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 8AE491201C9; Mon, 19 Apr 2021 15:14:11 -0400 (EDT) In-Reply-To: (Daniel Mendler's message of "Mon, 19 Apr 2021 20:02:44 +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:268189 Archived-At: Hi Daniel, > 1. Allow history=3Dt 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". > From 42a99f69032b801a402d716280a50b4e27d1238f Mon Sep 17 00:00:00 2001 > From: Daniel Mendler > Date: Mon, 19 Apr 2021 15:40:00 +0200 > Subject: [PATCH 2/6] minibuffer.el: Use completion--message instead of > minibuffer-message > > * minibuffer.el: Use completion--message consistently for the messages > "Incomplete", "Sole completion" and "No completions". > --- > lisp/minibuffer.el | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el > index 06a5e1e988..e4da79ad2b 100644 > --- a/lisp/minibuffer.el > +++ b/lisp/minibuffer.el > @@ -1423,7 +1423,7 @@ minibuffer-force-complete-and-exit > ;; test-completion, then we shouldn't exit, but that should be rare. > (lambda () > (if minibuffer--require-match > - (minibuffer-message "Incomplete") > + (completion--message "Incomplete") > ;; If a match is not required, exit after all. > (exit-minibuffer))))) >=20=20 > @@ -2008,7 +2008,7 @@ minibuffer-completion-help > ;; the sole completion, then hide (previous&stale) completions. > (minibuffer-hide-completions) > (ding) > - (minibuffer-message > + (completion--message > (if completions "Sole completion" "No completions"))) >=20=20 > (let* ((last (last completions)) > --=20 > 2.20.1 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. I pushed your "use a hash-table" patch to fix the N=B2 complexity. > From 827c17d1645cce8d37a4a65369bea29e36681f3e Mon Sep 17 00:00:00 2001 > From: Daniel Mendler > Date: Mon, 19 Apr 2021 13:06:54 +0200 > Subject: [PATCH 4/6] completion-all-sorted-completions: Respect completion > boundaries > > * 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 "/". > --- > lisp/minibuffer.el | 36 +++++++++++++++++++++++++++++------- > 1 file changed, 29 insertions(+), 7 deletions(-) > > diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el > index d728c791d3..c7aec9665e 100644 > --- a/lisp/minibuffer.el > +++ b/lisp/minibuffer.el > @@ -1396,14 +1396,36 @@ > (let* ((hist (symbol-value minibuffer-history-variable)) > (hash (make-hash-table :test #'equal :size (lengt= h hist))) > (index 0) > - (def (car-safe minibuffer-default))) > + (def (car-safe minibuffer-default)) > + (bounds (completion-boundaries > + (substring string 0 (- (point) start)) > + minibuffer-completion-table > + minibuffer-completion-predicate > + "")) > + (pre (substring string 0 (car bounds))) > + (pre-len (length pre))) > + ;; Default comes first. > + (when (stringp def) > + (setq hist (cons def hist))) > ;; Record history positions in hash > + (if (equal "" pre) > + (progn > (dolist (c hist) > (unless (gethash c hash) > (puthash c index hash)) > - (cl-incf index)) > - (when (stringp def) > - (puthash def -1 hash)) > + (cl-incf index))) > + ;; Remove prefix from history strings, in order to > + ;; handle completion boundaries. > + (dolist (c hist) > + (when (string-prefix-p pre c) > + ;; Special handling of file name candidates: > + ;; Drop everything after the first / after the p= refix. > + (let ((pos (and minibuffer-completing-file-name > + (string-match-p "/" c pre-len)))) > + (setq c (substring c pre-len (and pos (1+ pos)= )))) > + (unless (gethash c hash) > + (puthash c index hash))) > + (cl-incf index))) > ;; Decorate elements with history position > (let ((c all)) > (while c 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. > 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 i= n a > single pass > > * lisp/minibuffer.el (completion-all-sorted-completions): Decorate > each candidate with history position and length such that the list can > be sorted 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. 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. > - (setq all (sort all #'car-less-than-car)) > + (setq all (sort all (lambda (c1 c2) > + (or (< (car c1) (car c2)) > + (and (=3D (car c1) (car c2)) > + (string< (cdr c1) (cdr = c2))))))) 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)). Stefan