bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

Hello,

I think this patch is self-explanatory.  In order to get the ding you
have to "(setopt completion-auto-help 'visible)" and try to complete
something successfully.  I was doing 'M-x eshell-command-mo <TAB>' to
complete the symbol eshell-command-mode.

I'm not sure if that's actually the complete minimal setup, but I don't
think you guys have to bother reproducing it since it's pretty obvious
my patch is an improvement.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-minibuffer.el-minibuffer-completion-help-Only-d.patch --]
[-- Type: text/x-patch, Size: 1358 bytes --]

From 991ecaa8501a4eee8ab5073587462a0f7e36c488 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Mon, 5 Jun 2023 13:34:59 -0400
Subject: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding
 when appropriate.

(minibuffer-completion-help): Ensure ding is not called on a
successful completion.  Ensure ding is not called on a failure if
completion-fail-discreetly is set.  Also change "No completions" to
"No match" as that is what is used elsewhere.
---
 lisp/minibuffer.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index a1379913886..d26866370f1 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2384,9 +2384,11 @@ These include:
           ;; If there are no completions, or if the current input is already
           ;; the sole completion, then hide (previous&stale) completions.
           (minibuffer-hide-completions)
-          (ding)
-          (completion--message
-           (if completions "Sole completion" "No completions")))
+          (if completions
+              (completion--message "Sole completion")
+            (unless completion-fail-discreetly
+	      (ding)
+	      (completion--message "No match"))))
 
       (let* ((last (last completions))
              (base-size (or (cdr last) 0))
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 17 bytes --]


Thanks,

Morgan

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Date: Mon, 05 Jun 2023 13:50:02 -0400
> 
> I think this patch is self-explanatory.  In order to get the ding you
> have to "(setopt completion-auto-help 'visible)" and try to complete
> something successfully.  I was doing 'M-x eshell-command-mo <TAB>' to
> complete the symbol eshell-command-mode.
> 
> I'm not sure if that's actually the complete minimal setup, but I don't
> think you guys have to bother reproducing it since it's pretty obvious
> my patch is an improvement.

Sorry, no, it isn't obvious to me.  So please do provide a complete
recipe for reproducing the problem, starting frm "emacs -Q".

Thanks.

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Morgan Smith]

Eli Zaretskii <eliz@gnu.org> writes:

>
> Sorry, no, it isn't obvious to me.  So please do provide a complete
> recipe for reproducing the problem, starting frm "emacs -Q".
>
> Thanks.

I apologize for my assumption.

emacs -Q
(setopt completion-auto-help 'visible)
(debug-on-entry 'ding) ;; just so we know when it dings

`M-x eshell` then spam TAB until the completion buffer pops up.  Now
type in enough to select a unique completion.  I'm completing
`eshell-command` so I'll make the minibuffer look like 'eshell-comman'.

Now press one final TAB to select the only available completion.  It
should ding.

Obviously it shouldn't ding in that situation since it is a success.
Also that situation doesn't respect completion-fail-discreetly if you
where to type in a failing completion before the final TAB press.

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Cc: 63913@debbugs.gnu.org
> Date: Mon, 05 Jun 2023 15:09:02 -0400
> 
> emacs -Q
> (setopt completion-auto-help 'visible)
> (debug-on-entry 'ding) ;; just so we know when it dings
> 
> `M-x eshell` then spam TAB until the completion buffer pops up.  Now
> type in enough to select a unique completion.  I'm completing
> `eshell-command` so I'll make the minibuffer look like 'eshell-comman'.
> 
> Now press one final TAB to select the only available completion.  It
> should ding.

I cannot reproduce this, in the latest build from the emacs-29 branch
(what will soon become Emacs 29.1).  In which version of Emacs do you
see this, and on what OS?

> Also that situation doesn't respect completion-fail-discreetly if you
> where to type in a failing completion before the final TAB press.

Recipe, please.

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 137 bytes --]

I'm running on commit 7ca1d782f5910d0c3978c6798a45c6854ec668c7 from the
master branch.

I really hoping you can reproduce it with this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-for-when-completion-auto-help-is-visible.patch --]
[-- Type: text/x-patch, Size: 1915 bytes --]

From a1f095c8b26000d7526ce4791767c8a69eb915ea Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 6 Jun 2023 15:02:57 -0400
Subject: [PATCH] Add test for when completion-auto-help is 'visible

* test/lisp/minibuffer-tests.el (completion-auto-help-test): Add for
for when completion-auto-help is 'visible.  Also test for successful
completion message.
---
 test/lisp/minibuffer-tests.el | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 1de8e56cbd4..a67fc555772 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -353,12 +353,23 @@
             '("a" "ab" "ac")
           (execute-kbd-macro (kbd "a TAB TAB"))
           (should (equal (car messages) "Complete, but not unique"))
-          (should-not (get-buffer-window "*Completions*" 0))))
+          (should-not (get-buffer-window "*Completions*" 0))
+          (execute-kbd-macro (kbd "b TAB"))
+          (should (equal (car messages) "Sole completion"))))
       (let ((completion-auto-help t))
         (completing-read-with-minibuffer-setup
             '("a" "ab" "ac")
           (execute-kbd-macro (kbd "a TAB TAB"))
-          (should (get-buffer-window "*Completions*" 0)))))))
+          (should (get-buffer-window "*Completions*" 0))
+          (execute-kbd-macro (kbd "b TAB"))
+          (should (equal (car messages) "Sole completion"))))
+      (let ((completion-auto-help 'visible))
+        (completing-read-with-minibuffer-setup
+         '("a" "ab" "ac" "achoo")
+         (execute-kbd-macro (kbd "a TAB TAB"))
+         (should (get-buffer-window "*Completions*" 0))
+         (execute-kbd-macro (kbd "ch TAB"))
+         (should (equal (car messages) "Sole completion")))))))
 
 (ert-deftest completion-auto-select-test ()
   (let ((completion-auto-select t))
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 503 bytes --]


It should error out because apparently dings cause ert errors.  I did
not know that before but I am happy about it.

Eli Zaretskii <eliz@gnu.org> writes:

>
>> Also that situation doesn't respect completion-fail-discreetly if you
>> where to type in a failing completion before the final TAB press.
>
> Recipe, please.

I said this from a static analysis point of view but I couldn't seem to
reproduce this with a test.  It might be a dead code pathway but it
would still be nice for it to be correct.

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Cc: 63913@debbugs.gnu.org
> Date: Tue, 06 Jun 2023 15:08:26 -0400
> 
> I'm running on commit 7ca1d782f5910d0c3978c6798a45c6854ec668c7 from the
> master branch.
> 
> I really hoping you can reproduce it with this:

Apologies, it sounds like my original testing was faulty, as I can now
reproduce your original recipe.  What's more, it happens in Emacs 29
as well.

I've added to the discussion folks that know more about this than I
do.  Stefan, Ergus: any comments to the issue or about the proposed
patch?  (I'm not sure we need to change the message when we do "ding",
but that's a separate issue, I think.)

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I've added to the discussion folks that know more about this than I
> do.  Stefan, Ergus: any comments to the issue or about the proposed
> patch?  (I'm not sure we need to change the message when we do "ding",
> but that's a separate issue, I think.)

The patch looks good to me.  Thank you, Morgan.


        Stefan

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Morgan Smith <Morgan.J.Smith@outlook.com>,  Jimmy Aguilar Mena
>  <spacibba@aol.com>,  63913@debbugs.gnu.org
> Date: Thu, 08 Jun 2023 12:54:37 -0400
> 
> > I've added to the discussion folks that know more about this than I
> > do.  Stefan, Ergus: any comments to the issue or about the proposed
> > patch?  (I'm not sure we need to change the message when we do "ding",
> > but that's a separate issue, I think.)
> 
> The patch looks good to me.  Thank you, Morgan.

Thanks, installed on the emacs-29 branch, and closing the bug.

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Morgan Smith]

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>
> Thanks, installed on the emacs-29 branch, and closing the bug.

Would you also consider installing the test patch I sent previously?
I've reattached it here for your convenience.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-for-when-completion-auto-help-is-visible.patch --]
[-- Type: text/x-patch, Size: 1915 bytes --]

From d2265c26afa087ca38e449490c63e07e60224a82 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Tue, 6 Jun 2023 15:02:57 -0400
Subject: [PATCH] Add test for when completion-auto-help is 'visible

* test/lisp/minibuffer-tests.el (completion-auto-help-test): Add for
for when completion-auto-help is 'visible.  Also test for successful
completion message.
---
 test/lisp/minibuffer-tests.el | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 1de8e56cbd4..a67fc555772 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -353,12 +353,23 @@
             '("a" "ab" "ac")
           (execute-kbd-macro (kbd "a TAB TAB"))
           (should (equal (car messages) "Complete, but not unique"))
-          (should-not (get-buffer-window "*Completions*" 0))))
+          (should-not (get-buffer-window "*Completions*" 0))
+          (execute-kbd-macro (kbd "b TAB"))
+          (should (equal (car messages) "Sole completion"))))
       (let ((completion-auto-help t))
         (completing-read-with-minibuffer-setup
             '("a" "ab" "ac")
           (execute-kbd-macro (kbd "a TAB TAB"))
-          (should (get-buffer-window "*Completions*" 0)))))))
+          (should (get-buffer-window "*Completions*" 0))
+          (execute-kbd-macro (kbd "b TAB"))
+          (should (equal (car messages) "Sole completion"))))
+      (let ((completion-auto-help 'visible))
+        (completing-read-with-minibuffer-setup
+         '("a" "ab" "ac" "achoo")
+         (execute-kbd-macro (kbd "a TAB TAB"))
+         (should (get-buffer-window "*Completions*" 0))
+         (execute-kbd-macro (kbd "ch TAB"))
+         (should (equal (car messages) "Sole completion")))))))
 
 (ert-deftest completion-auto-select-test ()
   (let ((completion-auto-select t))
-- 
2.40.1

bug#63913: [PATCH] * lisp/minibuffer.el (minibuffer-completion-help): Only ding when appropriate.

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  spacibba@aol.com,
>   63913-done@debbugs.gnu.org
> Date: Sat, 10 Jun 2023 13:34:32 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >
> > Thanks, installed on the emacs-29 branch, and closing the bug.
> 
> Would you also consider installing the test patch I sent previously?
> I've reattached it here for your convenience.

Installed.  But please in the future try to follow better our
conventions for log messages, to avoid the need for me to manually fix
the log message when installing:

 . always mention the bug number in the log message
 . quote symbols 'like this'
 . make sure the lines are not too long (they will be longer by 8
   columns when converted to a ChangeLog as part of tarring the
   release)