[DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash
Tomi Ollila
tomi.ollila at iki.fi
Wed Dec 19 07:52:42 PST 2012
On Tue, Dec 18 2012, Austin Clements <amdragon at MIT.EDU> 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 at 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 at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list