Ethan Glasser-Camp <ethan.glasser.camp at gmail.com> writes:
> Mark Walters <markwalters1009 at 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
