Date: Tue, 18 Dec 2012 13:48:25 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [DRAFT PATCH] emacs: Eliminate buffer invisibility specs from
	show and wash
Message-ID: <20121218184825.GM6187@mit.edu>
References: <1355812810-32747-1-git-send-email-amdragon@mit.edu>
	<87licvqlbb.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87licvqlbb.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

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