[PATCH] emacs: show: improve handling of mark read tagging errors

[PATCH] emacs: show: improve handling of mark read tagging errors

[Mark Walters]

Previously if a marking read tag change (i.e., removing the unread
tag) failed for some reason, such as a locked database, then no more
mark read tag changes would be attempted in that buffer.

This handles the error more gracefully. There is not much we can do
yet about dealing with the error itself, and marking read is probably
not important enough to warrant keeping a queue of pending changes or
anything.

However this commit changes it so that

- we do try and make future mark read tag changes.

- we display the tag state correctly: i.e. we don't display the tag as
  deleted (no strike through)

- and since we know the tag change failed we can try to mark this
  message read in the future. Indeed, since the code uses the
  post-command hook we will try again on the next keypress (unless the
  user has left the message).

Since we could have many failed attempts at tagging, we just show a
message once per buffer.
---

This came up on irc today. The only real question is where to catch
the error. I think manual tag changes should give an actual error, so
we only want to catch errors from automatic tag-changes. By far the
most common of these is the mark read change, so just catch errors for
that.

Best wishes

Mark


emacs/notmuch-show.el | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index fea39fa..1753cac 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1691,6 +1691,9 @@ marked as unread, i.e. the tag changes in
     (apply 'notmuch-show-tag-message
 	   (notmuch-tag-change-list notmuch-show-mark-read-tags unread))))
 
+(defvar notmuch-show--seen-has-errored nil)
+(make-variable-buffer-local 'notmuch-show--seen-has-errored)
+
 (defun notmuch-show-seen-current-message (start end)
   "Mark the current message read if it is open.
 
@@ -1698,8 +1701,14 @@ We only mark it read once: if it is changed back then that is a
 user decision and we should not override it."
   (when (and (notmuch-show-message-visible-p)
 	     (not (notmuch-show-get-prop :seen)))
-	(notmuch-show-mark-read)
-	(notmuch-show-set-prop :seen t)))
+    (condition-case err
+	(progn
+	  (notmuch-show-mark-read)
+	  (notmuch-show-set-prop :seen t))
+      (error
+       (unless notmuch-show--seen-has-errored
+	 (setq notmuch-show--seen-has-errored 't)
+	 (message "Warning: marking message read failed"))))))
 
 (defun notmuch-show-command-hook ()
   (when (eq major-mode 'notmuch-show-mode)
-- 
2.1.4

[PATCH] emacs: show: improve handling of mark read tagging errors

[Mark Walters]

Previously if a marking read tag change (i.e., removing the unread
tag) failed for some reason, such as a locked database, then no more
mark read tag changes would be attempted in that buffer.

This handles the error more gracefully. There is not much we can do
yet about dealing with the error itself, and marking read is probably
not important enough to warrant keeping a queue of pending changes or
anything.

However this commit changes it so that

- we do try and make future mark read tag changes.

- we display the tag state correctly: i.e. we don't display the tag as
  deleted (no strike through)

- and since we know the tag change failed we can try to mark this
  message read in the future. Indeed, since the code uses the
  post-command hook we will try again on the next keypress (unless the
  user has left the message).

We indicate to the user that these mark read tag changes may have
failed in the header-line.
---

This is a slightly changed version of the previous patch, based on
some comments and a patch Tomi had on irc.

The changes are:

- we allow the debugger to run if debug-on-error is set

- the condition-case now protects any customised mark-read function

- the fact that a tagging error may have occured is displayed in the headerline

Best wishes

Mark





emacs/notmuch-show.el | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index fea39fa..6d3149b 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1701,12 +1701,23 @@ user decision and we should not override it."
 	(notmuch-show-mark-read)
 	(notmuch-show-set-prop :seen t)))
 
+(defvar notmuch-show--seen-has-errored nil)
+(make-variable-buffer-local 'notmuch-show--seen-has-errored)
+
 (defun notmuch-show-command-hook ()
   (when (eq major-mode 'notmuch-show-mode)
     ;; We need to redisplay to get window-start and window-end correct.
     (redisplay)
     (save-excursion
-      (funcall notmuch-show-mark-read-function (window-start) (window-end)))))
+      (condition-case err
+	  (funcall notmuch-show-mark-read-function (window-start) (window-end))
+	((debug error)
+	 (unless notmuch-show--seen-has-errored
+	   (setq notmuch-show--seen-has-errored 't)
+	   (setq header-line-format
+		 (concat header-line-format
+			 (propertize "  [some mark read tag changes may have failed]"
+				     'face font-lock-warning-face)))))))))
 
 (defun notmuch-show-filter-thread (query)
   "Filter or LIMIT the current thread based on a new query string.
-- 
2.1.4

[PATCH v3] emacs: show: improve handling of mark read tagging errors

[Mark Walters]

Previously if a marking read tag change (i.e., removing the unread
tag) failed for some reason, such as a locked database, then no more
mark read tag changes would be attempted in that buffer.

This handles the error more gracefully. There is not much we can do
yet about dealing with the error itself, and marking read is probably
not important enough to warrant keeping a queue of pending changes or
anything.

However this commit changes it so that

- we do try and make future mark read tag changes.

- we display the tag state correctly: i.e. we don't display the tag as
  deleted (no strike through)

- and since we know the tag change failed we can try to mark this
  message read in the future. Indeed, since the code uses the
  post-command hook we will try again on the next keypress (unless the
  user has left the message).

We indicate to the user that these mark read tag changes may have
failed in the header-line and by a message in the echo area.
---

Hi

The best level of user notification in case of an error is
unclear. The best we came up with on irc is this one:

On first error, the headerline is changed to say (in warning face)
that some mark read tag changes may have failed.

On each error, which will occur on each call to
notmuch-show-command-hook (so roughly after each keypress) we write
the error to the error buffer and we send a message to the echo area.

In principle I would like to send a single message to the echo area
and have it persist for a few seconds. However, the echo area is
cleared after each keypress so this seems difficult. Moreover, this
clearing means if we send the message a single time and the user
enters the message with repeated cursor-downs then the message will
disappear as soon as it is displayed.

In the future we might want to modify the error code to be something like the
message buffer and say "last error repeated x times", but that can
come later.

Best wishes

Mark


 emacs/notmuch-show.el | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index fea39fa..406f418 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1701,12 +1701,27 @@ user decision and we should not override it."
 	(notmuch-show-mark-read)
 	(notmuch-show-set-prop :seen t)))
 
+(defvar notmuch-show--seen-has-errored nil)
+(make-variable-buffer-local 'notmuch-show--seen-has-errored)
+
 (defun notmuch-show-command-hook ()
   (when (eq major-mode 'notmuch-show-mode)
     ;; We need to redisplay to get window-start and window-end correct.
     (redisplay)
     (save-excursion
-      (funcall notmuch-show-mark-read-function (window-start) (window-end)))))
+      (condition-case nil
+	  (funcall notmuch-show-mark-read-function (window-start) (window-end))
+	((debug error)
+	 ;; The call chain from notmuch-show-mark-read-function writes
+	 ;; and error to the error buffer before calling the error, so
+	 ;; we do not need to do that here. Just tell the user.
+	 (message "Warning -- marking message read failed.")
+	 (unless notmuch-show--seen-has-errored
+	   (setq notmuch-show--seen-has-errored 't)
+	   (setq header-line-format
+		 (concat header-line-format
+			 (propertize "  [some mark read tag changes may have failed]"
+				     'face font-lock-warning-face)))))))))
 
 (defun notmuch-show-filter-thread (query)
   "Filter or LIMIT the current thread based on a new query string.
-- 
2.1.4

Re: [PATCH v3] emacs: show: improve handling of mark read tagging errors

[Tomi Ollila]

On Fri, Jun 10 2016, Mark Walters <markwalters1009@gmail.com> wrote:

> Previously if a marking read tag change (i.e., removing the unread
> tag) failed for some reason, such as a locked database, then no more
> mark read tag changes would be attempted in that buffer.
>
> This handles the error more gracefully. There is not much we can do
> yet about dealing with the error itself, and marking read is probably
> not important enough to warrant keeping a queue of pending changes or
> anything.
>
> However this commit changes it so that
>
> - we do try and make future mark read tag changes.
>
> - we display the tag state correctly: i.e. we don't display the tag as
>   deleted (no strike through)
>
> - and since we know the tag change failed we can try to mark this
>   message read in the future. Indeed, since the code uses the
>   post-command hook we will try again on the next keypress (unless the
>   user has left the message).
>
> We indicate to the user that these mark read tag changes may have
> failed in the header-line and by a message in the echo area.
> ---
>
> Hi
>
> The best level of user notification in case of an error is
> unclear. The best we came up with on irc is this one:
>
> On first error, the headerline is changed to say (in warning face)
> that some mark read tag changes may have failed.
>
> On each error, which will occur on each call to
> notmuch-show-command-hook (so roughly after each keypress) we write
> the error to the error buffer and we send a message to the echo area.
>
> In principle I would like to send a single message to the echo area
> and have it persist for a few seconds. However, the echo area is
> cleared after each keypress so this seems difficult. Moreover, this
> clearing means if we send the message a single time and the user
> enters the message with repeated cursor-downs then the message will
> disappear as soon as it is displayed.
>
> In the future we might want to modify the error code to be something like the
> message buffer and say "last error repeated x times", but that can
> come later.
>
> Best wishes
>
> Mark
>
>
>  emacs/notmuch-show.el | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index fea39fa..406f418 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1701,12 +1701,27 @@ user decision and we should not override it."
>  	(notmuch-show-mark-read)
>  	(notmuch-show-set-prop :seen t)))
>  
> +(defvar notmuch-show--seen-has-errored nil)
> +(make-variable-buffer-local 'notmuch-show--seen-has-errored)
> +
>  (defun notmuch-show-command-hook ()
>    (when (eq major-mode 'notmuch-show-mode)
>      ;; We need to redisplay to get window-start and window-end correct.
>      (redisplay)
>      (save-excursion
> -      (funcall notmuch-show-mark-read-function (window-start) (window-end)))))
> +      (condition-case nil
> +	  (funcall notmuch-show-mark-read-function (window-start) (window-end))
> +	((debug error)
> +	 ;; The call chain from notmuch-show-mark-read-function writes
> +	 ;; and error to the error buffer before calling the error, so
> +	 ;; we do not need to do that here. Just tell the user.

I had a bit of difficulties to test this since:

notmuch-show-mark-read-function's value is (lambda
  (start end)
    (notmuch-show-do-seen start
                          (point)
                          1000000))

Original value was
notmuch-show-seen-current-message

(yes, I've heard there if helper called devel/try-emacs-mua but I just
ignored that knowledge >;) -- actually I put this to my production use,
replacing my old solution...)

But, If user changes that then it can be expected that errors are handled, too...


I'll keep running this; is there any better way to test this than

	(setq notmuch-command "broken")

Change looks good...


Tomi


> +	 (message "Warning -- marking message read failed.")
> +	 (unless notmuch-show--seen-has-errored
> +	   (setq notmuch-show--seen-has-errored 't)
> +	   (setq header-line-format
> +		 (concat header-line-format
> +			 (propertize "  [some mark read tag changes may have failed]"
> +				     'face font-lock-warning-face)))))))))
>  
>  (defun notmuch-show-filter-thread (query)
>    "Filter or LIMIT the current thread based on a new query string.

Re: [PATCH v3] emacs: show: improve handling of mark read tagging errors

[Mark Walters]

On Fri, 10 Jun 2016, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Fri, Jun 10 2016, Mark Walters <markwalters1009@gmail.com> wrote:
>
>> Previously if a marking read tag change (i.e., removing the unread
>> tag) failed for some reason, such as a locked database, then no more
>> mark read tag changes would be attempted in that buffer.
>>
>> This handles the error more gracefully. There is not much we can do
>> yet about dealing with the error itself, and marking read is probably
>> not important enough to warrant keeping a queue of pending changes or
>> anything.
>>
>> However this commit changes it so that
>>
>> - we do try and make future mark read tag changes.
>>
>> - we display the tag state correctly: i.e. we don't display the tag as
>>   deleted (no strike through)
>>
>> - and since we know the tag change failed we can try to mark this
>>   message read in the future. Indeed, since the code uses the
>>   post-command hook we will try again on the next keypress (unless the
>>   user has left the message).
>>
>> We indicate to the user that these mark read tag changes may have
>> failed in the header-line and by a message in the echo area.
>> ---
>>
>> Hi
>>
>> The best level of user notification in case of an error is
>> unclear. The best we came up with on irc is this one:
>>
>> On first error, the headerline is changed to say (in warning face)
>> that some mark read tag changes may have failed.
>>
>> On each error, which will occur on each call to
>> notmuch-show-command-hook (so roughly after each keypress) we write
>> the error to the error buffer and we send a message to the echo area.
>>
>> In principle I would like to send a single message to the echo area
>> and have it persist for a few seconds. However, the echo area is
>> cleared after each keypress so this seems difficult. Moreover, this
>> clearing means if we send the message a single time and the user
>> enters the message with repeated cursor-downs then the message will
>> disappear as soon as it is displayed.
>>
>> In the future we might want to modify the error code to be something like the
>> message buffer and say "last error repeated x times", but that can
>> come later.
>>
>> Best wishes
>>
>> Mark
>>
>>
>>  emacs/notmuch-show.el | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index fea39fa..406f418 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -1701,12 +1701,27 @@ user decision and we should not override it."
>>  	(notmuch-show-mark-read)
>>  	(notmuch-show-set-prop :seen t)))
>>  
>> +(defvar notmuch-show--seen-has-errored nil)
>> +(make-variable-buffer-local 'notmuch-show--seen-has-errored)
>> +
>>  (defun notmuch-show-command-hook ()
>>    (when (eq major-mode 'notmuch-show-mode)
>>      ;; We need to redisplay to get window-start and window-end correct.
>>      (redisplay)
>>      (save-excursion
>> -      (funcall notmuch-show-mark-read-function (window-start) (window-end)))))
>> +      (condition-case nil
>> +	  (funcall notmuch-show-mark-read-function (window-start) (window-end))
>> +	((debug error)
>> +	 ;; The call chain from notmuch-show-mark-read-function writes
>> +	 ;; and error to the error buffer before calling the error, so
>> +	 ;; we do not need to do that here. Just tell the user.
>
> I had a bit of difficulties to test this since:
>
> notmuch-show-mark-read-function's value is (lambda
>   (start end)
>     (notmuch-show-do-seen start
>                           (point)
>                           1000000))
>
> Original value was
> notmuch-show-seen-current-message
>
> (yes, I've heard there if helper called devel/try-emacs-mua but I just
> ignored that knowledge >;) -- actually I put this to my production use,
> replacing my old solution...)
>
> But, If user changes that then it can be expected that errors are handled, too...

This is why I preferred your approach of putting the condition-case in
the post-command-hook (over my approach of putting it in
notmuch-show-seen-current-message) as it should be robust to
customisation. I hope you meant it was hard to test as you couldn't
cause an error once you were in the buffer (as the whole buffer was
marked unread when you first entered).


> I'll keep running this; is there any better way to test this than
>
> 	(setq notmuch-command "broken")

Yes.

notmuch tag --batch

is great for this. It locks the database for writes until you ctrl-d to
exit.

Best wishes

Mark


>
> Change looks good...
>
>
> Tomi
>
>
>> +	 (message "Warning -- marking message read failed.")
>> +	 (unless notmuch-show--seen-has-errored
>> +	   (setq notmuch-show--seen-has-errored 't)
>> +	   (setq header-line-format
>> +		 (concat header-line-format
>> +			 (propertize "  [some mark read tag changes may have failed]"
>> +				     'face font-lock-warning-face)))))))))
>>  
>>  (defun notmuch-show-filter-thread (query)
>>    "Filter or LIMIT the current thread based on a new query string.

Re: [PATCH] emacs: show: improve handling of mark read tagging errors

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Previously if a marking read tag change (i.e., removing the unread
> tag) failed for some reason, such as a locked database, then no more
> mark read tag changes would be attempted in that buffer.
>
> This handles the error more gracefully. There is not much we can do
> yet about dealing with the error itself, and marking read is probably
> not important enough to warrant keeping a queue of pending changes or
> anything.

pushed

d