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

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

[Mark Walters]

This patch aims to do the same as
id:1354496317-24564-1-git-send-email-markwalters1009@gmail.com but
rather than reloading the buffer it uses invisibility. 

Most of the invisibility stuff was taken from notmuch-wash and adapted
to this situation.

The general interface is that any part can have its visibility
toggled: in this version by "t" on the part button. In addition "RET"
on the part button of any "not shown" part will show it.

Whilst a main driver was being able to view different parts of a
multipart/alternative message, this also allows the user to toggle
images or large inline text attachments (eg text/x-tex) for
example. Indeed, even multipart part buttons can be toggled which
remove the whole tree beneath: it is unclear whether this last is useful.

The invisibility approach has some advantages over the reloading
approach. It does not disrupt the rest of the buffer (eg
collapsed/expanded citations remain), it does not require an extra
call to the database with the possible addition of messages to the
buffer, and it fits more naturally with the other hidden/not-hidden
items.

Best wishes

Mark


Mark Walters (3):
  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/notmuch-show.el |  117 +++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 93 insertions(+), 24 deletions(-)

-- 
1.7.9.1

[PATCH 1/3] 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 |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index d7fa10e..f8ce037 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -483,17 +483,18 @@ 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 (if name (concat name ": ") "")
+			    declared-type
+			    (if (not (string-equal declared-type content-type))
+				(concat " (as " content-type ")")
+			      "")
+			    (or 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 2/3] emacs: show: add overlays for each part

[Mark Walters]

This make 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 a not-shown variable which is used to request the part be
hidden by default down to the overlay but this is not acted on yet.
---
 emacs/notmuch-show.el |   62 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index f8ce037..3215ebc 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -569,10 +569,9 @@ message at DEPTH in the current thread."
     ;; 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)"))))
+	      (notmuch-show-insert-bodypart msg inner-part depth
+					    (not (or notmuch-show-all-multipart/alternative-parts
+						     (string= chosen-type inner-type))))))
 	  inner-parts)
 
     (when notmuch-show-indent-multipart
@@ -840,17 +839,52 @@ 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-insert-part-overlays (msg beg end not-shown)
+  "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.
+    (unless (or (not button) (eq part-beg end))
+      (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 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
+	(overlay-put overlay 'priority 10)
+	(overlay-put overlay 'type "part")
+	;; Now we have to add invis-spec to every overlay this
+	;; overlay contains, otherwise these inner overlays will
+	;; override this one.
+	(mapc (lambda (inner)
+		(when (and (>= (overlay-start inner) part-beg)
+			   (<= (overlay-end inner) end))
+		  (overlay-put inner 'invisible
+			       (cons invis-spec (overlay-get inner 'invisible)))))
+	      (overlays-in part-beg end))
+
+	(button-put button 'invisibility-spec invis-spec)
+	(button-put button 'overlay overlay))
+      (goto-char (point-max)))))
+
+(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
+  "Insert the body part PART at depth DEPTH in the current thread.
+
+If not-shown is TRUE 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-insert-part-overlays msg beg (point) not-shown)))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
-- 
1.7.9.1

[PATCH 3/3] 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 |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 3215ebc..a4daff8 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -478,6 +478,7 @@ message at DEPTH in the current thread."
     (define-key map "v" 'notmuch-show-part-button-view)
     (define-key map "o" 'notmuch-show-part-button-interactively-view)
     (define-key map "|" 'notmuch-show-part-button-pipe)
+    (define-key map "t" 'notmuch-show-toggle-invisible-part-action)
     map)
   "Submap for button commands")
 (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
@@ -555,6 +556,31 @@ 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-invisible-part-action (&optional button no-redisplay)
+  (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 " ]" " (not shown) ]"))
+	(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)))))
+      (unless no-redisplay
+	(force-window-update)
+	(redisplay t)))))
+
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
@@ -867,6 +893,11 @@ message at DEPTH in the current thread."
 
 	(button-put button 'invisibility-spec invis-spec)
 	(button-put button 'overlay overlay))
+
+      ;; We toggle the button for hidden parts as that gets the
+      ;; button label right.
+      (when not-shown
+	(notmuch-show-toggle-invisible-part-action button t))
       (goto-char (point-max)))))
 
 (defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
@@ -1996,7 +2027,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 (invisible-p (button-get button 'invisibility-spec))
+	(notmuch-show-toggle-invisible-part-action 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] emacs: show: make RET always toggle parts where plausible

[Mark Walters]

This makes RET toggle the visibility of any part which has a component
that can be displayed in the buffer. This included text parts (plain/
html/ x-tex etc) and images. Parts which cannot be displayed (eg pdf)
RET acts as before and saves/views etc.

Definite actions can always be accessed with s,v and o (eg v to view a
text/html part in a browser).

---

This is a slight tweak at Jani's suggestion on irc. Applies on top of
the previous series.



 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 a4daff8..e319e5c 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -2028,7 +2028,7 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
 (defun notmuch-show-part-button-default (&optional button)
   (interactive)
   (let ((button (or button (button-at (point)))))
-    (if (invisible-p (button-get button 'invisibility-spec))
+    (if (button-get button 'invisibility-spec)
 	(notmuch-show-toggle-invisible-part-action button)
       (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))))
 
-- 
1.7.9.1

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

[Jani Nikula]

Hi Mark -

On Wed, 05 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> This patch aims to do the same as
> id:1354496317-24564-1-git-send-email-markwalters1009@gmail.com but
> rather than reloading the buffer it uses invisibility. 
>
> Most of the invisibility stuff was taken from notmuch-wash and adapted
> to this situation.
>
> The general interface is that any part can have its visibility
> toggled: in this version by "t" on the part button. In addition "RET"
> on the part button of any "not shown" part will show it.

I tested the patches (did not look at the code), and with the addition
of your RET patch on top, I think the series is perfect! I very much
like the functionality. Must have.

> Whilst a main driver was being able to view different parts of a
> multipart/alternative message, this also allows the user to toggle
> images or large inline text attachments (eg text/x-tex) for
> example. Indeed, even multipart part buttons can be toggled which
> remove the whole tree beneath: it is unclear whether this last is useful.

It's very nice that this is a general solution showing/hiding parts, and
the original multipart/alternative problem is just a special case this
handles. And if there's any more tweaking the special case needs, it
should be done on top of this series, afterwards.

> The invisibility approach has some advantages over the reloading
> approach. It does not disrupt the rest of the buffer (eg
> collapsed/expanded citations remain), it does not require an extra
> call to the database with the possible addition of messages to the
> buffer, and it fits more naturally with the other hidden/not-hidden
> items.

Yes, much better than the original (which already was good).

Thanks for doing this.

BR,
Jani.


>
> Best wishes
>
> Mark
>
>
> Mark Walters (3):
>   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/notmuch-show.el |  117 +++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 93 insertions(+), 24 deletions(-)
>
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Austin Clements]

On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> 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 |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index d7fa10e..f8ce037 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -483,17 +483,18 @@ 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 (if name (concat name ": ") "")
> +			    declared-type
> +			    (if (not (string-equal declared-type content-type))
> +				(concat " (as " content-type ")")
> +			      "")
> +			    (or comment ""))))
> +

This patch is obviously fine, but if you don't mind some drive-by
cleanup, this could be made simpler by taking advantage of how concat
omits nils:

(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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/3] emacs: show: add overlays for each part

[Austin Clements]

On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> This make notmuch-show-insert-bodypart add an overlay for any

s/make/makes/

> 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 a not-shown variable which is used to request the part be

not-shown is really an argument (I found this confusing).

> hidden by default down to the overlay but this is not acted on yet.
> ---
>  emacs/notmuch-show.el |   62 +++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index f8ce037..3215ebc 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -569,10 +569,9 @@ message at DEPTH in the current thread."
>      ;; 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)"))))
> +	      (notmuch-show-insert-bodypart msg inner-part depth
> +					    (not (or notmuch-show-all-multipart/alternative-parts

Since notmuch-show-all-multipart/alternative-parts was basically a hack
around our poor multipart/alternative support, I think this series (or a
follow up patch) should change its default to nil or even eliminate it
entirely.

> +						     (string= chosen-type inner-type))))))

You could let-bind the (not (or ..)) to some variable ("hide" perhaps)
in the let above to avoid this crazy line length.

>  	  inner-parts)
>  
>      (when notmuch-show-indent-multipart
> @@ -840,17 +839,52 @@ 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-insert-part-overlays (msg beg end not-shown)

s/not-shown/hide/?  Or hidden?

> +  "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.
> +    (unless (or (not button) (eq part-beg end))

(when (and button (/= part-beg end)) ...) ?

> +      (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))

Non-trivial buffer invisibility specs are really bad for performance
(Emacs' renderer does the obvious O(n^2) thing when rendering a buffer
with an invisibility spec).  Unfortunately, because of notmuch-wash and
the way overlays with trivial 'invisible properties combine with
overlays with list-type 'invisible properties combine, I don't think it
can be avoided.  But if we get rid of buffer invisibility specs from
notmuch-wash, this code can also get much simpler.

> +	(overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)

This will leave the "(not shown)" in the part header if isearch unfolds
the part.

Do we even want isearch to unfold parts?  It's not clear we do for
multipart/alternative.  If we do, probably the right thing is something
like

(overlay-put overlay 'notmuch-show-part-button button)
(overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)

(defun notmuch-show-part-isearch-open (overlay)
  (notmuch-show-toggle-invisible-part-action
   (overlay-get overlay 'notmuch-show-part-button)))

> +	(overlay-put overlay 'priority 10)
> +	(overlay-put overlay 'type "part")
> +	;; Now we have to add invis-spec to every overlay this
> +	;; overlay contains, otherwise these inner overlays will
> +	;; override this one.

Interesting.  In the simple case of using nil or t for 'invisible, the
specs do combine as one would expect, but you're right that, with a
non-trivial invisibility-spec, the highest priority overlay wins.  It's
too bad we don't know the depth of the part or we could just set the
overlay priority.  This is another thing that would go away if we didn't
use buffer invisibility-specs.

> +	(mapc (lambda (inner)

I would use a (dolist (inner (overlays-in part-beg end)) ...) here.
Seems a little more readable.

> +		(when (and (>= (overlay-start inner) part-beg)
> +			   (<= (overlay-end inner) end))
> +		  (overlay-put inner 'invisible
> +			       (cons invis-spec (overlay-get inner 'invisible)))))
> +	      (overlays-in part-beg end))
> +
> +	(button-put button 'invisibility-spec invis-spec)
> +	(button-put button 'overlay overlay))
> +      (goto-char (point-max)))))

This goto-char seems oddly out of place, since it has nothing to do with
overlay creation.  Was it supposed to be here instead of in
notmuch-show-insert-bodypart?  Is it even necessary?

> +
> +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)

Same comment about not-shown.  (Also in the commit message.)

> +  "Insert the body part PART at depth DEPTH in the current thread.
> +
> +If not-shown is TRUE then initially hide this part."

s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/

>    (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-insert-part-overlays msg beg (point) not-shown)))
>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
> -- 
> 1.7.9.1

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

[Austin Clements]

On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> 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 |   36 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 35 insertions(+), 1 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 3215ebc..a4daff8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -478,6 +478,7 @@ message at DEPTH in the current thread."
>      (define-key map "v" 'notmuch-show-part-button-view)
>      (define-key map "o" 'notmuch-show-part-button-interactively-view)
>      (define-key map "|" 'notmuch-show-part-button-pipe)
> +    (define-key map "t" 'notmuch-show-toggle-invisible-part-action)
>      map)
>    "Submap for button commands")
>  (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
> @@ -555,6 +556,31 @@ 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-invisible-part-action (&optional button no-redisplay)
> +  (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 " ]" " (not shown) ]"))

s/not shown/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)))))
> +      (unless no-redisplay
> +	(force-window-update)
> +	(redisplay t)))))

Is the t argument necessary?  Actually, are either (force-window-update)
or (redisplay) necessary?

> +
>  (defun notmuch-show-multipart/*-to-list (part)
>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
>  	  (plist-get part :content)))
> @@ -867,6 +893,11 @@ message at DEPTH in the current thread."
>  
>  	(button-put button 'invisibility-spec invis-spec)
>  	(button-put button 'overlay overlay))
> +
> +      ;; We toggle the button for hidden parts as that gets the
> +      ;; button label right.
> +      (when not-shown
> +	(notmuch-show-toggle-invisible-part-action button t))
>        (goto-char (point-max)))))
>  
>  (defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
> @@ -1996,7 +2027,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 (invisible-p (button-get button 'invisibility-spec))
> +	(notmuch-show-toggle-invisible-part-action 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

Re: [PATCH] emacs: show: make RET always toggle parts where plausible

[Austin Clements]

I would just fold this patch in with the rest of the series.  This is
definitely the right way to toggle.

On Wed, 05 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> This makes RET toggle the visibility of any part which has a component
> that can be displayed in the buffer. This included text parts (plain/
> html/ x-tex etc) and images. Parts which cannot be displayed (eg pdf)
> RET acts as before and saves/views etc.
>
> Definite actions can always be accessed with s,v and o (eg v to view a
> text/html part in a browser).
>
> ---
>
> This is a slight tweak at Jani's suggestion on irc. Applies on top of
> the previous series.
>
>
>
>  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 a4daff8..e319e5c 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -2028,7 +2028,7 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
>  (defun notmuch-show-part-button-default (&optional button)
>    (interactive)
>    (let ((button (or button (button-at (point)))))
> -    (if (invisible-p (button-get button 'invisibility-spec))
> +    (if (button-get button 'invisibility-spec)
>  	(notmuch-show-toggle-invisible-part-action button)
>        (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))))
>  
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/3] emacs: show: add overlays for each part

[Mark Walters]

Hi 

Many thanks for the review: I will post a new version shortly. Some
comments inline below:

On Tue, 11 Dec 2012, Austin Clements <aclements@csail.mit.edu> wrote:
> On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> This make notmuch-show-insert-bodypart add an overlay for any
>
> s/make/makes/
>
>> 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 a not-shown variable which is used to request the part be
>
> not-shown is really an argument (I found this confusing).
>
>> hidden by default down to the overlay but this is not acted on yet.
>> ---
>>  emacs/notmuch-show.el |   62 +++++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 48 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index f8ce037..3215ebc 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -569,10 +569,9 @@ message at DEPTH in the current thread."
>>      ;; 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)"))))
>> +	      (notmuch-show-insert-bodypart msg inner-part depth
>> +					    (not (or notmuch-show-all-multipart/alternative-parts
>
> Since notmuch-show-all-multipart/alternative-parts was basically a hack
> around our poor multipart/alternative support, I think this series (or a
> follow up patch) should change its default to nil or even eliminate it
> entirely.

I have added a patch at the end setting this to nil by default.

>> +						     (string= chosen-type inner-type))))))
>
> You could let-bind the (not (or ..)) to some variable ("hide" perhaps)
> in the let above to avoid this crazy line length.
>
>>  	  inner-parts)
>>  
>>      (when notmuch-show-indent-multipart
>> @@ -840,17 +839,52 @@ 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-insert-part-overlays (msg beg end not-shown)
>
> s/not-shown/hide/?  Or hidden?
>
>> +  "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.
>> +    (unless (or (not button) (eq part-beg end))
>
> (when (and button (/= part-beg end)) ...) ?
>
>> +      (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))
>
> Non-trivial buffer invisibility specs are really bad for performance
> (Emacs' renderer does the obvious O(n^2) thing when rendering a buffer
> with an invisibility spec).  Unfortunately, because of notmuch-wash and
> the way overlays with trivial 'invisible properties combine with
> overlays with list-type 'invisible properties combine, I don't think it
> can be avoided.  But if we get rid of buffer invisibility specs from
> notmuch-wash, this code can also get much simpler.

How do you plan to get rid of the invisibility properties from notmuch
wash? 

>> +	(overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
>
> This will leave the "(not shown)" in the part header if isearch unfolds
> the part.
>
> Do we even want isearch to unfold parts?  It's not clear we do for
> multipart/alternative.  If we do, probably the right thing is something
> like

I don't think we want to search hidden bodyparts so I just deleted this
line.

>
> (overlay-put overlay 'notmuch-show-part-button button)
> (overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)
>
> (defun notmuch-show-part-isearch-open (overlay)
>   (notmuch-show-toggle-invisible-part-action
>    (overlay-get overlay 'notmuch-show-part-button)))
>
>> +	(overlay-put overlay 'priority 10)
>> +	(overlay-put overlay 'type "part")
>> +	;; Now we have to add invis-spec to every overlay this
>> +	;; overlay contains, otherwise these inner overlays will
>> +	;; override this one.
>
> Interesting.  In the simple case of using nil or t for 'invisible, the
> specs do combine as one would expect, but you're right that, with a
> non-trivial invisibility-spec, the highest priority overlay wins.  It's
> too bad we don't know the depth of the part or we could just set the
> overlay priority.  This is another thing that would go away if we didn't
> use buffer invisibility-specs.

I don't think priorities get it right. As far as I could tell if you
have two overlays at different priorities both with invisibility
properties then the higher priority overlay decides visibility by
itself: that is if it says invisible then the text is invisible, and if
it says visible then the text is visible.

>
>> +	(mapc (lambda (inner)
>
> I would use a (dolist (inner (overlays-in part-beg end)) ...) here.
> Seems a little more readable.
>
>> +		(when (and (>= (overlay-start inner) part-beg)
>> +			   (<= (overlay-end inner) end))
>> +		  (overlay-put inner 'invisible
>> +			       (cons invis-spec (overlay-get inner 'invisible)))))
>> +	      (overlays-in part-beg end))
>> +
>> +	(button-put button 'invisibility-spec invis-spec)
>> +	(button-put button 'overlay overlay))
>> +      (goto-char (point-max)))))
>
> This goto-char seems oddly out of place, since it has nothing to do with
> overlay creation.  Was it supposed to be here instead of in
> notmuch-show-insert-bodypart?  Is it even necessary?

It was suppose to be in notmuch-show-insert-body-part. It was necessary
but I have wrapped the toggle-button call in the third patch and now it
does not seem necessary.

It is possible that the toggle-button function should not move point: if
that got changed the save-excursion here could be removed. (I will
discuss this in my reply to your comments on the next patch).

Best wishes 

Mark

>
>> +
>> +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
>
> Same comment about not-shown.  (Also in the commit message.)
>
>> +  "Insert the body part PART at depth DEPTH in the current thread.
>> +
>> +If not-shown is TRUE then initially hide this part."
>
> s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/
>
>>    (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-insert-part-overlays msg beg (point) not-shown)))
>>  
>>  (defun notmuch-show-insert-body (msg body depth)
>>    "Insert the body BODY at depth DEPTH in the current thread."
>> -- 
>> 1.7.9.1

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

[Mark Walters]

On Tue, 11 Dec 2012, Austin Clements <aclements@csail.mit.edu> wrote:
> On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> 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 |   36 +++++++++++++++++++++++++++++++++++-
>>  1 files changed, 35 insertions(+), 1 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index 3215ebc..a4daff8 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -478,6 +478,7 @@ message at DEPTH in the current thread."
>>      (define-key map "v" 'notmuch-show-part-button-view)
>>      (define-key map "o" 'notmuch-show-part-button-interactively-view)
>>      (define-key map "|" 'notmuch-show-part-button-pipe)
>> +    (define-key map "t" 'notmuch-show-toggle-invisible-part-action)
>>      map)
>>    "Submap for button commands")
>>  (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
>> @@ -555,6 +556,31 @@ 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-invisible-part-action (&optional button no-redisplay)
>> +  (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 " ]" " (not shown) ]"))
>
> s/not shown/hidden/?

Done

>
>> +	(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)))))

This is where point gets left by this toggle button function: I don't
know if it should move point. Mostly it only matters if the user uses
the mouse (as otherwise point is on the button).

>> +      (unless no-redisplay
>> +	(force-window-update)
>> +	(redisplay t)))))
>
> Is the t argument necessary?  Actually, are either (force-window-update)
> or (redisplay) necessary?

None of this seems needed: I had just copied it from notmuch-wash. 

Best wishes

Mark

>> +
>>  (defun notmuch-show-multipart/*-to-list (part)
>>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
>>  	  (plist-get part :content)))
>> @@ -867,6 +893,11 @@ message at DEPTH in the current thread."
>>  
>>  	(button-put button 'invisibility-spec invis-spec)
>>  	(button-put button 'overlay overlay))
>> +
>> +      ;; We toggle the button for hidden parts as that gets the
>> +      ;; button label right.
>> +      (when not-shown
>> +	(notmuch-show-toggle-invisible-part-action button t))
>>        (goto-char (point-max)))))
>>  
>>  (defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
>> @@ -1996,7 +2027,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 (invisible-p (button-get button 'invisibility-spec))
>> +	(notmuch-show-toggle-invisible-part-action 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

Re: [PATCH 2/3] emacs: show: add overlays for each part

[Austin Clements]

Quoth Mark Walters on Dec 13 at  8:54 am:
> >> +						     (string= chosen-type inner-type))))))
> >
> > You could let-bind the (not (or ..)) to some variable ("hide" perhaps)
> > in the let above to avoid this crazy line length.
> >
> >>  	  inner-parts)
> >>  
> >>      (when notmuch-show-indent-multipart
> >> @@ -840,17 +839,52 @@ 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-insert-part-overlays (msg beg end not-shown)
> >
> > s/not-shown/hide/?  Or hidden?
> >
> >> +  "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.
> >> +    (unless (or (not button) (eq part-beg end))
> >
> > (when (and button (/= part-beg end)) ...) ?
> >
> >> +      (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))
> >
> > Non-trivial buffer invisibility specs are really bad for performance
> > (Emacs' renderer does the obvious O(n^2) thing when rendering a buffer
> > with an invisibility spec).  Unfortunately, because of notmuch-wash and
> > the way overlays with trivial 'invisible properties combine with
> > overlays with list-type 'invisible properties combine, I don't think it
> > can be avoided.  But if we get rid of buffer invisibility specs from
> > notmuch-wash, this code can also get much simpler.
> 
> How do you plan to get rid of the invisibility properties from notmuch
> wash? 

Not the invisibility properties.  The invisibility specs.  The
performance impact and difficult composition comes from using buffer
invisibility specs, but the 'invisible text property can be simply nil
or t.  These are just as easy to manipulate as the generated
invisibility symbols we use all over the place (you just manipulate
the property directly instead of the buffer invisibility spec).  They
also compose like you'd want (if any overlay over some text has
'invisible set to t, then that text is invisible).

> >> +	(overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
> >
> > This will leave the "(not shown)" in the part header if isearch unfolds
> > the part.
> >
> > Do we even want isearch to unfold parts?  It's not clear we do for
> > multipart/alternative.  If we do, probably the right thing is something
> > like
> 
> I don't think we want to search hidden bodyparts so I just deleted this
> line.
> 
> >
> > (overlay-put overlay 'notmuch-show-part-button button)
> > (overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)
> >
> > (defun notmuch-show-part-isearch-open (overlay)
> >   (notmuch-show-toggle-invisible-part-action
> >    (overlay-get overlay 'notmuch-show-part-button)))
> >
> >> +	(overlay-put overlay 'priority 10)
> >> +	(overlay-put overlay 'type "part")
> >> +	;; Now we have to add invis-spec to every overlay this
> >> +	;; overlay contains, otherwise these inner overlays will
> >> +	;; override this one.
> >
> > Interesting.  In the simple case of using nil or t for 'invisible, the
> > specs do combine as one would expect, but you're right that, with a
> > non-trivial invisibility-spec, the highest priority overlay wins.  It's
> > too bad we don't know the depth of the part or we could just set the
> > overlay priority.  This is another thing that would go away if we didn't
> > use buffer invisibility-specs.
> 
> I don't think priorities get it right. As far as I could tell if you
> have two overlays at different priorities both with invisibility
> properties then the higher priority overlay decides visibility by
> itself: that is if it says invisible then the text is invisible, and if
> it says visible then the text is visible.

Yes.  I think I was confused when I claimed we could use priorities,
since I can't reconstruct how I thought we could do that.