From: Mark Walters <markwalters1009@gmail.com>
To: notmuch@notmuchmail.org
Subject: Re: [PATCH v4 0/5] Use invisibility to toggle display of all parts
	including multipart
In-Reply-To: <1355781287-6010-1-git-send-email-markwalters1009@gmail.com>
References: <1355781287-6010-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.14+236~g1d0044f (http://notmuchmail.org) Emacs/24.2.1
	(x86_64-pc-linux-gnu)
Date: Tue, 18 Dec 2012 08:54:03 +0000
Message-ID: <87ip7zu4es.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Mon, 17 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> This is version 4 of this series (previous version at
> id:1355559338-14313-1-git-send-email-markwalters1009@gmail.com).
>
> The only change should be a bugfix which, for reasons I don't
> understand, only causes a problem on emacs 24. The problem is that the
> part invisibility code looks for a part button at the start of the
> region. This gets confused if there is a part with no part button
> (this is the case for the first part if it is text/plain) and the part
> starts with a button (as can happen if the message starts with the
> reply as in the first test in test/emacs-show).
>
> This checks that the button is a part button before creating the part
> overlay.

I don't think the above is very clear so I will try to explain it more
fully. 

The invisibility overlay for a part needs to be `linked' to the part
header button so that the part header button can toggle the overlay
visibility. The overlay is created and linked to this button after the
whole part has been inserted (including any notmuch-wash stuff). 

I could have made insert-part-header return the button it made and pass
it back up the call chain to the the create-overlays function but
instead I chose to make create-overlays just take the button at the
start of the part.

Now if the first part is text/plain then notmuch does not insert a
[text/plain] button so the code checks for this case by making sure the
part does start with a button, and if not it does not create the part
overlay (there is no button to toggle it so no point in an overlay).

However, if the first part is text/plain and notmuch wash happens to
make a button at the very start of the part then the create-overlays
function did still create an overlay *and* link it to the button. This
linking overwrote some of the things notmuch wash had attached to its
button (eg the button :overlay property) and that caused things to
break.

I still do not know why emacs 23 and emacs 24 behave differently, but
regardless the change from v3 is a clear bugfix: we just make sure it is
a notmuch-show-insert-part-header button not a notmuch-wash button
before we do the overlay creation/linking to the button. This version
does that by looking for a :base-label property of the button which
insert-part-header buttons have but notmuch-wash buttons do
not. (Obviously there are other ways this check could be done)

Best wishes

Mark



> The diff is below the diffstat. 
>
> Best wishes
>
> Mark
>
>
>
> Mark Walters (5):
>   emacs: show: modify insert-part-header to save the button text
>   emacs: show: add overlays for each part
>   emacs: show: add invisibility button action
>   emacs: wash: fix fake-diff part to include msg parameter
>   emacs: show: set default show-all-multipart/alternatives to nil
>
>  emacs/notmuch-show.el |  115 ++++++++++++++++++++++++++++++++++++++-----------
>  emacs/notmuch-wash.el |    2 +-
>  2 files changed, 90 insertions(+), 27 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5ca6fe2..3816e32 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -865,8 +865,10 @@ message at DEPTH in the current thread."
>    (let* ((button (button-at beg))
>  	 (part-beg (and button (1+ (button-end button)))))
>  
> -    ;; If the part contains no text we do not make it toggleable.
> -    (when (and button (/= part-beg end))
> +    ;; If the part contains no text we do not make it toggleable. We
> +    ;; also need to check that the button is a genuine part button not
> +    ;; a notmuch-wash button.
> +    (when (and button (/= part-beg end) (button-get button :base-label))
>        (let ((base-label (button-get button :base-label))
>  	    (overlay (make-overlay part-beg end))
>  	    (message-invis-spec (plist-get msg :message-invis-spec))
> -- 
> 1.7.9.1