Date: Fri, 31 May 2013 19:12:39 -0400
From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH v3 3/5] emacs: show: pass button to create-overlays
Message-ID: <20130531231239.GD5999@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>
	<87vc5yg329.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87vc5yg329.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Jun 01 at 12:08 am:
> 
> > 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?

Yeah, I just came across that.  If you want to fix it up, personally I
would use git rebase -i and mark this patch as "edit", then amend this
commit to fix the indentation, continue the rebase, and clean up the
inevitable rebase conflict when it replays the next patch.

> >
> >>  
> >>  (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.

Ah, okay.

> 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."