#79265 - [PATCH] Treat point more consistently in PCM completion

GNU bug report logs - #79265
[PATCH] Treat point more consistently in PCM completion

Package: emacs;

Reported by: Spencer Baugh <sbaugh <at> janestreet.com>

Date: Mon, 18 Aug 2025 18:54:01 UTC

Severity: normal

Tags: patch

Done: Dmitry Gutov <dmitry <at> gutov.dev>

View this message in rfc822 format

From: Spencer Baugh <sbaugh <at> janestreet.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Tue, 19 Aug 2025 12:03:22 -0400

[Message part 1 (text/plain, inline)]

Dmitry Gutov <dmitry <at> gutov.dev> writes: > On 18/08/2025 21:53, Spencer Baugh wrote: >> Properly fix bug#38458, which is fundamentally an issue with >> completion-ignore-case, by checking if the completions are >> unique ignoring case. When the completions are unique, the >> normal code to delete a wildcard naturally causes point to be >> moved to the end of the minibuffer, which is the correct >> behavior. >> Now that the bug is fixed properly, remove a hack which >> previously was used to "fix" it, which made point behave >> inconsistently if it was in the middle of the minibuffer versus >> at the end of the minibuffer. > > Thanks, the description makes sense and the tests are great to have. > > I wonder if Stefan will want to comment though. > > Just one thing: > >> + ;; They're all the same string, ignoring case. >> + (seq-every-p >> + (lambda (elem) (string-equal-ignore-case elem prefix)) >> + comps))))) > > Is this performance-sensitive? Using a less abstracted function like > cl-every of even a (cl-loop for ... always ...) could be faster. > > There is a (dolist) above it which iterates across segments of the > pattern, so whatever overhead is added could be multiplied; OT2H I > haven't tested it myself yet, maybe all bottlenecks are somewhere > else. Hm, good point. After thinking about it more, I realized that the second try-completion call above somewhat duplicates the check I added, and that I could refactor in general to improve performance here. So I reworked the change a bit to combine my check with the second try-completion call. This should have better performance even though we aren't using the C implementation of try-completion anymore, since we're now doing much less work than try-completion does. (I also refactored to use a cond instead of (or (and ...) (and ...)), which I think makes this logic a fair bit clearer)

From 9465045d19a8676847cc8932ca66a40aabe2af59 Mon Sep 17 00:00:00 2001 From: Spencer Baugh <sbaugh <at> janestreet.com> Date: Mon, 18 Aug 2025 14:50:44 -0400 Subject: [PATCH] Treat point more consistently in PCM completion Properly fix bug#38458, which is fundamentally an issue with completion-ignore-case, by checking if the completions are unique ignoring case. When the completions are unique, the normal code to delete a wildcard naturally causes point to be moved to the end of the minibuffer, which is the correct behavior. Now that the bug is fixed properly, remove a hack which previously was used to "fix" it, which made point behave inconsistently if it was in the middle of the minibuffer versus at the end of the minibuffer. * lisp/minibuffer.el (completion-pcm--merge-completions): Respect completion-ignore-case when checking for completion uniqueness. (bug#79265) (completion-pcm--string->pattern) (completion-pcm--optimize-pattern): Allow point at the end of the pattern. * test/lisp/minibuffer-tests.el (completion-table-test-quoting) (completion-test--pcm-bug38458, completion-pcm-test-8): Update tests for more correct behavior. --- lisp/minibuffer.el | 25 +++++++++++++++---------- test/lisp/minibuffer-tests.el | 28 +++++++++++++++++++++------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el index 689cda7edab..c7efba10fec 100644 --- a/lisp/minibuffer.el +++ b/lisp/minibuffer.el @@ -4237,7 +4237,7 @@ completion-pcm--string->pattern "Split STRING into a pattern. A pattern is a list where each element is either a string or a symbol, see `completion-pcm--merge-completions'." - (if (and point (< point (length string))) + (if (and point (<= point (length string))) (let ((prefix (substring string 0 point)) (suffix (substring string point))) (append (completion-pcm--string->pattern prefix) @@ -4298,12 +4298,6 @@ completion-pcm--optimize-pattern (pcase p (`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_) (setq p (cdr p))) - ;; This is not just a performance improvement: it turns a - ;; terminating `point' into an implicit `any', which affects - ;; the final position of point (because `point' gets turned - ;; into a non-greedy ".*?" regexp whereas we need it to be - ;; greedy when it's at the end, see bug#38458). - (`(point) (setq p nil)) ;Implicit terminating `any'. (_ (push (pop p) n)))) (nreverse n))) @@ -4755,9 +4749,20 @@ completion-pcm--merge-completions ;; In practice, it doesn't seem to make any difference. (setq ccs (nreverse ccs)) (let* ((prefix (try-completion fixed comps)) - (unique (or (and (eq prefix t) (setq prefix fixed)) - (and (stringp prefix) - (eq t (try-completion prefix comps)))))) + (unique + (cond + ((eq prefix t) + (setq prefix fixed) + t) + ((stringp prefix) + ;; `try-completion' is almost this, but it doesn't + ;; handle `completion-ignore-case' the way we want. + (not (cl-some + (lambda (comp) + ;; PREFIX could complete to COMP, so it's not unique. + (and (string-prefix-p prefix comp completion-ignore-case) + (not (= (length prefix) (length comp))))) + comps)))))) ;; If there's only one completion, `elem' is not useful ;; any more: it can only match the empty string. ;; FIXME: in some cases, it may be necessary to turn an diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el index f9a26d17e58..59b72899e22 100644 --- a/test/lisp/minibuffer-tests.el +++ b/test/lisp/minibuffer-tests.el @@ -100,10 +100,10 @@ completion-table-test-quoting ;; Test that $$ in input is properly unquoted. ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion") ;; Test that env-vars are preserved. - ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest") - ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest") + ("lisp/c${CTTQ1}et/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test") + ("lisp/ced${CTTQ2}se-u-c" "lisp/ced${CTTQ2}semantic-utest-c.test") ;; Test that env-vars don't prevent partial-completion. - ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest") + ("lis/c${CTTQ1}/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test") )) (should (equal (completion-try-completion input #'completion--file-name-table @@ -118,11 +118,11 @@ completion-table-test-quoting ;; When an env var is in the completion bounds, try-completion ;; won't change letter case. ("lisp/c${CTTQ1}E" "lisp/c${CTTQ1}Et/") - ("lisp/ced${CTTQ2}SE-U" "lisp/ced${CTTQ2}SEmantic-utest") + ("lisp/ced${CTTQ2}SE-U-c" "lisp/ced${CTTQ2}SEmantic-utest-c.test") ;; If the env var is before the completion bounds, try-completion ;; *will* change letter case. - ("lisp/c${CTTQ1}et/SE-U" "lisp/c${CTTQ1}et/semantic-utest") - ("lis/c${CTTQ1}/SE-U" "lisp/c${CTTQ1}et/semantic-utest") + ("lisp/c${CTTQ1}et/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test") + ("lis/c${CTTQ1}/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test") )) (should (equal (car (completion-try-completion input #'completion--file-name-table @@ -224,7 +224,11 @@ completion-test--pcm-bug38458 (completion-pcm--merge-try '("tes" point "ing") '("Testing" "testing") "" "")) - '("testing" . 4)))) + '("testing" . 7))) + (should (equal + (let ((completion-ignore-case t)) + (completion-pcm-try-completion "tes" '("Testing" "testing") nil 3)) + '("testing" . 7)))) (ert-deftest completion-pcm-test-1 () ;; Point is at end, this does not match anything @@ -318,6 +322,16 @@ completion-pcm-test-7 '(prefix any "bar" any) '("xbarxfoo" "ybaryfoo") "" "") '("bar" . 3)))) +(ert-deftest completion-pcm-test-8 () + ;; try-completion inserts the common prefix and suffix at point. + (should (equal (completion-pcm-try-completion + "r" '("fooxbar" "fooybar") nil 0) + '("foobar" . 3))) + ;; Even if point is at the end of the minibuffer. + (should (equal (completion-pcm-try-completion + "" '("fooxbar" "fooybar") nil 0) + '("foobar" . 3)))) + (ert-deftest completion-substring-test-1 () ;; One third of a match! (should (equal -- 2.43.7

This bug report was last modified 9 days ago.

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.

GNU bug report logs - #79265 [PATCH] Treat point more consistently in PCM completion

GNU bug report logs - #79265
[PATCH] Treat point more consistently in PCM completion