70/74bd09d91624f7cc5c96b1a0d61de665c4c1a5

   1 Return-Path: <amdragon@mit.edu>\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 2CB00431FC3\r
   6         for <notmuch@notmuchmail.org>; Thu, 30 May 2013 15:31:15 -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: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id 5hFv4qRtbRoU for <notmuch@notmuchmail.org>;\r
  16         Thu, 30 May 2013 15:31:09 -0700 (PDT)\r
  17 Received: from dmz-mailsec-scanner-4.mit.edu (dmz-mailsec-scanner-4.mit.edu\r
  18         [18.9.25.15])\r
  19         by olra.theworths.org (Postfix) with ESMTP id 3CE8A431FD2\r
  20         for <notmuch@notmuchmail.org>; Thu, 30 May 2013 15:31:09 -0700 (PDT)\r
  21 X-AuditID: 1209190f-b7f256d000005616-b2-51a7d327d69c\r
  22 Received: from mailhub-auth-2.mit.edu ( [18.7.62.36])\r
  23         by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 04.FA.22038.723D7A15; Thu, 30 May 2013 18:31:03 -0400 (EDT)\r
  25 Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11])\r
  26         by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id r4UMV2Ck019853; \r
  27         Thu, 30 May 2013 18:31:03 -0400\r
  28 Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91])\r
  29         (authenticated bits=0)\r
  30         (User authenticated as amdragon@ATHENA.MIT.EDU)\r
  31         by outgoing.mit.edu (8.13.8/8.12.4) with ESMTP id r4UMUxNt012623\r
  32         (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT);\r
  33         Thu, 30 May 2013 18:31:01 -0400\r
  34 Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80)\r
  35         (envelope-from <amdragon@mit.edu>)\r
  36         id 1UiBN5-0005tg-O0; Thu, 30 May 2013 18:30:59 -0400\r
  37 From: Austin Clements <aclements@csail.mit.edu>\r
  38 To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org\r
  39 Subject: Re: [PATCH v2 2/3] emacs: show: move the insertion of the header\r
  40         button to the top level\r
  41 In-Reply-To: <1369555061-21361-3-git-send-email-markwalters1009@gmail.com>\r
  42 References: <1369555061-21361-1-git-send-email-markwalters1009@gmail.com>\r
  43         <1369555061-21361-3-git-send-email-markwalters1009@gmail.com>\r
  44 User-Agent: Notmuch/0.15.2+83~g8bee3c4 (http://notmuchmail.org) Emacs/23.4.1\r
  45         (i486-pc-linux-gnu)\r
  46 Date: Thu, 30 May 2013 18:30:59 -0400\r
  47 Message-ID: <87a9nc2j8c.fsf@awakening.csail.mit.edu>\r
  48 MIME-Version: 1.0\r
  49 Content-Type: text/plain; charset=us-ascii\r
  50 X-Brightmail-Tracker:\r
  51  H4sIAAAAAAAAA+NgFnrNIsWRmVeSWpSXmKPExsUixG6noqt+eXmgwZrDKhar5/JYXL85k9mB\r
  52         yWPnrLvsHs9W3WIOYIrisklJzcksSy3St0vgyji0egN7warUiiM/L7I0MD4P6mLk5JAQMJE4\r
  53         MHcxM4QtJnHh3nq2LkYuDiGBfYwSL35+YYJwNjJKLGu6zwrhnGaSOHLsCDtIi5DAEkaJ1dfS\r
  54         QGw2AX2JFWsnsYLYIgKuEk+/fQYbKyyQKPFxyV+wek4BL4lPx6+wQPS2M0q873UAsUUFEiRW\r
  55         3j3BCGKzCKhK7H8wDWgOBwcv0Hkb11mDhHkFBCVOznwC1sosoCVx499LpgmMArOQpGYhSS1g\r
  56         ZFrFKJuSW6Wbm5iZU5yarFucnJiXl1qka6KXm1mil5pSuokRFI6ckvw7GL8dVDrEKMDBqMTD\r
  57         m5G0PFCINbGsuDL3EKMkB5OSKO/PU0AhvqT8lMqMxOKM+KLSnNTiQ4wSHMxKIryd54ByvCmJ\r
  58         lVWpRfkwKWkOFiVx3qspN/2FBNITS1KzU1MLUotgsjIcHEoSvPMuAjUKFqWmp1akZeaUIKSZ\r
  59         ODhBhvMADT8DUsNbXJCYW5yZDpE/xajLsfn85HeMQix5+XmpUuK8h0CKBECKMkrz4ObA0sgr\r
  60         RnGgt4R5l4BU8QBTENykV0BLmICWPLEGW1KSiJCSamBUv+hfonnivN/LrKjqlYZbMup/n7gS\r
  61         +WzLRj1vlvUi+u75q244F8TlSYbPrEk1t1tiz2a1R81/9lz7k1ODV3/4uUTpywHvuin6tToW\r
  62         ezNe28krz1Hg+anLO+PN9EiG5pYF9ziiDrFYZ6ybnqghozu1Q+rA9D2qB1a9+H2aI/E5/7vc\r
  63         l+Zzr+9QYinOSDTUYi4qTgQAYrJMrf4CAAA=\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: Thu, 30 May 2013 22:31:15 -0000\r
  77 \r
  78 On Sun, 26 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:\r
  79 > Previously each of the part insertion handlers inserted the part\r
  80 > button themselves. Move this up into\r
  81 > notmuch-show-insert-bodypart. Since a small number of the handlers\r
  82 > modify the button (the encryption/signature ones) we need to pass the\r
  83 > header button as an argument into the individual part insertion\r
  84 > handlers.\r
  85 >\r
  86 > The patch is large but mostly simple. The only things of note are that\r
  87 > we let the text/plain handler insert part buttons itself (as it does\r
  88 > not always insert one and it applies motmuch-wash to the whole part\r
  89 > including the button. Also this is by far the most important case so\r
  90 > this reduces the risk of annoying side effects.\r
  91 > ---\r
  92 >  emacs/notmuch-show.el |   87 +++++++++++++++++++++++-------------------------\r
  93 >  1 files changed, 42 insertions(+), 45 deletions(-)\r
  94 >\r
  95 > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
  96 > index 51bda31..591ad56 100644\r
  97 > --- a/emacs/notmuch-show.el\r
  98 > +++ b/emacs/notmuch-show.el\r
  99 > @@ -576,8 +576,7 @@ message at DEPTH in the current thread."\r
 100 >    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))\r
 101 >         (plist-get part :content)))\r
 102 >  \r
 103 > -(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type)\r
 104 > -  (notmuch-show-insert-part-header nth declared-type content-type nil)\r
 105 > +(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type button)\r
 106 >    (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))\r
 107 >       (inner-parts (plist-get part :content))\r
 108 >       (start (point)))\r
 109 > @@ -654,8 +653,7 @@ message at DEPTH in the current thread."\r
 110 >         content-type)\r
 111 >        nil)))\r
 112 >  \r
 113 > -(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type)\r
 114 > -  (notmuch-show-insert-part-header nth declared-type content-type nil)\r
 115 > +(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type button)\r
 116 >    (let ((inner-parts (plist-get part :content))\r
 117 >       (start (point)))\r
 118 >  \r
 119 > @@ -674,16 +672,15 @@ message at DEPTH in the current thread."\r
 120 >        (indent-rigidly start (point) 1)))\r
 121 >    t)\r
 122 >  \r
 123 > -(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type)\r
 124 > -  (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil)))\r
 125 > -    (button-put button 'face 'notmuch-crypto-part-header)\r
 126 > -    ;; add signature status button if sigstatus provided\r
 127 > -    (if (plist-member part :sigstatus)\r
 128 > -     (let* ((from (notmuch-show-get-header :From msg))\r
 129 > -            (sigstatus (car (plist-get part :sigstatus))))\r
 130 > -       (notmuch-crypto-insert-sigstatus-button sigstatus from))\r
 131 > -      ;; if we're not adding sigstatus, tell the user how they can get it\r
 132 > -      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))\r
 133 > +(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type button)\r
 134 > +  (button-put button 'face 'notmuch-crypto-part-header)\r
 135 > +  ;; add signature status button if sigstatus provided\r
 136 > +  (if (plist-member part :sigstatus)\r
 137 > +      (let* ((from (notmuch-show-get-header :From msg))\r
 138 > +          (sigstatus (car (plist-get part :sigstatus))))\r
 139 > +     (notmuch-crypto-insert-sigstatus-button sigstatus from))\r
 140 > +    ;; if we're not adding sigstatus, tell the user how they can get it\r
 141 > +    (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))\r
 142 >  \r
 143 >    (let ((inner-parts (plist-get part :content))\r
 144 >       (start (point)))\r
 145 > @@ -696,20 +693,19 @@ message at DEPTH in the current thread."\r
 146 >        (indent-rigidly start (point) 1)))\r
 147 >    t)\r
 148 >  \r
 149 > -(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type)\r
 150 > -  (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil)))\r
 151 > -    (button-put button 'face 'notmuch-crypto-part-header)\r
 152 > -    ;; add encryption status button if encstatus specified\r
 153 > -    (if (plist-member part :encstatus)\r
 154 > -     (let ((encstatus (car (plist-get part :encstatus))))\r
 155 > -       (notmuch-crypto-insert-encstatus-button encstatus)\r
 156 > -       ;; add signature status button if sigstatus specified\r
 157 > -       (if (plist-member part :sigstatus)\r
 158 > -           (let* ((from (notmuch-show-get-header :From msg))\r
 159 > -                  (sigstatus (car (plist-get part :sigstatus))))\r
 160 > -             (notmuch-crypto-insert-sigstatus-button sigstatus from))))\r
 161 > -      ;; if we're not adding encstatus, tell the user how they can get it\r
 162 > -      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))\r
 163 > +(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type button)\r
 164 > +  (button-put button 'face 'notmuch-crypto-part-header)\r
 165 > +  ;; add encryption status button if encstatus specified\r
 166 > +  (if (plist-member part :encstatus)\r
 167 > +      (let ((encstatus (car (plist-get part :encstatus))))\r
 168 > +     (notmuch-crypto-insert-encstatus-button encstatus)\r
 169 > +     ;; add signature status button if sigstatus specified\r
 170 > +     (if (plist-member part :sigstatus)\r
 171 > +         (let* ((from (notmuch-show-get-header :From msg))\r
 172 > +                (sigstatus (car (plist-get part :sigstatus))))\r
 173 > +           (notmuch-crypto-insert-sigstatus-button sigstatus from))))\r
 174 > +    ;; if we're not adding encstatus, tell the user how they can get it\r
 175 > +    (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))\r
 176 >  \r
 177 >    (let ((inner-parts (plist-get part :content))\r
 178 >       (start (point)))\r
 179 > @@ -722,8 +718,7 @@ message at DEPTH in the current thread."\r
 180 >        (indent-rigidly start (point) 1)))\r
 181 >    t)\r
 182 >  \r
 183 > -(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type)\r
 184 > -  (notmuch-show-insert-part-header nth declared-type content-type nil)\r
 185 > +(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type button)\r
 186 >    (let ((inner-parts (plist-get part :content))\r
 187 >       (start (point)))\r
 188 >      ;; Show all of the parts.\r
 189 > @@ -735,8 +730,7 @@ message at DEPTH in the current thread."\r
 190 >        (indent-rigidly start (point) 1)))\r
 191 >    t)\r
 192 >  \r
 193 > -(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type)\r
 194 > -  (notmuch-show-insert-part-header nth declared-type content-type nil)\r
 195 > +(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type button)\r
 196 >    (let* ((message (car (plist-get part :content)))\r
 197 >        (body (car (plist-get message :body)))\r
 198 >        (start (point)))\r
 199 > @@ -757,7 +751,7 @@ message at DEPTH in the current thread."\r
 200 >        (indent-rigidly start (point) 1)))\r
 201 >    t)\r
 202 >  \r
 203 > -(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type)\r
 204 > +(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type button)\r
 205 >    (let ((start (point)))\r
 206 >      ;; If this text/plain part is not the first part in the message,\r
 207 >      ;; insert a header to make this clear.\r
 208 \r
 209 I know you're trying to minimize side-effects, but I think this will\r
 210 also complicate maintenance.  I'd rather see this call to\r
 211 notmuch-show-insert-part-header removed and have the only call and all\r
 212 special-case logic be in notmuch-show-insert-bodypart.  As it is,\r
 213 reasoning about exactly when a part button is inserted requires\r
 214 understanding the conjunction of two widely separated parts of the code.\r
 215 \r
 216 > @@ -770,8 +764,7 @@ message at DEPTH in the current thread."\r
 217 >       (run-hook-with-args 'notmuch-show-insert-text/plain-hook msg depth))))\r
 218 >    t)\r
 219 >  \r
 220 > -(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type)\r
 221 > -  (notmuch-show-insert-part-header nth declared-type content-type (plist-get part :filename))\r
 222 > +(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type button)\r
 223 >    (insert (with-temp-buffer\r
 224 >           (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))\r
 225 >           ;; notmuch-get-bodypart-content provides "raw", non-converted\r
 226 > @@ -794,8 +787,8 @@ message at DEPTH in the current thread."\r
 227 >    t)\r
 228 >  \r
 229 >  ;; For backwards compatibility.\r
 230 > -(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type)\r
 231 > -  (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type))\r
 232 > +(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type button)\r
 233 > +  (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type button))\r
 234 >  \r
 235 >  (defun notmuch-show-get-mime-type-of-application/octet-stream (part)\r
 236 >    ;; If we can deduce a MIME type from the filename of the attachment,\r
 237 > @@ -813,7 +806,7 @@ message at DEPTH in the current thread."\r
 238 >               nil))\r
 239 >         nil))))\r
 240 >  \r
 241 > -(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)\r
 242 > +(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type button)\r
 243 >    ;; text/html handler to work around bugs in renderers and our\r
 244 >    ;; invisibile parts code. In particular w3m sets up a keymap which\r
 245 >    ;; "leaks" outside the invisible region and causes strange effects\r
 246 > @@ -821,11 +814,10 @@ message at DEPTH in the current thread."\r
 247 >    ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map\r
 248 >    ;; remains).\r
 249 >    (let ((mm-inline-text-html-with-w3m-keymap nil))\r
 250 > -    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type)))\r
 251 > +    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type button)))\r
 252 >  \r
 253 > -(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)\r
 254 > +(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type button)\r
 255 >    ;; This handler _must_ succeed - it is the handler of last resort.\r
 256 > -  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))\r
 257 >    (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto)\r
 258 >    t)\r
 259 >  \r
 260 > @@ -848,13 +840,13 @@ message at DEPTH in the current thread."\r
 261 >  \r
 262 >  ;; \f\r
 263 \r
 264 >  \r
 265 > -(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type)\r
 266 > +(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type button)\r
 267 >    (let ((handlers (notmuch-show-handlers-for content-type)))\r
 268 >      ;; Run the content handlers until one of them returns a non-nil\r
 269 >      ;; value.\r
 270 >      (while (and handlers\r
 271 >               (not (condition-case err\r
 272 > -                      (funcall (car handlers) msg part content-type nth depth declared-type)\r
 273 > +                      (funcall (car handlers) msg part content-type nth depth declared-type button)\r
 274 >                      (error (progn\r
 275 >                               (insert "!!! Bodypart insert error: ")\r
 276 >                               (insert (error-message-string err))\r
 277 > @@ -882,6 +874,9 @@ message at DEPTH in the current thread."\r
 278 >    "Insert the body part PART at depth DEPTH in the current thread.\r
 279 >  \r
 280 >  If HIDE is non-nil then initially hide this part."\r
 281 > +\r
 282 > +  ;; We handle text/plain specially as its code does things before\r
 283 > +  ;; inserting a part button (and does not always insert a part button).\r
 284 \r
 285 I think this comment would be clearer right above the button binding (or\r
 286 maybe the separate diff hunks just make it more confusing than it is).\r
 287 \r
 288 >    (let* ((content-type (downcase (plist-get part :content-type)))\r
 289 >        (mime-type (or (and (string= content-type "application/octet-stream")\r
 290 >                            (notmuch-show-get-mime-type-of-application/octet-stream part))\r
 291 > @@ -889,9 +884,11 @@ If HIDE is non-nil then initially hide this part."\r
 292 >                            "text/x-diff")\r
 293 >                       content-type))\r
 294 >        (nth (plist-get part :id))\r
 295 > -      (beg (point)))\r
 296 > +      (beg (point))\r
 297 > +      (button (unless (string= mime-type "text/plain")\r
 298 > +                (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))\r
 299 \r
 300 If you take my suggestion above, this would become something like\r
 301 \r
 302 (unless (and (string= mime-type "text/plain) (<= nth 1)) ...)\r
 303 \r
 304 or maybe clearer\r
 305 \r
 306 (when (or (not (string= mime-type "text/plain")) (> nth 1)) ...)\r
 307 \r
 308 >  \r
 309 > -    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type)\r
 310 > +    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type button)\r
 311 >      ;; Some of the body part handlers leave point somewhere up in the\r
 312 >      ;; part, so we make sure that we're down at the end.\r
 313 >      (goto-char (point-max))\r
 314 > -- \r
 315 > 1.7.9.1\r
 316 >\r
 317 > _______________________________________________\r
 318 > notmuch mailing list\r
 319 > notmuch@notmuchmail.org\r
 320 > http://notmuchmail.org/mailman/listinfo/notmuch\r