From ba9fbe60023b87519d6c49f7aab74ae879b3e165 Mon Sep 17 00:00:00 2001 From: Mark Walters Date: Tue, 18 Dec 2012 14:59:56 +0000 Subject: [PATCH] Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash --- 5b/cf58a67dfa0cb4beca9f9be3135fc20c28146f | 405 ++++++++++++++++++++++ 1 file changed, 405 insertions(+) create mode 100644 5b/cf58a67dfa0cb4beca9f9be3135fc20c28146f diff --git a/5b/cf58a67dfa0cb4beca9f9be3135fc20c28146f b/5b/cf58a67dfa0cb4beca9f9be3135fc20c28146f new file mode 100644 index 000000000..cdf71a460 --- /dev/null +++ b/5b/cf58a67dfa0cb4beca9f9be3135fc20c28146f @@ -0,0 +1,405 @@ +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 AEE3B431FAF + for ; Tue, 18 Dec 2012 07:00:10 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -1.098 +X-Spam-Level: +X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5 + tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001, + 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 w6Z8DuFMCxyR for ; + Tue, 18 Dec 2012 07:00:09 -0800 (PST) +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 5B13C431FAE + for ; Tue, 18 Dec 2012 07:00:08 -0800 (PST) +Received: from smtp.qmul.ac.uk ([138.37.6.40]) + by mail2.qmul.ac.uk with esmtp (Exim 4.71) + (envelope-from ) + id 1TkyeE-0000JP-5R; Tue, 18 Dec 2012 15:00:00 +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 1TkyeD-0007F2-06; Tue, 18 Dec 2012 14:59:57 +0000 +From: Mark Walters +To: Austin Clements , notmuch@notmuchmail.org +Subject: Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from + show and wash +In-Reply-To: <1355812810-32747-1-git-send-email-amdragon@mit.edu> +References: <1355812810-32747-1-git-send-email-amdragon@mit.edu> +User-Agent: Notmuch/0.14+236~g1d0044f (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Tue, 18 Dec 2012 14:59:56 +0000 +Message-ID: <87ip7zz9qr.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: e67e966bf39865ad87a4e38a086da98d (of first 20000 bytes) +X-SpamAssassin-Score: -1.8 +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.8 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.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay + * domain + * 0.5 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: Tue, 18 Dec 2012 15:00:10 -0000 + + +On Tue, 18 Dec 2012, Austin Clements wrote: +> Previously, all visibility in show buffers for headers, message +> bodies, and washed text was specified by generating one or more +> symbols for each region and creating overlays with their 'invisible +> property set to carefully crafted combinations of these symbols. +> Visibility was controlled not by modifying the overlays directly, but +> by adding and removing the generated symbols from a gigantic buffer +> invisibilty spec. +> +> This has myriad negative consequences. It's slow because Emacs' +> display engine has to traverse the buffer invisibility list for every +> overlay and, since every overlay has its own symbol, this makes +> rendering O(N^2) in the number of overlays. It composes poorly +> because symbol-type 'invisible properties are taken from the highest +> priority overlay over a given character (which is often ambiguous!), +> rather than being gathered from all overlays over a character. As a +> result, we have to include symbols related to message hiding in the +> wash code lest the wash overlays un-hide parts of hidden messages. It +> also requires various workarounds for isearch to properly open +> overlays, to set up buffer-invisibility-spec for +> remove-from-invisibility-spec to work right, and to explicitly refresh +> the display after updating the buffer invisibility spec. +> +> None of this is necessary. +> +> This patch converts show and wash to use simple boolean 'invisible +> properties and to not use the buffer invisibility spec. Rather than +> adding and removing generated symbols from the invisibility spec, the +> code now directly toggles the 'invisible property of the appropriate +> overlay. This speeds up rendering because the display engine only has +> to check the boolean values of the overlays over a character. It +> composes nicely because text will be invisible if *any* overlay over +> it has 'invisible t, which means we can overlap invisibility overlays +> with abandon. We no longer need any of the workarounds mentioned +> above. And it fixes a minor bug for free: now, when isearch opens a +> washed region, the button text will update to say "Click/Enter to +> hide" rather than remaining unchanged. +> --- + +This looks really good: the diffstat is great (even if we ignore the +loss of the isearch-workaround it is still 17 insertions for 55 +deletions!) and the new code works as one would expect with none of this +nasty adding outer overlay invisbility properties to all inner overlay +lists stuff. + +> Mark's part toggling code got me thinking about how needlessly +> complicated our other show-mode invisibility code was. This patch is +> a shot at fixing that. It will require a bit of reworking after part +> toggling goes in (owning to the aforementioned non-composability, part +> toggling needs to be intimately aware of wash and message hiding). +> However, I think this patch should wait until after the release +> freeze; this code is fragile (though much less so after this patch), +> so I'd rather it soak for a release than cause user-visible bugs for +> no user-visible gain. + +I have modified the part invisibility stuff to apply on top of this +patch and it loses 20 lines, and those are the 20 lines which are +`non-obvious' (the create-part-overlays function becomes almost +trivial). + +It is not clear to me that putting this in is more dangerous +than not: with the current code the part-invisibility is inherently +fragile and with this it feels much less so. One example of this is the +bug Jani found (with fake x-diff parts): it is not a bug when applied on +top of the series because we do not have to do strange things with the +invisibility specs. + +Incidentally, the invisibility specs approach seems to date back to very +early notmuch (2009): I can't find any comments on why this approach was +chosen instead of Austin's. + +Best wishes + +Mark + + + + + +> +> emacs/notmuch-show.el | 42 +++------------------ +> emacs/notmuch-wash.el | 97 ++++++------------------------------------------- +> 2 files changed, 17 insertions(+), 122 deletions(-) +> +> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el +> index 7d9f8a9..4bdd5af 100644 +> --- a/emacs/notmuch-show.el +> +++ b/emacs/notmuch-show.el +> @@ -872,27 +872,8 @@ message at DEPTH in the current thread." +> message-start message-end +> content-start content-end +> headers-start headers-end +> - body-start body-end +> - (headers-invis-spec (notmuch-show-make-symbol "header")) +> - (message-invis-spec (notmuch-show-make-symbol "message")) +> (bare-subject (notmuch-show-strip-re (plist-get headers :Subject)))) +> +> - ;; Set `buffer-invisibility-spec' to `nil' (a list), otherwise +> - ;; removing items from `buffer-invisibility-spec' (which is what +> - ;; `notmuch-show-headers-visible' and +> - ;; `notmuch-show-message-visible' do) is a no-op and has no +> - ;; effect. This caused threads with only matching messages to have +> - ;; those messages hidden initially because +> - ;; `buffer-invisibility-spec' stayed `t'. +> - ;; +> - ;; This needs to be set here (rather than just above the call to +> - ;; `notmuch-show-headers-visible') because some of the part +> - ;; rendering or body washing functions +> - ;; (e.g. `notmuch-wash-text/plain-citations') manipulate +> - ;; `buffer-invisibility-spec'). +> - (when (eq buffer-invisibility-spec t) +> - (setq buffer-invisibility-spec nil)) +> - +> (setq message-start (point-marker)) +> +> (notmuch-show-insert-headerline headers +> @@ -904,9 +885,6 @@ message at DEPTH in the current thread." +> +> (setq content-start (point-marker)) +> +> - (plist-put msg :headers-invis-spec headers-invis-spec) +> - (plist-put msg :message-invis-spec message-invis-spec) +> - +> ;; Set `headers-start' to point after the 'Subject:' header to be +> ;; compatible with the existing implementation. This just sets it +> ;; to after the first header. +> @@ -924,7 +902,6 @@ message at DEPTH in the current thread." +> +> (setq notmuch-show-previous-subject bare-subject) +> +> - (setq body-start (point-marker)) +> ;; A blank line between the headers and the body. +> (insert "\n") +> (notmuch-show-insert-body msg (plist-get msg :body) +> @@ -932,7 +909,6 @@ message at DEPTH in the current thread." +> ;; Ensure that the body ends with a newline. +> (unless (bolp) +> (insert "\n")) +> - (setq body-end (point-marker)) +> (setq content-end (point-marker)) +> +> ;; Indent according to the depth in the thread. +> @@ -945,11 +921,9 @@ message at DEPTH in the current thread." +> ;; message. +> (put-text-property message-start message-end :notmuch-message-extent (cons message-start message-end)) +> +> - (let ((headers-overlay (make-overlay headers-start headers-end)) +> - (invis-specs (list headers-invis-spec message-invis-spec))) +> - (overlay-put headers-overlay 'invisible invis-specs) +> - (overlay-put headers-overlay 'priority 10)) +> - (overlay-put (make-overlay body-start body-end) 'invisible message-invis-spec) +> + ;; Create overlays used to control visibility +> + (plist-put msg :headers-overlay (make-overlay headers-start headers-end)) +> + (plist-put msg :message-overlay (make-overlay headers-start content-end)) +> +> (plist-put msg :depth depth) +> +> @@ -1349,18 +1323,12 @@ effects." +> ;; Functions relating to the visibility of messages and their +> ;; components. +> +> -(defun notmuch-show-element-visible (props visible-p spec-property) +> - (let ((spec (plist-get props spec-property))) +> - (if visible-p +> - (remove-from-invisibility-spec spec) +> - (add-to-invisibility-spec spec)))) +> - +> (defun notmuch-show-message-visible (props visible-p) +> - (notmuch-show-element-visible props visible-p :message-invis-spec) +> + (overlay-put (plist-get props :message-overlay) 'invisible (not visible-p)) +> (notmuch-show-set-prop :message-visible visible-p props)) +> +> (defun notmuch-show-headers-visible (props visible-p) +> - (notmuch-show-element-visible props visible-p :headers-invis-spec) +> + (overlay-put (plist-get props :headers-overlay) 'invisible (not visible-p)) +> (notmuch-show-set-prop :headers-visible visible-p props)) +> +> ;; Functions for setting and getting attributes of the current +> diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el +> index 7d003a2..d6db4fa 100644 +> --- a/emacs/notmuch-wash.el +> +++ b/emacs/notmuch-wash.el +> @@ -96,10 +96,10 @@ this many characters or at the window width (whichever one is +> lower).") +> +> (defun notmuch-wash-toggle-invisible-action (cite-button) +> - (let ((invis-spec (button-get cite-button 'invisibility-spec))) +> - (if (invisible-p invis-spec) +> - (remove-from-invisibility-spec invis-spec) +> - (add-to-invisibility-spec invis-spec))) +> + ;; Toggle overlay visibility +> + (let ((overlay (button-get cite-button 'overlay))) +> + (overlay-put overlay 'invisible (not (overlay-get overlay 'invisible)))) +> + ;; Update button text +> (let* ((new-start (button-start cite-button)) +> (overlay (button-get cite-button 'overlay)) +> (button-label (notmuch-wash-button-label overlay)) +> @@ -110,9 +110,7 @@ lower).") +> (let ((old-end (button-end cite-button))) +> (move-overlay cite-button new-start (point)) +> (delete-region (point) old-end)) +> - (goto-char (min old-point (1- (button-end cite-button))))) +> - (force-window-update) +> - (redisplay t)) +> + (goto-char (min old-point (1- (button-end cite-button)))))) +> +> (define-button-type 'notmuch-wash-button-invisibility-toggle-type +> 'action 'notmuch-wash-toggle-invisible-action +> @@ -132,8 +130,8 @@ lower).") +> :supertype 'notmuch-wash-button-invisibility-toggle-type) +> +> (defun notmuch-wash-region-isearch-show (overlay) +> - (dolist (invis-spec (overlay-get overlay 'invisible)) +> - (remove-from-invisibility-spec invis-spec))) +> + (notmuch-wash-toggle-invisible-action +> + (overlay-get overlay 'notmuch-wash-button))) +> +> (defun notmuch-wash-button-label (overlay) +> (let* ((type (overlay-get overlay 'type)) +> @@ -158,14 +156,10 @@ that PREFIX should not include a newline." +> ;; since the newly created symbol has no plist. +> +> (let ((overlay (make-overlay beg end)) +> - (message-invis-spec (plist-get msg :message-invis-spec)) +> - (invis-spec (make-symbol (concat "notmuch-" type "-region"))) +> (button-type (intern-soft (concat "notmuch-wash-button-" +> type "-toggle-type")))) +> - (add-to-invisibility-spec invis-spec) +> - (overlay-put overlay 'invisible (list invis-spec message-invis-spec)) +> + (overlay-put overlay 'invisible t) +> (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show) +> - (overlay-put overlay 'priority 10) +> (overlay-put overlay 'type type) +> (goto-char (1+ end)) +> (save-excursion +> @@ -174,10 +168,10 @@ that PREFIX should not include a newline." +> (insert-before-markers prefix)) +> (let ((button-beg (point))) +> (insert-before-markers (notmuch-wash-button-label overlay) "\n") +> - (make-button button-beg (1- (point)) +> - 'invisibility-spec invis-spec +> - 'overlay overlay +> - :type button-type))))) +> + (let ((button (make-button button-beg (1- (point)) +> + 'overlay overlay +> + :type button-type))) +> + (overlay-put overlay 'notmuch-wash-button button)))))) +> +> (defun notmuch-wash-excerpt-citations (msg depth) +> "Excerpt citations and up to one signature." +> @@ -382,71 +376,4 @@ for error." +> +> ;; +> +> -;; Temporary workaround for Emacs bug #8721 +> -;; http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8721 +> - +> -(defun notmuch-isearch-range-invisible (beg end) +> - "Same as `isearch-range-invisible' but with fixed Emacs bug #8721." +> - (when (/= beg end) +> - ;; Check that invisibility runs up to END. +> - (save-excursion +> - (goto-char beg) +> - (let (;; can-be-opened keeps track if we can open some overlays. +> - (can-be-opened (eq search-invisible 'open)) +> - ;; the list of overlays that could be opened +> - (crt-overlays nil)) +> - (when (and can-be-opened isearch-hide-immediately) +> - (isearch-close-unnecessary-overlays beg end)) +> - ;; If the following character is currently invisible, +> - ;; skip all characters with that same `invisible' property value. +> - ;; Do that over and over. +> - (while (and (< (point) end) (invisible-p (point))) +> - (if (invisible-p (get-text-property (point) 'invisible)) +> - (progn +> - (goto-char (next-single-property-change (point) 'invisible +> - nil end)) +> - ;; if text is hidden by an `invisible' text property +> - ;; we cannot open it at all. +> - (setq can-be-opened nil)) +> - (when can-be-opened +> - (let ((overlays (overlays-at (point))) +> - ov-list +> - o +> - invis-prop) +> - (while overlays +> - (setq o (car overlays) +> - invis-prop (overlay-get o 'invisible)) +> - (if (invisible-p invis-prop) +> - (if (overlay-get o 'isearch-open-invisible) +> - (setq ov-list (cons o ov-list)) +> - ;; We found one overlay that cannot be +> - ;; opened, that means the whole chunk +> - ;; cannot be opened. +> - (setq can-be-opened nil))) +> - (setq overlays (cdr overlays))) +> - (if can-be-opened +> - ;; It makes sense to append to the open +> - ;; overlays list only if we know that this is +> - ;; t. +> - (setq crt-overlays (append ov-list crt-overlays))))) +> - (goto-char (next-overlay-change (point))))) +> - ;; See if invisibility reaches up thru END. +> - (if (>= (point) end) +> - (if (and can-be-opened (consp crt-overlays)) +> - (progn +> - (setq isearch-opened-overlays +> - (append isearch-opened-overlays crt-overlays)) +> - (mapc 'isearch-open-overlay-temporary crt-overlays) +> - nil) +> - (setq isearch-hidden t))))))) +> - +> -(defadvice isearch-range-invisible (around notmuch-isearch-range-invisible-advice activate) +> - "Call `notmuch-isearch-range-invisible' instead of the original +> -`isearch-range-invisible' when in `notmuch-show-mode' mode." +> - (if (eq major-mode 'notmuch-show-mode) +> - (setq ad-return-value (notmuch-isearch-range-invisible beg end)) +> - ad-do-it)) +> - +> -;; +> - +> (provide 'notmuch-wash) +> -- +> 1.7.10.4 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch -- 2.26.2