From 6b01b8efd32c2d7f9bb01eec02e634bc9fd0fe6a Mon Sep 17 00:00:00 2001 From: Tomi Ollila Date: Wed, 19 Dec 2012 17:52:42 +0200 Subject: [PATCH] Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash --- ba/468cb6721716187b1372e72161802af51cc890 | 359 ++++++++++++++++++++++ 1 file changed, 359 insertions(+) create mode 100644 ba/468cb6721716187b1372e72161802af51cc890 diff --git a/ba/468cb6721716187b1372e72161802af51cc890 b/ba/468cb6721716187b1372e72161802af51cc890 new file mode 100644 index 000000000..2167f6f03 --- /dev/null +++ b/ba/468cb6721716187b1372e72161802af51cc890 @@ -0,0 +1,359 @@ +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 49067431FAF + for ; Wed, 19 Dec 2012 07:52:51 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: 0 +X-Spam-Level: +X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] + 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 phsgYU+iuDm2 for ; + Wed, 19 Dec 2012 07:52:49 -0800 (PST) +Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) + by olra.theworths.org (Postfix) with ESMTP id 64281431FAE + for ; Wed, 19 Dec 2012 07:52:49 -0800 (PST) +Received: from guru.guru-group.fi (localhost [IPv6:::1]) + by guru.guru-group.fi (Postfix) with ESMTP id F2039100094; + Wed, 19 Dec 2012 17:52:42 +0200 (EET) +From: Tomi Ollila +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+211~g71b47e9 (http://notmuchmail.org) Emacs/24.2.1 + (x86_64-unknown-linux-gnu) +X-Face: HhBM'cA~ +MIME-Version: 1.0 +Content-Type: text/plain +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: Wed, 19 Dec 2012 15:52:51 -0000 + +On Tue, Dec 18 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. +> --- +> +> 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. + +What is the current thought of getting this (and series of +id:1355858880-16329-1-git-send-email-markwalters1009@gmail.com ) +pushed ? The patches doesn't look bad to me and these seems to +work very well... + +Tomi + + +> +> 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