On Mon, Jan 19 2015, Mark Walters <markwalters1009 at gmail.com> wrote: > On Mon, 12 May 2014, David Edmondson <dme at dme.org> wrote: >> Whether to insert part headers should depend on the details of the >> message being cited. > > Hi > > Overall I like this series and it does fix two annoying bugs (not being > able to reply to ref822 messages and (correctly) including parts > which have application/octet-stream but are actually text parts). > > The one problem is getting the right choice for part headers in the > reply text. I think getting this wrong will irritate users even if the > overall result is better. > > My guess at the correct logic is: > 1) omit the part header for any empty part: (ie a part we don't display > such as a pdf file).
This seems wrong to me (i.e. it's not what I would want ;-). Showing the part header in the reply acknowledges that it was part of the message that I'm replying to. Even more, consider a message: Hi, attached are the two PDF documents that we discussed. This is the version with Fred's suggested comments: [ application/pdf: document 1 ] This is the version with my proposed alternative edits and much more content added: [ application/pdf: document 2 ] The part headers form a significant part of the content. In a reply I'd like to see them, so that I can add comments appropriately. (I realise that commenting 'in-line' is out of fashion in many places now, but for more complex discussions I still prefer it.) I still like the original rule that I proposed: the reply should include whatever is in the 'show' buffer, modulo content that was elided due to washing. > 2) omit multipart/* part headers > 3) include all other part headers > 4) except omit the first part header (perhaps only in the case it is > text/plain) > > My reasoning for each is > 1) there is no point in saying we had a part which we are omitting. > 2) all the subparts of multipart/* will get there own header which > should be sufficient. > 3) we want to keep the parts distinguished > 4) except we don't need to do that with the first part. > > Note for 4) it would be good to have a multipart/alternative with > subparts text/plain and text/html just give the text/plain with no part > header. > > I include a patch below which does all of these apart from 4) as I > couldn't see a clean way of implementing it. Any suggestions? > > It should apply on top of patch 6 or 7 instead of 8. The key change is > that it always puts in a button and then deletes it if unwanted: this > makes doing 1) above easy. > > It does break some tests, nothing unexpected except an interaction with > the way we wash text/plain parts: we remove leading blank lines from the > first text/plain part (because it doesn't have a button) but not from > subsequent ones (because they do). Because this code always has the > second case it doesn't remove a leading blank line of the first part. > > Best wishes > > Mark > > > From 8f198b38e76e050ae8d20d866748c41ccf79f3d4 Mon Sep 17 00:00:00 2001 > From: Mark Walters <markwalters1009 at gmail.com> > Date: Mon, 19 Jan 2015 14:39:25 +0000 > Subject: [PATCH] emacs show/reply modify part handling > > Modify the part handling so that we always insert the button and > delete it afterwards if not wanted. The advantage is that we can > decide whether to keep the part button based on what the insertion > code does. In particular the reply code can omit the button for all > parts with no displayable content. > --- > emacs/notmuch-mua.el | 5 +++-- > emacs/notmuch-show.el | 39 +++++++++++++++++++++++++-------------- > 2 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el > index 0ca9eed..6060f33 100644 > --- a/emacs/notmuch-mua.el > +++ b/emacs/notmuch-mua.el > @@ -29,6 +29,7 @@ > (eval-when-compile (require 'cl)) > > (declare-function notmuch-show-insert-body "notmuch-show" (msg body depth)) > +(declare-function notmuch-show-insert-header-p-reply "notmuch-show" (part > empty-part)) > > ;; > > @@ -223,8 +224,8 @@ Note that these functions use `mail-citation-hook' if > that is non-nil." > ;; citations, etc. in the original message before > ;; quoting. > ((notmuch-show-insert-text/plain-hook nil) > - ;; Don't insert part buttons. > - (notmuch-show-insert-header-p-function > #'notmuch-show-insert-header-p-never)) > + ;; Insert part buttons appropriate for a reply. > + (notmuch-show-insert-header-p-function > #'notmuch-show-insert-header-p-reply)) > (notmuch-show-insert-body original (plist-get original > :body) 0) > (buffer-substring-no-properties (point-min) (point-max))))) > > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el > index 4a0899f..2cdb5a8 100644 > --- a/emacs/notmuch-show.el > +++ b/emacs/notmuch-show.el > @@ -909,16 +909,24 @@ message at DEPTH in the current thread." > "text/x-diff") > content-type))) > > -(defun notmuch-show-insert-header-p-smart (part) > +(defun notmuch-show-insert-header-p-smart (part empty-part) > "Return non-NIL if a header button should be inserted for this part." > (let ((mime-type (notmuch-show-mime-type part))) > (not (and (string= mime-type "text/plain") > (<= (plist-get part :id) 1))))) > > -(defun notmuch-show-insert-header-p-always (part) > +(defun notmuch-show-insert-header-p-reply (part empty-part) > + "Return non-NIL if a header button should be inserted for this part." > + (let ((mime-type (notmuch-show-mime-type part))) > + (not (or empty-part > + (notmuch-match-content-type mime-type "multipart/*") > + (and (string= mime-type "text/plain") > + (<= (plist-get part :id) 1)))))) > + > +(defun notmuch-show-insert-header-p-always (part empty-part) > t) > > -(defun notmuch-show-insert-header-p-never (part) > +(defun notmuch-show-insert-header-p-never (part empty-part) > nil) > > (defun notmuch-show-insert-bodypart (msg part depth &optional hide) > @@ -936,8 +944,8 @@ is t, hide the part initially and show the button." > (show-part (not (equal hide t))) > ;; We omit the part button for the first (or only) part if > ;; this is text/plain. > - (button (when (funcall notmuch-show-insert-header-p-function part) > - (notmuch-show-insert-part-header nth mime-type content-type > (plist-get part :filename)))) > + (button-beg (point)) > + (button (notmuch-show-insert-part-header nth mime-type content-type > (plist-get part :filename))) > (content-beg (point))) > > ;; Store the computed mime-type for later use (e.g. by attachment > handlers). > @@ -952,15 +960,18 @@ is t, hide the part initially and show the button." > ;; Some of the body part handlers leave point somewhere up in the > ;; part, so we make sure that we're down at the end. > (goto-char (point-max)) > - ;; Ensure that the part ends with a carriage return. > - (unless (bolp) > - (insert "\n")) > - ;; We do not create the overlay for hidden (lazy) parts until > - ;; they are inserted. > - (if show-part > - (notmuch-show-create-part-overlays button content-beg (point)) > - (save-excursion > - (notmuch-show-toggle-part-invisibility button))) > + (let ((empty-part (equal (point) content-beg))) > + (if (not (funcall notmuch-show-insert-header-p-function part > empty-part)) > + (delete-region button-beg content-beg) > + ;; Ensure that the part ends with a carriage return. > + (unless (bolp) > + (insert "\n")) > + ;; We do not create the overlay for hidden (lazy) parts until > + ;; they are inserted. > + (if show-part > + (notmuch-show-create-part-overlays button content-beg (point)) > + (save-excursion > + (notmuch-show-toggle-part-invisibility button))))) > (notmuch-show-record-part-information part beg (point)))) > > (defun notmuch-show-insert-body (msg body depth) > -- > 2.1.3