From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <aclements@csail.mit.edu>, notmuch@notmuchmail.org,
	Adam Wolfe Gordon <awg+notmuch@xvx.ca>
Subject: Re: [PATCH v3 3/5] emacs: show: pass button to create-overlays
In-Reply-To: <874ndi3gd7.fsf@awakening.csail.mit.edu>
References: <1370024806-6616-1-git-send-email-markwalters1009@gmail.com>
	<1370024806-6616-4-git-send-email-markwalters1009@gmail.com>
	<874ndi3gd7.fsf@awakening.csail.mit.edu>
User-Agent: Notmuch/0.14+255~gff3cc55 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Sat, 01 Jun 2013 00:08:46 +0100
Message-ID: <87vc5yg329.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


> On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>> Now that the bodypart code has the button we can pass that to
>> create-overlays and simplify that.
>> ---
>>  emacs/notmuch-show.el |   26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index be3aeac..66cc3a5 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -851,21 +851,19 @@ message at DEPTH in the current thread."
>>        (setq handlers (cdr handlers))))
>>    t)
>>  
>> -(defun notmuch-show-create-part-overlays (msg beg end hide)
>> +(defun notmuch-show-create-part-overlays (button beg end hide)
>>    "Add an overlay to the part between BEG and END"
>> -  (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. 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))
>> -      (button-put button 'overlay (make-overlay part-beg end))
>> -      ;; We toggle the button for hidden parts as that gets the
>> +
>> +  ;; If there is no button (i.e., the part is text/plain and the first
>> +  ;; part) or if the part has no content then we don't make the part
>> +  ;; toggleable.
>> +  (when (and button (/= beg end))
>> +    (button-put button 'overlay (make-overlay beg end))
>> +    ;; We toggle the button for hidden parts as that gets the
>>        ;; button label right.
>>        (save-excursion
>>  	(when hide
>> -	  (notmuch-show-toggle-part-invisibility button))))))
>> +	  (notmuch-show-toggle-part-invisibility button)))))
>
> I might just be mis-following the diff here, but shouldn't the above get
> re-indented?

I think you are right: it got a bit messed up when I tried to split up
the patch. I don't think it matters much as these lines get deleted in
the next patch. Incidentally is there a good git work flow for fixing up
something like this where you just want to change the midpoint of a
series?

>
>>  
>>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>>    "Insert the body part PART at depth DEPTH in the current thread.
>> @@ -879,10 +877,10 @@ If HIDE is non-nil then initially hide this part."
>>  			     "text/x-diff")
>>  			content-type))
>>  	 (nth (plist-get part :id))
>> -	 (beg (point))
>>  	 ;; We omit the part button for the first (or only) part if this is text/plain.
>>  	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
>> -		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))
>> +		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>> +	 (beg (point)))
>
> Was this swap necessary?

This is needed: inserting a part header moves point and this means that
point is at the start of the part content rather than the start of the
part button.

Best wishes

Mark

>
>>  
>>      (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
>>      ;; Some of the body part handlers leave point somewhere up in the
>> @@ -891,7 +889,7 @@ If HIDE is non-nil then initially hide this part."
>>      ;; Ensure that the part ends with a carriage return.
>>      (unless (bolp)
>>        (insert "\n"))
>> -    (notmuch-show-create-part-overlays msg beg (point) hide)))
>> +    (notmuch-show-create-part-overlays button beg (point) hide)))
>>  
>>  (defun notmuch-show-insert-body (msg body depth)
>>    "Insert the body BODY at depth DEPTH in the current thread."
>> -- 
>> 1.7.10.4