e2/0b7821694471e467158584b7e09ec2d4350394

   1 Return-Path: <m.walters@qmul.ac.uk>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id 0C81F431FB6\r
   6         for <notmuch@notmuchmail.org>; Sun, 28 Oct 2012 01:01:12 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -1.075\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-1.075 tagged_above=-999 required=5\r
  12         tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
  13         HS_INDEX_PARAM=0.023, NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3]\r
  14         autolearn=disabled\r
  15 Received: from olra.theworths.org ([127.0.0.1])\r
  16         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  17         with ESMTP id uUOXOQCC5nuQ for <notmuch@notmuchmail.org>;\r
  18         Sun, 28 Oct 2012 01:01:08 -0700 (PDT)\r
  19 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
  20         (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
  21         (No client certificate requested)\r
  22         by olra.theworths.org (Postfix) with ESMTPS id F2816431FAF\r
  23         for <notmuch@notmuchmail.org>; Sun, 28 Oct 2012 01:01:07 -0700 (PDT)\r
  24 Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
  25         by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
  26         (envelope-from <m.walters@qmul.ac.uk>)\r
  27         id 1TSNno-0001kV-24; Sun, 28 Oct 2012 08:01:04 +0000\r
  28 Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
  29         by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
  30         (envelope-from <m.walters@qmul.ac.uk>)\r
  31         id 1TSNnn-0001HH-L1; Sun, 28 Oct 2012 08:00:59 +0000\r
  32 From: Mark Walters <markwalters1009@gmail.com>\r
  33 To: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>, notmuch@notmuchmail.org\r
  34 Subject: Re: [PATCH (draft) 1/2] emacs: allow the user to toggle the\r
  35         visibility of multipart/alternative parts\r
  36 In-Reply-To: <87sj8zh2zy.fsf@betacantrips.com>\r
  37 References: <1351152563-27277-1-git-send-email-markwalters1009@gmail.com>\r
  38         <1351152563-27277-2-git-send-email-markwalters1009@gmail.com>\r
  39         <87sj8zh2zy.fsf@betacantrips.com>\r
  40 User-Agent: Notmuch/0.14+69~g024ea36 (http://notmuchmail.org) Emacs/23.4.1\r
  41         (i486-pc-linux-gnu)\r
  42 Date: Sun, 28 Oct 2012 08:00:58 +0000\r
  43 Message-ID: <87objnkoud.fsf@qmul.ac.uk>\r
  44 MIME-Version: 1.0\r
  45 Content-Type: text/plain; charset=us-ascii\r
  46 X-Sender-Host-Address: 93.97.24.31\r
  47 X-QM-SPAM-Info: Sender has good ham record.  :)\r
  48 X-QM-Body-MD5: e97fb7d8e7c9c1ff2b54acf6e29a6b82 (of first 20000 bytes)\r
  49 X-SpamAssassin-Score: -1.5\r
  50 X-SpamAssassin-SpamBar: -\r
  51 X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
  52         determine if it is\r
  53         spam. We require at least 5.0 points to mark a message as spam.\r
  54         This message scored -1.5 points.\r
  55         Summary of the scoring: \r
  56         * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
  57         *      medium trust\r
  58         *      [138.37.6.40 listed in list.dnswl.org]\r
  59         * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
  60         provider *      (markwalters1009[at]gmail.com)\r
  61         *  0.5 QM_SPAMMYURI URI: \".info\" uri is common in spam.\r
  62         *  0.3 AWL AWL: From: address is in the auto white-list\r
  63 X-QM-Scan-Virus: ClamAV says the message is clean\r
  64 X-BeenThere: notmuch@notmuchmail.org\r
  65 X-Mailman-Version: 2.1.13\r
  66 Precedence: list\r
  67 List-Id: "Use and development of the notmuch mail system."\r
  68         <notmuch.notmuchmail.org>\r
  69 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  70         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  71 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  72 List-Post: <mailto:notmuch@notmuchmail.org>\r
  73 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  74 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  75         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  76 X-List-Received-Date: Sun, 28 Oct 2012 08:01:12 -0000\r
  77 \r
  78 \r
  79 Ethan Glasser-Camp <ethan.glasser.camp@gmail.com> writes:\r
  80 \r
  81 > Mark Walters <markwalters1009@gmail.com> writes:\r
  82 >\r
  83 >> This patch adds a keybinding to the buttons in the notmuch-show emacs\r
  84 >> buffer to allow the user to toggle the visibility of each part of a\r
  85 >> message in the show buffer.  This is particularly useful for\r
  86 >> multipart/alternative parts where the parts are not really\r
  87 >> alternatives but contain different information.\r
  88 >> ---\r
  89 >>  emacs/notmuch-show.el |   47 +++++++++++++++++++++++++++++++++++++++--------\r
  90 >>  1 files changed, 39 insertions(+), 8 deletions(-)\r
  91 >>\r
  92 >> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
  93 >> index 0f54259..9157669 100644\r
  94 >> --- a/emacs/notmuch-show.el\r
  95 >> +++ b/emacs/notmuch-show.el\r
  96 >> @@ -155,6 +155,10 @@ indentation."\r
  97 >>  (make-variable-buffer-local 'notmuch-show-indent-content)\r
  98 >>  (put 'notmuch-show-indent-content 'permanent-local t)\r
  99 >>\r
 100 >> +(defvar notmuch-show-message-multipart/alternative-display-parts nil)\r
 101 >> +(make-variable-buffer-local 'notmuch-show-message-multipart/alternative-display-parts)\r
 102 >> +(put 'notmuch-show-message-multipart/alternative-display-parts 'permanent-local t)\r
 103 >> +\r
 104 >>  (defcustom notmuch-show-stash-mlarchive-link-alist\r
 105 >>    '(("Gmane" . "http://mid.gmane.org/")\r
 106 >>      ("MARC" . "http://marc.info/?i=")\r
 107 >> @@ -455,6 +459,7 @@ message at DEPTH in the current thread."\r
 108 >>      (define-key map "v" 'notmuch-show-part-button-view)\r
 109 >>      (define-key map "o" 'notmuch-show-part-button-interactively-view)\r
 110 >>      (define-key map "|" 'notmuch-show-part-button-pipe)\r
 111 >> +    (define-key map "t" 'notmuch-show-part-button-internally-show)\r
 112 >>      map)\r
 113 >>    "Submap for button commands")\r
 114 >>  (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)\r
 115 >> @@ -531,6 +536,16 @@ message at DEPTH in the current thread."\r
 116 >>      (let ((handle (mm-make-handle (current-buffer) (list content-type))))\r
 117 >>        (mm-pipe-part handle))))\r
 118 >>\r
 119 >> +(defun notmuch-show-internally-show-part (message-id nth &optional filename content-type)\r
 120 >> +  "Set a part to be displayed internally"\r
 121 >> +  (let ((current-parts (lax-plist-get notmuch-show-message-multipart/alternative-display-parts message-id)))\r
 122 >> +    (setq notmuch-show-message-multipart/alternative-display-parts\r
 123 >> +       (lax-plist-put notmuch-show-message-multipart/alternative-display-parts message-id\r
 124 >> +                      (if (memq nth current-parts)\r
 125 >> +                          (delq nth current-parts)\r
 126 >> +                        (cons nth current-parts)))))\r
 127 >> +  (notmuch-show-refresh-view))\r
 128 >> +\r
 129 >>  (defun notmuch-show-multipart/*-to-list (part)\r
 130 >>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))\r
 131 >>         (plist-get part :content)))\r
 132 >> @@ -543,12 +558,15 @@ message at DEPTH in the current thread."\r
 133 >>      ;; This inserts all parts of the chosen type rather than just one,\r
 134 >>      ;; but it's not clear that this is the wrong thing to do - which\r
 135 >>      ;; should be chosen if there are more than one that match?\r
 136 >> +\r
 137 >> +    ;; The variable user-parts says which parts should override the\r
 138 >> +    ;; default so we use xor (handcoded since lisp does not have it).\r
 139 >\r
 140 > I don't follow the comment. user-parts isn't used in this\r
 141 > function. Neither is xor.\r
 142 \r
 143 Sorry that is vestigial from when the logic was in this function. The\r
 144 "xor" now occurs in the other function.\r
 145 \r
 146 >\r
 147 >>      (mapc (lambda (inner-part)\r
 148 >>           (let ((inner-type (plist-get inner-part :content-type)))\r
 149 >> -           (if (or notmuch-show-all-multipart/alternative-parts\r
 150 >> -                   (string= chosen-type inner-type))\r
 151 >> -               (notmuch-show-insert-bodypart msg inner-part depth)\r
 152 >> -             (notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))\r
 153 >> +           (notmuch-show-insert-bodypart msg inner-part depth\r
 154 >> +                                         (not (or notmuch-show-all-multipart/alternative-parts\r
 155 >> +                                                  (string=\r
 156 >>           chosen-type inner-type))))))\r
 157 >\r
 158 > For what it's worth, I found this not-shown logic very confusing, and\r
 159 > have had to think about it seven or eight different times to make sure I\r
 160 > understood what's going on. I'm not sure why exactly this is, though I\r
 161 > could offer hypotheses -- the fact that it's split across two functions,\r
 162 > or the fiddling with mime-types. I'm satisfied that it's correct, but I\r
 163 > wish it could be made clearer.\r
 164 \r
 165 I think the reason I went for the split is that I wanted all parts to be\r
 166 togglable: I think having some parts which can be collapsed and some\r
 167 which can't is a recipe for confusion (particularly as they might both\r
 168 be labelled "[text/html]" for example). I think this means the split is\r
 169 required: the first is "what does notmuch want to do with this part" and\r
 170 the second is "what does the user want to do"\r
 171 \r
 172 > This is just armchair hypothesizing, but here are some ideas that might\r
 173 > make it more obvious what's going on: bringing the user-parts logic into\r
 174 > this function; making user-parts, instead of a "t" meaning "user has\r
 175 > toggled this", something like 'opened or 'closed and if user-parts for\r
 176 > this message is absent, falling back to this calculation; alternately,\r
 177 > prefilling user-parts with t when show is invoked, according to this\r
 178 > calculation, and then not using it any more; moving this\r
 179 > not-shown calculation into a separate function, something like notmuch-show-get-message-visibility.\r
 180 \r
 181 I would be happy to use the 'open 'closed for each part which I guess\r
 182 would make the list for each message a plist. It can be something of a\r
 183 separate function but the we would need something like the not-shown\r
 184 variable as the function notmuch-show-insert-bodypart does not know\r
 185 whether it was called from the top level or from one of the multipart levels.\r
 186 \r
 187 > I guess I jumped into this series halfway, but why are we doing this\r
 188 > with the wipe/redraw technique instead of just using invisible overlays,\r
 189 > like we do more generally with notmuch-show? I think I agree that\r
 190 > toggling individual parts is a good UI approach, and this isn't a bad\r
 191 > way to implement it, but I wonder if we could do it better/easier if we\r
 192 > used emacs's builtin functionality.\r
 193 \r
 194 I think overlays would be better but I have always found them difficult\r
 195 to use. If this feels like the right user interface then maybe it could\r
 196 be pushed and then something better can be added (as it should not break\r
 197 things for the user).  I basically don't use this functionality so I was\r
 198 hoping for feedback from those who do as to whether the user interface\r
 199 is reasonable.\r
 200 \r
 201 Best wishes\r
 202 \r
 203 Mark\r
 204 \r
 205 \r