reply to messages with message/rfc822 parts

reply to messages with message/rfc822 parts

[Mark Walters]

Hello

I was trying to reply to a message I had forwarded to someone (to update
the information sent in the first message) and came across some strange
behaviour.


The initial forward was done using notmuch-emacs: this took the message
and sent it as a message/rfc822 mimetype complete message. Since I had
added some text at the top this meant the message as a whole was
multipart/mixed with my text/plain at the top and the message/rfc822
below.

Then I tried to reply to this message and the text/plain part was
included in the reply but the message/rfc822 part was not. In this case
the message/rfc822 just had headers and a text/plain subpart so I would
have expected it to be included.

I imagine we actually want to recurse into the message/rfc822 part
including relevant subparts. I tried tweaking notmuch-mua.el to do this
but, so far, I have failed. (I will have another look)

Best wishes

Mark

[RFC] [PATCH] emacs/notmuch-mua: Generate improved cited text for replies

[David Edmondson]

Use the message display code to generate message text to cite in
replies.
---

This breaks the tests, which know about the details of how the reply
buffer looks in emacs. I will fix that of course, if this approach is
considered acceptable.

The original implementation took a simplistic view of how a reply to a
complex message could be generated (essentially it tried to reduce it
to a flat list of text parts). Given that we already have code to
display more complex message structure, it seems obvious to use it.

 emacs/notmuch-mua.el | 34 ++++------------------------------
 1 file changed, 4 insertions(+), 30 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 95e4a4d..a8cff3d 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -28,7 +28,7 @@
 
 (eval-when-compile (require 'cl))
 
-(declare-function notmuch-show-insert-bodypart "notmuch-show" (msg part depth &optional hide))
+(declare-function notmuch-show-insert-body "notmuch-show" (msg body depth))
 
 ;;
 
@@ -123,31 +123,6 @@ list."
 	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
 	  do (notmuch-mua-reply-crypto (plist-get part :content))))
 
-(defun notmuch-mua-get-quotable-parts (parts)
-  (loop for part in parts
-	if (notmuch-match-content-type (plist-get part :content-type) "multipart/alternative")
-	  collect (let* ((subparts (plist-get part :content))
-			(types (mapcar (lambda (part) (plist-get part :content-type)) subparts))
-			(chosen-type (car (notmuch-multipart/alternative-choose types))))
-		   (loop for part in (reverse subparts)
-			 if (notmuch-match-content-type (plist-get part :content-type) chosen-type)
-			 return part))
-	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
-	  append (notmuch-mua-get-quotable-parts (plist-get part :content))
-	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
-	  collect part))
-
-(defun notmuch-mua-insert-quotable-part (message part)
-  ;; We don't want text properties leaking from the show renderer into
-  ;; the reply so we use a temp buffer. Also we don't want hooks, such
-  ;; as notmuch-wash-*, to be run on the quotable part so we set
-  ;; notmuch-show-insert-text/plain-hook to nil.
-  (insert (with-temp-buffer
-	    (let ((notmuch-show-insert-text/plain-hook nil))
-	      ;; Show the part but do not add buttons.
-	      (notmuch-show-insert-bodypart message part 0 'no-buttons))
-	    (buffer-substring-no-properties (point-min) (point-max)))))
-
 ;; There is a bug in emacs 23's message.el that results in a newline
 ;; not being inserted after the References header, so the next header
 ;; is concatenated to the end of it. This function fixes the problem,
@@ -225,10 +200,9 @@ list."
 	(insert "From: " from "\n")
 	(insert "Date: " date "\n\n")
 
-	;; Get the parts of the original message that should be quoted; this includes
-	;; all the text parts, except the non-preferred ones in a multipart/alternative.
-	(let ((quotable-parts (notmuch-mua-get-quotable-parts (plist-get original :body))))
-	  (mapc (apply-partially 'notmuch-mua-insert-quotable-part original) quotable-parts))
+	(insert (with-temp-buffer
+		  (notmuch-show-insert-body original (plist-get original :body) 0)
+		  (buffer-substring-no-properties (point-min) (point-max))))
 
 	(set-mark (point))
 	(goto-char start)
-- 
2.0.0.rc0

[PATCH v2] emacs: Improve the cited message included in replies

[David Edmondson]

emacs: Improve the cited message included in replies

v2:
- Don't run the text/plain hooks when generating the message to quote.


David Edmondson (1):
  emacs/notmuch-mua: Generate improved cited text for replies

 emacs/notmuch-mua.el | 38 ++++++++------------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

-- 
2.0.0.rc0

[PATCH v2] emacs/notmuch-mua: Generate improved cited text for replies

[David Edmondson]

Use the message display code to generate message text to cite in
replies.
---
 emacs/notmuch-mua.el | 38 ++++++++------------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 95e4a4d..09c922f 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -28,7 +28,7 @@
 
 (eval-when-compile (require 'cl))
 
-(declare-function notmuch-show-insert-bodypart "notmuch-show" (msg part depth &optional hide))
+(declare-function notmuch-show-insert-body "notmuch-show" (msg body depth))
 
 ;;
 
@@ -123,31 +123,6 @@ list."
 	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
 	  do (notmuch-mua-reply-crypto (plist-get part :content))))
 
-(defun notmuch-mua-get-quotable-parts (parts)
-  (loop for part in parts
-	if (notmuch-match-content-type (plist-get part :content-type) "multipart/alternative")
-	  collect (let* ((subparts (plist-get part :content))
-			(types (mapcar (lambda (part) (plist-get part :content-type)) subparts))
-			(chosen-type (car (notmuch-multipart/alternative-choose types))))
-		   (loop for part in (reverse subparts)
-			 if (notmuch-match-content-type (plist-get part :content-type) chosen-type)
-			 return part))
-	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
-	  append (notmuch-mua-get-quotable-parts (plist-get part :content))
-	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
-	  collect part))
-
-(defun notmuch-mua-insert-quotable-part (message part)
-  ;; We don't want text properties leaking from the show renderer into
-  ;; the reply so we use a temp buffer. Also we don't want hooks, such
-  ;; as notmuch-wash-*, to be run on the quotable part so we set
-  ;; notmuch-show-insert-text/plain-hook to nil.
-  (insert (with-temp-buffer
-	    (let ((notmuch-show-insert-text/plain-hook nil))
-	      ;; Show the part but do not add buttons.
-	      (notmuch-show-insert-bodypart message part 0 'no-buttons))
-	    (buffer-substring-no-properties (point-min) (point-max)))))
-
 ;; There is a bug in emacs 23's message.el that results in a newline
 ;; not being inserted after the References header, so the next header
 ;; is concatenated to the end of it. This function fixes the problem,
@@ -225,10 +200,13 @@ list."
 	(insert "From: " from "\n")
 	(insert "Date: " date "\n\n")
 
-	;; Get the parts of the original message that should be quoted; this includes
-	;; all the text parts, except the non-preferred ones in a multipart/alternative.
-	(let ((quotable-parts (notmuch-mua-get-quotable-parts (plist-get original :body))))
-	  (mapc (apply-partially 'notmuch-mua-insert-quotable-part original) quotable-parts))
+	(insert (with-temp-buffer
+		  ;; Don't attempt to clean up messages, excerpt
+		  ;; citations, etc. in the original message before
+		  ;; quoting.
+		  (let ((notmuch-show-insert-text/plain-hook nil))
+		    (notmuch-show-insert-body original (plist-get original :body) 0)
+		    (buffer-substring-no-properties (point-min) (point-max)))))
 
 	(set-mark (point))
 	(goto-char start)
-- 
2.0.0.rc0

Re: [PATCH v2] emacs: Improve the cited message included in replies

[Mark Walters]

On Thu, 08 May 2014, David Edmondson <dme@dme.org> wrote:
> emacs: Improve the cited message included in replies
>
> v2:
> - Don't run the text/plain hooks when generating the message to quote.
>

In principle I like this approach: keeping show and reply closely linked
seems good.

At the moment, as you say, the tests don't all pass. The first reason is
that this puts in buttons for the parts. Stopping that happening is not
completely trivial as we need to make sure that the instruction gets
passed down to sub-parts of multiparts etc. (You could argue that the
'no-button option to notmuch-show-insert-bodypart is buggy as it only
stops the top level button for the part)

Secondly, the existing code only includes text sub-parts of the
message. I would think your version might include any sub-parts show is
configured to display, including, say images. (However, in my testing
images didn't seem to be included: I am not sure why.)

I can't tell how much work it is to modify show to take account of these
things, so am not sure if this is the best approach, or just adding
something to deal with rfc822 to our existing reply code is easier.

Best wishes

Mark

Re: [PATCH v2] emacs: Improve the cited message included in replies

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 2940 bytes --]

On Sat, May 10 2014, Mark Walters wrote:
> On Thu, 08 May 2014, David Edmondson <dme@dme.org> wrote:
>> emacs: Improve the cited message included in replies
>>
>> v2:
>> - Don't run the text/plain hooks when generating the message to quote.
>>
>
> In principle I like this approach: keeping show and reply closely linked
> seems good.
>
> At the moment, as you say, the tests don't all pass. The first reason is
> that this puts in buttons for the parts. Stopping that happening is not
> completely trivial as we need to make sure that the instruction gets
> passed down to sub-parts of multiparts etc. (You could argue that the
> 'no-button option to notmuch-show-insert-bodypart is buggy as it only
> stops the top level button for the part)

That seems straightforward. I was sure that there was a variable listing
part types that didn't require buttons (which could be let-bound), but I
must have been imagining it.

Inserting the button text for the trivial case (single text/plain part)
is already special-cased in the show code. In the case where only a
single part is being shown (e.g. multipart/alternative with text/plain
and text/html with the text/html hidden) it would make sense _not_ to
show the button text. For more complex messages (e.g. multipart/mixed
with text/plain and message/rfc822, where the message/rfc822 contains
multiple parts), showing the button text seems useful to allow the
different sub-sections of the reply to be distinguished.

Perhaps there is an approach based on the complexity of the quoted
message that should determine whether the button text is inserted (which
might also apply (in modified form?) in normal message display)? Normal
message display has to allow for some interaction with the parts
(show/hide, etc.), which doesn't apply to citation.

> Secondly, the existing code only includes text sub-parts of the
> message. I would think your version might include any sub-parts show is
> configured to display, including, say images. (However, in my testing
> images didn't seem to be included: I am not sure why.)

Eek.

Inserting the images in the reply buffer itself would not be
difficult. What should happen when the user hits 'send'? We currently
don't have a composition mode that would allow us to generate useful
output in that case. Adding one feels like a lot of work. In many cases
it would be necessary to transform the message into HTML to properly
represent the content.

MML (the markup used in `message-mode') is really not designed for
something this complex.

> I can't tell how much work it is to modify show to take account of these
> things, so am not sure if this is the best approach, or just adding
> something to deal with rfc822 to our existing reply code is easier.

The approach in this patch mostly involves removing code - adding
special case code to notmuch-mua.el to support message/rfc822 involves
_adding_ a bunch more complex code (I tried that first).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH v2] emacs: Improve the cited message included in replies

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> On Sat, May 10 2014, Mark Walters wrote:
>> On Thu, 08 May 2014, David Edmondson <dme@dme.org> wrote:
>>> emacs: Improve the cited message included in replies
>>>
>>> v2:
>>> - Don't run the text/plain hooks when generating the message to quote.
>>>
>>
>> In principle I like this approach: keeping show and reply closely linked
>> seems good.
>>
>> At the moment, as you say, the tests don't all pass. The first reason is
>> that this puts in buttons for the parts. Stopping that happening is not
>> completely trivial as we need to make sure that the instruction gets
>> passed down to sub-parts of multiparts etc. (You could argue that the
>> 'no-button option to notmuch-show-insert-bodypart is buggy as it only
>> stops the top level button for the part)
>
> That seems straightforward. I was sure that there was a variable listing
> part types that didn't require buttons (which could be let-bound), but I
> must have been imagining it.
>
> Inserting the button text for the trivial case (single text/plain part)
> is already special-cased in the show code. In the case where only a
> single part is being shown (e.g. multipart/alternative with text/plain
> and text/html with the text/html hidden) it would make sense _not_ to
> show the button text. For more complex messages (e.g. multipart/mixed
> with text/plain and message/rfc822, where the message/rfc822 contains
> multiple parts), showing the button text seems useful to allow the
> different sub-sections of the reply to be distinguished.

Yes that is true: I hadn't thought of that.

> Perhaps there is an approach based on the complexity of the quoted
> message that should determine whether the button text is inserted (which
> might also apply (in modified form?) in normal message display)? Normal
> message display has to allow for some interaction with the parts
> (show/hide, etc.), which doesn't apply to citation.
>
>> Secondly, the existing code only includes text sub-parts of the
>> message. I would think your version might include any sub-parts show is
>> configured to display, including, say images. (However, in my testing
>> images didn't seem to be included: I am not sure why.)
>
> Eek.

I phrased this badly. I don't want images included: I just couldn't see
why they weren't with your current patch (as they are displayed in show
mode)

>
> Inserting the images in the reply buffer itself would not be
> difficult. What should happen when the user hits 'send'? We currently
> don't have a composition mode that would allow us to generate useful
> output in that case. Adding one feels like a lot of work. In many cases
> it would be necessary to transform the message into HTML to properly
> represent the content.
>
> MML (the markup used in `message-mode') is really not designed for
> something this complex.
>
>> I can't tell how much work it is to modify show to take account of these
>> things, so am not sure if this is the best approach, or just adding
>> something to deal with rfc822 to our existing reply code is easier.
>
> The approach in this patch mostly involves removing code - adding
> special case code to notmuch-mua.el to support message/rfc822 involves
> _adding_ a bunch more complex code (I tried that first).

I guess my concern with adding to the show code is that is already more
complicated than I would like (invisibility, hidden parts, lazy parts
etc). But I am very definitely interested in seeing what a more finished
version of your patch would look like.

Best wishes

Mark

Re: [PATCH v2] emacs: Improve the cited message included in replies

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Mon, May 12 2014, Mark Walters wrote:
>>> Secondly, the existing code only includes text sub-parts of the
>>> message. I would think your version might include any sub-parts show is
>>> configured to display, including, say images. (However, in my testing
>>> images didn't seem to be included: I am not sure why.)
>>
>> Eek.
>
> I phrased this badly. I don't want images included: I just couldn't see
> why they weren't with your current patch (as they are displayed in show
> mode)

`buffer-substring-no-properties' grabs the text of the rendered buffer
without any properties, and images are inserted using properties.

You could replace it with `buffer-substring' to see the images in the
reply buffer (but see previous comments about what happens when you
send).

> I guess my concern with adding to the show code is that is already
> more complicated than I would like (invisibility, hidden parts, lazy
> parts etc).

One complex piece of code that knows how to iterate over messages
display the parts seems enough - why have two?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

[PATCH v3 0/9] emacs: Improve the cited message included in replies

[David Edmondson]

emacs: Improve the cited message included in replies

I tried to do things in small increments to make it easier to review.

v2:
- Don't run the text/plain hooks when generating the message to quote.

v3:
- Remove the 'no-button code, as it's no longer used.
- Control the insertion of part headers using a function.
- Fix the tests.


David Edmondson (9):
  emacs/show: Re-arrange determination if a part header is necessary
  emacs/show: Allow the user to decide when part headers should be
    inserted
  emacs/show: Accommodate the lack of part header buttons
  emacs/mua: Generate improved cited text for replies
  emacs/show: Remove the 'no-buttons option of
    `notmuch-show-insert-bodypart'
  emacs/mua: Don't insert part headers in citations
  test: Update the test output to accord with the reply changes
  emacs/mua: Insert part headers depending on the message
  test: Update the test output to accord with more reply changes

 emacs/notmuch-mua.el  |  82 +++++++++++++++++++-----------
 emacs/notmuch-show.el | 135 +++++++++++++++++++++++++++++++-------------------
 test/T310-emacs.sh    |  44 ++++++++++++++++
 3 files changed, 180 insertions(+), 81 deletions(-)

-- 
2.0.0.rc0

[PATCH v3 1/9] emacs/show: Re-arrange determination if a part header is necessary

[David Edmondson]

Move the determination of whether a part header is required to a
distinct function.
---
 emacs/notmuch-show.el | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 10fc872..ec99141 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -871,6 +871,21 @@ message at DEPTH in the current thread."
       ;; showable this returns nil.
       (notmuch-show-create-part-overlays button part-beg part-end))))
 
+(defun notmuch-show-mime-type (part)
+  "Return the correct mime-type to use for PART."
+  (let ((content-type (downcase (plist-get part :content-type))))
+    (or (and (string= content-type "application/octet-stream")
+	     (notmuch-show-get-mime-type-of-application/octet-stream part))
+	(and (string= content-type "inline patch")
+	     "text/x-diff")
+	content-type)))
+
+(defun notmuch-show-insert-header-p (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-bodypart (msg part depth &optional hide)
   "Insert the body part PART at depth DEPTH in the current thread.
 
@@ -881,19 +896,15 @@ is t, hide the part initially and show the button. If HIDE is
 useful for quoting in replies)."
 
   (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))
-			(and (string= content-type "inline patch")
-			     "text/x-diff")
-			content-type))
+	 (mime-type (notmuch-show-mime-type part))
 	 (nth (plist-get part :id))
 	 (beg (point))
 	 ;; Hide the part initially if HIDE is t.
 	 (show-part (not (equal hide t)))
 	 ;; We omit the part button for the first (or only) part if
 	 ;; this is text/plain, or HIDE is 'no-buttons.
-	 (button (unless (or (equal hide 'no-buttons)
-			     (and (string= mime-type "text/plain") (<= nth 1)))
+	 (button (when (and (not (equal hide 'no-buttons))
+			    (notmuch-show-insert-header-p part))
 		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
 	 (content-beg (point)))
 
-- 
2.0.0.rc0

[PATCH v3 2/9] emacs/show: Allow the user to decide when part headers should be inserted

[David Edmondson]

Make the function that determines whether a part header should be
inserted a user controlled, with some example functions.
---
 emacs/notmuch-show.el | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index ec99141..f78a0ab 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -135,6 +135,17 @@ indentation."
   :type 'boolean
   :group 'notmuch-show)
 
+(defcustom notmuch-show-insert-header-p-function 'notmuch-show-insert-header-p-smart
+  "A function to call to determine whether a MIME part should have a header button.
+
+The function is passed one argument, PART - the MIME part in
+question."
+  :type 'function
+  :group 'notmuch-show
+  :options '(notmuch-show-insert-header-p-smart
+	     notmuch-show-insert-header-p-always
+	     notmuch-show-insert-header-p-never))
+
 (defvar notmuch-show-thread-id nil)
 (make-variable-buffer-local 'notmuch-show-thread-id)
 (put 'notmuch-show-thread-id 'permanent-local t)
@@ -880,12 +891,18 @@ message at DEPTH in the current thread."
 	     "text/x-diff")
 	content-type)))
 
-(defun notmuch-show-insert-header-p (part)
+(defun notmuch-show-insert-header-p-smart (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)
+  t)
+
+(defun notmuch-show-insert-header-p-never (part)
+  nil)
+
 (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
   "Insert the body part PART at depth DEPTH in the current thread.
 
@@ -904,7 +921,7 @@ useful for quoting in replies)."
 	 ;; We omit the part button for the first (or only) part if
 	 ;; this is text/plain, or HIDE is 'no-buttons.
 	 (button (when (and (not (equal hide 'no-buttons))
-			    (notmuch-show-insert-header-p part))
+			    (funcall notmuch-show-insert-header-p-function part))
 		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
 	 (content-beg (point)))
 
-- 
2.0.0.rc0

[PATCH v3 3/9] emacs/show: Accommodate the lack of part header buttons

[David Edmondson]

Various pieces of code assumed (reasonably) that part header buttons
are present. Modify them to avoid problems if no part headers were
inserted.
---
 emacs/notmuch-show.el | 88 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 40 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f78a0ab..e511655 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -494,36 +494,37 @@ message at DEPTH in the current thread."
 
 (defun notmuch-show-toggle-part-invisibility (&optional button)
   (interactive)
-  (let* ((button (or button (button-at (point))))
-	 (overlay (button-get button 'overlay))
-	 (lazy-part (button-get button :notmuch-lazy-part)))
-    ;; We have a part to toggle if there is an overlay or if there is a lazy part.
-    ;; If neither is present we cannot toggle the part so we just return nil.
-    (when (or overlay lazy-part)
-      (let* ((show (button-get button :notmuch-part-hidden))
-	     (new-start (button-start button))
-	     (button-label (button-get button :base-label))
-	     (old-point (point))
-	     (properties (text-properties-at (button-start button)))
-	     (inhibit-read-only t))
-	;; Toggle the button itself.
-	(button-put button :notmuch-part-hidden (not show))
-	(goto-char new-start)
-	(insert "[ " button-label (if show " ]" " (hidden) ]"))
-	(set-text-properties new-start (point) properties)
-	(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))))
-	;; Return nil if there is a lazy-part, it is empty, and we are
-	;; trying to show it.  In all other cases return t.
-	(if lazy-part
-	    (when show
-	      (button-put button :notmuch-lazy-part nil)
-	      (notmuch-show-lazy-part lazy-part button))
-	  ;; else there must be an overlay.
-	  (overlay-put overlay 'invisible (not show))
-	  t)))))
+  (let ((button (or button (button-at (point)))))
+    (when button
+      (let ((overlay (button-get button 'overlay))
+	    (lazy-part (button-get button :notmuch-lazy-part)))
+	;; We have a part to toggle if there is an overlay or if there is a lazy part.
+	;; If neither is present we cannot toggle the part so we just return nil.
+	(when (or overlay lazy-part)
+	  (let* ((show (button-get button :notmuch-part-hidden))
+		 (new-start (button-start button))
+		 (button-label (button-get button :base-label))
+		 (old-point (point))
+		 (properties (text-properties-at (button-start button)))
+		 (inhibit-read-only t))
+	    ;; Toggle the button itself.
+	    (button-put button :notmuch-part-hidden (not show))
+	    (goto-char new-start)
+	    (insert "[ " button-label (if show " ]" " (hidden) ]"))
+	    (set-text-properties new-start (point) properties)
+	    (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))))
+	    ;; Return nil if there is a lazy-part, it is empty, and we are
+	    ;; trying to show it.  In all other cases return t.
+	    (if lazy-part
+		(when show
+		  (button-put button :notmuch-lazy-part nil)
+		  (notmuch-show-lazy-part lazy-part button))
+	      ;; else there must be an overlay.
+	      (overlay-put overlay 'invisible (not show))
+	      t)))))))
 
 ;; MIME part renderers
 
@@ -632,14 +633,17 @@ message at DEPTH in the current thread."
   t)
 
 (defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth button)
-  (button-put button 'face 'notmuch-crypto-part-header)
-  ;; add signature status button if sigstatus provided
+  (when 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."))
+    ;; If we're not adding the signature status, tell the user how
+    ;; they can get it.
+    (when button
+      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))
 
   (let ((inner-parts (plist-get part :content))
 	(start (point)))
@@ -653,17 +657,20 @@ message at DEPTH in the current thread."
   t)
 
 (defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth button)
-  (button-put button 'face 'notmuch-crypto-part-header)
-  ;; add encryption status button if encstatus specified
+  (when button
+    (button-put button 'face 'notmuch-crypto-part-header))
+  ;; Add encryption status button if encryption status is 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
+	;; Add signature status button if signature status is
+	;; 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
+    ;; If we're not adding the encryption status, 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))
@@ -930,8 +937,9 @@ useful for quoting in replies)."
 
     (if show-part
         (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
-      (button-put button :notmuch-lazy-part
-                  (list msg part mime-type nth depth button)))
+      (when button
+	(button-put button :notmuch-lazy-part
+		    (list msg part mime-type nth depth 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.
-- 
2.0.0.rc0

[PATCH v3 4/9] emacs/mua: Generate improved cited text for replies

[David Edmondson]

Use the message display code to generate message text to cite in
replies.
---
 emacs/notmuch-mua.el | 38 ++++++++------------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 95e4a4d..09c922f 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -28,7 +28,7 @@
 
 (eval-when-compile (require 'cl))
 
-(declare-function notmuch-show-insert-bodypart "notmuch-show" (msg part depth &optional hide))
+(declare-function notmuch-show-insert-body "notmuch-show" (msg body depth))
 
 ;;
 
@@ -123,31 +123,6 @@ list."
 	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
 	  do (notmuch-mua-reply-crypto (plist-get part :content))))
 
-(defun notmuch-mua-get-quotable-parts (parts)
-  (loop for part in parts
-	if (notmuch-match-content-type (plist-get part :content-type) "multipart/alternative")
-	  collect (let* ((subparts (plist-get part :content))
-			(types (mapcar (lambda (part) (plist-get part :content-type)) subparts))
-			(chosen-type (car (notmuch-multipart/alternative-choose types))))
-		   (loop for part in (reverse subparts)
-			 if (notmuch-match-content-type (plist-get part :content-type) chosen-type)
-			 return part))
-	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
-	  append (notmuch-mua-get-quotable-parts (plist-get part :content))
-	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
-	  collect part))
-
-(defun notmuch-mua-insert-quotable-part (message part)
-  ;; We don't want text properties leaking from the show renderer into
-  ;; the reply so we use a temp buffer. Also we don't want hooks, such
-  ;; as notmuch-wash-*, to be run on the quotable part so we set
-  ;; notmuch-show-insert-text/plain-hook to nil.
-  (insert (with-temp-buffer
-	    (let ((notmuch-show-insert-text/plain-hook nil))
-	      ;; Show the part but do not add buttons.
-	      (notmuch-show-insert-bodypart message part 0 'no-buttons))
-	    (buffer-substring-no-properties (point-min) (point-max)))))
-
 ;; There is a bug in emacs 23's message.el that results in a newline
 ;; not being inserted after the References header, so the next header
 ;; is concatenated to the end of it. This function fixes the problem,
@@ -225,10 +200,13 @@ list."
 	(insert "From: " from "\n")
 	(insert "Date: " date "\n\n")
 
-	;; Get the parts of the original message that should be quoted; this includes
-	;; all the text parts, except the non-preferred ones in a multipart/alternative.
-	(let ((quotable-parts (notmuch-mua-get-quotable-parts (plist-get original :body))))
-	  (mapc (apply-partially 'notmuch-mua-insert-quotable-part original) quotable-parts))
+	(insert (with-temp-buffer
+		  ;; Don't attempt to clean up messages, excerpt
+		  ;; citations, etc. in the original message before
+		  ;; quoting.
+		  (let ((notmuch-show-insert-text/plain-hook nil))
+		    (notmuch-show-insert-body original (plist-get original :body) 0)
+		    (buffer-substring-no-properties (point-min) (point-max)))))
 
 	(set-mark (point))
 	(goto-char start)
-- 
2.0.0.rc0

[PATCH v3 5/9] emacs/show: Remove the 'no-buttons option of `notmuch-show-insert-bodypart'

[David Edmondson]

No code uses the 'no-buttons argument to
`notmuch-show-insert-bodypart', so remove it.
---
 emacs/notmuch-show.el | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e511655..981b922 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -915,9 +915,7 @@ message at DEPTH in the current thread."
 
 HIDE determines whether to show or hide the part and the button
 as follows: If HIDE is nil, show the part and the button. If HIDE
-is t, hide the part initially and show the button. If HIDE is
-'no-buttons, show the part but do not add any buttons (this is
-useful for quoting in replies)."
+is t, hide the part initially and show the button."
 
   (let* ((content-type (downcase (plist-get part :content-type)))
 	 (mime-type (notmuch-show-mime-type part))
@@ -926,9 +924,8 @@ useful for quoting in replies)."
 	 ;; Hide the part initially if HIDE is t.
 	 (show-part (not (equal hide t)))
 	 ;; We omit the part button for the first (or only) part if
-	 ;; this is text/plain, or HIDE is 'no-buttons.
-	 (button (when (and (not (equal hide 'no-buttons))
-			    (funcall notmuch-show-insert-header-p-function part))
+	 ;; 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))))
 	 (content-beg (point)))
 
-- 
2.0.0.rc0

[PATCH v3 6/9] emacs/mua: Don't insert part headers in citations

[David Edmondson]

Avoid inserting part headers when rendering a message body for
citation purposes.
---
 emacs/notmuch-mua.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 09c922f..c800c38 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -201,10 +201,13 @@ list."
 	(insert "Date: " date "\n\n")
 
 	(insert (with-temp-buffer
-		  ;; Don't attempt to clean up messages, excerpt
-		  ;; citations, etc. in the original message before
-		  ;; quoting.
-		  (let ((notmuch-show-insert-text/plain-hook nil))
+		  (let
+		      ;; Don't attempt to clean up messages, excerpt
+		      ;; 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))
 		    (notmuch-show-insert-body original (plist-get original :body) 0)
 		    (buffer-substring-no-properties (point-min) (point-max)))))
 
-- 
2.0.0.rc0

[PATCH v3 7/9] test: Update the test output to accord with the reply changes

[David Edmondson]

Replying to a message with multiple parts will now typically include
content from several parts (whereas previously only the first part was
used). Update the expected output from the emacs reply tests
accordingly.
---
 test/T310-emacs.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
index ac966e5..95bb67e 100755
--- a/test/T310-emacs.sh
+++ b/test/T310-emacs.sh
@@ -473,6 +473,38 @@ Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 > and http://mail-index.netbsd.org/pkgsrc-bugs/2006/06/07/msg016808.htmlspecifically
 > uses 64 as the
 > buffer size.
+> From e3bc4bbd7b9d0d086816ab5f8f2d6ffea1dd3ea4 Mon Sep 17 00:00:00 2001
+> From: Alexander Botero-Lowry <alex.boterolowry@gmail.com>
+> Date: Tue, 17 Nov 2009 11:30:39 -0800
+> Subject: [PATCH] Deal with situation where sysconf(_SC_GETPW_R_SIZE_MAX) returns -1
+>
+> ---
+>  notmuch-config.c |    2 ++
+>  1 files changed, 2 insertions(+), 0 deletions(-)
+>
+> diff --git a/notmuch-config.c b/notmuch-config.c
+> index 248149c..e7220d8 100644
+> --- a/notmuch-config.c
+> +++ b/notmuch-config.c
+> @@ -77,6 +77,7 @@ static char *
+>  get_name_from_passwd_file (void *ctx)
+>  {
+>      long pw_buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
+> +    if (pw_buf_size == -1) pw_buf_size = 64;
+>      char *pw_buf = talloc_zero_size (ctx, pw_buf_size);
+>      struct passwd passwd, *ignored;
+>      char *name;
+> @@ -101,6 +102,7 @@ static char *
+>  get_username_from_passwd_file (void *ctx)
+>  {
+>      long pw_buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
+> +    if (pw_buf_size == -1) pw_buf_size = 64;
+>      char *pw_buf = talloc_zero_size (ctx, pw_buf_size);
+>      struct passwd passwd, *ignored;
+>      char *name;
+> -- 
+> 1.6.5.2
+>
 > _______________________________________________
 > notmuch mailing list
 > notmuch@notmuchmail.org
-- 
2.0.0.rc0

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

[David Edmondson]

Whether to insert part headers should depend on the details of the
message being cited.
---
 emacs/notmuch-mua.el | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index c800c38..239cc1a 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -132,6 +132,47 @@ list."
   (funcall original-func header references)
   (unless (bolp) (insert "\n")))
 
+(defun notmuch-mua-reply-determine-part-function (message)
+  "Determine the part header rendering function to use when
+citing MESSAGE."
+
+  (let* ((body (plist-get message :body))
+	 (first-part (car body)))
+    (cond
+     ;; If there are multiple top-level parts, we need the
+     ;; headers. Will this ever happen?
+     ((> (length body) 1)
+      #'notmuch-show-insert-header-p-always)
+     
+     ;; If the type of the part is multipart/mixed, we need to see the
+     ;; part headers.
+     ((notmuch-match-content-type (plist-get first-part :content-type) "multipart/mixed")
+      #'notmuch-show-insert-header-p-always)
+
+     ;; If it is multipart/alternative the renderer will choose a
+     ;; default part and render it. There's no need to show the
+     ;; alternative part buttons, as they are not active.
+     ((notmuch-match-content-type (plist-get first-part :content-type) "multipart/alternative")
+      #'notmuch-show-insert-header-p-never)
+
+     ;; If it's a multipart/signed with a single text/* part and a
+     ;; signature, we don't need to see the headers.
+     ((let ((inner-content (plist-get first-part :content)))
+	(and (notmuch-match-content-type (plist-get first-part :content-type) "multipart/signed")
+	     (eq (length inner-content) 2)
+	     (notmuch-match-content-type (plist-get (nth 0 inner-content) :content-type) "text/*")
+	     (notmuch-match-content-type (plist-get (nth 1 inner-content) :content-type) "application/pgp-signature")))
+      #'notmuch-show-insert-header-p-never)
+
+     ;; If the type of the part is text/*, we don't need to see the
+     ;; part headers.
+     ((notmuch-match-content-type (plist-get first-part :content-type) "text/*")
+      #'notmuch-show-insert-header-p-never)
+     
+     ;; Otherwise insert the part headers.
+     (t
+      #'notmuch-show-insert-header-p-always))))
+
 (defun notmuch-mua-reply (query-string &optional sender reply-all)
   (let ((args '("reply" "--format=sexp" "--format-version=1"))
 	(process-crypto notmuch-show-process-crypto)
@@ -206,8 +247,8 @@ list."
 		      ;; 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))
+		       ;; Determine how to insert part headers.
+		       (notmuch-show-insert-header-p-function (notmuch-mua-reply-determine-part-function original)))
 		    (notmuch-show-insert-body original (plist-get original :body) 0)
 		    (buffer-substring-no-properties (point-min) (point-max)))))
 
-- 
2.0.0.rc0

[PATCH v3 9/9] test: Update the test output to accord with more reply changes

[David Edmondson]

When replying to complex messages, some part headers may be
shown. Update the expected test output accordingly.
---
 test/T310-emacs.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
index 95bb67e..fa15e03 100755
--- a/test/T310-emacs.sh
+++ b/test/T310-emacs.sh
@@ -394,6 +394,10 @@ User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Adrian Perez de Castro <aperez@igalia.com> writes:
 
+> [ multipart/mixed ]
+> [ multipart/signed ]
+> [ text/plain ]
+>
 > Hello to all,
 >
 > I have just heard about Not Much today in some random Linux-related news
@@ -433,6 +437,8 @@ Adrian Perez de Castro <aperez@igalia.com> writes:
 > -- 
 > Adrian Perez de Castro <aperez@igalia.com>
 > Igalia - Free Software Engineering
+> [ signature.asc: application/pgp-signature ]
+> [ text/plain ]
 > _______________________________________________
 > notmuch mailing list
 > notmuch@notmuchmail.org
@@ -457,6 +463,9 @@ User-Agent: Notmuch/XXX Emacs/XXX
 --text follows this line--
 Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 
+> [ multipart/mixed ]
+> [ multipart/alternative ]
+> [ text/plain ]
 > I saw the announcement this morning, and was very excited, as I had been
 > hoping sup would be turned into a library,
 > since I like the concept more than the UI (I'd rather an emacs interface).
@@ -473,6 +482,8 @@ Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 > and http://mail-index.netbsd.org/pkgsrc-bugs/2006/06/07/msg016808.htmlspecifically
 > uses 64 as the
 > buffer size.
+> [ text/html (hidden) ]
+> [ 0001-Deal-with-situation-where-sysconf-_SC_GETPW_R_SIZE_M.patch: application/octet-stream (as text/x-diff) ]
 > From e3bc4bbd7b9d0d086816ab5f8f2d6ffea1dd3ea4 Mon Sep 17 00:00:00 2001
 > From: Alexander Botero-Lowry <alex.boterolowry@gmail.com>
 > Date: Tue, 17 Nov 2009 11:30:39 -0800
@@ -505,6 +516,7 @@ Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
 > -- 
 > 1.6.5.2
 >
+> [ text/plain ]
 > _______________________________________________
 > notmuch mailing list
 > notmuch@notmuchmail.org
-- 
2.0.0.rc0

Re: [PATCH v3 0/9] emacs: Improve the cited message included in replies

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> emacs: Improve the cited message included in replies
>
> I tried to do things in small increments to make it easier to review.
>
> v2:
> - Don't run the text/plain hooks when generating the message to quote.
>
> v3:
> - Remove the 'no-button code, as it's no longer used.
> - Control the insertion of part headers using a function.
> - Fix the tests.

I think I broadly like this series. I haven't thought through all the
ramifications yet so this is just some first thoughts. I will also send
some comments on individual patches.

In notmuch-show we go to notmuch-show-insert-part-*/* to
notmuch-mm-display-part-inline and then leave the decision to inline to
mm-inlined-types. I think this means that, by default, we will not
inline signatures amongst other things.

So at the least I think we should decide whether we want to override
mm-inlined-types. Alternatively, and in my view preferably, we could
have a function or variable of our own which says which parts to
include. Indeed, if do it with a function we might be able to make an
option to reply to mean "include parts currently shown in the
notmuch-show buffer" which might be nice.

There is a related question and possible bug that we might be able to
do something about at the same time: should we include text parts in the
reply if they have content-disposition attachment? I have been caught
about by this on one occasion replying to a message with a 500K log file
attached (and notmuch-show/wash becomes very slow with a 500K message!)

Finally, I am not sure whether I like having buttons in the reply. My
instinct is against, but they do add context.

Best wishes

Mark




>
>
> David Edmondson (9):
>   emacs/show: Re-arrange determination if a part header is necessary
>   emacs/show: Allow the user to decide when part headers should be
>     inserted
>   emacs/show: Accommodate the lack of part header buttons
>   emacs/mua: Generate improved cited text for replies
>   emacs/show: Remove the 'no-buttons option of
>     `notmuch-show-insert-bodypart'
>   emacs/mua: Don't insert part headers in citations
>   test: Update the test output to accord with the reply changes
>   emacs/mua: Insert part headers depending on the message
>   test: Update the test output to accord with more reply changes
>
>  emacs/notmuch-mua.el  |  82 +++++++++++++++++++-----------
>  emacs/notmuch-show.el | 135 +++++++++++++++++++++++++++++++-------------------
>  test/T310-emacs.sh    |  44 ++++++++++++++++
>  3 files changed, 180 insertions(+), 81 deletions(-)
>
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 1/9] emacs/show: Re-arrange determination if a part header is necessary

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> Move the determination of whether a part header is required to a
> distinct function.
> ---
>  emacs/notmuch-show.el | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 10fc872..ec99141 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -871,6 +871,21 @@ message at DEPTH in the current thread."
>        ;; showable this returns nil.
>        (notmuch-show-create-part-overlays button part-beg part-end))))
>  
> +(defun notmuch-show-mime-type (part)
> +  "Return the correct mime-type to use for PART."
> +  (let ((content-type (downcase (plist-get part :content-type))))
> +    (or (and (string= content-type "application/octet-stream")
> +	     (notmuch-show-get-mime-type-of-application/octet-stream part))
> +	(and (string= content-type "inline patch")
> +	     "text/x-diff")
> +	content-type)))
> +
> +(defun notmuch-show-insert-header-p (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)))))

My only query here is whether a notmuch-show-hide-header-p (part) might
make some of the logic clearer?

MW


> +
>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>    "Insert the body part PART at depth DEPTH in the current thread.
>  
> @@ -881,19 +896,15 @@ is t, hide the part initially and show the button. If HIDE is
>  useful for quoting in replies)."
>  
>    (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))
> -			(and (string= content-type "inline patch")
> -			     "text/x-diff")
> -			content-type))
> +	 (mime-type (notmuch-show-mime-type part))
>  	 (nth (plist-get part :id))
>  	 (beg (point))
>  	 ;; Hide the part initially if HIDE is t.
>  	 (show-part (not (equal hide t)))
>  	 ;; We omit the part button for the first (or only) part if
>  	 ;; this is text/plain, or HIDE is 'no-buttons.
> -	 (button (unless (or (equal hide 'no-buttons)
> -			     (and (string= mime-type "text/plain") (<= nth 1)))
> +	 (button (when (and (not (equal hide 'no-buttons))
> +			    (notmuch-show-insert-header-p part))
>  		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>  	 (content-beg (point)))
>  
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 2/9] emacs/show: Allow the user to decide when part headers should be inserted

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> Make the function that determines whether a part header should be
> inserted a user controlled, with some example functions.
> ---
>  emacs/notmuch-show.el | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index ec99141..f78a0ab 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -135,6 +135,17 @@ indentation."
>    :type 'boolean
>    :group 'notmuch-show)
>  
> +(defcustom notmuch-show-insert-header-p-function 'notmuch-show-insert-header-p-smart
> +  "A function to call to determine whether a MIME part should have a header button.
> +
> +The function is passed one argument, PART - the MIME part in
> +question."
> +  :type 'function
> +  :group 'notmuch-show
> +  :options '(notmuch-show-insert-header-p-smart
> +	     notmuch-show-insert-header-p-always
> +	     notmuch-show-insert-header-p-never))
> +

I think this is nice as a defcustom for testing/seeing what happens, but I am
not sure we want it exposed longer term: for example never makes it
nearly impossible to view pdf parts.

Best wishes

Mark



>  (defvar notmuch-show-thread-id nil)
>  (make-variable-buffer-local 'notmuch-show-thread-id)
>  (put 'notmuch-show-thread-id 'permanent-local t)
> @@ -880,12 +891,18 @@ message at DEPTH in the current thread."
>  	     "text/x-diff")
>  	content-type)))
>  
> -(defun notmuch-show-insert-header-p (part)
> +(defun notmuch-show-insert-header-p-smart (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)
> +  t)
> +
> +(defun notmuch-show-insert-header-p-never (part)
> +  nil)
> +
>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>    "Insert the body part PART at depth DEPTH in the current thread.
>  
> @@ -904,7 +921,7 @@ useful for quoting in replies)."
>  	 ;; We omit the part button for the first (or only) part if
>  	 ;; this is text/plain, or HIDE is 'no-buttons.
>  	 (button (when (and (not (equal hide 'no-buttons))
> -			    (notmuch-show-insert-header-p part))
> +			    (funcall notmuch-show-insert-header-p-function part))
>  		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>  	 (content-beg (point)))
>  
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 3/9] emacs/show: Accommodate the lack of part header buttons

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> Various pieces of code assumed (reasonably) that part header buttons
> are present. Modify them to avoid problems if no part headers were
> inserted.

Despite your reservations on irc this looks fine to me.

MW

> ---
>  emacs/notmuch-show.el | 88 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 48 insertions(+), 40 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index f78a0ab..e511655 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -494,36 +494,37 @@ message at DEPTH in the current thread."
>  
>  (defun notmuch-show-toggle-part-invisibility (&optional button)
>    (interactive)
> -  (let* ((button (or button (button-at (point))))
> -	 (overlay (button-get button 'overlay))
> -	 (lazy-part (button-get button :notmuch-lazy-part)))
> -    ;; We have a part to toggle if there is an overlay or if there is a lazy part.
> -    ;; If neither is present we cannot toggle the part so we just return nil.
> -    (when (or overlay lazy-part)
> -      (let* ((show (button-get button :notmuch-part-hidden))
> -	     (new-start (button-start button))
> -	     (button-label (button-get button :base-label))
> -	     (old-point (point))
> -	     (properties (text-properties-at (button-start button)))
> -	     (inhibit-read-only t))
> -	;; Toggle the button itself.
> -	(button-put button :notmuch-part-hidden (not show))
> -	(goto-char new-start)
> -	(insert "[ " button-label (if show " ]" " (hidden) ]"))
> -	(set-text-properties new-start (point) properties)
> -	(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))))
> -	;; Return nil if there is a lazy-part, it is empty, and we are
> -	;; trying to show it.  In all other cases return t.
> -	(if lazy-part
> -	    (when show
> -	      (button-put button :notmuch-lazy-part nil)
> -	      (notmuch-show-lazy-part lazy-part button))
> -	  ;; else there must be an overlay.
> -	  (overlay-put overlay 'invisible (not show))
> -	  t)))))
> +  (let ((button (or button (button-at (point)))))
> +    (when button
> +      (let ((overlay (button-get button 'overlay))
> +	    (lazy-part (button-get button :notmuch-lazy-part)))
> +	;; We have a part to toggle if there is an overlay or if there is a lazy part.
> +	;; If neither is present we cannot toggle the part so we just return nil.
> +	(when (or overlay lazy-part)
> +	  (let* ((show (button-get button :notmuch-part-hidden))
> +		 (new-start (button-start button))
> +		 (button-label (button-get button :base-label))
> +		 (old-point (point))
> +		 (properties (text-properties-at (button-start button)))
> +		 (inhibit-read-only t))
> +	    ;; Toggle the button itself.
> +	    (button-put button :notmuch-part-hidden (not show))
> +	    (goto-char new-start)
> +	    (insert "[ " button-label (if show " ]" " (hidden) ]"))
> +	    (set-text-properties new-start (point) properties)
> +	    (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))))
> +	    ;; Return nil if there is a lazy-part, it is empty, and we are
> +	    ;; trying to show it.  In all other cases return t.
> +	    (if lazy-part
> +		(when show
> +		  (button-put button :notmuch-lazy-part nil)
> +		  (notmuch-show-lazy-part lazy-part button))
> +	      ;; else there must be an overlay.
> +	      (overlay-put overlay 'invisible (not show))
> +	      t)))))))
>  
>  ;; MIME part renderers
>  
> @@ -632,14 +633,17 @@ message at DEPTH in the current thread."
>    t)
>  
>  (defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth button)
> -  (button-put button 'face 'notmuch-crypto-part-header)
> -  ;; add signature status button if sigstatus provided
> +  (when 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."))
> +    ;; If we're not adding the signature status, tell the user how
> +    ;; they can get it.
> +    (when button
> +      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))
>  
>    (let ((inner-parts (plist-get part :content))
>  	(start (point)))
> @@ -653,17 +657,20 @@ message at DEPTH in the current thread."
>    t)
>  
>  (defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth button)
> -  (button-put button 'face 'notmuch-crypto-part-header)
> -  ;; add encryption status button if encstatus specified
> +  (when button
> +    (button-put button 'face 'notmuch-crypto-part-header))
> +  ;; Add encryption status button if encryption status is 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
> +	;; Add signature status button if signature status is
> +	;; 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
> +    ;; If we're not adding the encryption status, 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))
> @@ -930,8 +937,9 @@ useful for quoting in replies)."
>  
>      (if show-part
>          (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
> -      (button-put button :notmuch-lazy-part
> -                  (list msg part mime-type nth depth button)))
> +      (when button
> +	(button-put button :notmuch-lazy-part
> +		    (list msg part mime-type nth depth 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.
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 5/9] emacs/show: Remove the 'no-buttons option of `notmuch-show-insert-bodypart'

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> No code uses the 'no-buttons argument to
> `notmuch-show-insert-bodypart', so remove it.
> ---
>  emacs/notmuch-show.el | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index e511655..981b922 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -915,9 +915,7 @@ message at DEPTH in the current thread."
>  
>  HIDE determines whether to show or hide the part and the button
>  as follows: If HIDE is nil, show the part and the button. If HIDE
> -is t, hide the part initially and show the button. If HIDE is
> -'no-buttons, show the part but do not add any buttons (this is
> -useful for quoting in replies)."
> +is t, hide the part initially and show the button."
>  
>    (let* ((content-type (downcase (plist-get part :content-type)))
>  	 (mime-type (notmuch-show-mime-type part))
> @@ -926,9 +924,8 @@ useful for quoting in replies)."
>  	 ;; Hide the part initially if HIDE is t.
>  	 (show-part (not (equal hide t)))

Just a triviality: we might as well make the above (not hide) now.

MW

>  	 ;; We omit the part button for the first (or only) part if
> -	 ;; this is text/plain, or HIDE is 'no-buttons.
> -	 (button (when (and (not (equal hide 'no-buttons))
> -			    (funcall notmuch-show-insert-header-p-function part))
> +	 ;; 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))))
>  	 (content-beg (point)))
>  
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 4/9] emacs/mua: Generate improved cited text for replies

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> Use the message display code to generate message text to cite in
> replies.

So this is the key change. I am trying to work out what the actual
changes are here: in your commit message for the test update 7/9 you say
that the old code only output the first part. My impression of the
deleted code is that that is not the case (but I don't grok cl very
well).

I think the test change is because in show we do some content-type
guessing of application/octet-stream which the below doesn't do.

But we may also have some things with mm-inlined-types as mentioned in
my earlier reply. Anyway if you can point out any other cases where it
is changed that would be great.

If you go for a function deciding which parts to include then it might
be possible to have a midpoint where we are the same as before, and then
tweak the function to get whatever behaviour we think is best. That
might make it easy to see what is tidying/unification and what is
enhancement.

Incidentally, thank you for splitting this series so finely: I did find
that made it a lot easier to review.

Best wishes

Mark


> ---
>  emacs/notmuch-mua.el | 38 ++++++++------------------------------
>  1 file changed, 8 insertions(+), 30 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 95e4a4d..09c922f 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -28,7 +28,7 @@
>  
>  (eval-when-compile (require 'cl))
>  
> -(declare-function notmuch-show-insert-bodypart "notmuch-show" (msg part depth &optional hide))
> +(declare-function notmuch-show-insert-body "notmuch-show" (msg body depth))
>  
>  ;;
>  
> @@ -123,31 +123,6 @@ list."
>  	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
>  	  do (notmuch-mua-reply-crypto (plist-get part :content))))
>  
> -(defun notmuch-mua-get-quotable-parts (parts)
> -  (loop for part in parts
> -	if (notmuch-match-content-type (plist-get part :content-type) "multipart/alternative")
> -	  collect (let* ((subparts (plist-get part :content))
> -			(types (mapcar (lambda (part) (plist-get part :content-type)) subparts))
> -			(chosen-type (car (notmuch-multipart/alternative-choose types))))
> -		   (loop for part in (reverse subparts)
> -			 if (notmuch-match-content-type (plist-get part :content-type) chosen-type)
> -			 return part))
> -	else if (notmuch-match-content-type (plist-get part :content-type) "multipart/*")
> -	  append (notmuch-mua-get-quotable-parts (plist-get part :content))
> -	else if (notmuch-match-content-type (plist-get part :content-type) "text/*")
> -	  collect part))
> -
> -(defun notmuch-mua-insert-quotable-part (message part)
> -  ;; We don't want text properties leaking from the show renderer into
> -  ;; the reply so we use a temp buffer. Also we don't want hooks, such
> -  ;; as notmuch-wash-*, to be run on the quotable part so we set
> -  ;; notmuch-show-insert-text/plain-hook to nil.
> -  (insert (with-temp-buffer
> -	    (let ((notmuch-show-insert-text/plain-hook nil))
> -	      ;; Show the part but do not add buttons.
> -	      (notmuch-show-insert-bodypart message part 0 'no-buttons))
> -	    (buffer-substring-no-properties (point-min) (point-max)))))
> -
>  ;; There is a bug in emacs 23's message.el that results in a newline
>  ;; not being inserted after the References header, so the next header
>  ;; is concatenated to the end of it. This function fixes the problem,
> @@ -225,10 +200,13 @@ list."
>  	(insert "From: " from "\n")
>  	(insert "Date: " date "\n\n")
>  
> -	;; Get the parts of the original message that should be quoted; this includes
> -	;; all the text parts, except the non-preferred ones in a multipart/alternative.
> -	(let ((quotable-parts (notmuch-mua-get-quotable-parts (plist-get original :body))))
> -	  (mapc (apply-partially 'notmuch-mua-insert-quotable-part original) quotable-parts))
> +	(insert (with-temp-buffer
> +		  ;; Don't attempt to clean up messages, excerpt
> +		  ;; citations, etc. in the original message before
> +		  ;; quoting.
> +		  (let ((notmuch-show-insert-text/plain-hook nil))
> +		    (notmuch-show-insert-body original (plist-get original :body) 0)
> +		    (buffer-substring-no-properties (point-min) (point-max)))))
>  
>  	(set-mark (point))
>  	(goto-char start)
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 1/9] emacs/show: Re-arrange determination if a part header is necessary

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]

On Mon, May 12 2014, Mark Walters wrote:
> On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
>> Move the determination of whether a part header is required to a
>> distinct function.
>> ---
>>  emacs/notmuch-show.el | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index 10fc872..ec99141 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -871,6 +871,21 @@ message at DEPTH in the current thread."
>>        ;; showable this returns nil.
>>        (notmuch-show-create-part-overlays button part-beg part-end))))
>>  
>> +(defun notmuch-show-mime-type (part)
>> +  "Return the correct mime-type to use for PART."
>> +  (let ((content-type (downcase (plist-get part :content-type))))
>> +    (or (and (string= content-type "application/octet-stream")
>> +	     (notmuch-show-get-mime-type-of-application/octet-stream part))
>> +	(and (string= content-type "inline patch")
>> +	     "text/x-diff")
>> +	content-type)))
>> +
>> +(defun notmuch-show-insert-header-p (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)))))
>
> My only query here is whether a notmuch-show-hide-header-p (part) might
> make some of the logic clearer?

You're suggesting to simply invert the sense of the function?
>
> MW
>
>
>> +
>>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>>    "Insert the body part PART at depth DEPTH in the current thread.
>>  
>> @@ -881,19 +896,15 @@ is t, hide the part initially and show the button. If HIDE is
>>  useful for quoting in replies)."
>>  
>>    (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))
>> -			(and (string= content-type "inline patch")
>> -			     "text/x-diff")
>> -			content-type))
>> +	 (mime-type (notmuch-show-mime-type part))
>>  	 (nth (plist-get part :id))
>>  	 (beg (point))
>>  	 ;; Hide the part initially if HIDE is t.
>>  	 (show-part (not (equal hide t)))
>>  	 ;; We omit the part button for the first (or only) part if
>>  	 ;; this is text/plain, or HIDE is 'no-buttons.
>> -	 (button (unless (or (equal hide 'no-buttons)
>> -			     (and (string= mime-type "text/plain") (<= nth 1)))
>> +	 (button (when (and (not (equal hide 'no-buttons))
>> +			    (notmuch-show-insert-header-p part))
>>  		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>>  	 (content-beg (point)))
>>  
>> -- 
>> 2.0.0.rc0
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH v3 2/9] emacs/show: Allow the user to decide when part headers should be inserted

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]

On Mon, May 12 2014, Mark Walters wrote:
> On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
>> Make the function that determines whether a part header should be
>> inserted a user controlled, with some example functions.
>> ---
>>  emacs/notmuch-show.el | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index ec99141..f78a0ab 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -135,6 +135,17 @@ indentation."
>>    :type 'boolean
>>    :group 'notmuch-show)
>>  
>> +(defcustom notmuch-show-insert-header-p-function 'notmuch-show-insert-header-p-smart
>> +  "A function to call to determine whether a MIME part should have a header button.
>> +
>> +The function is passed one argument, PART - the MIME part in
>> +question."
>> +  :type 'function
>> +  :group 'notmuch-show
>> +  :options '(notmuch-show-insert-header-p-smart
>> +	     notmuch-show-insert-header-p-always
>> +	     notmuch-show-insert-header-p-never))
>> +
>
> I think this is nice as a defcustom for testing/seeing what happens, but I am
> not sure we want it exposed longer term: for example never makes it
> nearly impossible to view pdf parts.

Okay.
>
> Best wishes
>
> Mark
>
>
>
>>  (defvar notmuch-show-thread-id nil)
>>  (make-variable-buffer-local 'notmuch-show-thread-id)
>>  (put 'notmuch-show-thread-id 'permanent-local t)
>> @@ -880,12 +891,18 @@ message at DEPTH in the current thread."
>>  	     "text/x-diff")
>>  	content-type)))
>>  
>> -(defun notmuch-show-insert-header-p (part)
>> +(defun notmuch-show-insert-header-p-smart (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)
>> +  t)
>> +
>> +(defun notmuch-show-insert-header-p-never (part)
>> +  nil)
>> +
>>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>>    "Insert the body part PART at depth DEPTH in the current thread.
>>  
>> @@ -904,7 +921,7 @@ useful for quoting in replies)."
>>  	 ;; We omit the part button for the first (or only) part if
>>  	 ;; this is text/plain, or HIDE is 'no-buttons.
>>  	 (button (when (and (not (equal hide 'no-buttons))
>> -			    (notmuch-show-insert-header-p part))
>> +			    (funcall notmuch-show-insert-header-p-function part))
>>  		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>>  	 (content-beg (point)))
>>  
>> -- 
>> 2.0.0.rc0
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH v3 0/9] emacs: Improve the cited message included in replies

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 4129 bytes --]

Firstly, I don't think that the code resulting from this patch series is
beyond improvement - the intention was really only that it be better
than the current implementation.

On Mon, May 12 2014, Mark Walters wrote:
> On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
>> emacs: Improve the cited message included in replies
>>
>> I tried to do things in small increments to make it easier to review.
>>
>> v2:
>> - Don't run the text/plain hooks when generating the message to quote.
>>
>> v3:
>> - Remove the 'no-button code, as it's no longer used.
>> - Control the insertion of part headers using a function.
>> - Fix the tests.
>
> I think I broadly like this series. I haven't thought through all the
> ramifications yet so this is just some first thoughts. I will also send
> some comments on individual patches.

Thanks!

> In notmuch-show we go to notmuch-show-insert-part-*/* to
> notmuch-mm-display-part-inline and then leave the decision to inline to
> mm-inlined-types. I think this means that, by default, we will not
> inline signatures amongst other things.

The rule is essentially: whatever text would be shown when the message
is displayed for reading, without any of the washing.

> So at the least I think we should decide whether we want to override
> mm-inlined-types.

I'm not really clear on the benefits of this. Could you explain?

> Alternatively, and in my view preferably, we could have a function or
> variable of our own which says which parts to include. Indeed, if do
> it with a function we might be able to make an option to reply to mean
> "include parts currently shown in the notmuch-show buffer" which might
> be nice.

That seems over complicated to me. The rule (above) from this series
is easy to understand and work with. Other mechanisms could be
implemented later, of course.

> There is a related question and possible bug that we might be able to
> do something about at the same time: should we include text parts in the
> reply if they have content-disposition attachment? I have been caught
> about by this on one occasion replying to a message with a 500K log file
> attached (and notmuch-show/wash becomes very slow with a 500K
> message!)

This is really a question of what happens in `show' mode. If it is
currently displaying text parts with content-disposition attachment,
that sounds like a bug that should be fixed (which would mean that the
cited message wouldn't include that part either).

> Finally, I am not sure whether I like having buttons in the reply. My
> instinct is against, but they do add context.

The last patch in the series is an example of trying to do the right
thing - show the part headers when they are necessary for proper
understanding, but elide them in all other cases.

The mechanism used to do this is pretty crude in the patch. One could
imagine a better implementation that examines the depth of the part
tree, etc.

> Best wishes
>
> Mark
>
>
>
>
>>
>>
>> David Edmondson (9):
>>   emacs/show: Re-arrange determination if a part header is necessary
>>   emacs/show: Allow the user to decide when part headers should be
>>     inserted
>>   emacs/show: Accommodate the lack of part header buttons
>>   emacs/mua: Generate improved cited text for replies
>>   emacs/show: Remove the 'no-buttons option of
>>     `notmuch-show-insert-bodypart'
>>   emacs/mua: Don't insert part headers in citations
>>   test: Update the test output to accord with the reply changes
>>   emacs/mua: Insert part headers depending on the message
>>   test: Update the test output to accord with more reply changes
>>
>>  emacs/notmuch-mua.el  |  82 +++++++++++++++++++-----------
>>  emacs/notmuch-show.el | 135 +++++++++++++++++++++++++++++++-------------------
>>  test/T310-emacs.sh    |  44 ++++++++++++++++
>>  3 files changed, 180 insertions(+), 81 deletions(-)
>>
>> -- 
>> 2.0.0.rc0
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH v3 4/9] emacs/mua: Generate improved cited text for replies

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On Mon, May 12 2014, Mark Walters wrote:
> On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
>> Use the message display code to generate message text to cite in
>> replies.
>
> So this is the key change. I am trying to work out what the actual
> changes are here: in your commit message for the test update 7/9 you say
> that the old code only output the first part. My impression of the
> deleted code is that that is not the case (but I don't grok cl very
> well).

The comment in the test change was intended only to refer to the content
displayed in that specific test. You are correct that text from several
parts could previously have been used (and all smashed together in a way
that made it difficult to see the boundaries between the parts...).

> I think the test change is because in show we do some content-type
> guessing of application/octet-stream which the below doesn't do.
>
> But we may also have some things with mm-inlined-types as mentioned in
> my earlier reply. Anyway if you can point out any other cases where it
> is changed that would be great.

message/rfc822 was the initial driver. If you now reply to a
multipart/mixed that contains text/plain and message/rfc822 parts, you
will see a significant difference.

I'm not sure if you have it, but
id:mailman.1.1399324386.31850.notmuch@notmuchmail.org was the message I
used often when playing around.

> If you go for a function deciding which parts to include then it might
> be possible to have a midpoint where we are the same as before, and then
> tweak the function to get whatever behaviour we think is best. That
> might make it easy to see what is tidying/unification and what is
> enhancement.

I will look into that - it's a good suggestion, though it actually runs
counter to:

> Incidentally, thank you for splitting this series so finely: I did find
> that made it a lot easier to review.

Making the new code (which relies on the 'show' rendering code) behave
just like the old code will involve adding a bunch of complication to
this specific patch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 310 bytes --]

Re: [PATCH v3 0/9] emacs: Improve the cited message included in replies

[Mark Walters]

On Tue, 13 May 2014, David Edmondson <dme@dme.org> wrote:
> Firstly, I don't think that the code resulting from this patch series is
> beyond improvement - the intention was really only that it be better
> than the current implementation.
>
> On Mon, May 12 2014, Mark Walters wrote:
>> On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
>>> emacs: Improve the cited message included in replies
>>>
>>> I tried to do things in small increments to make it easier to review.
>>>
>>> v2:
>>> - Don't run the text/plain hooks when generating the message to quote.
>>>
>>> v3:
>>> - Remove the 'no-button code, as it's no longer used.
>>> - Control the insertion of part headers using a function.
>>> - Fix the tests.
>>
>> I think I broadly like this series. I haven't thought through all the
>> ramifications yet so this is just some first thoughts. I will also send
>> some comments on individual patches.
>
> Thanks!
>
>> In notmuch-show we go to notmuch-show-insert-part-*/* to
>> notmuch-mm-display-part-inline and then leave the decision to inline to
>> mm-inlined-types. I think this means that, by default, we will not
>> inline signatures amongst other things.
>
> The rule is essentially: whatever text would be shown when the message
> is displayed for reading, without any of the washing.

Yes I think that is a good rule. That means that essentially all my
comments end up being more a (future) feature request that we make
choosing which parts to view in notmuch-show more configurable.

I do have a slight worry about whether there are any cases that people
will want parts shown in their emacs-show but not in any reply but I
can't think of any.

>> So at the least I think we should decide whether we want to override
>> mm-inlined-types.
>
> I'm not really clear on the benefits of this. Could you explain?

This was just the above slight worry.

>> Alternatively, and in my view preferably, we could have a function or
>> variable of our own which says which parts to include. Indeed, if do
>> it with a function we might be able to make an option to reply to mean
>> "include parts currently shown in the notmuch-show buffer" which might
>> be nice.
>
> That seems over complicated to me. The rule (above) from this series
> is easy to understand and work with. Other mechanisms could be
> implemented later, of course.

Yes, as you say, leave this for later (if ever).
>
>> There is a related question and possible bug that we might be able to
>> do something about at the same time: should we include text parts in the
>> reply if they have content-disposition attachment? I have been caught
>> about by this on one occasion replying to a message with a 500K log file
>> attached (and notmuch-show/wash becomes very slow with a 500K
>> message!)
>
> This is really a question of what happens in `show' mode. If it is
> currently displaying text parts with content-disposition attachment,
> that sounds like a bug that should be fixed (which would mean that the
> cited message wouldn't include that part either).

Yes I agree.

>> Finally, I am not sure whether I like having buttons in the reply. My
>> instinct is against, but they do add context.
>
> The last patch in the series is an example of trying to do the right
> thing - show the part headers when they are necessary for proper
> understanding, but elide them in all other cases.
>
> The mechanism used to do this is pretty crude in the patch. One could
> imagine a better implementation that examines the depth of the part
> tree, etc.

OK I will try and review these soon.

Best wishes

Mark

>
>> Best wishes
>>
>> Mark
>>
>>
>>
>>
>>>
>>>
>>> David Edmondson (9):
>>>   emacs/show: Re-arrange determination if a part header is necessary
>>>   emacs/show: Allow the user to decide when part headers should be
>>>     inserted
>>>   emacs/show: Accommodate the lack of part header buttons
>>>   emacs/mua: Generate improved cited text for replies
>>>   emacs/show: Remove the 'no-buttons option of
>>>     `notmuch-show-insert-bodypart'
>>>   emacs/mua: Don't insert part headers in citations
>>>   test: Update the test output to accord with the reply changes
>>>   emacs/mua: Insert part headers depending on the message
>>>   test: Update the test output to accord with more reply changes
>>>
>>>  emacs/notmuch-mua.el  |  82 +++++++++++++++++++-----------
>>>  emacs/notmuch-show.el | 135 +++++++++++++++++++++++++++++++-------------------
>>>  test/T310-emacs.sh    |  44 ++++++++++++++++
>>>  3 files changed, 180 insertions(+), 81 deletions(-)
>>>
>>> -- 
>>> 2.0.0.rc0
>>>
>>> _______________________________________________
>>> notmuch mailing list
>>> notmuch@notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 7/9] test: Update the test output to accord with the reply changes

[Mark Walters]

On Mon, 12 May 2014, David Edmondson <dme@dme.org> wrote:
> Replying to a message with multiple parts will now typically include
> content from several parts (whereas previously only the first part was
> used). Update the expected output from the emacs reply tests
> accordingly.

I will reply here rather than to id:cunr43y56wy.fsf@hotblack-desiato.hh.sledj.net

My point was that the change in this test is not because we are now
looking beyond the first part. It is because a part with content-type
application/octet-stream (which we didn't include in replies in the old
scheme, and wouldn't generally in the new scheme) is redesignated by
notmuch-show as a content-type text/x-diff, and thus displayed in the
buffer. Hence the new code includes it in the reply.

You can see this more clearly in patch 9/9 when this part gets a header button

+> [ 0001-Deal-with-situation-where-sysconf-_SC_GETPW_R_SIZE_M.patch: application/octet-stream (as text/x-diff) ]

Anyway, my point is that this is a *good* thing: the change is not a
large intrusive thing of showing lots more parts (as this commit message
sort of suggests) but a small change of it now correctly including a
part which got sent with the wrong content-type.

Best wishes

Mark



> ---
>  test/T310-emacs.sh | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/test/T310-emacs.sh b/test/T310-emacs.sh
> index ac966e5..95bb67e 100755
> --- a/test/T310-emacs.sh
> +++ b/test/T310-emacs.sh
> @@ -473,6 +473,38 @@ Alex Botero-Lowry <alex.boterolowry@gmail.com> writes:
>  > and http://mail-index.netbsd.org/pkgsrc-bugs/2006/06/07/msg016808.htmlspecifically
>  > uses 64 as the
>  > buffer size.
> +> From e3bc4bbd7b9d0d086816ab5f8f2d6ffea1dd3ea4 Mon Sep 17 00:00:00 2001
> +> From: Alexander Botero-Lowry <alex.boterolowry@gmail.com>
> +> Date: Tue, 17 Nov 2009 11:30:39 -0800
> +> Subject: [PATCH] Deal with situation where sysconf(_SC_GETPW_R_SIZE_MAX) returns -1
> +>
> +> ---
> +>  notmuch-config.c |    2 ++
> +>  1 files changed, 2 insertions(+), 0 deletions(-)
> +>
> +> diff --git a/notmuch-config.c b/notmuch-config.c
> +> index 248149c..e7220d8 100644
> +> --- a/notmuch-config.c
> +> +++ b/notmuch-config.c
> +> @@ -77,6 +77,7 @@ static char *
> +>  get_name_from_passwd_file (void *ctx)
> +>  {
> +>      long pw_buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
> +> +    if (pw_buf_size == -1) pw_buf_size = 64;
> +>      char *pw_buf = talloc_zero_size (ctx, pw_buf_size);
> +>      struct passwd passwd, *ignored;
> +>      char *name;
> +> @@ -101,6 +102,7 @@ static char *
> +>  get_username_from_passwd_file (void *ctx)
> +>  {
> +>      long pw_buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
> +> +    if (pw_buf_size == -1) pw_buf_size = 64;
> +>      char *pw_buf = talloc_zero_size (ctx, pw_buf_size);
> +>      struct passwd passwd, *ignored;
> +>      char *name;
> +> -- 
> +> 1.6.5.2
> +>
>  > _______________________________________________
>  > notmuch mailing list
>  > notmuch@notmuchmail.org
> -- 
> 2.0.0.rc0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Mark Walters]

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

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

[David Edmondson]

On Mon, Jan 19 2015, Mark Walters <markwalters1009@gmail.com> wrote:
> 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).

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@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

Re: [PATCH v3 0/9] emacs: Improve the cited message included in replies

[David Edmondson]

I'd like to resurrect this set of changes. There was discussion last
time around, but I'm unclear about any conclusion. Should I just merge
them forward and re-post?

On Mon, May 12 2014, David Edmondson <dme@dme.org> wrote:
> emacs: Improve the cited message included in replies
>
> I tried to do things in small increments to make it easier to review.
>
> v2:
> - Don't run the text/plain hooks when generating the message to quote.
>
> v3:
> - Remove the 'no-button code, as it's no longer used.
> - Control the insertion of part headers using a function.
> - Fix the tests.
>
>
> David Edmondson (9):
>   emacs/show: Re-arrange determination if a part header is necessary
>   emacs/show: Allow the user to decide when part headers should be
>     inserted
>   emacs/show: Accommodate the lack of part header buttons
>   emacs/mua: Generate improved cited text for replies
>   emacs/show: Remove the 'no-buttons option of
>     `notmuch-show-insert-bodypart'
>   emacs/mua: Don't insert part headers in citations
>   test: Update the test output to accord with the reply changes
>   emacs/mua: Insert part headers depending on the message
>   test: Update the test output to accord with more reply changes
>
>  emacs/notmuch-mua.el  |  82 +++++++++++++++++++-----------
>  emacs/notmuch-show.el | 135 +++++++++++++++++++++++++++++++-------------------
>  test/T310-emacs.sh    |  44 ++++++++++++++++
>  3 files changed, 180 insertions(+), 81 deletions(-)
>
> -- 
> 2.0.0.rc0

Re: reply to messages with message/rfc822 parts

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Hello
>
> I was trying to reply to a message I had forwarded to someone (to update
> the information sent in the first message) and came across some strange
> behaviour.
>
>
> The initial forward was done using notmuch-emacs: this took the message
> and sent it as a message/rfc822 mimetype complete message. Since I had
> added some text at the top this meant the message as a whole was
> multipart/mixed with my text/plain at the top and the message/rfc822
> below.
>
> Then I tried to reply to this message and the text/plain part was
> included in the reply but the message/rfc822 part was not. In this case
> the message/rfc822 just had headers and a text/plain subpart so I would
> have expected it to be included.
>
> I imagine we actually want to recurse into the message/rfc822 part
> including relevant subparts. I tried tweaking notmuch-mua.el to do this
> but, so far, I have failed. (I will have another look)

As far as I know this problem is now fixed.

d