[PATCH v4 0/5] Use invisibility to toggle display of all parts including multipart

[PATCH v4 0/5] Use invisibility to toggle display of all parts including multipart

[Mark Walters]

This is version 4 of this series (previous version at
id:1355559338-14313-1-git-send-email-markwalters1009@gmail.com).

The only change should be a bugfix which, for reasons I don't
understand, only causes a problem on emacs 24. The problem is that the
part invisibility code looks for a part button at the start of the
region. This gets confused if there is a part with no part button
(this is the case for the first part if it is text/plain) and the part
starts with a button (as can happen if the message starts with the
reply as in the first test in test/emacs-show).

This checks that the button is a part button before creating the part
overlay.

The diff is below the diffstat. 

Best wishes

Mark



Mark Walters (5):
  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: wash: fix fake-diff part to include msg parameter
  emacs: show: set default show-all-multipart/alternatives to nil

 emacs/notmuch-show.el |  115 ++++++++++++++++++++++++++++++++++++++-----------
 emacs/notmuch-wash.el |    2 +-
 2 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5ca6fe2..3816e32 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -865,8 +865,10 @@ message at DEPTH in the current thread."
   (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.
-    (when (and button (/= part-beg end))
+    ;; 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))
-- 
1.7.9.1

[PATCH v4 1/5] 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 7d9f8a9..c39d2fb 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 v4 2/5] 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 |   62 +++++++++++++++++++++++++++++++++++++------------
 1 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index c39d2fb..7a81519 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,50 @@ 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))
+      (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)))))
+
+(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 v4 3/5] emacs: show: add invisibility button action

[Mark Walters]

This adds a button action to show hidden parts. In this version "t"
toggles the visibility of a part. In addition "RET" on a non-shown
part shows it.

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

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 7a81519..05931b7 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -554,6 +554,28 @@ 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))
+	 (invis-spec (button-get button 'invisibility-spec))
+	 (show (invisible-p invis-spec)))
+    (when overlay
+      (if show
+	  (remove-from-invisibility-spec invis-spec)
+	(add-to-invisibility-spec invis-spec))
+      (let* ((new-start (button-start button))
+	     (button-label (button-get button :base-label))
+	     (old-point (point))
+	     (inhibit-read-only t))
+	(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)))
@@ -864,7 +886,13 @@ message at DEPTH in the current thread."
 			       (cons invis-spec (overlay-get inner 'invisible)))))
 
 	(button-put button 'invisibility-spec invis-spec)
-	(button-put button 'overlay overlay)))))
+	(button-put button 'overlay overlay))
+
+      ;; 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.
@@ -2002,7 +2030,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 'invisibility-spec)
+	(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 v4 4/5] emacs: wash: fix fake-diff part to include msg parameter

[Mark Walters]

The fake-diff part from notmuch wash did not pass the msg parameter to
notmuch-show-insert-bodypart. Previously this did not matter, but is
now needed to get the overlays right for invisibility.
---
 emacs/notmuch-wash.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 7d003a2..b2affba 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -378,7 +378,7 @@ for error."
 			       (plist-get
 				(plist-get msg :headers) :Subject))))
 	(delete-region (point-min) (point-max))
-	(notmuch-show-insert-bodypart nil part depth)))))
+	(notmuch-show-insert-bodypart msg part depth)))))
 
 ;;
 
-- 
1.7.9.1

[PATCH v4 5/5] 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 05931b7..355ff49 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 v4 0/5] Use invisibility to toggle display of all parts including multipart

[Mark Walters]

On Mon, 17 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> This is version 4 of this series (previous version at
> id:1355559338-14313-1-git-send-email-markwalters1009@gmail.com).
>
> The only change should be a bugfix which, for reasons I don't
> understand, only causes a problem on emacs 24. The problem is that the
> part invisibility code looks for a part button at the start of the
> region. This gets confused if there is a part with no part button
> (this is the case for the first part if it is text/plain) and the part
> starts with a button (as can happen if the message starts with the
> reply as in the first test in test/emacs-show).
>
> This checks that the button is a part button before creating the part
> overlay.

I don't think the above is very clear so I will try to explain it more
fully. 

The invisibility overlay for a part needs to be `linked' to the part
header button so that the part header button can toggle the overlay
visibility. The overlay is created and linked to this button after the
whole part has been inserted (including any notmuch-wash stuff). 

I could have made insert-part-header return the button it made and pass
it back up the call chain to the the create-overlays function but
instead I chose to make create-overlays just take the button at the
start of the part.

Now if the first part is text/plain then notmuch does not insert a
[text/plain] button so the code checks for this case by making sure the
part does start with a button, and if not it does not create the part
overlay (there is no button to toggle it so no point in an overlay).

However, if the first part is text/plain and notmuch wash happens to
make a button at the very start of the part then the create-overlays
function did still create an overlay *and* link it to the button. This
linking overwrote some of the things notmuch wash had attached to its
button (eg the button :overlay property) and that caused things to
break.

I still do not know why emacs 23 and emacs 24 behave differently, but
regardless the change from v3 is a clear bugfix: we just make sure it is
a notmuch-show-insert-part-header button not a notmuch-wash button
before we do the overlay creation/linking to the button. This version
does that by looking for a :base-label property of the button which
insert-part-header buttons have but notmuch-wash buttons do
not. (Obviously there are other ways this check could be done)

Best wishes

Mark



> The diff is below the diffstat. 
>
> Best wishes
>
> Mark
>
>
>
> Mark Walters (5):
>   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: wash: fix fake-diff part to include msg parameter
>   emacs: show: set default show-all-multipart/alternatives to nil
>
>  emacs/notmuch-show.el |  115 ++++++++++++++++++++++++++++++++++++++-----------
>  emacs/notmuch-wash.el |    2 +-
>  2 files changed, 90 insertions(+), 27 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5ca6fe2..3816e32 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -865,8 +865,10 @@ message at DEPTH in the current thread."
>    (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.
> -    (when (and button (/= part-beg end))
> +    ;; 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))
> -- 
> 1.7.9.1

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

[Austin Clements]

Quoth Mark Walters on Dec 18 at  8:54 am:
> On Mon, 17 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> > This is version 4 of this series (previous version at
> > id:1355559338-14313-1-git-send-email-markwalters1009@gmail.com).
> >
> > The only change should be a bugfix which, for reasons I don't
> > understand, only causes a problem on emacs 24. The problem is that the
> > part invisibility code looks for a part button at the start of the
> > region. This gets confused if there is a part with no part button
> > (this is the case for the first part if it is text/plain) and the part
> > starts with a button (as can happen if the message starts with the
> > reply as in the first test in test/emacs-show).
> >
> > This checks that the button is a part button before creating the part
> > overlay.
> 
> I don't think the above is very clear so I will try to explain it more
> fully. 
> 
> The invisibility overlay for a part needs to be `linked' to the part
> header button so that the part header button can toggle the overlay
> visibility. The overlay is created and linked to this button after the
> whole part has been inserted (including any notmuch-wash stuff). 
> 
> I could have made insert-part-header return the button it made and pass
> it back up the call chain to the the create-overlays function but
> instead I chose to make create-overlays just take the button at the
> start of the part.
> 
> Now if the first part is text/plain then notmuch does not insert a
> [text/plain] button so the code checks for this case by making sure the
> part does start with a button, and if not it does not create the part
> overlay (there is no button to toggle it so no point in an overlay).
> 
> However, if the first part is text/plain and notmuch wash happens to
> make a button at the very start of the part then the create-overlays
> function did still create an overlay *and* link it to the button. This
> linking overwrote some of the things notmuch wash had attached to its
> button (eg the button :overlay property) and that caused things to
> break.
> 
> I still do not know why emacs 23 and emacs 24 behave differently, but
> regardless the change from v3 is a clear bugfix: we just make sure it is
> a notmuch-show-insert-part-header button not a notmuch-wash button
> before we do the overlay creation/linking to the button. This version
> does that by looking for a :base-label property of the button which
> insert-part-header buttons have but notmuch-wash buttons do
> not. (Obviously there are other ways this check could be done)

Now I understand.  LGTM.  Do you want to go ahead and push this or
would you rather get my wash/show cleanup in and push your reworked
version of the series?