[PATCH] emacs: show: refresh buffer did not remove overlays

[PATCH] emacs: show: refresh buffer did not remove overlays

[Mark Walters]

Previously refreshing the notmuch show buffer did not remove overlays
which meant that if the user refreshed a message with images the
images would remain and then the new text was added after.

One might have guessed that erase-buffer would have removed them but
it seems not.  Thus force the removal of overlays with remove-overlay.
---
The new toggle-parts code makes this bug much more likely to trigger
(as the user is quite likely to toggle a part in a message with an
image). However, it was already present if anyone tried refreshing a
show buffer with an image in it.

It would be good if someone could check whether there is anything else
that also needs to be manually removed. But, for me at least, this
seems to fix the problem.

Many thanks to Jani for finding the bug and helping with the diagnosis.

Best wishes

Mark




 emacs/notmuch-show.el |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index c8c1657..e89dba2 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1214,6 +1214,9 @@ reset based on the original query."
 		   (setq notmuch-show-message-multipart/alternative-display-parts nil)
 		 (notmuch-show-capture-state))))
     (erase-buffer)
+    ;; erase-buffer does not seem to remove overlays so do it manually.
+    ;; This can lead to weird effects such as remaining images.
+    (remove-overlays)
     (notmuch-show-build-buffer)
     (if state
 	(notmuch-show-apply-state state)
-- 
1.7.9.1

Re: [PATCH] emacs: show: refresh buffer did not remove overlays

[Austin Clements]

Quoth Mark Walters on Dec 03 at  1:11 pm:
> Previously refreshing the notmuch show buffer did not remove overlays
> which meant that if the user refreshed a message with images the
> images would remain and then the new text was added after.
> 
> One might have guessed that erase-buffer would have removed them but
> it seems not.  Thus force the removal of overlays with remove-overlay.
> ---
> The new toggle-parts code makes this bug much more likely to trigger
> (as the user is quite likely to toggle a part in a message with an
> image). However, it was already present if anyone tried refreshing a
> show buffer with an image in it.
> 
> It would be good if someone could check whether there is anything else
> that also needs to be manually removed. But, for me at least, this
> seems to fix the problem.
> 
> Many thanks to Jani for finding the bug and helping with the diagnosis.

From the source for erase-buffer it's clear that it is not intended to
delete non-evaporating overlays (and overlays are non-evaporating by
default).

AFAIK, the only other things that stick around are buffer-local
variables (which we clear in notmuch-show-refresh-view ->
notmuch-show-build-buffer -> notmuch-show-mode) and markers.

> Best wishes
> 
> Mark
> 
> 
> 
> 
>  emacs/notmuch-show.el |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index c8c1657..e89dba2 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1214,6 +1214,9 @@ reset based on the original query."
>  		   (setq notmuch-show-message-multipart/alternative-display-parts nil)
>  		 (notmuch-show-capture-state))))
>      (erase-buffer)

This isn't exactly related to your patch, but it looks like
notmuch-show-build-buffer also calls erase-buffer, so I wonder if this
is redundant.

> +    ;; erase-buffer does not seem to remove overlays so do it manually.
> +    ;; This can lead to weird effects such as remaining images.

I parse this as "Doing this manually can lead to weird effects...".
Maybe

erase-buffer does not seem to remove overlays, which can lead to weird
effects such as remaining images, so remove them manually.

?

> +    (remove-overlays)

For performance, it's probably slightly better to do this before the
erase-buffer so that erase-buffer doesn't have to shift around the
overlays.

>      (notmuch-show-build-buffer)
>      (if state
>  	(notmuch-show-apply-state state)

[PATCH v2] emacs: show: refresh buffer did not remove overlays

[Mark Walters]

Previously refreshing the notmuch show buffer did not remove overlays
which meant that if the user refreshed a message with images the
images would remain and then the new text was added after.

One might have guessed that erase-buffer would have removed them but
it seems not.  Thus force the removal of overlays with remove-overlays.
---

This version fixes the problems that Austin mentioned in his review. I
have not investigated whether the erase-buffer here can be removed: it
looks like it could be but the worst that this does is call
erase-buffer twice (slightly wasteful but no harm).

Best wishes

Mark


 emacs/notmuch-show.el |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 4d6c014..20f8997 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1185,6 +1185,10 @@ reset based on the original query."
   (let ((inhibit-read-only t)
 	(state (unless reset-state
 		 (notmuch-show-capture-state))))
+    ;; erase-buffer does not seem to remove overlays, which can lead
+    ;; to weird effects such as remaining images, so remove them
+    ;; manually.
+    (remove-overlays)
     (erase-buffer)
     (notmuch-show-build-buffer)
     (if state
-- 
1.7.9.1

Re: [PATCH v2] emacs: show: refresh buffer did not remove overlays

[Austin Clements]

LGTM.

On Wed, 05 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> Previously refreshing the notmuch show buffer did not remove overlays
> which meant that if the user refreshed a message with images the
> images would remain and then the new text was added after.
>
> One might have guessed that erase-buffer would have removed them but
> it seems not.  Thus force the removal of overlays with remove-overlays.
> ---
>
> This version fixes the problems that Austin mentioned in his review. I
> have not investigated whether the erase-buffer here can be removed: it
> looks like it could be but the worst that this does is call
> erase-buffer twice (slightly wasteful but no harm).
>
> Best wishes
>
> Mark
>
>
>  emacs/notmuch-show.el |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 4d6c014..20f8997 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1185,6 +1185,10 @@ reset based on the original query."
>    (let ((inhibit-read-only t)
>  	(state (unless reset-state
>  		 (notmuch-show-capture-state))))
> +    ;; erase-buffer does not seem to remove overlays, which can lead
> +    ;; to weird effects such as remaining images, so remove them
> +    ;; manually.
> +    (remove-overlays)
>      (erase-buffer)
>      (notmuch-show-build-buffer)
>      (if state
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2] emacs: show: refresh buffer did not remove overlays

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Previously refreshing the notmuch show buffer did not remove overlays
> which meant that if the user refreshed a message with images the
> images would remain and then the new text was added after.

pushed,

d