bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

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


In gnus.user there was a longish conversation about how to better report
the failure of Gnus mail source fetching to the user.

I originally went off on a grand adventure of defining custom errors for
various kinds of Gnus situations, for use in flow control. I still think
that's a good idea, but `nnheader-report' currently does the core of
that job for backend-specific errors, and it's fairly well-developed,
and there's not necessarily anything that needs fixing there.

And the original bug report was more about making errors visible to the
user than flow control, so I went back to that. Apart from fundamental
backend errors, other errors and failures are surfaced with
`gnus-message' and `gnus-error'. Both are gated by the integer value of
`gnus-verbose': higher numbers indicate less-important messages.

The more I fooled with things, the more it looked like improvements
could be made in `gnus-error'. It does a few things:

- Calls `ding'. This function returns nil on my system, dunno if it does
  anything on other systems.
- Displays the error using `message'.
- If the error level is a float, it uses the "float part" as a number of
  seconds to `sit-for' while displaying the error message. So an error
  level 4.5 would (if `gnus-verbose' were 4 or lower) display for 5
  seconds.

So obviously the purpose of this function is to get the user's
attention, in appropriate situations. But `ding' doesn't seem to do
anything, and there are only three places in the Gnus codebase where
`gnus-error' is called with a float value. That means there are only
three places where the `message' isn't immediately swallowed by whatever
comes next (and there's almost always another message coming next),
which really means there are only three places where `gnus-error' does
anything different from `gnus-message'.

The point here was to alert the user to failures in a non-annoying way,
and I think the warnings facility might be a better way of doing that.
`delay-warning', in particular, with a custom warning buffer. The
attached is a code sketch of that. Some points:

- It's hard-coded to prevent buffer pop-up, instead letting the user add
  the buffer to Gnus' window configuration, or call an interactive
  command to see it. We could also do something like put a note in the
  mode-line when there are new log messages to view.
- I've referred to all this as "logging" rather than "warnings", as that
  seems more general.
- gnus-message can also add strings to `gnus-action-message-log', which
  is consulted at the end of startup and maybe displayed with
  `gnus-final-warning'. That only happens once, at startup; it seems
  like a complicated mechanism to only use once. I think this could be
  replaced by warnings.
- Actually I think both `gnus-message' and `gnus-error' could be
  replaced with a `gnus-log' function, but that's something that could
  be played with later on.

I think the main concerns here are making sure the user actually sees
important messages, through a combination of splitting them off into
their own buffer, so they don't get lost in *Messages*, and potentially
delaying display until a particular action is complete, and the user has
a chance to see them.


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

diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index a777157f89..58edc55877 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -511,15 +511,10 @@ gnus-final-warning
 	     (mapconcat #'identity gnus-action-message-log "; "))))
 
 (defun gnus-error (level &rest args)
-  "Beep an error if LEVEL is equal to or less than `gnus-verbose'.
+  "Log an error if LEVEL is equal to or less than `gnus-verbose'.
 ARGS are passed to `message'."
   (when (<= (floor level) gnus-verbose)
-    (apply #'message args)
-    (ding)
-    (let (duration)
-      (when (and (floatp level)
-		 (not (zerop (setq duration (* 10 (- level (floor level)))))))
-	(sit-for duration))))
+    (delay-warning '(gnus) (apply #'message args) :warning "*Gnus Log*"))
   nil)
 
 (defun gnus-split-references (references)
@@ -1252,6 +1247,16 @@ gnus-create-info-command
     (setq gnus-info-buffer (current-buffer))
     (gnus-configure-windows 'info)))
 
+(defun gnus-display-log ()
+  "Pop up a window displaying the *Gnus Log* buffer."
+  (interactive)
+  ;; We just use plain `display-buffer' here.  If the user wants to
+  ;; compose the log buffer as part of a Gnus window configuration,
+  ;; they can do that in `gnus-buffer-configuration'.  If they want to
+  ;; control how this display works, they can configure
+  ;; `display-buffer-alist'.
+  (display-buffer "*Gnus Log*"))
+
 (defun gnus-not-ignore (&rest _args)
   t)
 
diff --git a/lisp/gnus/gnus-win.el b/lisp/gnus/gnus-win.el
index 8ac4e39fa5..9fafe88bcf 100644
--- a/lisp/gnus/gnus-win.el
+++ b/lisp/gnus/gnus-win.el
@@ -188,6 +188,7 @@ gnus-window-to-buffer
     (score-trace . "*Score Trace*")
     (split-trace . "*Split Trace*")
     (info . gnus-info-buffer)
+    (log . "*Gnus Log*")
     (category . gnus-category-buffer)
     (article-copy . gnus-article-copy)
     (draft . gnus-draft-buffer)
diff --git a/lisp/gnus/gnus.el b/lisp/gnus/gnus.el
index 6644cc4d81..c162b58d22 100644
--- a/lisp/gnus/gnus.el
+++ b/lisp/gnus/gnus.el
@@ -48,6 +48,11 @@ gnus-spam-resend-to
 (defvar gnus-ham-resend-to)
 (defvar gnus-spam-process-newsgroups)
 
+;; We suppress the display of all Gnus warnings: they go to a separate
+;; buffer, and should only be displayed as part of Gnus' own window
+;; display routines (or explicitly with `gnus-display-log').
+
+(cl-pushnew 'gnus warning-suppress-types)
 
 (defgroup gnus nil
   "The coffee-brewing, all singing, all dancing, kitchen sink newsreader."
diff --git a/lisp/gnus/mail-source.el b/lisp/gnus/mail-source.el
index af0a198376..c2ec48cc86 100644
--- a/lisp/gnus/mail-source.el
+++ b/lisp/gnus/mail-source.el
@@ -554,18 +554,15 @@ mail-source-fetch
 		 (condition-case err
 		     (funcall function source callback)
 		   (error
-		    (if (and (not mail-source-ignore-errors)
-			     (not
-			      (yes-or-no-p
-			       (format "Mail source %s error (%s).  Continue? "
-				       (if (memq ':password source)
-					   (let ((s (copy-sequence source)))
-					     (setcar (cdr (memq ':password s))
-						     "********")
-					     s)
-					 source)
-				       (cadr err)))))
-		      (error "Cannot get new mail"))
+		    (unless mail-source-ignore-errors
+		      (error "Mail source %s error (%s)"
+                             (if (memq ':password source)
+                                 (let ((s (copy-sequence source)))
+                                   (setcar (cdr (memq ':password s))
+	                                   "********")
+                                   s)
+                               source)
+                             (cadr err)))
 		    0)))))))))
 
 (declare-function gnus-message "gnus-util" (level &rest args))
diff --git a/lisp/gnus/nnmail.el b/lisp/gnus/nnmail.el
index bcf01cfa9e..a9a778f458 100644
--- a/lisp/gnus/nnmail.el
+++ b/lisp/gnus/nnmail.el
@@ -1842,7 +1842,7 @@ nnmail-get-new-mail-1
 							    src)))
 			      ansym))))
 		      ((error quit)
-		       (message "Mail source %s failed: %s" source cond)
+		       (gnus-error 5 cond)
 		       0)))
 	  (cl-incf total new)
 	  (cl-incf i)))

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> - Calls `ding'. This function returns nil on my system, dunno if it does
>   anything on other systems.

(ding) gives a beep and/or flashes the "visual bell", but many people
has switched off both -- so you get neither.

> That means there are only
> three places where the `message' isn't immediately swallowed by whatever
> comes next (and there's almost always another message coming next),
> which really means there are only three places where `gnus-error' does
> anything different from `gnus-message'.

Well, it depends on how you've configured your messaging level...

> I think the main concerns here are making sure the user actually sees
> important messages, through a combination of splitting them off into
> their own buffer, so they don't get lost in *Messages*, and potentially
> delaying display until a particular action is complete, and the user has
> a chance to see them.

My worry is that we're annoying users with warnings that aren't interesting.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

On 10/24/21 20:37 PM, Lars Ingebrigtsen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> - Calls `ding'. This function returns nil on my system, dunno if it does
>>   anything on other systems.
>
> (ding) gives a beep and/or flashes the "visual bell", but many people
> has switched off both -- so you get neither.
>
>> That means there are only
>> three places where the `message' isn't immediately swallowed by whatever
>> comes next (and there's almost always another message coming next),
>> which really means there are only three places where `gnus-error' does
>> anything different from `gnus-message'.
>
> Well, it depends on how you've configured your messaging level...

But both `gnus-error' and `gnus-message' consult the same verbosity
option, so they're not really much different.

>> I think the main concerns here are making sure the user actually sees
>> important messages, through a combination of splitting them off into
>> their own buffer, so they don't get lost in *Messages*, and potentially
>> delaying display until a particular action is complete, and the user has
>> a chance to see them.
>
> My worry is that we're annoying users with warnings that aren't
> interesting.

I haven't changed how `gnus-verbose' works, so that filter does the same
thing it always has. The only real difference is that messages/errors go
into a dedicated buffer, and the user has the option to keep the buffer
hidden (the default), pop it up with a command, or fit it into their
existing Gnus window configuration however they like.

For instance, my *Group* configuration contains nothing but the *Group*
buffer: the right two-thirds of the frame are whitespace. I'd probably
make the *Gnus Log* buffer always visible there, and always hide it
otherwise.

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 10/24/21 20:37 PM, Lars Ingebrigtsen wrote:
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> - Calls `ding'. This function returns nil on my system, dunno if it does
>>>   anything on other systems.
>>
>> (ding) gives a beep and/or flashes the "visual bell", but many people
>> has switched off both -- so you get neither.
>>
>>> That means there are only
>>> three places where the `message' isn't immediately swallowed by whatever
>>> comes next (and there's almost always another message coming next),
>>> which really means there are only three places where `gnus-error' does
>>> anything different from `gnus-message'.
>>
>> Well, it depends on how you've configured your messaging level...
>
> But both `gnus-error' and `gnus-message' consult the same verbosity
> option, so they're not really much different.

Though to be clear I'm not advocating getting rid of either of these
now, nothing that drastic. I'd like to put the basics in place, get some
feedback from users, and see what makes sense.

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I haven't changed how `gnus-verbose' works, so that filter does the same
> thing it always has. The only real difference is that messages/errors go
> into a dedicated buffer, and the user has the option to keep the buffer
> hidden (the default), pop it up with a command, or fit it into their
> existing Gnus window configuration however they like.

Sounds good.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I haven't changed how `gnus-verbose' works, so that filter does the same
>> thing it always has. The only real difference is that messages/errors go
>> into a dedicated buffer, and the user has the option to keep the buffer
>> hidden (the default), pop it up with a command, or fit it into their
>> existing Gnus window configuration however they like.
>
> Sounds good.

Well, I'm about 75% happy with this. I've attached both the patch I'm
running, and a screenshot of my *Group* configuration with the logging
window visible (I've set `gnus-verbose' to 10). I will suppress the urge
to apologize for my *Group* buffer appearance, I'm in the middle of some
home improvements.

The rightmost blob is the warning category: the "(gnus)". With fuller
adoption it might look like "(gnus nntp)" or "(gnus draft)" or "(gnus
search)", etc. I've made it so `gnus-add-timestamp-to-message' also adds
a timestamp here, though that's not activated in the screenshot.

The main awkwardness come from the fact that warnings.el is kind of set
up for you to incorporate it into other packages, but kind of not. You
can opt to split warnings off into your own buffer, which is great. But
in this case, whether and how warnings are logged and/or displayed
should be mostly controlled by Gnus' existing mechanisms (window
configuration and `gnus-verbose'), and I'm fighting warnings.el on that:

- `warning-suppress-types' should belong solely to the user, but I have
  to push `(gnus)' onto it to shut off automatic display, and move
  display control to Gnus's own knobs.
- There's no :info warning level, and it's unclear if `warning-levels'
  is fair game for packages to fool with. Instructions are given, but
  pushing your own values there will mess up eg `warning-numeric-level',
  and it's hard to know if it's really okay.
- The biggest eyesore in the screenshot is the acres of "disable showing
  disable logging" buttons, which are hardcoded. Again, Gnus has its own
  knobs for these.

Lastly, the messages coming from Gnus are very much set up for regular
message display, in particular the "<message>...done" pattern, which
doesn't work with the warnings setup. If we only use delayed warnings
then each time the warnings are output it runs `delayed-warnings-hook',
which runs `collapse-delayed-warnings', which might be an okay place to
collapse the "...done" lines into one. That hook is supposed to run as
part of the `post-command-hook', though, and it doesn't always run, and
I haven't figured out why yet.

All in all, it nearly works :( Probably there would need to be some
small design changes to warnings.el to make it nice. I still think this
or something like it is worth doing.

Eri


[-- Attachment #2: screenshot.png --]
[-- Type: image/png, Size: 189248 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-WIP-on-using-warnings-in-Gnus.patch --]
[-- Type: text/x-patch, Size: 7773 bytes --]

From c56bc92f9b41df5b302606458697cd49a0f98762 Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Fri, 22 Oct 2021 10:44:26 -0700
Subject: [PATCH] WIP on using warnings in Gnus

---
 lisp/gnus/gnus-start.el  |  1 +
 lisp/gnus/gnus-util.el   | 61 ++++++++++++++++++++++++++--------------
 lisp/gnus/gnus-win.el    |  1 +
 lisp/gnus/gnus.el        |  7 ++---
 lisp/gnus/mail-source.el | 21 ++++++--------
 lisp/gnus/nnmail.el      |  2 +-
 6 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/lisp/gnus/gnus-start.el b/lisp/gnus/gnus-start.el
index 606bd3a39a..ffbe656c14 100644
--- a/lisp/gnus/gnus-start.el
+++ b/lisp/gnus/gnus-start.el
@@ -1603,6 +1603,7 @@ gnus-get-unread-articles
 	 (type-cache nil)
 	 (gnus-agent-article-local-times 0)
 	 (archive-method (gnus-server-to-method "archive"))
+         (warning-series t)
 	 info group active method cmethod
 	 method-type method-group-list entry)
     (gnus-message 6 "Checking new news...")
diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index a777157f89..915ef5aee8 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -35,6 +35,7 @@
 (eval-when-compile (require 'cl-lib))
 
 (require 'seq)
+(require 'warnings)
 (require 'time-date)
 (require 'text-property-search)
 
@@ -440,6 +441,12 @@ gnus-add-timestamp-to-message
 		       :value t)
 		 (const :tag "No timestamp" nil)))
 
+(defcustom gnus-log-all-messages nil
+  "When non-nil, echo `gnus-message' messages to \"*Gnus Log*\"."
+  :version "29.1"
+  :group 'gnus-start
+  :type 'boolean)
+
 (eval-when-compile
   (defmacro gnus-message-with-timestamp-1 (format-string args)
     (let ((timestamp '(format-time-string "%Y%m%dT%H%M%S.%3N> " time)))
@@ -474,7 +481,18 @@ gnus-add-timestamp-to-message
 	       (t
 		(apply #'message ,format-string ,args)))))))
 
-(defvar gnus-action-message-log nil)
+(defun gnus-setup-logging ()
+  ;; We suppress the display of all Gnus warnings: they go to a
+  ;; separate buffer, and should only be displayed as part of Gnus'
+  ;; own window display routines (or explicitly with
+  ;; `gnus-display-log').
+  (cl-pushnew '(gnus) warning-suppress-types :test #'equal)
+  (cl-pushnew '(:gnus-info "%s ") warning-levels :test #'equal))
+
+(defun gnus-log-with-timestamp (_severity warning-entry)
+  (when gnus-add-timestamp-to-message
+    (insert (format-time-string "%Y%m%dT%H%M%S> " (current-time))))
+  warning-entry)
 
 (defun gnus-message-with-timestamp (format-string &rest args)
   "Display message with timestamp.  Arguments are the same as `message'.
@@ -493,33 +511,22 @@ gnus-message
       (let ((message
 	     (if gnus-add-timestamp-to-message
 		 (apply #'gnus-message-with-timestamp args)
-	       (apply #'message args))))
-	(when (and (consp gnus-action-message-log)
-		   (<= level 3))
-	  (push message gnus-action-message-log))
-	message)
+	       (apply #'message args)))
+            (warning-prefix-function #'gnus-log-with-timestamp))
+	(when gnus-log-all-messages
+          (display-warning '(gnus) message :gnus-info "*Gnus Log*"))
+        message)
     ;; We have to do this format thingy here even if the result isn't
     ;; shown - the return value has to be the same as the return value
     ;; from `message'.
     (apply #'format-message args)))
 
-(defun gnus-final-warning ()
-  (when (and (consp gnus-action-message-log)
-	     (setq gnus-action-message-log
-		   (delete nil gnus-action-message-log)))
-    (message "Warning: %s"
-	     (mapconcat #'identity gnus-action-message-log "; "))))
-
 (defun gnus-error (level &rest args)
-  "Beep an error if LEVEL is equal to or less than `gnus-verbose'.
+  "Log an error if LEVEL is equal to or less than `gnus-verbose'.
 ARGS are passed to `message'."
-  (when (<= (floor level) gnus-verbose)
-    (apply #'message args)
-    (ding)
-    (let (duration)
-      (when (and (floatp level)
-		 (not (zerop (setq duration (* 10 (- level (floor level)))))))
-	(sit-for duration))))
+  (when (<= level gnus-verbose)
+    (let ((warning-prefix-function #'gnus-log-with-timestamp))
+      (delay-warning '(gnus) (apply #'message args) :error "*Gnus Log*")))
   nil)
 
 (defun gnus-split-references (references)
@@ -1252,6 +1259,18 @@ gnus-create-info-command
     (setq gnus-info-buffer (current-buffer))
     (gnus-configure-windows 'info)))
 
+(defun gnus-display-log ()
+  "Pop up a window displaying the *Gnus Log* buffer."
+  (interactive)
+  ;; We just use plain `display-buffer' here.  If the user wants to
+  ;; compose the log buffer as part of a Gnus window configuration,
+  ;; they can do that in `gnus-buffer-configuration'.  If they want to
+  ;; control how this display works, they can configure
+  ;; `display-buffer-alist'.
+  (if (gnus-buffer-live-p "*Gnus Log*")
+      (display-buffer "*Gnus Log*")
+    (message "No log to display")))
+
 (defun gnus-not-ignore (&rest _args)
   t)
 
diff --git a/lisp/gnus/gnus-win.el b/lisp/gnus/gnus-win.el
index 8ac4e39fa5..9fafe88bcf 100644
--- a/lisp/gnus/gnus-win.el
+++ b/lisp/gnus/gnus-win.el
@@ -188,6 +188,7 @@ gnus-window-to-buffer
     (score-trace . "*Score Trace*")
     (split-trace . "*Split Trace*")
     (info . gnus-info-buffer)
+    (log . "*Gnus Log*")
     (category . gnus-category-buffer)
     (article-copy . gnus-article-copy)
     (draft . gnus-draft-buffer)
diff --git a/lisp/gnus/gnus.el b/lisp/gnus/gnus.el
index dbbbb71e57..6f420b7bd8 100644
--- a/lisp/gnus/gnus.el
+++ b/lisp/gnus/gnus.el
@@ -48,7 +48,6 @@ gnus-spam-resend-to
 (defvar gnus-ham-resend-to)
 (defvar gnus-spam-process-newsgroups)
 
-
 (defgroup gnus nil
   "The coffee-brewing, all singing, all dancing, kitchen sink newsreader."
   :group 'news
@@ -4203,9 +4202,9 @@ gnus
 	      (subr-native-elisp-p (symbol-function 'gnus)))
     (message "You should compile Gnus")
     (sit-for 2))
-  (let ((gnus-action-message-log (list nil)))
-    (gnus-1 arg dont-connect child)
-    (gnus-final-warning)))
+  (gnus-setup-logging)
+  (let ((warning-series t))
+    (gnus-1 arg dont-connect child)))
 
 (declare-function debbugs-gnu "ext:debbugs-gnu"
 		  (severities &optional packages archivedp suppress tags))
diff --git a/lisp/gnus/mail-source.el b/lisp/gnus/mail-source.el
index af0a198376..c2ec48cc86 100644
--- a/lisp/gnus/mail-source.el
+++ b/lisp/gnus/mail-source.el
@@ -554,18 +554,15 @@ mail-source-fetch
 		 (condition-case err
 		     (funcall function source callback)
 		   (error
-		    (if (and (not mail-source-ignore-errors)
-			     (not
-			      (yes-or-no-p
-			       (format "Mail source %s error (%s).  Continue? "
-				       (if (memq ':password source)
-					   (let ((s (copy-sequence source)))
-					     (setcar (cdr (memq ':password s))
-						     "********")
-					     s)
-					 source)
-				       (cadr err)))))
-		      (error "Cannot get new mail"))
+		    (unless mail-source-ignore-errors
+		      (error "Mail source %s error (%s)"
+                             (if (memq ':password source)
+                                 (let ((s (copy-sequence source)))
+                                   (setcar (cdr (memq ':password s))
+	                                   "********")
+                                   s)
+                               source)
+                             (cadr err)))
 		    0)))))))))
 
 (declare-function gnus-message "gnus-util" (level &rest args))
diff --git a/lisp/gnus/nnmail.el b/lisp/gnus/nnmail.el
index bcf01cfa9e..a9a778f458 100644
--- a/lisp/gnus/nnmail.el
+++ b/lisp/gnus/nnmail.el
@@ -1842,7 +1842,7 @@ nnmail-get-new-mail-1
 							    src)))
 			      ansym))))
 		      ((error quit)
-		       (message "Mail source %s failed: %s" source cond)
+		       (gnus-error 5 cond)
 		       0)))
 	  (cl-incf total new)
 	  (cl-incf i)))
-- 
2.33.1

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I will suppress the urge
> to apologize for my *Group* buffer appearance, I'm in the middle of some
> home improvements.

😋

> Lastly, the messages coming from Gnus are very much set up for regular
> message display, in particular the "<message>...done" pattern, which
> doesn't work with the warnings setup.

I'm having a bit of a problem with imagining the use case for that bit.
I don't think any users will ever want to look at a log/warning buffer
containing "Sorting threads..." etc -- it looks like a debugging tool
that's should be totally internal and not involve *Warnings* at all.

I don't think any other packages do anything like that with their
logging facilities.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I will suppress the urge
>> to apologize for my *Group* buffer appearance, I'm in the middle of some
>> home improvements.
>
> 😋
>
>> Lastly, the messages coming from Gnus are very much set up for regular
>> message display, in particular the "<message>...done" pattern, which
>> doesn't work with the warnings setup.
>
> I'm having a bit of a problem with imagining the use case for that bit.
> I don't think any users will ever want to look at a log/warning buffer
> containing "Sorting threads..." etc -- it looks like a debugging tool
> that's should be totally internal and not involve *Warnings* at all.
>
> I don't think any other packages do anything like that with their
> logging facilities.

Well that's mostly because I've turned `gnus-verbose' up to 10, and also
introduced an option `gnus-log-all-messages' which doubles
`gnus-message' output to the logging buffer. That is nil by default, but
I've set it to t here, so this is absolutely as loud as the logging
could possibly be.

If we adopt something like this, I think it would be worth doing a bit
of shuffling regarding which messages are sent through `gnus-error' and
which through `gnus-message'. Probably I'd introduce a `gnus-log'
function, have `gnus-error' call that, and replace some `gnus-message'
calls with `gnus-log'.

Ideally there would be no reason to have the option
`gnus-log-all-messages' at all, and the "Sorting threads..." messages
would only ever be messages.

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Well that's mostly because I've turned `gnus-verbose' up to 10, and also
> introduced an option `gnus-log-all-messages' which doubles
> `gnus-message' output to the logging buffer. That is nil by default, but
> I've set it to t here, so this is absolutely as loud as the logging
> could possibly be.

🙀

> If we adopt something like this, I think it would be worth doing a bit
> of shuffling regarding which messages are sent through `gnus-error' and
> which through `gnus-message'. Probably I'd introduce a `gnus-log'
> function, have `gnus-error' call that, and replace some `gnus-message'
> calls with `gnus-log'.
>
> Ideally there would be no reason to have the option
> `gnus-log-all-messages' at all, and the "Sorting threads..." messages
> would only ever be messages.

This reminds me -- I've been wondering whether we should basically drop
all of those ... very verbose messaging thingies and use
`with-delayed-message' instead everywhere.  That is, even with my low
verbosity message, this is what I'm getting when I hit `g':

Checking new news...
nnimap quimby.gnus.org splitting mail...done
nnimap read 0k from quimby.gnus.org
Reading active file from mltest via nnml...
Reading incoming mail from file... [2 times]
nnml: Reading incoming mail (no new mail)...done
Reading active file from mltest via nnml...done
Reading active file from archive via nnfolder...done
Reading active file from archive via nnfolder...done
Reading active file via nndraft...done
Reading active file via nnmbox...done
Checking new news...done

Except the nnimap one, all the rest are instantaneous and just flash
past me.  But we output them because we don't know whether a backend is
slow or not.  With `with-delayed-message' we could avoid all the
"Reading active" messages altogether in a normal setup, but they would
appear if the backend was slow.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

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

Continuing a more piecemeal approach to this, here's a patch to change
the mail source error behavior. I'm floating it for review because it
involves removing a user-facing option. If this goes in, should that
removal be mentioned in NEWS?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-gnus-error-to-report-mail-source-failures.patch --]
[-- Type: text/x-patch, Size: 2935 bytes --]

From 9d4359a72cd042a6357773c62fb70fc0c678a3cf Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Sat, 4 Dec 2021 12:37:14 -0800
Subject: [PATCH] Use gnus-error to report mail-source failures

* lisp/gnus/mail-source.el (mail-source-fetch): Instead of querying
the user on mail sources errors -- in effect asking "do you want to
continue, or halt the process?" -- log the error with `gnus-error',
severity 5. The query didn't provide any meaningful control; error
reporting is all that's needed.
(mail-source-ignore-errors): Obsolete this option; users can see the
error or not by configuring `gnus-verbose'.
* doc/misc/gnus.texi (Mail Source Customization): Remove mention of
the above option from the manual.
---
 doc/misc/gnus.texi       |  4 ----
 lisp/gnus/mail-source.el | 19 +++++++------------
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index a18afec02e..6ffc057ba1 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -15447,10 +15447,6 @@ Mail Source Customization
 files.  This variable only applies when
 @code{mail-source-delete-incoming} is a positive number.
 
-@item mail-source-ignore-errors
-@vindex mail-source-ignore-errors
-If non-@code{nil}, ignore errors when reading mail from a mail source.
-
 @item mail-source-directory
 @vindex mail-source-directory
 Directory where incoming mail source files (if any) will be stored.  The
diff --git a/lisp/gnus/mail-source.el b/lisp/gnus/mail-source.el
index af0a198376..efdddea69f 100644
--- a/lisp/gnus/mail-source.el
+++ b/lisp/gnus/mail-source.el
@@ -224,12 +224,9 @@ mail-sources
 					   (const :format "" :value :plugged)
 					   (boolean :tag "Plugged"))))))))
 
-(defcustom mail-source-ignore-errors nil
-  "Ignore errors when querying mail sources.
-If nil, the user will be prompted when an error occurs.  If non-nil,
-the error will be ignored."
-  :version "22.1"
-  :type 'boolean)
+(make-obsolete-variable 'mail-source-ignore-errors
+                        "configure `gnus-verbose' instead"
+                        "29.1")
 
 (defcustom mail-source-primary-source nil
   "Primary source for incoming mail.
@@ -554,18 +551,16 @@ mail-source-fetch
 		 (condition-case err
 		     (funcall function source callback)
 		   (error
-		    (if (and (not mail-source-ignore-errors)
-			     (not
-			      (yes-or-no-p
-			       (format "Mail source %s error (%s).  Continue? "
+                    (gnus-error
+                     5
+                     (format "Mail source %s error (%s)"
 				       (if (memq ':password source)
 					   (let ((s (copy-sequence source)))
 					     (setcar (cdr (memq ':password s))
 						     "********")
 					     s)
 					 source)
-				       (cadr err)))))
-		      (error "Cannot get new mail"))
+				       (cadr err)))
 		    0)))))))))
 
 (declare-function gnus-message "gnus-util" (level &rest args))
-- 
2.34.1

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Lars Ingebrigtsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Continuing a more piecemeal approach to this, here's a patch to change
> the mail source error behavior. I'm floating it for review because it
> involves removing a user-facing option. If this goes in, should that
> removal be mentioned in NEWS?

Looks OK to me.  And, yes, it should be mentioned in NEWS.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Continuing a more piecemeal approach to this, here's a patch to change
>> the mail source error behavior. I'm floating it for review because it
>> involves removing a user-facing option. If this goes in, should that
>> removal be mentioned in NEWS?
>
> Looks OK to me.  And, yes, it should be mentioned in NEWS.

This was more than half a year ago -- was there any progress here?

bug#51335: 29.0.50; Use warnings facility for reporting Gnus errors

[Eric Abrahamsen]

On 09/13/22 16:45 PM, Lars Ingebrigtsen wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>>> Continuing a more piecemeal approach to this, here's a patch to change
>>> the mail source error behavior. I'm floating it for review because it
>>> involves removing a user-facing option. If this goes in, should that
>>> removal be mentioned in NEWS?
>>
>> Looks OK to me.  And, yes, it should be mentioned in NEWS.
>
> This was more than half a year ago -- was there any progress here?

Gah... how did this get bogged down? I think I was originally trying to
abuse warnings.el as a general logging library, and eventually realized
that that simply wasn't going to work, and that sort of took the wind
out of my sails. But the patch here is probably a fine idea no matter
what.

I know you had more general concerns about the verbosity of Gnus'
message output, and that should still be addressed at some point, but my
head is very much out of this particular problem space. I guess I'd like
to apply this patch and close this bug, and come back to the problem
later, with a different solution.

WDYT?

Eric