[PATCH v5 0/4] Use invisibility to toggle display of all parts including multipart

[PATCH v5 0/4] Use invisibility to toggle display of all parts including multipart

[Mark Walters]

This is an alternative version of
id:1355781287-6010-1-git-send-email-markwalters1009@gmail.com based on
top of Austin's patch at
id:1355812810-32747-1-git-send-email-amdragon@mit.edu

Austin's patch significantly simplifies the invisibility handling
taking this series down from 90/27 to 68/26 in diffstat terms.

In general terms Austin's approach has to be the right thing to do:
what we want to do just before the freeze is less clear. My view is
that we should go with Austin's approach now so that at least any bugs
we get from it and (more likely) from this series apply to master as
well. 

I am posting this series to make it easier for people to judge the two
approaches when finished (ie with part invisibility too).

I attach a trimmed diff from v4 below the diffstat (note this was a
diff with -U10 which I have trimmed so it is purely for information)

Note we no longer need patch 4/5 because in this approach the overlays
do not need to know about other overlays.

Best wishes

Mark






Mark Walters (4):
  emacs: show: modify insert-part-header to save the button text
  emacs: show: add overlays for each part
  emacs: show: add invisibility button action
  emacs: show: set default show-all-multipart/alternatives to nil

 emacs/notmuch-show.el |   94 +++++++++++++++++++++++++++++++++++-------------
 1 files changed, 68 insertions(+), 26 deletions(-)


Diff from v4

 emacs/notmuch-show.el |   33 ++++++---------------------------
 emacs/notmuch-wash.el |    2 +-
 2 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index ee67fec..5751d98 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -551,31 +551,28 @@ message at DEPTH in the current thread."
 
 ;; This is taken from notmuch-wash: maybe it should be unified?
 (defun notmuch-show-toggle-part-invisibility (&optional button)
   (interactive)
   (let* ((button (or button (button-at (point))))
-	 (overlay (button-get button 'overlay))
-	 (invis-spec (button-get button 'invisibility-spec))
-	 (show (invisible-p invis-spec)))
+	 (overlay (button-get button 'overlay)))
     (when overlay
-      (if show
-	  (remove-from-invisibility-spec invis-spec)
-	(add-to-invisibility-spec invis-spec))
-      (let* ((new-start (button-start button))
+      (let* ((show (overlay-get overlay 'invisible))
+	     (new-start (button-start button))
 	     (button-label (button-get button :base-label))
 	     (old-point (point))
 	     (inhibit-read-only t))
+	(overlay-put overlay 'invisible (not show))
 	(goto-char new-start)
 	(insert "[ " button-label (if show " ]" " (hidden) ]"))
 	(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))))))))

@@ -862,39 +859,21 @@ message at DEPTH in the current thread."
 
 (defun notmuch-show-create-part-overlays (msg beg end hide)
   "Add an overlay to the part between BEG and END"
   (let* ((button (button-at beg))
 	 (part-beg (and button (1+ (button-end button)))))
 
     ;; If the part contains no text we do not make it toggleable. We
     ;; also need to check that the button is a genuine part button not
     ;; a notmuch-wash button.
     (when (and button (/= part-beg end) (button-get button :base-label))
-      (let ((base-label (button-get button :base-label))
-	    (overlay (make-overlay part-beg end))
-	    (message-invis-spec (plist-get msg :message-invis-spec))
-	    (invis-spec (make-symbol "notmuch-part-region")))
-
-	(overlay-put overlay 'invisible (list invis-spec message-invis-spec))
-	(overlay-put overlay 'priority 10)
-	;; Now we have to add invis-spec to every overlay this
-	;; overlay contains, otherwise these inner overlays will
-	;; override this one.
-	(dolist (inner (overlays-in part-beg end))
-	  (when (and (>= (overlay-start inner) part-beg)
-			   (<= (overlay-end inner) end))
-		  (overlay-put inner 'invisible
-			       (cons invis-spec (overlay-get inner 'invisible)))))
-
-	(button-put button 'invisibility-spec invis-spec)
-	(button-put button 'overlay overlay))
-
+      (button-put button 'overlay (make-overlay part-beg end))
       ;; We toggle the button for hidden parts as that gets the
       ;; button label right.
       (save-excursion
 	(when hide
 	  (notmuch-show-toggle-part-invisibility button))))))

@@ -1992,21 +1971,21 @@ If optional argument MLA is non-nil, use the provided key instead of prompting
 (defun notmuch-show-part-button-default (&optional button)
   (interactive)
   (let ((button (or button (button-at (point)))))
-    (if (button-get button 'invisibility-spec)
+    (if (button-get button 'overlay)
 	(notmuch-show-toggle-part-invisibility button)
       (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))))

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index e10ab66..d6db4fa 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -365,15 +365,15 @@ for error."
       (save-restriction
 	(narrow-to-region patch-start patch-end)
 	(setq part (plist-put part :content-type "inline-patch-fake-part"))
 	(setq part (plist-put part :content (buffer-string)))
 	(setq part (plist-put part :id -1))
 	(setq part (plist-put part :filename
 			      (notmuch-wash-subject-to-patch-filename
 			       (plist-get
 				(plist-get msg :headers) :Subject))))
 	(delete-region (point-min) (point-max))
-	(notmuch-show-insert-bodypart msg part depth)))))
+	(notmuch-show-insert-bodypart nil part depth)))))
 
 ;;
 
 (provide 'notmuch-wash)

-- 
1.7.9.1

[PATCH v5 1/4] emacs: show: modify insert-part-header to save the button text

[Mark Walters]

This just make notmuch-show-insert-part-header save the basic button
text for parts as an attribute. This makes it simpler for the button
action (added in a later patch) to reword the label as appropriate (eg
append "(not shown)" or not as appropriate).
---
 emacs/notmuch-show.el |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 4bdd5af..5248fba 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -483,17 +483,17 @@ message at DEPTH in the current thread."
 (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
 
 (defun notmuch-show-insert-part-header (nth content-type declared-type &optional name comment)
-  (let ((button))
+  (let ((button)
+	(base-label (concat (when name (concat name ": "))
+			    declared-type
+			    (unless (string-equal declared-type content-type)
+			      (concat " (as " content-type ")"))
+			    comment)))
+
     (setq button
 	  (insert-button
-	   (concat "[ "
-		   (if name (concat name ": ") "")
-		   declared-type
-		   (if (not (string-equal declared-type content-type))
-		       (concat " (as " content-type ")")
-		     "")
-		   (or comment "")
-		   " ]")
+	   (concat "[ " base-label " ]")
+	   :base-label base-label
 	   :type 'notmuch-show-part-button-type
 	   :notmuch-part nth
 	   :notmuch-filename name
-- 
1.7.9.1

[PATCH v5 2/4] emacs: show: add overlays for each part

[Mark Walters]

This makes notmuch-show-insert-bodypart add an overlay for any
non-trivial part with a button header (currently the first text/plain
part does not have a button). At this point the overlay is available
to the button but there is no action using it yet.

In addition the argument HIDE is passed down to
notmuch-show-insert-part-overlays to request that the part be hidden
by default but this is not acted on yet.
---
 emacs/notmuch-show.el |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5248fba..dc86b43 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -567,11 +567,10 @@ message at DEPTH in the current thread."
     ;; but it's not clear that this is the wrong thing to do - which
     ;; should be chosen if there are more than one that match?
     (mapc (lambda (inner-part)
-	    (let ((inner-type (plist-get inner-part :content-type)))
-	      (if (or notmuch-show-all-multipart/alternative-parts
-		      (string= chosen-type inner-type))
-		  (notmuch-show-insert-bodypart msg inner-part depth)
-		(notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))
+	    (let* ((inner-type (plist-get inner-part :content-type))
+		  (hide (not (or notmuch-show-all-multipart/alternative-parts
+			   (string= chosen-type inner-type)))))
+	      (notmuch-show-insert-bodypart msg inner-part depth hide)))
 	  inner-parts)
 
     (when notmuch-show-indent-multipart
@@ -839,17 +838,33 @@ message at DEPTH in the current thread."
       (setq handlers (cdr handlers))))
   t)
 
-(defun notmuch-show-insert-bodypart (msg part depth)
-  "Insert the body part PART at depth DEPTH in the current thread."
+(defun notmuch-show-create-part-overlays (msg beg end hide)
+  "Add an overlay to the part between BEG and END"
+  (let* ((button (button-at beg))
+	 (part-beg (and button (1+ (button-end button)))))
+
+    ;; If the part contains no text we do not make it toggleable. We
+    ;; also need to check that the button is a genuine part button not
+    ;; a notmuch-wash button.
+    (when (and button (/= part-beg end) (button-get button :base-label))
+	(button-put button 'overlay (make-overlay part-beg end)))))
+
+(defun notmuch-show-insert-bodypart (msg part depth &optional hide)
+  "Insert the body part PART at depth DEPTH in the current thread.
+
+If HIDE is non-nil then initially hide this part."
   (let ((content-type (downcase (plist-get part :content-type)))
-	(nth (plist-get part :id)))
-    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type))
-  ;; 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")))
+	(nth (plist-get part :id))
+	(beg (point)))
+
+    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type)
+    ;; 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"))
+    (notmuch-show-create-part-overlays msg beg (point) hide)))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
-- 
1.7.9.1

[PATCH v5 3/4] emacs: show: add invisibility button action

[Mark Walters]

This adds a button action to show hidden parts. In this version "RET"
toggles the visibility of any part which puts content in the buffer
(as opposed to attachments such as application/pdf).

The button is used to hide parts when appropriate (eg text/html in
multipart/alternative).
---
 emacs/notmuch-show.el |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index dc86b43..f9366d0 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -554,6 +554,25 @@ message at DEPTH in the current thread."
     (let ((handle (mm-make-handle (current-buffer) (list content-type))))
       (mm-pipe-part handle))))
 
+;; This is taken from notmuch-wash: maybe it should be unified?
+(defun notmuch-show-toggle-part-invisibility (&optional button)
+  (interactive)
+  (let* ((button (or button (button-at (point))))
+	 (overlay (button-get button 'overlay)))
+    (when overlay
+      (let* ((show (overlay-get overlay 'invisible))
+	     (new-start (button-start button))
+	     (button-label (button-get button :base-label))
+	     (old-point (point))
+	     (inhibit-read-only t))
+	(overlay-put overlay 'invisible (not show))
+	(goto-char new-start)
+	(insert "[ " button-label (if show " ]" " (hidden) ]"))
+	(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))))))))
+
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
@@ -847,7 +866,12 @@ message at DEPTH in the current thread."
     ;; also need to check that the button is a genuine part button not
     ;; a notmuch-wash button.
     (when (and button (/= part-beg end) (button-get button :base-label))
-	(button-put button 'overlay (make-overlay part-beg end)))))
+      (button-put button 'overlay (make-overlay part-beg end))
+      ;; We toggle the button for hidden parts as that gets the
+      ;; button label right.
+      (save-excursion
+	(when hide
+	  (notmuch-show-toggle-part-invisibility button))))))
 
 (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
   "Insert the body part PART at depth DEPTH in the current thread.
@@ -1953,7 +1977,10 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
 
 (defun notmuch-show-part-button-default (&optional button)
   (interactive)
-  (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))
+  (let ((button (or button (button-at (point)))))
+    (if (button-get button 'overlay)
+	(notmuch-show-toggle-part-invisibility button)
+      (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))))
 
 (defun notmuch-show-part-button-save (&optional button)
   (interactive)
-- 
1.7.9.1

[PATCH v5 4/4] emacs: show: set default show-all-multipart/alternatives to nil

[Mark Walters]

Now that the invisibility display of parts is present we no longer
need to force the display of all multipart/alternatives: users can
toggle them for themselves when needed.
---
 emacs/notmuch-show.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f9366d0..5751d98 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -94,7 +94,7 @@ visible for any given message."
   :group 'notmuch-hooks)
 
 ;; Mostly useful for debugging.
-(defcustom notmuch-show-all-multipart/alternative-parts t
+(defcustom notmuch-show-all-multipart/alternative-parts nil
   "Should all parts of multipart/alternative parts be shown?"
   :type 'boolean
   :group 'notmuch-show)
-- 
1.7.9.1

Re: [PATCH v5 0/4] Use invisibility to toggle display of all parts including multipart

[Austin Clements]

Quoth Mark Walters on Dec 18 at  7:27 pm:
> This is an alternative version of
> id:1355781287-6010-1-git-send-email-markwalters1009@gmail.com based on
> top of Austin's patch at
> id:1355812810-32747-1-git-send-email-amdragon@mit.edu
> 
> Austin's patch significantly simplifies the invisibility handling
> taking this series down from 90/27 to 68/26 in diffstat terms.
> 
> In general terms Austin's approach has to be the right thing to do:
> what we want to do just before the freeze is less clear. My view is
> that we should go with Austin's approach now so that at least any bugs
> we get from it and (more likely) from this series apply to master as
> well. 
> 
> I am posting this series to make it easier for people to judge the two
> approaches when finished (ie with part invisibility too).
> 
> I attach a trimmed diff from v4 below the diffstat (note this was a
> diff with -U10 which I have trimmed so it is purely for information)
> 
> Note we no longer need patch 4/5 because in this approach the overlays
> do not need to know about other overlays.
> 
> Best wishes
> 
> Mark

LGTM.

Re: [PATCH v5 1/4] emacs: show: modify insert-part-header to save the button text

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> This just make notmuch-show-insert-part-header save the basic button
> text for parts as an attribute. This makes it simpler for the button
> action (added in a later patch) to reword the label as appropriate (eg
> append "(not shown)" or not as appropriate).

Pushed. At a quick try, it seems very nice. 

I had to rebase patch 2, please double check I didn't mangle something.

Also, definitely NEWS-worthy...

d