[PATCH v3 8/9] emacs/mua: Insert part headers depending on the message

[David Edmondson]

On Mon, Jan 19 2015, Mark Walters <markwalters1009 at gmail.com> wrote:
> On Mon, 12 May 2014, David Edmondson <dme at dme.org> wrote:
>> Whether to insert part headers should depend on the details of the
>> message being cited.
>
> Hi
>
> Overall I like this series and it does fix two annoying bugs (not being
> able to reply to ref822 messages and (correctly) including parts
> which have application/octet-stream but are actually text parts).
>
> The one problem is getting the right choice for part headers in the
> reply text. I think getting this wrong will irritate users even if the
> overall result is better.
>
> My guess at the correct logic is:
> 1) omit the part header for any empty part: (ie a part we don't display
> such as a pdf file).

This seems wrong to me (i.e. it's not what I would want ;-).

Showing the part header in the reply acknowledges that it was part of
the message that I'm replying to.

Even more, consider a message:

  Hi, attached are the two PDF documents that we discussed.
  
  This is the version with Fred's suggested comments:

  [ application/pdf: document 1 ]

  This is the version with my proposed alternative edits and much more
  content added:

  [ application/pdf: document 2 ]

The part headers form a significant part of the content. In a reply I'd
like to see them, so that I can add comments appropriately. (I realise
that commenting 'in-line' is out of fashion in many places now, but for
more complex discussions I still prefer it.)

I still like the original rule that I proposed: the reply should include
whatever is in the 'show' buffer, modulo content that was elided due to
washing.

> 2) omit multipart/* part headers
> 3) include all other part headers
> 4) except omit the first part header (perhaps only in the case it is text/plain)
>
> My reasoning for each is
> 1) there is no point in saying we had a part which we are omitting.
> 2) all the subparts of multipart/* will get there own header which
> should be sufficient.
> 3) we want to keep the parts distinguished
> 4) except we don't need to do that with the first part.
>
> Note for 4) it would be good to have a multipart/alternative with
> subparts text/plain and text/html just give the text/plain with no part
> header.
>
> I include a patch below which does all of these apart from 4) as I
> couldn't see a clean way of implementing it. Any suggestions?
>
> It should apply on top of patch 6 or 7 instead of 8. The key change is
> that it always puts in a button and then deletes it if unwanted: this
> makes doing 1) above easy. 
>
> It does break some tests, nothing unexpected except an interaction with
> the way we wash text/plain parts: we remove leading blank lines from the
> first text/plain part (because it doesn't have a button) but not from
> subsequent ones (because they do). Because this code always has the
> second case it doesn't remove a leading blank line of the first part.
>
> Best wishes
>
> Mark
>
>
> From 8f198b38e76e050ae8d20d866748c41ccf79f3d4 Mon Sep 17 00:00:00 2001
> From: Mark Walters <markwalters1009 at gmail.com>
> Date: Mon, 19 Jan 2015 14:39:25 +0000
> Subject: [PATCH] emacs show/reply modify part handling
>
> Modify the part handling so that we always insert the button and
> delete it afterwards if not wanted. The advantage is that we can
> decide whether to keep the part button based on what the insertion
> code does. In particular the reply code can omit the button for all
> parts with no displayable content.
> ---
>  emacs/notmuch-mua.el  |  5 +++--
>  emacs/notmuch-show.el | 39 +++++++++++++++++++++++++--------------
>  2 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 0ca9eed..6060f33 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -29,6 +29,7 @@
>  (eval-when-compile (require 'cl))
>  
>  (declare-function notmuch-show-insert-body "notmuch-show" (msg body depth))
> +(declare-function notmuch-show-insert-header-p-reply "notmuch-show" (part empty-part))
>  
>  ;;
>  
> @@ -223,8 +224,8 @@ Note that these functions use `mail-citation-hook' if that is non-nil."
>  		      ;; citations, etc. in the original message before
>  		      ;; quoting.
>  		      ((notmuch-show-insert-text/plain-hook nil)
> -		       ;; Don't insert part buttons.
> -		       (notmuch-show-insert-header-p-function #'notmuch-show-insert-header-p-never))
> +		       ;; Insert part buttons appropriate for a reply.
> +		       (notmuch-show-insert-header-p-function #'notmuch-show-insert-header-p-reply))
>  		    (notmuch-show-insert-body original (plist-get original :body) 0)
>  		    (buffer-substring-no-properties (point-min) (point-max)))))
>  
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 4a0899f..2cdb5a8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -909,16 +909,24 @@ message at DEPTH in the current thread."
>  	     "text/x-diff")
>  	content-type)))
>  
> -(defun notmuch-show-insert-header-p-smart (part)
> +(defun notmuch-show-insert-header-p-smart (part empty-part)
>    "Return non-NIL if a header button should be inserted for this part."
>    (let ((mime-type (notmuch-show-mime-type part)))
>      (not (and (string= mime-type "text/plain")
>  	      (<= (plist-get part :id) 1)))))
>  
> -(defun notmuch-show-insert-header-p-always (part)
> +(defun notmuch-show-insert-header-p-reply (part empty-part)
> +  "Return non-NIL if a header button should be inserted for this part."
> +  (let ((mime-type (notmuch-show-mime-type part)))
> +    (not (or empty-part
> +	     (notmuch-match-content-type mime-type "multipart/*")
> +	     (and (string= mime-type "text/plain")
> +		  (<= (plist-get part :id) 1))))))
> +
> +(defun notmuch-show-insert-header-p-always (part empty-part)
>    t)
>  
> -(defun notmuch-show-insert-header-p-never (part)
> +(defun notmuch-show-insert-header-p-never (part empty-part)
>    nil)
>  
>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
> @@ -936,8 +944,8 @@ is t, hide the part initially and show the button."
>  	 (show-part (not (equal hide t)))
>  	 ;; We omit the part button for the first (or only) part if
>  	 ;; this is text/plain.
> -	 (button (when (funcall notmuch-show-insert-header-p-function part)
> -		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
> +	 (button-beg (point))
> +	 (button (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))
>  	 (content-beg (point)))
>  
>      ;; Store the computed mime-type for later use (e.g. by attachment handlers).
> @@ -952,15 +960,18 @@ is t, hide the part initially and show the 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))
> -    ;; Ensure that the part ends with a carriage return.
> -    (unless (bolp)
> -      (insert "\n"))
> -    ;; We do not create the overlay for hidden (lazy) parts until
> -    ;; they are inserted.
> -    (if show-part
> -	(notmuch-show-create-part-overlays button content-beg (point))
> -      (save-excursion
> -	(notmuch-show-toggle-part-invisibility button)))
> +    (let ((empty-part (equal (point) content-beg)))
> +      (if (not (funcall notmuch-show-insert-header-p-function part empty-part))
> +	  (delete-region button-beg content-beg)
> +	;; Ensure that the part ends with a carriage return.
> +	(unless (bolp)
> +	  (insert "\n"))
> +	;; We do not create the overlay for hidden (lazy) parts until
> +	;; they are inserted.
> +	(if show-part
> +	    (notmuch-show-create-part-overlays button content-beg (point))
> +	  (save-excursion
> +	    (notmuch-show-toggle-part-invisibility button)))))
>      (notmuch-show-record-part-information part beg (point))))
>  
>  (defun notmuch-show-insert-body (msg body depth)
> -- 
> 2.1.3