From: Tomi Ollila <tomi.ollila@iki.fi>
To: Austin Clements <amdragon@MIT.EDU>, 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)
Date: Wed, 19 Dec 2012 17:52:42 +0200
Message-ID: <m2vcby2g51.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Tue, Dec 18 2012, Austin Clements <amdragon@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@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