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