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>
Message #34 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Thu, 21 Aug 2025 16:44:40 -0400
[Message part 1 (text/plain, inline)]
Spencer Baugh <sbaugh <at> janestreet.com> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> - (while (setq next (match-end i))
>>> - (push (substring str last next) chopped)
>>> - (setq last next)
>>> + (while (setq next (match-string i str))
>>> + (push next chopped)
>>
>> IIUC this means that when `completion-ignore-case` is t we will keep the
>> capitalization typed by the user rather than adjust it based on the
>> completion candidates.
>>
>> E.g.
>>
>> (let ((completion-ignore-case t))
>> (completion-pcm--merge-completions '("ABC" "ABD") '("a")))
>>
>> will complete "a" to "aB" rather than to "AB".
>>
>> See commit 681e0e7c02c4f8ff6d316c8c24001106cb847023 (which should have
>> come with a regression test, obviously).
>
> ...except, you're right, of course it does cause this issue. Thanks for catching this.
>
> I'll write a test for this and try to find a further change to the code
> which retains the improvement in clarity while still fixing this issue.
>
> It should work to just arbitrarily take the capitalization from the
> first completion in the list, I think, right? That also will make
> things more consistent, I think?
No, that's not correct, actually. I need to use try-completion to merge
them to get nice behavior.
So how about this? This mostly reverts my earlier patch, so it may be
easier to review it relative to the old version before my earlier patch.
[0001-Use-capitalization-from-completions-in-pcm-try-again.patch (text/x-patch, inline)]
From 85a4b867646d563fc6e05edb2f6782073df77df8 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Thu, 21 Aug 2025 16:42:00 -0400
Subject: [PATCH] Use capitalization from completions in pcm-try, again
In b511c38bba5354ff21c697e4d27279bf73e4d3cf I tried to simplify
and optimize completion-pcm--merge-completions, but I broke
earlier behavior added by
681e0e7c02c4f8ff6d316c8c24001106cb847023 which made
completion-pcm-try-completion would change the case of text to
match the completions. Support this behavior again and add a
test checking this.
* lisp/minibuffer.el (completion-pcm--pattern->regex): When
GROUP is group-all, also group strings in PATTERN.
(completion-pcm--merge-completions): Preserve case from parts of
completions matching fixed strings in PATTERN. (bug#79265)
* test/lisp/minibuffer-tests.el (completion-pcm-bug4219): Add.
---
lisp/minibuffer.el | 55 +++++++++++++++++++++--------------
test/lisp/minibuffer-tests.el | 8 +++++
2 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 467ef4591db..67f3f924ffd 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4226,7 +4226,11 @@ completion-pcm--pattern->regex
(mapconcat
(lambda (x)
(cond
- ((stringp x) (regexp-quote x))
+ ((stringp x)
+ (let ((re (regexp-quote x)))
+ (if (eq group 'group-all)
+ (concat "\\(" re "\\)")
+ re)))
(t
(let ((re (if (eq x 'any-delim)
(concat completion-pcm--delim-wild-regex "*?")
@@ -4625,45 +4629,52 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (concat
- (completion-pcm--pattern->regex pattern 'group)
- ;; The implicit trailing `any' is greedy.
- "\\([^z-a]*\\)"))
+ (let ((re (completion-pcm--pattern->regex pattern 'group-all))
(ccs ())) ;Chopped completions.
- ;; First match each string against PATTERN as a regex and extract
- ;; the text matched by each wildcard.
+ ;; First chop each string into the parts corresponding to each
+ ;; non-constant element of `pattern', using regexp-matching.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
(let ((chopped ())
+ (last 0)
(i 1)
next)
- (while (setq next (match-string i str))
- (push next chopped)
+ (while (setq next (match-end i))
+ (push (substring str last next) chopped)
+ (setq last next)
(setq i (1+ i)))
+ ;; Add the text corresponding to the implicit trailing `any'.
+ (push (substring str last) chopped)
(push (nreverse chopped) ccs))))
- ;; Then for each of those wildcards, extract the commonality between them.
+ ;; Then for each of those non-constant elements, extract the
+ ;; commonality between them.
(let ((res ())
;; Accumulate each stretch of wildcards, and process them as a unit.
(wildcards ()))
;; Make the implicit trailing `any' explicit.
(dolist (elem (append pattern '(any)))
- (if (stringp elem)
- (progn
- (push elem res)
- (setq wildcards nil))
- (let ((comps ()))
+ (let ((comps ()))
+ (dolist (cc (prog1 ccs (setq ccs nil)))
+ (push (car cc) comps)
+ (push (cdr cc) ccs))
+ ;; Might improve the likelihood to avoid choosing
+ ;; different capitalizations in different parts.
+ ;; In practice, it doesn't seem to make any difference.
+ (setq ccs (nreverse ccs))
+ (if (stringp elem)
+ (let ((shared (try-completion elem comps)))
+ ;; Use `try-completion' to merge the strings from each
+ ;; completion, which may differ in case.
+ (push (cond ((eq shared t) elem)
+ ((stringp shared) shared)
+ (t ""))
+ res)
+ (setq wildcards nil))
(push elem wildcards)
- (dolist (cc (prog1 ccs (setq ccs nil)))
- (push (car cc) comps)
- (push (cdr cc) ccs))
- ;; Might improve the likelihood to avoid choosing
- ;; different capitalizations in different parts.
- ;; In practice, it doesn't seem to make any difference.
- (setq ccs (nreverse ccs))
(let* ((prefix (try-completion "" comps))
(unique (or (and (eq prefix t) (setq prefix ""))
(and (stringp prefix)
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index c2c37e63012..e5868351619 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -332,6 +332,14 @@ completion-pcm-test-8
"" '("fooxbar" "fooybar") nil 0)
'("foobar" . 3))))
+(ert-deftest completion-pcm-bug4219 ()
+ ;; With `completion-ignore-case', try-completion should change the
+ ;; case of existing text when the completions have different casing.
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "a" '("ABC" "ABD") nil 1))
+ '("AB" . 2))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.