From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 0C81F431FB6 for ; Sun, 28 Oct 2012 01:01:12 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: -1.075 X-Spam-Level: X-Spam-Status: No, score=-1.075 tagged_above=-999 required=5 tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001, HS_INDEX_PARAM=0.023, NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uUOXOQCC5nuQ for ; Sun, 28 Oct 2012 01:01:08 -0700 (PDT) Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id F2816431FAF for ; Sun, 28 Oct 2012 01:01:07 -0700 (PDT) Received: from smtp.qmul.ac.uk ([138.37.6.40]) by mail2.qmul.ac.uk with esmtp (Exim 4.71) (envelope-from ) id 1TSNno-0001kV-24; Sun, 28 Oct 2012 08:01:04 +0000 Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost) by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69) (envelope-from ) id 1TSNnn-0001HH-L1; Sun, 28 Oct 2012 08:00:59 +0000 From: Mark Walters To: Ethan Glasser-Camp , notmuch@notmuchmail.org Subject: Re: [PATCH (draft) 1/2] emacs: allow the user to toggle the visibility of multipart/alternative parts In-Reply-To: <87sj8zh2zy.fsf@betacantrips.com> References: <1351152563-27277-1-git-send-email-markwalters1009@gmail.com> <1351152563-27277-2-git-send-email-markwalters1009@gmail.com> <87sj8zh2zy.fsf@betacantrips.com> User-Agent: Notmuch/0.14+69~g024ea36 (http://notmuchmail.org) Emacs/23.4.1 (i486-pc-linux-gnu) Date: Sun, 28 Oct 2012 08:00:58 +0000 Message-ID: <87objnkoud.fsf@qmul.ac.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Sender-Host-Address: 93.97.24.31 X-QM-SPAM-Info: Sender has good ham record. :) X-QM-Body-MD5: e97fb7d8e7c9c1ff2b54acf6e29a6b82 (of first 20000 bytes) X-SpamAssassin-Score: -1.5 X-SpamAssassin-SpamBar: - X-SpamAssassin-Report: The QM spam filters have analysed this message to determine if it is spam. We require at least 5.0 points to mark a message as spam. This message scored -1.5 points. Summary of the scoring: * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, * medium trust * [138.37.6.40 listed in list.dnswl.org] * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider * (markwalters1009[at]gmail.com) * 0.5 QM_SPAMMYURI URI: \".info\" uri is common in spam. * 0.3 AWL AWL: From: address is in the auto white-list X-QM-Scan-Virus: ClamAV says the message is clean X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 28 Oct 2012 08:01:12 -0000 Ethan Glasser-Camp writes: > Mark Walters 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