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

[Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>]

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