improve "next locus from <buffer>" messages

improve "next locus from <buffer>" messages

[Stephen Leake]

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

Currently, next-error always outputs a message "next locus from
<buffer>". In the vast majority of cases, <buffer> does not change from
one message to the next, so this message is just annoying.

The attached patch changes it so the message is only output when the
locus changes.

It also gets rid of the "first/current/previous" options; what matters
is where the _next_ error will come from.

Comments?

-- 
-- Stephe

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: error_locus.diff --]
[-- Type: text/x-patch, Size: 2870 bytes --]

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 5bfb0bf901..1d5b932ba1 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1825,6 +1825,8 @@ compilation-start
 	(goto-char (point-max))))
 
     ;; Make it so the next C-x ` will use this buffer.
+    (when (not (eq outbuf next-error-last-buffer))
+      (message "next locus from %s" outbuf))
     (setq next-error-last-buffer outbuf)))
 
 (defun compilation-set-window-height (window)
diff --git a/lisp/simple.el b/lisp/simple.el
index 306df96766..832c62ffb3 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -312,34 +312,31 @@ next-error
       ;; We know here that next-error-function is a valid symbol we can funcall
       (with-current-buffer buffer
         (funcall next-error-function (prefix-numeric-value arg) reset)
-        (next-error-found buffer (current-buffer))
-        (message "%s locus from %s"
-                 (cond (reset                             "First")
-                       ((eq (prefix-numeric-value arg) 0) "Current")
-                       ((< (prefix-numeric-value arg) 0)  "Previous")
-                       (t                                 "Next"))
-                 next-error-last-buffer)))))
+        (next-error-found buffer (current-buffer))))))
 
 (defun next-error-internal ()
   "Visit the source code corresponding to the `next-error' message at point."
   (let ((buffer (current-buffer)))
     ;; We know here that next-error-function is a valid symbol we can funcall
     (funcall next-error-function 0 nil)
-    (next-error-found buffer (current-buffer))
-    (message "Current locus from %s" next-error-last-buffer)))
+    (next-error-found buffer (current-buffer))))
 
 (defun next-error-found (&optional from-buffer to-buffer)
   "Function to call when the next locus is found and displayed.
 FROM-BUFFER is a buffer from which next-error navigated,
 and TO-BUFFER is a target buffer."
-  (setq next-error-last-buffer (or from-buffer (current-buffer)))
-  (when to-buffer
-    (with-current-buffer to-buffer
-      (setq next-error-buffer from-buffer)))
-  (when next-error-recenter
-    (recenter next-error-recenter))
-  (funcall next-error-found-function from-buffer to-buffer)
-  (run-hooks 'next-error-hook))
+  (let ((prev next-error-last-buffer)
+        (next (or from-buffer (current-buffer))))
+    (when (not (eq prev next))
+      (message "next locus from %s" next))
+    (setq next-error-last-buffer next)
+    (when to-buffer
+      (with-current-buffer to-buffer
+        (setq next-error-buffer from-buffer)))
+    (when next-error-recenter
+      (recenter next-error-recenter))
+    (funcall next-error-found-function from-buffer to-buffer)
+    (run-hooks 'next-error-hook)))
 
 (defun next-error-select-buffer (buffer)
   "Select a `next-error' capable BUFFER and set it as the last used.

Re: improve "next locus from <buffer>" messages

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Date: Wed, 03 Apr 2019 09:50:20 -0800
> 
> Currently, next-error always outputs a message "next locus from
> <buffer>". In the vast majority of cases, <buffer> does not change from
> one message to the next, so this message is just annoying.
> 
> The attached patch changes it so the message is only output when the
> locus changes.
> 
> It also gets rid of the "first/current/previous" options; what matters
> is where the _next_ error will come from.
> 
> Comments?

Comment: why change long-time default behavior based on personal
preferences?  If indeed some people are annoyed by these simple
messages, maybe a defcustom is in order, but changing the default
behavior unconditionally due to such reasons sounds gross to me.

Thanks.

Re: improve "next locus from <buffer>" messages

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

>> Currently, next-error always outputs a message "next locus from
>> <buffer>". In the vast majority of cases, <buffer> does not change from
>> one message to the next, so this message is just annoying.
>> 
>> The attached patch changes it so the message is only output when the
>> locus changes.
>> 
>> It also gets rid of the "first/current/previous" options; what matters
>> is where the _next_ error will come from.
>> 
>> Comments?
>
> Comment: why change long-time default behavior based on personal
> preferences?  

These messages are not in emacs-26, so I don't think they count as
"long-time".

-- 
-- Stephe

Re: improve "next locus from <buffer>" messages

[Stephen Leake]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Currently, next-error always outputs a message "next locus from
>>> <buffer>". In the vast majority of cases, <buffer> does not change from
>>> one message to the next, so this message is just annoying.
>>> 
>>> The attached patch changes it so the message is only output when the
>>> locus changes.
>>> 
>>> It also gets rid of the "first/current/previous" options; what matters
>>> is where the _next_ error will come from.
>>> 
>>> Comments?
>>
>> Comment: why change long-time default behavior based on personal
>> preferences?  
>
> These messages are not in emacs-26, so I don't think they count as
> "long-time".

If you are refering to the "first/current/previous", they were not
always accurate; often the first error from a locus prints "next error
from ...", because next-error is invoked without reset.

-- 
-- Stephe

Re: improve "next locus from <buffer>" messages

[Felician Nemeth]

>>>> The attached patch changes it so the message is only output when the
>>>> locus changes.

>>>> Comments?

I'm not completely sure, but the proposed patch seems to assume that
next-error is only called from compilation-start.  xref doesn't use the
compilation (mirror) mode, but it uses the next-error functionality.

Re: improve "next locus from <buffer>" messages

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Date: Wed, 03 Apr 2019 12:53:41 -0800
> 
> > Comment: why change long-time default behavior based on personal
> > preferences?  
> 
> These messages are not in emacs-26, so I don't think they count as
> "long-time".

You are right, sorry.  It's just that the changes which introduced
them were discussed quite some time ago.

But the main issue still stands: why change something based on
personal preferences?  This function is called from many other places:
xref, multi-occur, even some Dired commands (AFAIR), and it can be
called intermittently in different contexts.  I think displaying the
context in these situations is generally a good idea.

Re: improve "next locus from <buffer>" messages

[Stephen Leake]

Felician Nemeth <felician.nemeth@gmail.com> writes:

>>>>> The attached patch changes it so the message is only output when the
>>>>> locus changes.
>
>>>>> Comments?
>
> I'm not completely sure, but the proposed patch seems to assume that
> next-error is only called from compilation-start.  

No, that assumption is not made. I added the message in
compilation-start because otherwise it does not print a message when the
locus is changed.

I did not search the rest of emacs for places that set
next-error-last-buffer, so there may be other places that need a
message.

Yes; xref--xref-buffer-mode sets next-error-last-buffer without a
message.

It makes sense to print a message only when the error locus
change might be unexpected. If the user has started a compilation, they
expect error messages to come from *compilation*; similarly for *xref*.
The original discussion included things like flymake, which is invoked
in the background.

I'll take out the message in compilation-start.

-- 
-- Stephe

Re: improve "next locus from <buffer>" messages

[Stephen Leake]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stephen Leake <stephen_leake@stephe-leake.org>
>> Date: Wed, 03 Apr 2019 12:53:41 -0800
>> 
>> > Comment: why change long-time default behavior based on personal
>> > preferences?  
>> 
>> These messages are not in emacs-26, so I don't think they count as
>> "long-time".
>
> You are right, sorry.  It's just that the changes which introduced
> them were discussed quite some time ago.

Yes. I've only started using master for development recently, so I only
noticed this now.

> But the main issue still stands: why change something based on
> personal preferences? This function is called from many other places:
> xref, multi-occur, even some Dired commands (AFAIR), and it can be
> called intermittently in different contexts. I think displaying the
> context in these situations is generally a good idea.

I agree, but I don't think it makes sense to print the same message over
and over again, when nothing has changed. Isn't there a general
rule to not fill *Messages* with noise?

xref and compile are the only places that change next-error-last-buffer
directly (in the core emacs sources; EPLA packages may also do this).

All of those other places still print the message when the locus has
changed. However, with this patch they only print "next error locus"; I
guess in come cases the "first/current/previous" messages might make
sense.

I'll put back the "first/current/previous", and add a custom option to
control this.

-- 
-- Stephe

Re: improve "next locus from <buffer>" messages

[Dmitry Gutov]

On 04.04.2019 16:41, Stephen Leake wrote:

> It makes sense to print a message only when the error locus
> change might be unexpected. If the user has started a compilation, they
> expect error messages to come from *compilation*; similarly for *xref*.

That sounds silly. Why not do that in the next-error feature itself?

Also, it seems Yuri is missing from this discussion.

Re: improve "next locus from <buffer>" messages

[Stephen Leake]

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

Stephen Leake <stephen_leake@stephe-leake.org> writes:


> I'll put back the "first/current/previous", and add a custom option to
> control this.

Updated patch attached. I added a NEWS entry; I'm not clear if the new
defcustom should be mentioned in the Emacs manual at (emacs) Compilation
Mode.

-- 
-- Stephe

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: error_locus.diff --]
[-- Type: text/x-patch, Size: 2972 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index 26c761ae01..59a97b6b9c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -303,6 +303,10 @@ and directory-local variables.
 'with-connection-local-profiles'.  No argument 'profiles' needed any
 longer.
 
+---
+** next-error-suppress-locus-message controls when `next-error'
+   outputs a message about the error locus.
+
 \f
 * Editing Changes in Emacs 27.1
 
diff --git a/lisp/simple.el b/lisp/simple.el
index 306df96766..4bb4116842 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -110,6 +110,15 @@ next-error-hook
   :type 'hook
   :group 'next-error)
 
+(defcustom next-error-suppress-locus-message nil
+  "If nil, `next-error' always outputs the current error buffer.
+If non-nil, the message is output only when the error buffer
+changes."
+  :group 'next-error
+  :type 'boolean
+  :safe #'booleanp
+  :version "27.1")
+
 (defvar next-error-highlight-timer nil)
 
 (defvar next-error-overlay-arrow-position nil)
@@ -312,21 +321,27 @@ next-error
       ;; We know here that next-error-function is a valid symbol we can funcall
       (with-current-buffer buffer
         (funcall next-error-function (prefix-numeric-value arg) reset)
-        (next-error-found buffer (current-buffer))
-        (message "%s locus from %s"
-                 (cond (reset                             "First")
-                       ((eq (prefix-numeric-value arg) 0) "Current")
-                       ((< (prefix-numeric-value arg) 0)  "Previous")
-                       (t                                 "Next"))
-                 next-error-last-buffer)))))
+        (let ((prev next-error-last-buffer))
+          (next-error-found buffer (current-buffer))
+          (when (or (not next-error-suppress-locus-message)
+                    (not (eq prev next-error-last-buffer)))
+            (message "%s locus from %s"
+                     (cond (reset                             "First")
+                           ((eq (prefix-numeric-value arg) 0) "Current")
+                           ((< (prefix-numeric-value arg) 0)  "Previous")
+                           (t                                 "Next"))
+                     next-error-last-buffer)))))))
 
 (defun next-error-internal ()
   "Visit the source code corresponding to the `next-error' message at point."
   (let ((buffer (current-buffer)))
     ;; We know here that next-error-function is a valid symbol we can funcall
     (funcall next-error-function 0 nil)
-    (next-error-found buffer (current-buffer))
-    (message "Current locus from %s" next-error-last-buffer)))
+    (let ((prev next-error-last-buffer))
+      (next-error-found buffer (current-buffer))
+      (when (or (not next-error-suppress-locus-message)
+                (not (eq prev next-error-last-buffer)))
+        (message "Current locus from %s" next-error-last-buffer)))))
 
 (defun next-error-found (&optional from-buffer to-buffer)
   "Function to call when the next locus is found and displayed.

Re: improve "next locus from <buffer>" messages

[Juri Linkov]

> +---
> +** next-error-suppress-locus-message controls when `next-error'
> +   outputs a message about the error locus.

Is this feature really expected to be used often to deserve the new
defcustom or a simple defvar would be enough?

In any case I recommend to rename it to next-error-verbosity.

Re: [SPAM UNSURE] Re: improve "next locus from <buffer>" messages

[Stephen Leake]

Juri Linkov <juri@linkov.net> writes:

>> +---
>> +** next-error-suppress-locus-message controls when `next-error'
>> +   outputs a message about the error locus.
>
> Is this feature really expected to be used often to deserve the new
> defcustom or a simple defvar would be enough?

If it's a defcustom, it is easier for people to find, if they are
bothered by the verbosity.

> In any case I recommend to rename it to next-error-verbosity.

ok.

-- 
-- Stephe

Re: [SPAM UNSURE] Re: improve "next locus from <buffer>" messages

[Juri Linkov]

>>> +---
>>> +** next-error-suppress-locus-message controls when `next-error'
>>> +   outputs a message about the error locus.
>>
>> Is this feature really expected to be used often to deserve the new
>> defcustom or a simple defvar would be enough?
>
> If it's a defcustom, it is easier for people to find, if they are
> bothered by the verbosity.
>
>> In any case I recommend to rename it to next-error-verbosity.
>
> ok.

Sorry, I realized only after your commit that a better name would be
next-error-verbose.  The word “verbosity” usually applies to
customization of the level of verbosity, but I see that you defined
a boolean variable for which “verbose” would be more appropriate.

Also this makes clear that the meaning of its value should be reversed -
to show verbose messages when the value is non-nil.  But I have no opinion
about its default value, if you want you can change it from nil to t.

Re: [SPAM UNSURE] Re: [SPAM UNSURE] Re: improve "next locus from <buffer>" messages

[Stephen Leake]

Juri Linkov <juri@linkov.net> writes:

>>>> +---
>>>> +** next-error-suppress-locus-message controls when `next-error'
>>>> +   outputs a message about the error locus.
>>>
>>> Is this feature really expected to be used often to deserve the new
>>> defcustom or a simple defvar would be enough?
>>
>> If it's a defcustom, it is easier for people to find, if they are
>> bothered by the verbosity.
>>
>>> In any case I recommend to rename it to next-error-verbosity.
>>
>> ok.
>
> Sorry, I realized only after your commit that a better name would be
> next-error-verbose.  

Makes sense

> Also this makes clear that the meaning of its value should be reversed -
> to show verbose messages when the value is non-nil.  

Yes, I realized that when I went to set it.

> But I have no opinion about its default value, if you want you can
> change it from nil to t.

On the principle of least bother to others, I'll make the default be the
verbose setting.

Thanks for your help.

-- 
-- Stephe