From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Spencer Baugh Newsgroups: gmane.emacs.bugs Subject: bug#65137: 29.1; completion-substring-try-completion doesn't return the longest common substring Date: Tue, 05 Sep 2023 15:51:14 -0400 Message-ID: References: <83a5v1okwm.fsf@gnu.org> <875y5pk8rj.fsf@catern.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="26527"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: sbaugh@catern.com, Eli Zaretskii , 65137@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Sep 05 21:52:13 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1qdc5t-0006eK-7Q for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 05 Sep 2023 21:52:13 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qdc5j-0006bI-AH; Tue, 05 Sep 2023 15:52:03 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qdc5i-0006Yr-2N for bug-gnu-emacs@gnu.org; Tue, 05 Sep 2023 15:52:02 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qdc5h-0007YG-QL for bug-gnu-emacs@gnu.org; Tue, 05 Sep 2023 15:52:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qdc5i-0005FL-Dr for bug-gnu-emacs@gnu.org; Tue, 05 Sep 2023 15:52:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Spencer Baugh Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 05 Sep 2023 19:52:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 65137 X-GNU-PR-Package: emacs Original-Received: via spool by 65137-submit@debbugs.gnu.org id=B65137.169394348520121 (code B ref 65137); Tue, 05 Sep 2023 19:52:02 +0000 Original-Received: (at 65137) by debbugs.gnu.org; 5 Sep 2023 19:51:25 +0000 Original-Received: from localhost ([127.0.0.1]:59426 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qdc56-0005ES-Ir for submit@debbugs.gnu.org; Tue, 05 Sep 2023 15:51:25 -0400 Original-Received: from mxout5.mail.janestreet.com ([64.215.233.18]:39931) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qdc52-0005EC-V8 for 65137@debbugs.gnu.org; Tue, 05 Sep 2023 15:51:22 -0400 In-Reply-To: (Stefan Monnier's message of "Tue, 29 Aug 2023 21:31:51 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:269397 Archived-At: --=-=-= Content-Type: text/plain Stefan Monnier writes: >> (completion-pcm--merge-completions '("ab" "ab") '(prefix "b")) >> -> ("b") > > Right this is a bug. > > (completion-pcm--merge-completions '("ab" "sab") '(prefix "b")) > > returns (correctly) > > ("b" "a" prefix) > > so > > (completion-pcm--merge-completions '("ab" "ab") '(prefix "b")) > > should return either ("b" "a") or ("b" "a" prefix). > > Could you accompany your patch of a regression test using > `completion-pcm--merge-completions` as above? Patch with test: --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-Correctly-handle-common-prefixes-in-substring-comple.patch >From 364a10aa54acd8de71a10d1ab98d059a27f26460 Mon Sep 17 00:00:00 2001 From: Spencer Baugh Date: Tue, 5 Sep 2023 15:40:06 -0400 Subject: [PATCH] Correctly handle common prefixes in substring completion Substring completion would previously not complete the longest common substring if that substring was a prefix of all the completion alternatives. Now it does. An explanation of this bug Substring completion is implemented by passing the `prefix' symbol as part of the pattern passed to completion-pcm--merge-completions. This symbol is supposed to cause completion-pcm--merge-completions to "grow" a completion of a common substring only from the "right" of the symbol (a common suffix), not from the "left" of the symbol (a common prefix). Yes, this is the opposite of what the name `prefix' would imply. When processing a symbolic element of the pattern, completion-pcm--merge-completions first finds the common prefix of all the completions in that part of the pattern (using try-completion). Then for `prefix' and other elements which want to complete a common suffix, the common prefix is removed from each element and then the common suffix is calculated with completion--common-suffix. If the common prefix covers the entirety of all the alternatives (i.e. when "unique" is true in the code), it's also a common suffix. In that case, the common suffix calculation (if it runs) is basically a no-op which will produce an empty string, since we removed the common prefix before running it. Before this change, `prefix' elements would unconditionally discard the common prefix, which produced the wrong result in the case that common prefix == common suffix. For example: (completion-pcm--merge-completions '("ab" "ab") '(prefix "b")) -> ("b") Now we detect this situation and include the common prefix in this case for `prefix' elements. Then we get: (completion-pcm--merge-completions '("ab" "ab") '(prefix "b")) -> ("b" "a") which is correct. * lisp/minibuffer.el (completion-pcm--merge-completions): Don't ignore a common suffix in a `prefix' pattern element when it's also a common prefix. * test/lisp/minibuffer-tests.el (completion-substring-test-5): Add a test. --- lisp/minibuffer.el | 4 +++- test/lisp/minibuffer-tests.el | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el index 35b359a75e2..ac054da61d2 100644 --- a/lisp/minibuffer.el +++ b/lisp/minibuffer.el @@ -4038,7 +4038,9 @@ completion-pcm--merge-completions (unique (or (and (eq prefix t) (setq prefix fixed)) (and (stringp prefix) (eq t (try-completion prefix comps)))))) - (unless (or (eq elem 'prefix) + ;; if the common prefix is unique, it also is a common + ;; suffix, so we should add it for `prefix' elements + (unless (or (and (eq elem 'prefix) (not unique)) (equal prefix "")) (push prefix res)) ;; If there's only one completion, `elem' is not useful diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el index ff58d35eb3e..4f92d7f841c 100644 --- a/test/lisp/minibuffer-tests.el +++ b/test/lisp/minibuffer-tests.el @@ -298,6 +298,19 @@ completion-substring-test-4 "jab" '("dabjabstabby" "many") nil 3))) 6))) +(ert-deftest completion-substring-test-5 () + ;; merge-completions needs to work correctly when + (should (equal + (completion-pcm--merge-completions '("ab" "sab") '(prefix "b")) + '("b" "a" prefix))) + (should (equal + (completion-pcm--merge-completions '("ab" "ab") '(prefix "b")) + '("b" "a"))) + ;; substring completion should successfully complete the entire string + (should (equal + (completion-substring-try-completion "b" '("ab" "ab") nil 0) + '("ab" . 2)))) + (ert-deftest completion-flex-test-1 () ;; Fuzzy match (should (equal -- 2.39.3 --=-=-= Content-Type: text/plain > Similarly, I see that > > (completion-pcm--merge-completions '("abr" "absabr") '(prefix "br")) > > returns > > ("br" prefix) > > whereas it should arguably return > > ("br" "a" prefix) > > [ Tho this may have the side effect that after this completion, `absabr` > won't be considered any more, if the `basic` completion comes before > `substring` :-( ] I did notice this too. I could try fixing/changing this too, but it does seem annoying when basic comes before substring - as it does by default in a number of completion categories. I wonder if we should move basic to after substring in those categories in completion-category-defaults? Or just remove basic from them. It doesn't seem like having both basic and substring in those lists has much point. Plus it would be a nice improvement to defaults - buffer completion in particular confused me for a long time before I understood how basic and substring interacted, so having only substring would ease understanding for new users. --=-=-=--