From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 2/3] emacs: show: move the insertion of the header
	button to the top level
In-Reply-To: <1369555061-21361-3-git-send-email-markwalters1009@gmail.com>
References: <1369555061-21361-1-git-send-email-markwalters1009@gmail.com>
	<1369555061-21361-3-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: Thu, 30 May 2013 18:30:59 -0400
Message-ID: <87a9nc2j8c.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sun, 26 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Previously each of the part insertion handlers inserted the part
> button themselves. Move this up into
> notmuch-show-insert-bodypart. Since a small number of the handlers
> modify the button (the encryption/signature ones) we need to pass the
> header button as an argument into the individual part insertion
> handlers.
>
> The patch is large but mostly simple. The only things of note are that
> we let the text/plain handler insert part buttons itself (as it does
> not always insert one and it applies motmuch-wash to the whole part
> including the button. Also this is by far the most important case so
> this reduces the risk of annoying side effects.
> ---
>  emacs/notmuch-show.el |   87 +++++++++++++++++++++++-------------------------
>  1 files changed, 42 insertions(+), 45 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 51bda31..591ad56 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -576,8 +576,7 @@ message at DEPTH in the current thread."
>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
>  	  (plist-get part :content)))
>  
> -(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type)
> -  (notmuch-show-insert-part-header nth declared-type content-type nil)
> +(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type button)
>    (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))
>  	(inner-parts (plist-get part :content))
>  	(start (point)))
> @@ -654,8 +653,7 @@ message at DEPTH in the current thread."
>  	  content-type)
>        nil)))
>  
> -(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type)
> -  (notmuch-show-insert-part-header nth declared-type content-type nil)
> +(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type button)
>    (let ((inner-parts (plist-get part :content))
>  	(start (point)))
>  
> @@ -674,16 +672,15 @@ message at DEPTH in the current thread."
>        (indent-rigidly start (point) 1)))
>    t)
>  
> -(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type)
> -  (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil)))
> -    (button-put button 'face 'notmuch-crypto-part-header)
> -    ;; add signature status button if sigstatus provided
> -    (if (plist-member part :sigstatus)
> -	(let* ((from (notmuch-show-get-header :From msg))
> -	       (sigstatus (car (plist-get part :sigstatus))))
> -	  (notmuch-crypto-insert-sigstatus-button sigstatus from))
> -      ;; if we're not adding sigstatus, tell the user how they can get it
> -      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))
> +(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type button)
> +  (button-put button 'face 'notmuch-crypto-part-header)
> +  ;; add signature status button if sigstatus provided
> +  (if (plist-member part :sigstatus)
> +      (let* ((from (notmuch-show-get-header :From msg))
> +	     (sigstatus (car (plist-get part :sigstatus))))
> +	(notmuch-crypto-insert-sigstatus-button sigstatus from))
> +    ;; if we're not adding sigstatus, tell the user how they can get it
> +    (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))
>  
>    (let ((inner-parts (plist-get part :content))
>  	(start (point)))
> @@ -696,20 +693,19 @@ message at DEPTH in the current thread."
>        (indent-rigidly start (point) 1)))
>    t)
>  
> -(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type)
> -  (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil)))
> -    (button-put button 'face 'notmuch-crypto-part-header)
> -    ;; add encryption status button if encstatus specified
> -    (if (plist-member part :encstatus)
> -	(let ((encstatus (car (plist-get part :encstatus))))
> -	  (notmuch-crypto-insert-encstatus-button encstatus)
> -	  ;; add signature status button if sigstatus specified
> -	  (if (plist-member part :sigstatus)
> -	      (let* ((from (notmuch-show-get-header :From msg))
> -		     (sigstatus (car (plist-get part :sigstatus))))
> -		(notmuch-crypto-insert-sigstatus-button sigstatus from))))
> -      ;; if we're not adding encstatus, tell the user how they can get it
> -      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))
> +(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type button)
> +  (button-put button 'face 'notmuch-crypto-part-header)
> +  ;; add encryption status button if encstatus specified
> +  (if (plist-member part :encstatus)
> +      (let ((encstatus (car (plist-get part :encstatus))))
> +	(notmuch-crypto-insert-encstatus-button encstatus)
> +	;; add signature status button if sigstatus specified
> +	(if (plist-member part :sigstatus)
> +	    (let* ((from (notmuch-show-get-header :From msg))
> +		   (sigstatus (car (plist-get part :sigstatus))))
> +	      (notmuch-crypto-insert-sigstatus-button sigstatus from))))
> +    ;; if we're not adding encstatus, tell the user how they can get it
> +    (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))
>  
>    (let ((inner-parts (plist-get part :content))
>  	(start (point)))
> @@ -722,8 +718,7 @@ message at DEPTH in the current thread."
>        (indent-rigidly start (point) 1)))
>    t)
>  
> -(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type)
> -  (notmuch-show-insert-part-header nth declared-type content-type nil)
> +(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type button)
>    (let ((inner-parts (plist-get part :content))
>  	(start (point)))
>      ;; Show all of the parts.
> @@ -735,8 +730,7 @@ message at DEPTH in the current thread."
>        (indent-rigidly start (point) 1)))
>    t)
>  
> -(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type)
> -  (notmuch-show-insert-part-header nth declared-type content-type nil)
> +(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type button)
>    (let* ((message (car (plist-get part :content)))
>  	 (body (car (plist-get message :body)))
>  	 (start (point)))
> @@ -757,7 +751,7 @@ message at DEPTH in the current thread."
>        (indent-rigidly start (point) 1)))
>    t)
>  
> -(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type)
> +(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type button)
>    (let ((start (point)))
>      ;; If this text/plain part is not the first part in the message,
>      ;; insert a header to make this clear.

I know you're trying to minimize side-effects, but I think this will
also complicate maintenance.  I'd rather see this call to
notmuch-show-insert-part-header removed and have the only call and all
special-case logic be in notmuch-show-insert-bodypart.  As it is,
reasoning about exactly when a part button is inserted requires
understanding the conjunction of two widely separated parts of the code.

> @@ -770,8 +764,7 @@ message at DEPTH in the current thread."
>  	(run-hook-with-args 'notmuch-show-insert-text/plain-hook msg depth))))
>    t)
>  
> -(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type)
> -  (notmuch-show-insert-part-header nth declared-type content-type (plist-get part :filename))
> +(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type button)
>    (insert (with-temp-buffer
>  	    (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))
>  	    ;; notmuch-get-bodypart-content provides "raw", non-converted
> @@ -794,8 +787,8 @@ message at DEPTH in the current thread."
>    t)
>  
>  ;; For backwards compatibility.
> -(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type)
> -  (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type))
> +(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type button)
> +  (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type button))
>  
>  (defun notmuch-show-get-mime-type-of-application/octet-stream (part)
>    ;; If we can deduce a MIME type from the filename of the attachment,
> @@ -813,7 +806,7 @@ message at DEPTH in the current thread."
>  		nil))
>  	  nil))))
>  
> -(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
> +(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type button)
>    ;; text/html handler to work around bugs in renderers and our
>    ;; invisibile parts code. In particular w3m sets up a keymap which
>    ;; "leaks" outside the invisible region and causes strange effects
> @@ -821,11 +814,10 @@ message at DEPTH in the current thread."
>    ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map
>    ;; remains).
>    (let ((mm-inline-text-html-with-w3m-keymap nil))
> -    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type)))
> +    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type button)))
>  
> -(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
> +(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type button)
>    ;; This handler _must_ succeed - it is the handler of last resort.
> -  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
>    (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto)
>    t)
>  
> @@ -848,13 +840,13 @@ message at DEPTH in the current thread."
>  
>  ;; 

>  
> -(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type)
> +(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type button)
>    (let ((handlers (notmuch-show-handlers-for content-type)))
>      ;; Run the content handlers until one of them returns a non-nil
>      ;; value.
>      (while (and handlers
>  		(not (condition-case err
> -			 (funcall (car handlers) msg part content-type nth depth declared-type)
> +			 (funcall (car handlers) msg part content-type nth depth declared-type button)
>  		       (error (progn
>  				(insert "!!! Bodypart insert error: ")
>  				(insert (error-message-string err))
> @@ -882,6 +874,9 @@ message at DEPTH in the current thread."
>    "Insert the body part PART at depth DEPTH in the current thread.
>  
>  If HIDE is non-nil then initially hide this part."
> +
> +  ;; We handle text/plain specially as its code does things before
> +  ;; inserting a part button (and does not always insert a part button).

I think this comment would be clearer right above the button binding (or
maybe the separate diff hunks just make it more confusing than it is).

>    (let* ((content-type (downcase (plist-get part :content-type)))
>  	 (mime-type (or (and (string= content-type "application/octet-stream")
>  			     (notmuch-show-get-mime-type-of-application/octet-stream part))
> @@ -889,9 +884,11 @@ If HIDE is non-nil then initially hide this part."
>  			     "text/x-diff")
>  			content-type))
>  	 (nth (plist-get part :id))
> -	 (beg (point)))
> +	 (beg (point))
> +	 (button (unless (string= mime-type "text/plain")
> +		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))

If you take my suggestion above, this would become something like

(unless (and (string= mime-type "text/plain) (<= nth 1)) ...)

or maybe clearer

(when (or (not (string= mime-type "text/plain")) (> nth 1)) ...)

>  
> -    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type)
> +    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type button)
>      ;; Some of the body part handlers leave point somewhere up in the
>      ;; part, so we make sure that we're down at the end.
>      (goto-char (point-max))
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch