From: Austin Clements Date: Tue, 11 Dec 2012 03:59:00 +0000 (+1900) Subject: Re: [PATCH 2/3] emacs: show: add overlays for each part X-Git-Url: http://git.tremily.us/gitweb.cgi?a=commitdiff_plain;h=7a410c5a25b2ad065bc361a8b1fedf8fed353a67;p=notmuch-archives.git Re: [PATCH 2/3] emacs: show: add overlays for each part --- diff --git a/d9/592aa40f974a2cc25369da93b9b8d3b5bba080 b/d9/592aa40f974a2cc25369da93b9b8d3b5bba080 new file mode 100644 index 000000000..6bb03d933 --- /dev/null +++ b/d9/592aa40f974a2cc25369da93b9b8d3b5bba080 @@ -0,0 +1,240 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id BAC9A431FAF + for ; Mon, 10 Dec 2012 19:59:06 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 EX9kYbymSZNS for ; + Mon, 10 Dec 2012 19:59:05 -0800 (PST) +Received: from dmz-mailsec-scanner-8.mit.edu (DMZ-MAILSEC-SCANNER-8.MIT.EDU + [18.7.68.37]) + by olra.theworths.org (Postfix) with ESMTP id A00DC431FAE + for ; Mon, 10 Dec 2012 19:59:05 -0800 (PST) +X-AuditID: 12074425-b7f606d0000008ea-0a-50c6af89f537 +Received: from mailhub-auth-1.mit.edu ( [18.9.21.35]) + by dmz-mailsec-scanner-8.mit.edu (Symantec Messaging Gateway) with SMTP + id 09.D7.02282.98FA6C05; Mon, 10 Dec 2012 22:59:05 -0500 (EST) +Received: from outgoing.mit.edu (OUTGOING-AUTH.MIT.EDU [18.7.22.103]) + by mailhub-auth-1.mit.edu (8.13.8/8.9.2) with ESMTP id qBB3x4jc023328; + Mon, 10 Dec 2012 22:59:04 -0500 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.6/8.12.4) with ESMTP id qBB3x0iC016515 + (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); + Mon, 10 Dec 2012 22:59:03 -0500 (EST) +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80) + (envelope-from ) + id 1TiGzk-0003ZN-FD; Mon, 10 Dec 2012 22:59:00 -0500 +From: Austin Clements +To: Mark Walters , notmuch@notmuchmail.org +Subject: Re: [PATCH 2/3] emacs: show: add overlays for each part +In-Reply-To: <1354663662-20524-3-git-send-email-markwalters1009@gmail.com> +References: <1354663662-20524-1-git-send-email-markwalters1009@gmail.com> + <1354663662-20524-3-git-send-email-markwalters1009@gmail.com> +User-Agent: Notmuch/0.14+159~g6895fee (http://notmuchmail.org) Emacs/23.4.1 + (i486-pc-linux-gnu) +Date: Mon, 10 Dec 2012 22:59:00 -0500 +Message-ID: <87txrtnssb.fsf@awakening.csail.mit.edu> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFrrPIsWRmVeSWpSXmKPExsUixCmqrNu5/liAwZo7rBar5/JYXL85k9mB + yWPnrLvsHs9W3WIOYIrisklJzcksSy3St0vgypgz9zFrwQHTiqX/pzI2MD7V6mLk5JAQMJE4 + MX0KC4QtJnHh3nq2LkYuDiGBfYwSHet3MEI4Gxgl5v38wg7hXGSS2PBkLxOEs4RRYueCf2D9 + bAL6EivWTmIFsUUEXCWefvvMDGILCzhIzJ+2kB3E5hTwkjj9di5YjZBAO6PE2X3aXYwcHKIC + 8RKzz/mAmCwCqhK7r4NN5AW6bt2ZRYwQtqDEyZlPwOLMAloSN/69ZJrAKDALSWoWktQCRqZV + jLIpuVW6uYmZOcWpybrFyYl5ealFuhZ6uZkleqkppZsYQeHI7qK6g3HCIaVDjAIcjEo8vBWq + xwKEWBPLiitzDzFKcjApifL6LgcK8SXlp1RmJBZnxBeV5qQWH2KU4GBWEuEtzQXK8aYkVlal + FuXDpKQ5WJTEeW+k3PQXEkhPLEnNTk0tSC2CycpwcChJ8M5bB9QoWJSanlqRlplTgpBm4uAE + Gc4DNFwKpIa3uCAxtzgzHSJ/ilFRSpy3FSQhAJLIKM2D64Wli1eM4kCvCPPmglTxAFMNXPcr + oMFMQINPCh4GGVySiJCSamD0PpP0WG2Fb5lhgLnCHr2qJ8n8VRdWcHFM0OkL8X/nFyswu7T9 + q4vnMnkrxqTin9LV5V0CAe/11torC5s0Fiuy5Iez3pbdYVoTXut70GO2TRv7vkupEdNt3vZM + rDuffr05Srhx1rp164tkPa4mKdz36A1ROKu/ReXW/xkbWF717ZW0d8voU2Ipzkg01GIuKk4E + AGrJA9TyAgAA +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: Tue, 11 Dec 2012 03:59:06 -0000 + +On Tue, 04 Dec 2012, Mark Walters wrote: +> This make notmuch-show-insert-bodypart add an overlay for any + +s/make/makes/ + +> non-trivial part with a button header (currently the first text/plain +> part does not have a button). At this point the overlay is available +> to the button but there is no action using it yet. +> +> In addition a not-shown variable which is used to request the part be + +not-shown is really an argument (I found this confusing). + +> hidden by default down to the overlay but this is not acted on yet. +> --- +> emacs/notmuch-show.el | 62 +++++++++++++++++++++++++++++++++++++----------- +> 1 files changed, 48 insertions(+), 14 deletions(-) +> +> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el +> index f8ce037..3215ebc 100644 +> --- a/emacs/notmuch-show.el +> +++ b/emacs/notmuch-show.el +> @@ -569,10 +569,9 @@ message at DEPTH in the current thread." +> ;; should be chosen if there are more than one that match? +> (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 + +Since notmuch-show-all-multipart/alternative-parts was basically a hack +around our poor multipart/alternative support, I think this series (or a +follow up patch) should change its default to nil or even eliminate it +entirely. + +> + (string= chosen-type inner-type)))))) + +You could let-bind the (not (or ..)) to some variable ("hide" perhaps) +in the let above to avoid this crazy line length. + +> inner-parts) +> +> (when notmuch-show-indent-multipart +> @@ -840,17 +839,52 @@ message at DEPTH in the current thread." +> (setq handlers (cdr handlers)))) +> t) +> +> -(defun notmuch-show-insert-bodypart (msg part depth) +> - "Insert the body part PART at depth DEPTH in the current thread." +> +(defun notmuch-show-insert-part-overlays (msg beg end not-shown) + +s/not-shown/hide/? Or hidden? + +> + "Add an overlay to the part between BEG and END" +> + (let* ((button (button-at beg)) +> + (part-beg (and button (1+ (button-end button))))) +> + +> + ;; If the part contains no text we do not make it toggleable. +> + (unless (or (not button) (eq part-beg end)) + +(when (and button (/= part-beg end)) ...) ? + +> + (let ((base-label (button-get button :base-label)) +> + (overlay (make-overlay part-beg end)) +> + (message-invis-spec (plist-get msg :message-invis-spec)) +> + (invis-spec (make-symbol "notmuch-part-region"))) +> + +> + (overlay-put overlay 'invisible (list invis-spec message-invis-spec)) + +Non-trivial buffer invisibility specs are really bad for performance +(Emacs' renderer does the obvious O(n^2) thing when rendering a buffer +with an invisibility spec). Unfortunately, because of notmuch-wash and +the way overlays with trivial 'invisible properties combine with +overlays with list-type 'invisible properties combine, I don't think it +can be avoided. But if we get rid of buffer invisibility specs from +notmuch-wash, this code can also get much simpler. + +> + (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show) + +This will leave the "(not shown)" in the part header if isearch unfolds +the part. + +Do we even want isearch to unfold parts? It's not clear we do for +multipart/alternative. If we do, probably the right thing is something +like + +(overlay-put overlay 'notmuch-show-part-button button) +(overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open) + +(defun notmuch-show-part-isearch-open (overlay) + (notmuch-show-toggle-invisible-part-action + (overlay-get overlay 'notmuch-show-part-button))) + +> + (overlay-put overlay 'priority 10) +> + (overlay-put overlay 'type "part") +> + ;; Now we have to add invis-spec to every overlay this +> + ;; overlay contains, otherwise these inner overlays will +> + ;; override this one. + +Interesting. In the simple case of using nil or t for 'invisible, the +specs do combine as one would expect, but you're right that, with a +non-trivial invisibility-spec, the highest priority overlay wins. It's +too bad we don't know the depth of the part or we could just set the +overlay priority. This is another thing that would go away if we didn't +use buffer invisibility-specs. + +> + (mapc (lambda (inner) + +I would use a (dolist (inner (overlays-in part-beg end)) ...) here. +Seems a little more readable. + +> + (when (and (>= (overlay-start inner) part-beg) +> + (<= (overlay-end inner) end)) +> + (overlay-put inner 'invisible +> + (cons invis-spec (overlay-get inner 'invisible))))) +> + (overlays-in part-beg end)) +> + +> + (button-put button 'invisibility-spec invis-spec) +> + (button-put button 'overlay overlay)) +> + (goto-char (point-max))))) + +This goto-char seems oddly out of place, since it has nothing to do with +overlay creation. Was it supposed to be here instead of in +notmuch-show-insert-bodypart? Is it even necessary? + +> + +> +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown) + +Same comment about not-shown. (Also in the commit message.) + +> + "Insert the body part PART at depth DEPTH in the current thread. +> + +> +If not-shown is TRUE then initially hide this part." + +s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/ + +> (let ((content-type (downcase (plist-get part :content-type))) +> - (nth (plist-get part :id))) +> - (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type)) +> - ;; Some of the body part handlers leave point somewhere up in the +> - ;; part, so we make sure that we're down at the end. +> - (goto-char (point-max)) +> - ;; Ensure that the part ends with a carriage return. +> - (unless (bolp) +> - (insert "\n"))) +> + (nth (plist-get part :id)) +> + (beg (point))) +> + +> + (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type) +> + ;; Some of the body part handlers leave point somewhere up in the +> + ;; part, so we make sure that we're down at the end. +> + (goto-char (point-max)) +> + ;; Ensure that the part ends with a carriage return. +> + (unless (bolp) +> + (insert "\n")) +> + (notmuch-show-insert-part-overlays msg beg (point) not-shown))) +> +> (defun notmuch-show-insert-body (msg body depth) +> "Insert the body BODY at depth DEPTH in the current thread." +> -- +> 1.7.9.1