[DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[Austin Clements]

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.

 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

Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[Mark Walters]

On Tue, 18 Dec 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.
> ---

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

Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[Mark Walters]

On Tue, 18 Dec 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.
>
>  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))

I am puzzled by this (though it was essentially the same before):
http://www.gnu.org/software/emacs/manual/html_node/elisp/Other-Plists.html
says that "This stores value as the value of the property property in
the property list plist. It may modify plist destructively, or it may
construct a new list structure without altering the old." Does this mean
that this might work but it might not?

Otherwise, though, this looks good to me.

My inclination is to get this in and then do the part invisibility on
top. One reason is that I think we definitely want to go this route
soon and if we do it just after the next release we may end up with bugs
being reported (bugs in my invisibility stuff) that there's not much
point in fixing as mainline has diverged.

Best wishes

Mark




>  
>      (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

Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[Austin Clements]

Quoth Mark Walters on Dec 18 at  6:14 pm:
> On Tue, 18 Dec 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.
> >
> >  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))
> 
> I am puzzled by this (though it was essentially the same before):
> http://www.gnu.org/software/emacs/manual/html_node/elisp/Other-Plists.html
> says that "This stores value as the value of the property property in
> the property list plist. It may modify plist destructively, or it may
> construct a new list structure without altering the old." Does this mean
> that this might work but it might not?

Yeah, I know.  The show code does this all over the place, so I was
being consistent.  Code like this depends on unspecified behavior, but
it is reliable given the actual implementation of plist-put, which
will always add new properties to the end of the plist by mutation.
Hence the only time the setq is *actually* necessary is when
plist-put-ing to an empty plist, and msg will never be an empty plist.

> Otherwise, though, this looks good to me.
> 
> My inclination is to get this in and then do the part invisibility on
> top. One reason is that I think we definitely want to go this route
> soon and if we do it just after the next release we may end up with bugs
> being reported (bugs in my invisibility stuff) that there's not much
> point in fixing as mainline has diverged.
> 
> Best wishes
> 
> Mark

Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[Tomi Ollila]

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

Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from show and wash

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> 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.

Pushed. 

d