d9/592aa40f974a2cc25369da93b9b8d3b5bba080

   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 BAC9A431FAF\r
   6         for <notmuch@notmuchmail.org>; Mon, 10 Dec 2012 19:59:06 -0800 (PST)\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 EX9kYbymSZNS for <notmuch@notmuchmail.org>;\r
  16         Mon, 10 Dec 2012 19:59:05 -0800 (PST)\r
  17 Received: from dmz-mailsec-scanner-8.mit.edu (DMZ-MAILSEC-SCANNER-8.MIT.EDU\r
  18         [18.7.68.37])\r
  19         by olra.theworths.org (Postfix) with ESMTP id A00DC431FAE\r
  20         for <notmuch@notmuchmail.org>; Mon, 10 Dec 2012 19:59:05 -0800 (PST)\r
  21 X-AuditID: 12074425-b7f606d0000008ea-0a-50c6af89f537\r
  22 Received: from mailhub-auth-1.mit.edu ( [18.9.21.35])\r
  23         by dmz-mailsec-scanner-8.mit.edu (Symantec Messaging Gateway) with SMTP\r
  24         id 09.D7.02282.98FA6C05; Mon, 10 Dec 2012 22:59:05 -0500 (EST)\r
  25 Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103])\r
  26         by mailhub-auth-1.mit.edu (8.13.8/8.9.2) with ESMTP id qBB3x4jc023328; \r
  27         Mon, 10 Dec 2012 22:59:04 -0500\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.6/8.12.4) with ESMTP id qBB3x0iC016515\r
  32         (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT);\r
  33         Mon, 10 Dec 2012 22:59:03 -0500 (EST)\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 1TiGzk-0003ZN-FD; Mon, 10 Dec 2012 22:59:00 -0500\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 2/3] emacs: show: add overlays for each part\r
  40 In-Reply-To: <1354663662-20524-3-git-send-email-markwalters1009@gmail.com>\r
  41 References: <1354663662-20524-1-git-send-email-markwalters1009@gmail.com>\r
  42         <1354663662-20524-3-git-send-email-markwalters1009@gmail.com>\r
  43 User-Agent: Notmuch/0.14+159~g6895fee (http://notmuchmail.org) Emacs/23.4.1\r
  44         (i486-pc-linux-gnu)\r
  45 Date: Mon, 10 Dec 2012 22:59:00 -0500\r
  46 Message-ID: <87txrtnssb.fsf@awakening.csail.mit.edu>\r
  47 MIME-Version: 1.0\r
  48 Content-Type: text/plain; charset=us-ascii\r
  49 X-Brightmail-Tracker:\r
  50  H4sIAAAAAAAAA+NgFrrPIsWRmVeSWpSXmKPExsUixCmqrNu5/liAwZo7rBar5/JYXL85k9mB\r
  51         yWPnrLvsHs9W3WIOYIrisklJzcksSy3St0vgypgz9zFrwQHTiqX/pzI2MD7V6mLk5JAQMJE4\r
  52         MX0KC4QtJnHh3nq2LkYuDiGBfYwSHet3MEI4Gxgl5v38wg7hXGSS2PBkLxOEs4RRYueCf2D9\r
  53         bAL6EivWTmIFsUUEXCWefvvMDGILCzhIzJ+2kB3E5hTwkjj9di5YjZBAO6PE2X3aXYwcHKIC\r
  54         8RKzz/mAmCwCqhK7r4NN5AW6bt2ZRYwQtqDEyZlPwOLMAloSN/69ZJrAKDALSWoWktQCRqZV\r
  55         jLIpuVW6uYmZOcWpybrFyYl5ealFuhZ6uZkleqkppZsYQeHI7qK6g3HCIaVDjAIcjEo8vBWq\r
  56         xwKEWBPLiitzDzFKcjApifL6LgcK8SXlp1RmJBZnxBeV5qQWH2KU4GBWEuEtzQXK8aYkVlal\r
  57         FuXDpKQ5WJTEeW+k3PQXEkhPLEnNTk0tSC2CycpwcChJ8M5bB9QoWJSanlqRlplTgpBm4uAE\r
  58         Gc4DNFwKpIa3uCAxtzgzHSJ/ilFRSpy3FSQhAJLIKM2D64Wli1eM4kCvCPPmglTxAFMNXPcr\r
  59         oMFMQINPCh4GGVySiJCSamD0PpP0WG2Fb5lhgLnCHr2qJ8n8VRdWcHFM0OkL8X/nFyswu7T9\r
  60         q4vnMnkrxqTin9LV5V0CAe/11torC5s0Fiuy5Iez3pbdYVoTXut70GO2TRv7vkupEdNt3vZM\r
  61         rDuffr05Srhx1rp164tkPa4mKdz36A1ROKu/ReXW/xkbWF717ZW0d8voU2Ipzkg01GIuKk4E\r
  62         AGrJA9TyAgAA\r
  63 X-BeenThere: notmuch@notmuchmail.org\r
  64 X-Mailman-Version: 2.1.13\r
  65 Precedence: list\r
  66 List-Id: "Use and development of the notmuch mail system."\r
  67         <notmuch.notmuchmail.org>\r
  68 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  69         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  70 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  71 List-Post: <mailto:notmuch@notmuchmail.org>\r
  72 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  73 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  74         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  75 X-List-Received-Date: Tue, 11 Dec 2012 03:59:06 -0000\r
  76 \r
  77 On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
  78 > This make notmuch-show-insert-bodypart add an overlay for any\r
  79 \r
  80 s/make/makes/\r
  81 \r
  82 > non-trivial part with a button header (currently the first text/plain\r
  83 > part does not have a button). At this point the overlay is available\r
  84 > to the button but there is no action using it yet.\r
  85 >\r
  86 > In addition a not-shown variable which is used to request the part be\r
  87 \r
  88 not-shown is really an argument (I found this confusing).\r
  89 \r
  90 > hidden by default down to the overlay but this is not acted on yet.\r
  91 > ---\r
  92 >  emacs/notmuch-show.el |   62 +++++++++++++++++++++++++++++++++++++-----------\r
  93 >  1 files changed, 48 insertions(+), 14 deletions(-)\r
  94 >\r
  95 > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
  96 > index f8ce037..3215ebc 100644\r
  97 > --- a/emacs/notmuch-show.el\r
  98 > +++ b/emacs/notmuch-show.el\r
  99 > @@ -569,10 +569,9 @@ message at DEPTH in the current thread."\r
 100 >      ;; should be chosen if there are more than one that match?\r
 101 >      (mapc (lambda (inner-part)\r
 102 >           (let ((inner-type (plist-get inner-part :content-type)))\r
 103 > -           (if (or notmuch-show-all-multipart/alternative-parts\r
 104 > -                   (string= chosen-type inner-type))\r
 105 > -               (notmuch-show-insert-bodypart msg inner-part depth)\r
 106 > -             (notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))\r
 107 > +           (notmuch-show-insert-bodypart msg inner-part depth\r
 108 > +                                         (not (or notmuch-show-all-multipart/alternative-parts\r
 109 \r
 110 Since notmuch-show-all-multipart/alternative-parts was basically a hack\r
 111 around our poor multipart/alternative support, I think this series (or a\r
 112 follow up patch) should change its default to nil or even eliminate it\r
 113 entirely.\r
 114 \r
 115 > +                                                  (string= chosen-type inner-type))))))\r
 116 \r
 117 You could let-bind the (not (or ..)) to some variable ("hide" perhaps)\r
 118 in the let above to avoid this crazy line length.\r
 119 \r
 120 >         inner-parts)\r
 121 >  \r
 122 >      (when notmuch-show-indent-multipart\r
 123 > @@ -840,17 +839,52 @@ message at DEPTH in the current thread."\r
 124 >        (setq handlers (cdr handlers))))\r
 125 >    t)\r
 126 >  \r
 127 > -(defun notmuch-show-insert-bodypart (msg part depth)\r
 128 > -  "Insert the body part PART at depth DEPTH in the current thread."\r
 129 > +(defun notmuch-show-insert-part-overlays (msg beg end not-shown)\r
 130 \r
 131 s/not-shown/hide/?  Or hidden?\r
 132 \r
 133 > +  "Add an overlay to the part between BEG and END"\r
 134 > +  (let* ((button (button-at beg))\r
 135 > +      (part-beg (and button (1+ (button-end button)))))\r
 136 > +\r
 137 > +    ;; If the part contains no text we do not make it toggleable.\r
 138 > +    (unless (or (not button) (eq part-beg end))\r
 139 \r
 140 (when (and button (/= part-beg end)) ...) ?\r
 141 \r
 142 > +      (let ((base-label (button-get button :base-label))\r
 143 > +         (overlay (make-overlay part-beg end))\r
 144 > +         (message-invis-spec (plist-get msg :message-invis-spec))\r
 145 > +         (invis-spec (make-symbol "notmuch-part-region")))\r
 146 > +\r
 147 > +     (overlay-put overlay 'invisible (list invis-spec message-invis-spec))\r
 148 \r
 149 Non-trivial buffer invisibility specs are really bad for performance\r
 150 (Emacs' renderer does the obvious O(n^2) thing when rendering a buffer\r
 151 with an invisibility spec).  Unfortunately, because of notmuch-wash and\r
 152 the way overlays with trivial 'invisible properties combine with\r
 153 overlays with list-type 'invisible properties combine, I don't think it\r
 154 can be avoided.  But if we get rid of buffer invisibility specs from\r
 155 notmuch-wash, this code can also get much simpler.\r
 156 \r
 157 > +     (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)\r
 158 \r
 159 This will leave the "(not shown)" in the part header if isearch unfolds\r
 160 the part.\r
 161 \r
 162 Do we even want isearch to unfold parts?  It's not clear we do for\r
 163 multipart/alternative.  If we do, probably the right thing is something\r
 164 like\r
 165 \r
 166 (overlay-put overlay 'notmuch-show-part-button button)\r
 167 (overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)\r
 168 \r
 169 (defun notmuch-show-part-isearch-open (overlay)\r
 170   (notmuch-show-toggle-invisible-part-action\r
 171    (overlay-get overlay 'notmuch-show-part-button)))\r
 172 \r
 173 > +     (overlay-put overlay 'priority 10)\r
 174 > +     (overlay-put overlay 'type "part")\r
 175 > +     ;; Now we have to add invis-spec to every overlay this\r
 176 > +     ;; overlay contains, otherwise these inner overlays will\r
 177 > +     ;; override this one.\r
 178 \r
 179 Interesting.  In the simple case of using nil or t for 'invisible, the\r
 180 specs do combine as one would expect, but you're right that, with a\r
 181 non-trivial invisibility-spec, the highest priority overlay wins.  It's\r
 182 too bad we don't know the depth of the part or we could just set the\r
 183 overlay priority.  This is another thing that would go away if we didn't\r
 184 use buffer invisibility-specs.\r
 185 \r
 186 > +     (mapc (lambda (inner)\r
 187 \r
 188 I would use a (dolist (inner (overlays-in part-beg end)) ...) here.\r
 189 Seems a little more readable.\r
 190 \r
 191 > +             (when (and (>= (overlay-start inner) part-beg)\r
 192 > +                        (<= (overlay-end inner) end))\r
 193 > +               (overlay-put inner 'invisible\r
 194 > +                            (cons invis-spec (overlay-get inner 'invisible)))))\r
 195 > +           (overlays-in part-beg end))\r
 196 > +\r
 197 > +     (button-put button 'invisibility-spec invis-spec)\r
 198 > +     (button-put button 'overlay overlay))\r
 199 > +      (goto-char (point-max)))))\r
 200 \r
 201 This goto-char seems oddly out of place, since it has nothing to do with\r
 202 overlay creation.  Was it supposed to be here instead of in\r
 203 notmuch-show-insert-bodypart?  Is it even necessary?\r
 204 \r
 205 > +\r
 206 > +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)\r
 207 \r
 208 Same comment about not-shown.  (Also in the commit message.)\r
 209 \r
 210 > +  "Insert the body part PART at depth DEPTH in the current thread.\r
 211 > +\r
 212 > +If not-shown is TRUE then initially hide this part."\r
 213 \r
 214 s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/\r
 215 \r
 216 >    (let ((content-type (downcase (plist-get part :content-type)))\r
 217 > -     (nth (plist-get part :id)))\r
 218 > -    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type))\r
 219 > -  ;; Some of the body part handlers leave point somewhere up in the\r
 220 > -  ;; part, so we make sure that we're down at the end.\r
 221 > -  (goto-char (point-max))\r
 222 > -  ;; Ensure that the part ends with a carriage return.\r
 223 > -  (unless (bolp)\r
 224 > -    (insert "\n")))\r
 225 > +     (nth (plist-get part :id))\r
 226 > +     (beg (point)))\r
 227 > +\r
 228 > +    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type)\r
 229 > +    ;; Some of the body part handlers leave point somewhere up in the\r
 230 > +    ;; part, so we make sure that we're down at the end.\r
 231 > +    (goto-char (point-max))\r
 232 > +    ;; Ensure that the part ends with a carriage return.\r
 233 > +    (unless (bolp)\r
 234 > +      (insert "\n"))\r
 235 > +    (notmuch-show-insert-part-overlays msg beg (point) not-shown)))\r
 236 >  \r
 237 >  (defun notmuch-show-insert-body (msg body depth)\r
 238 >    "Insert the body BODY at depth DEPTH in the current thread."\r
 239 > -- \r
 240 > 1.7.9.1\r