From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org,
	Adam Wolfe Gordon <awg+notmuch@xvx.ca>
Subject: Re: [PATCH v3 4/5] emacs: show: modify the way hidden state is
	recorded.
In-Reply-To: <1370024806-6616-5-git-send-email-markwalters1009@gmail.com>
References: <1370024806-6616-1-git-send-email-markwalters1009@gmail.com>
	<1370024806-6616-5-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.15.2+83~g8bee3c4 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Fri, 31 May 2013 19:09:42 -0400
Message-ID: <871u8m3fwp.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Previously, whether a part was hidden or shown was recorded in the
> invisibility/visibility of the part overlay. Since we are going to
> have lazily rendered parts with no overlay store the hidden/shown
> state in the part button itself.
>
> Additionally, in preparation for the invisible part handling move the
> actual hiding of the hidden parts to insert-bodypart from
> create-part-overlays.
>
> Finally, we will need to know whether a part-insertion has done
> anything (it won't if the invisible part cannot be displayed by emacs)
> so we slightly rejig the code order in
> notmuch-show-toggle-part-invisibility to make it easier for the
> function to set an appropriate return value.
> ---
>  emacs/notmuch-show.el |   23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 66cc3a5..89199e8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -495,6 +495,7 @@ message at DEPTH in the current thread."
>  	   (concat "[ " base-label " ]")
>  	   :base-label base-label
>  	   :type 'notmuch-show-part-button-type
> +	   :notmuch-part-hidden nil
>  	   :notmuch-part nth
>  	   :notmuch-filename name
>  	   :notmuch-content-type content-type))
> @@ -555,18 +556,20 @@ message at DEPTH in the current thread."
>    (let* ((button (or button (button-at (point))))
>  	 (overlay (button-get button 'overlay)))
>      (when overlay
> -      (let* ((show (overlay-get overlay 'invisible))
> +      (let* ((show (button-get button :notmuch-part-hidden))
>  	     (new-start (button-start button))
>  	     (button-label (button-get button :base-label))
>  	     (old-point (point))
>  	     (inhibit-read-only t))
> -	(overlay-put overlay 'invisible (not show))
> +	;; Toggle the button itself.
> +	(button-put button :notmuch-part-hidden (not show))
>  	(goto-char new-start)
>  	(insert "[ " button-label (if show " ]" " (hidden) ]"))
>  	(let ((old-end (button-end button)))
>  	  (move-overlay button new-start (point))
>  	  (delete-region (point) old-end))
> -	(goto-char (min old-point (1- (button-end button))))))))
> +	(goto-char (min old-point (1- (button-end button))))
> +	(overlay-put overlay 'invisible (not show))))))
>  
>  (defun notmuch-show-multipart/*-to-list (part)
>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
> @@ -851,7 +854,7 @@ message at DEPTH in the current thread."
>        (setq handlers (cdr handlers))))
>    t)
>  
> -(defun notmuch-show-create-part-overlays (button beg end hide)
> +(defun notmuch-show-create-part-overlays (button beg end)
>    "Add an overlay to the part between BEG and END"
>  
>    ;; If there is no button (i.e., the part is text/plain and the first
> @@ -859,11 +862,9 @@ message at DEPTH in the current thread."
>    ;; 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)))))
> +    ;; Return true if we created an overlay.
> +    t))
> +

Ah, I see the mis-indentation was only temporary.

>  
>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>    "Insert the body part PART at depth DEPTH in the current thread.
> @@ -889,7 +890,9 @@ 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 button beg (point) hide)))
> +    (notmuch-show-create-part-overlays button beg (point))
> +    (when hide
> +      (notmuch-show-toggle-part-invisibility button))))

This lost the save-excursion around
notmuch-show-toggle-part-invisibility.  I'm surprised that works, given
how notmuch-show-toggle-part-invisibility moves point, but then again, I
can believe that the planets happen to align here because you're hiding
a part and that part happens to go to (point-max).  Might be worth a
comment or putting back the save-excursion just so people don't have to
think about it.

>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
> -- 
> 1.7.10.4