Date: Mon, 3 Dec 2012 11:47:30 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH] emacs: show: refresh buffer did not remove overlays
Message-ID: <20121203164730.GE1020@mit.edu>
References: <1354540260-21814-1-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1354540260-21814-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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)