Date: Mon, 9 Sep 2013 09:56:39 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH] emacs: show: lazy part handling bugfix
Message-ID: <20130909135637.GF1426@mit.edu>
References: <87txhz14z6.fsf@qmul.ac.uk>
	<1378510125-10245-1-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1378510125-10245-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Nice sleuthing!  LGTM.

One question, though.  `notmuch-show-insert-bodypart' calls
`notmuch-show-toggle-part-invisibility' with point at the end of the
buffer and not on the button.  Without this patch, properties will be
nil as a result, but with this patch, they may not be.  This still
seems strictly more correct, but do you know why this is okay?  Are
the properties nil at this point regardless because the button is all
in an overlay and we don't apply the :notmuch-part text property until
after this call to `notmuch-show-toggle-part-invisibility'?

Quoth Mark Walters on Sep 07 at 12:28 am:
> The lazy part handler had a bug that it allowed the button to be
> toggled to be specified. During toggling it needs to save and restore
> the text-properties for the button but it actually saved the text
> properties at point rather than from the button.
> 
> In almost all cases this didn't matter as as point had the same text
> properties as the button. However, it is a bug and did cause incorrect
> behaviour in some cases: see id:87txhz14z6.fsf@qmul.ac.uk for details.
> ---
> This is exactly the same as the patch in the parent except it has a
> commit message.
> 
> Best wishes
> 
> Mark
> 
> 
>  emacs/notmuch-show.el |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 20844f0..0267574 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -503,7 +503,7 @@ message at DEPTH in the current thread."
>  	     (new-start (button-start button))
>  	     (button-label (button-get button :base-label))
>  	     (old-point (point))
> -	     (properties (text-properties-at (point)))
> +	     (properties (text-properties-at (button-start button)))
>  	     (inhibit-read-only t))
>  	;; Toggle the button itself.
>  	(button-put button :notmuch-part-hidden (not show))