From: Mark Walters <markwalters1009@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3 8/9] emacs/mua: Insert part headers depending on the
	message
In-Reply-To: <1399897769-26809-9-git-send-email-dme@dme.org>
References: <87sixdujkv.fsf@qmul.ac.uk>
	<1399897769-26809-1-git-send-email-dme@dme.org>
	<1399897769-26809-9-git-send-email-dme@dme.org>
User-Agent: Notmuch/0.18.1+86~gef5e66a (http://notmuchmail.org) Emacs/24.4.1
	(x86_64-pc-linux-gnu)
Date: Mon, 19 Jan 2015 20:06:51 +0000
Message-ID: <87h9vmk010.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list


On Mon, 12 May 2014, David Edmondson <dme@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).
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@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