Mark Walters <markwalters1009 at gmail.com> writes: > Some messages are sent as multipart/alternative but the alternatives > contain different information. This allows the user to cycle which > part to view. By default this is bound to 'W'. > --- > > This version at least uses the notmuch escaping for message-id which > makes me a bit happier: it probably doesn't have any nasty security > flaws. I do still feel that the lisp is a bit ugly though.
For what it's worth, I don't feel that this code is horrible. It seems like there remain design decisions to be made about how notmuch show "ought" to handle multipart/alternatives, but I can at least comment on this code. First, the use of a plist looks fine to me because most of the time it's going to have length 0. At most it will have one entry per message -- a few hundred. So I'm not worried about efficiency concerns. > (defcustom notmuch-show-stash-mlarchive-link-alist > '(("Gmane" . "http://mid.gmane.org/") > ("MARC" . "http://marc.info/?i=") > @@ -536,9 +540,19 @@ message at DEPTH in the current thread." > > (defun notmuch-show-insert-part-multipart/alternative (msg part content-type > nth depth declared-type) > (notmuch-show-insert-part-header nth declared-type content-type nil) > - (let ((chosen-type (car (notmuch-multipart/alternative-choose > (notmuch-show-multipart/*-to-list part)))) > - (inner-parts (plist-get part :content)) > - (start (point))) > + (let* ((chosen-nth (or (lax-plist-get > notmuch-show-message-multipart/alternative-display-part > + (notmuch-id-to-query (plist-get msg > :id))) 0)) > + (chosen-type (nth chosen-nth > + (notmuch-multipart/alternative-choose > (notmuch-show-multipart/*-to-list part)))) > + (inner-parts (plist-get part :content)) > + (start (point))) Changing let to let* makes me the slightest bit uneasy, although I'm not sure I could explain why. It would be nice if you could wrap the manipulation of notmuch-show-message-multipart/alternative-display-part in functions, ideally with names that are shorter than the variable they manipulate. Specifically, I think the definition of chosen-nth (which is almost repeated below) could be its own function, something like (notmuch-show-message-current-multipart msg), which could take a msg-id or a plist and do the plist-get and id-to-query that you do here. > + ;; If we have run out of possible content-types restart from the > beginning > + (unless chosen-type > + (setq chosen-type (car (notmuch-multipart/alternative-choose > (notmuch-show-multipart/*-to-list part)))) > + (setq notmuch-show-message-multipart/alternative-display-part > + (lax-plist-put > notmuch-show-message-multipart/alternative-display-part > + (notmuch-id-to-query (plist-get msg :id)) 0))) > + > ;; 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? > @@ -942,6 +956,16 @@ message at DEPTH in the current thread." > "Not processing cryptographic MIME parts.")) > (notmuch-show-refresh-view)) > > +(defun notmuch-show-cycle-message-multipart () > + "Cycle which part to display of a multipart messageToggle the display of > non-matching messages." This docstring is broken. > + (interactive) > + (let* ((msg-id (notmuch-show-get-message-id)) > + (next-part (1+ (or (lax-plist-get > notmuch-show-message-multipart/alternative-display-part msg-id) 0)))) > + (setq notmuch-show-message-multipart/alternative-display-part > + (lax-plist-put notmuch-show-message-multipart/alternative-display-part > msg-id next-part)) > + (message "Cycling multipart/alternative for current message") > + (notmuch-show-refresh-view))) Maybe move the reset-and-go-back-to-zero behavior to this function instead of in the display function. This opens you up to weird situations if one of the multipart/alternatives should disappear from a message or if some other function should change the alternative on display for a given message, but both of these seem unlikely to me.. Ethan