[PATCH (draft) 0/2] Allow emacs to toggle display of all parts including multipart

[PATCH (draft) 0/2] Allow emacs to toggle display of all parts including multipart

[Mark Walters]

This patch series adds a function to toggle the display of any emacs
part in the show buffer. This is particularly useful for
multipart/alternative where the parts have different information.

The first patch binds this toggle to 't' on the part button. This
patch works by itself.

The second patch makes it that "viewing" (ie v on the part-button) a
"not shown" part displays the part in the buffer. Note this is not a
toggle since having displayed the part in the buffer the user may wish
to view the part externally (eg if it is a text/html part).

Caveats: 
	 the patches work by reloading the whole buffer: this may mean
	 extra messages appear in the thread. This is not ideal but is
	 the same as toggling indentation etc.

	 the reload saves state as normal but this means the view
	 returns to the top of the current message. Since we know
	 exactly where the user was (ie on the button) this should be
	 restored.

	 for technical reasons the "view" part functionality in the
	 second patch only works for view called explicitly: not if
	 the user has customised the default action to be view and
	 calls that. This is easy to fix but the most natural way
	 would break existing customisation of this action.

	 reloading the buffer without keeping state does not currently
	 reset the toggled parts. Again this is easy to fix but lets
	 see if people like the general approach first.

Best wishes

Mark


Mark Walters (2):
  emacs: allow the user to toggle the visibility of
    multipart/alternative parts
  emacs: show: make "view part" show hidden parts

 emacs/notmuch-show.el |   53 ++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 44 insertions(+), 9 deletions(-)

-- 
1.7.9.1

[PATCH (draft) 1/2] emacs: allow the user to toggle the visibility of multipart/alternative parts

[Mark Walters]

This patch adds a keybinding to the buttons in the notmuch-show emacs
buffer to allow the user to toggle the visibility of each part of a
message in the show buffer.  This is particularly useful for
multipart/alternative parts where the parts are not really
alternatives but contain different information.
---
 emacs/notmuch-show.el |   47 +++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 0f54259..9157669 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -155,6 +155,10 @@ indentation."
 (make-variable-buffer-local 'notmuch-show-indent-content)
 (put 'notmuch-show-indent-content 'permanent-local t)
 
+(defvar notmuch-show-message-multipart/alternative-display-parts nil)
+(make-variable-buffer-local 'notmuch-show-message-multipart/alternative-display-parts)
+(put 'notmuch-show-message-multipart/alternative-display-parts 'permanent-local t)
+
 (defcustom notmuch-show-stash-mlarchive-link-alist
   '(("Gmane" . "http://mid.gmane.org/")
     ("MARC" . "http://marc.info/?i=")
@@ -455,6 +459,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-part-button-internally-show)
     map)
   "Submap for button commands")
 (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
@@ -531,6 +536,16 @@ message at DEPTH in the current thread."
     (let ((handle (mm-make-handle (current-buffer) (list content-type))))
       (mm-pipe-part handle))))
 
+(defun notmuch-show-internally-show-part (message-id nth &optional filename content-type)
+  "Set a part to be displayed internally"
+  (let ((current-parts (lax-plist-get notmuch-show-message-multipart/alternative-display-parts message-id)))
+    (setq notmuch-show-message-multipart/alternative-display-parts
+	  (lax-plist-put notmuch-show-message-multipart/alternative-display-parts message-id
+			 (if (memq nth current-parts)
+			     (delq nth current-parts)
+			   (cons nth current-parts)))))
+  (notmuch-show-refresh-view))
+
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
@@ -543,12 +558,15 @@ message at DEPTH in the current thread."
     ;; This inserts all parts of the chosen type rather than just one,
     ;; 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?
+
+    ;; The variable user-parts says which parts should override the
+    ;; default so we use xor (handcoded since lisp does not have it).
+
     (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
@@ -806,11 +824,20 @@ 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."
-  (let ((content-type (downcase (plist-get part :content-type)))
+(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 do not show the part unless the user
+has overridden the default for this part"
+  (let ((user-parts (lax-plist-get notmuch-show-message-multipart/alternative-display-parts
+				   (notmuch-id-to-query (plist-get msg :id))))
+	(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))
+    (when (memq nth user-parts)
+      (setq not-shown (not not-shown)))
+    (if not-shown
+	(notmuch-show-insert-part-header nth content-type content-type nil " (not shown)")
+      (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))
@@ -1895,6 +1922,10 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
   (interactive)
   (notmuch-show-part-button-internal button #'notmuch-show-pipe-part))
 
+(defun notmuch-show-part-button-internally-show (&optional button)
+  (interactive)
+  (notmuch-show-part-button-internal button #'notmuch-show-internally-show-part))
+
 (defun notmuch-show-part-button-internal (button handler)
   (let ((button (or button (button-at (point)))))
     (if button
-- 
1.7.9.1

Re: [PATCH (draft) 1/2] emacs: allow the user to toggle the visibility of multipart/alternative parts

[Ethan Glasser-Camp]

Mark Walters <markwalters1009@gmail.com> writes:

> This patch adds a keybinding to the buttons in the notmuch-show emacs
> buffer to allow the user to toggle the visibility of each part of a
> message in the show buffer.  This is particularly useful for
> multipart/alternative parts where the parts are not really
> alternatives but contain different information.
> ---
>  emacs/notmuch-show.el |   47 +++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 0f54259..9157669 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -155,6 +155,10 @@ indentation."
>  (make-variable-buffer-local 'notmuch-show-indent-content)
>  (put 'notmuch-show-indent-content 'permanent-local t)
>
> +(defvar notmuch-show-message-multipart/alternative-display-parts nil)
> +(make-variable-buffer-local 'notmuch-show-message-multipart/alternative-display-parts)
> +(put 'notmuch-show-message-multipart/alternative-display-parts 'permanent-local t)
> +
>  (defcustom notmuch-show-stash-mlarchive-link-alist
>    '(("Gmane" . "http://mid.gmane.org/")
>      ("MARC" . "http://marc.info/?i=")
> @@ -455,6 +459,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-part-button-internally-show)
>      map)
>    "Submap for button commands")
>  (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
> @@ -531,6 +536,16 @@ message at DEPTH in the current thread."
>      (let ((handle (mm-make-handle (current-buffer) (list content-type))))
>        (mm-pipe-part handle))))
>
> +(defun notmuch-show-internally-show-part (message-id nth &optional filename content-type)
> +  "Set a part to be displayed internally"
> +  (let ((current-parts (lax-plist-get notmuch-show-message-multipart/alternative-display-parts message-id)))
> +    (setq notmuch-show-message-multipart/alternative-display-parts
> +       (lax-plist-put notmuch-show-message-multipart/alternative-display-parts message-id
> +                      (if (memq nth current-parts)
> +                          (delq nth current-parts)
> +                        (cons nth current-parts)))))
> +  (notmuch-show-refresh-view))
> +
>  (defun notmuch-show-multipart/*-to-list (part)
>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
>         (plist-get part :content)))
> @@ -543,12 +558,15 @@ message at DEPTH in the current thread."
>      ;; This inserts all parts of the chosen type rather than just one,
>      ;; 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?
> +
> +    ;; The variable user-parts says which parts should override the
> +    ;; default so we use xor (handcoded since lisp does not have it).

I don't follow the comment. user-parts isn't used in this
function. Neither is xor.

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

For what it's worth, I found this not-shown logic very confusing, and
have had to think about it seven or eight different times to make sure I
understood what's going on. I'm not sure why exactly this is, though I
could offer hypotheses -- the fact that it's split across two functions,
or the fiddling with mime-types. I'm satisfied that it's correct, but I
wish it could be made clearer.

This is just armchair hypothesizing, but here are some ideas that might
make it more obvious what's going on: bringing the user-parts logic into
this function; making user-parts, instead of a "t" meaning "user has
toggled this", something like 'opened or 'closed and if user-parts for
this message is absent, falling back to this calculation; alternately,
prefilling user-parts with t when show is invoked, according to this
calculation, and then not using it any more; moving this
not-shown calculation into a separate function, something like notmuch-show-get-message-visibility.

I guess I jumped into this series halfway, but why are we doing this
with the wipe/redraw technique instead of just using invisible overlays,
like we do more generally with notmuch-show? I think I agree that
toggling individual parts is a good UI approach, and this isn't a bad
way to implement it, but I wonder if we could do it better/easier if we
used emacs's builtin functionality.

Thanks!

Ethan

Re: [PATCH (draft) 1/2] emacs: allow the user to toggle the visibility of multipart/alternative parts

[Mark Walters]

Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> writes:

> Mark Walters <markwalters1009@gmail.com> writes:
>
>> This patch adds a keybinding to the buttons in the notmuch-show emacs
>> buffer to allow the user to toggle the visibility of each part of a
>> message in the show buffer.  This is particularly useful for
>> multipart/alternative parts where the parts are not really
>> alternatives but contain different information.
>> ---
>>  emacs/notmuch-show.el |   47 +++++++++++++++++++++++++++++++++++++++--------
>>  1 files changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index 0f54259..9157669 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -155,6 +155,10 @@ indentation."
>>  (make-variable-buffer-local 'notmuch-show-indent-content)
>>  (put 'notmuch-show-indent-content 'permanent-local t)
>>
>> +(defvar notmuch-show-message-multipart/alternative-display-parts nil)
>> +(make-variable-buffer-local 'notmuch-show-message-multipart/alternative-display-parts)
>> +(put 'notmuch-show-message-multipart/alternative-display-parts 'permanent-local t)
>> +
>>  (defcustom notmuch-show-stash-mlarchive-link-alist
>>    '(("Gmane" . "http://mid.gmane.org/")
>>      ("MARC" . "http://marc.info/?i=")
>> @@ -455,6 +459,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-part-button-internally-show)
>>      map)
>>    "Submap for button commands")
>>  (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)
>> @@ -531,6 +536,16 @@ message at DEPTH in the current thread."
>>      (let ((handle (mm-make-handle (current-buffer) (list content-type))))
>>        (mm-pipe-part handle))))
>>
>> +(defun notmuch-show-internally-show-part (message-id nth &optional filename content-type)
>> +  "Set a part to be displayed internally"
>> +  (let ((current-parts (lax-plist-get notmuch-show-message-multipart/alternative-display-parts message-id)))
>> +    (setq notmuch-show-message-multipart/alternative-display-parts
>> +       (lax-plist-put notmuch-show-message-multipart/alternative-display-parts message-id
>> +                      (if (memq nth current-parts)
>> +                          (delq nth current-parts)
>> +                        (cons nth current-parts)))))
>> +  (notmuch-show-refresh-view))
>> +
>>  (defun notmuch-show-multipart/*-to-list (part)
>>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
>>         (plist-get part :content)))
>> @@ -543,12 +558,15 @@ message at DEPTH in the current thread."
>>      ;; This inserts all parts of the chosen type rather than just one,
>>      ;; 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?
>> +
>> +    ;; The variable user-parts says which parts should override the
>> +    ;; default so we use xor (handcoded since lisp does not have it).
>
> I don't follow the comment. user-parts isn't used in this
> function. Neither is xor.

Sorry that is vestigial from when the logic was in this function. The
"xor" now occurs in the other function.

>
>>      (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))))))
>
> For what it's worth, I found this not-shown logic very confusing, and
> have had to think about it seven or eight different times to make sure I
> understood what's going on. I'm not sure why exactly this is, though I
> could offer hypotheses -- the fact that it's split across two functions,
> or the fiddling with mime-types. I'm satisfied that it's correct, but I
> wish it could be made clearer.

I think the reason I went for the split is that I wanted all parts to be
togglable: I think having some parts which can be collapsed and some
which can't is a recipe for confusion (particularly as they might both
be labelled "[text/html]" for example). I think this means the split is
required: the first is "what does notmuch want to do with this part" and
the second is "what does the user want to do"

> This is just armchair hypothesizing, but here are some ideas that might
> make it more obvious what's going on: bringing the user-parts logic into
> this function; making user-parts, instead of a "t" meaning "user has
> toggled this", something like 'opened or 'closed and if user-parts for
> this message is absent, falling back to this calculation; alternately,
> prefilling user-parts with t when show is invoked, according to this
> calculation, and then not using it any more; moving this
> not-shown calculation into a separate function, something like notmuch-show-get-message-visibility.

I would be happy to use the 'open 'closed for each part which I guess
would make the list for each message a plist. It can be something of a
separate function but the we would need something like the not-shown
variable as the function notmuch-show-insert-bodypart does not know
whether it was called from the top level or from one of the multipart levels.

> I guess I jumped into this series halfway, but why are we doing this
> with the wipe/redraw technique instead of just using invisible overlays,
> like we do more generally with notmuch-show? I think I agree that
> toggling individual parts is a good UI approach, and this isn't a bad
> way to implement it, but I wonder if we could do it better/easier if we
> used emacs's builtin functionality.

I think overlays would be better but I have always found them difficult
to use. If this feels like the right user interface then maybe it could
be pushed and then something better can be added (as it should not break
things for the user).  I basically don't use this functionality so I was
hoping for feedback from those who do as to whether the user interface
is reasonable.

Best wishes

Mark

[PATCH (draft) 2/2] emacs: show: make "view part" show hidden parts

[Mark Walters]

This change means that hidden parts in the show buffer are inserted
into the buffer when the "view command" is called on that part (by
default v on the button).
---
 emacs/notmuch-show.el |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 9157669..293456d 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -836,7 +836,8 @@ has overridden the default for this part"
     (when (memq nth user-parts)
       (setq not-shown (not not-shown)))
     (if not-shown
-	(notmuch-show-insert-part-header nth content-type content-type nil " (not shown)")
+	(button-put (notmuch-show-insert-part-header nth content-type content-type nil " (not shown)")
+		    :part-not-shown 't)
       (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.
@@ -1912,7 +1913,10 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
 
 (defun notmuch-show-part-button-view (&optional button)
   (interactive)
-  (notmuch-show-part-button-internal button #'notmuch-show-view-part))
+  (let ((button (or button (button-at (point)))))
+    (if (and button (button-get button :part-not-shown))
+	(notmuch-show-part-button-internal button #'notmuch-show-internally-show-part)
+      (notmuch-show-part-button-internal button #'notmuch-show-view-part))))
 
 (defun notmuch-show-part-button-interactively-view (&optional button)
   (interactive)
-- 
1.7.9.1

Re: [PATCH (draft) 0/2] Allow emacs to toggle display of all parts including multipart

[Jani Nikula]

On Thu, 25 Oct 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> This patch series adds a function to toggle the display of any emacs
> part in the show buffer. This is particularly useful for
> multipart/alternative where the parts have different information.

I didn't look at the code, but I tried it. And I like it. There are some
annoyances and some things I'll still want to customize for myself, but
I really like it.

> The first patch binds this toggle to 't' on the part button. This
> patch works by itself.

I'd like to rebind RET to do the toggle by default. I don't think it's
very useful to be able to save multipart/alternative parts by default.

> The second patch makes it that "viewing" (ie v on the part-button) a
> "not shown" part displays the part in the buffer. Note this is not a
> toggle since having displayed the part in the buffer the user may wish
> to view the part externally (eg if it is a text/html part).
>
> Caveats: 
> 	 the patches work by reloading the whole buffer: this may mean
> 	 extra messages appear in the thread. This is not ideal but is
> 	 the same as toggling indentation etc.

It's all right.

> 	 the reload saves state as normal but this means the view
> 	 returns to the top of the current message. Since we know
> 	 exactly where the user was (ie on the button) this should be
> 	 restored.

This is annoying, and should be fixed eventually (but this can come
afterwards). One option would be to just take a line count from the
beginning of the message to the button, and return point to the button
after refresh. This should work even if new messages show up.

> 	 for technical reasons the "view" part functionality in the
> 	 second patch only works for view called explicitly: not if
> 	 the user has customised the default action to be view and
> 	 calls that. This is easy to fix but the most natural way
> 	 would break existing customisation of this action.
>
> 	 reloading the buffer without keeping state does not currently
> 	 reset the toggled parts. Again this is easy to fix but lets
> 	 see if people like the general approach first.

IMHO can be fixed later.

One more wishlist item, which also doesn't need to be part of this
series, or by you: I'd like to be able to specify *which*
multipart/alternative parts should be displayed in addition to
preferred. A list of (regexps matching) mime types to always open,
whether they're preferred or not. I've previously sent patches to this
effect, but they were never merged (I can repost if you want).

BR,
Jani.


>
> Best wishes
>
> Mark
>
>
> Mark Walters (2):
>   emacs: allow the user to toggle the visibility of
>     multipart/alternative parts
>   emacs: show: make "view part" show hidden parts
>
>  emacs/notmuch-show.el |   53 ++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 44 insertions(+), 9 deletions(-)
>
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH (draft) 0/2] Allow emacs to toggle display of all parts including multipart

[Jameson Graef Rollins]

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

On Sun, Dec 02 2012, Jani Nikula <jani@nikula.org> wrote:
>> The first patch binds this toggle to 't' on the part button. This
>> patch works by itself.
>
> I'd like to rebind RET to do the toggle by default. I don't think it's
> very useful to be able to save multipart/alternative parts by default.

In my opinion RET should always just display the part.  If the part is
inlineable, then it should toggle display of it in the show buffer.  If
it's not, it should open it in an external viewer.  I think that makes
the most sense, and agrees with the behavior of the collapsed citation
text buttons, for instance.

> One more wishlist item, which also doesn't need to be part of this
> series, or by you: I'd like to be able to specify *which*
> multipart/alternative parts should be displayed in addition to
> preferred. A list of (regexps matching) mime types to always open,
> whether they're preferred or not. I've previously sent patches to this
> effect, but they were never merged (I can repost if you want).

I still think that for multipart/alternatives parts we should just cycle
through display of the alternatives.  That's kind of the whole point of
this mime type.  Obviously display everything is better than not being
able to access the alternative parts at all.  But forcing
mutlipart/alternative to behave exactly like multipart/mixed seems to
miss the point to me.

This is something that could be worked off of this series, though, so
definitely don't take this as a mark against this work.  I think this is
a very useful usability improvement.  Thank you for working on it, Mark.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]