Date: Sat, 15 Dec 2012 00:16:50 -0500
From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH 2/3] emacs: show: add overlays for each part
Message-ID: <20121215051650.GE6187@mit.edu>
References: <1354663662-20524-1-git-send-email-markwalters1009@gmail.com>
	<1354663662-20524-3-git-send-email-markwalters1009@gmail.com>
	<87txrtnssb.fsf@awakening.csail.mit.edu>
	<87obhy72o1.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87obhy72o1.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Dec 13 at  8:54 am:
> >> +						     (string= chosen-type inner-type))))))
> >
> > You could let-bind the (not (or ..)) to some variable ("hide" perhaps)
> > in the let above to avoid this crazy line length.
> >
> >>  	  inner-parts)
> >>  
> >>      (when notmuch-show-indent-multipart
> >> @@ -840,17 +839,52 @@ message at DEPTH in the current thread."
> >>        (setq handlers (cdr handlers))))
> >>    t)
> >>  
> >> -(defun notmuch-show-insert-bodypart (msg part depth)
> >> -  "Insert the body part PART at depth DEPTH in the current thread."
> >> +(defun notmuch-show-insert-part-overlays (msg beg end not-shown)
> >
> > s/not-shown/hide/?  Or hidden?
> >
> >> +  "Add an overlay to the part between BEG and END"
> >> +  (let* ((button (button-at beg))
> >> +	 (part-beg (and button (1+ (button-end button)))))
> >> +
> >> +    ;; If the part contains no text we do not make it toggleable.
> >> +    (unless (or (not button) (eq part-beg end))
> >
> > (when (and button (/= part-beg end)) ...) ?
> >
> >> +      (let ((base-label (button-get button :base-label))
> >> +	    (overlay (make-overlay part-beg end))
> >> +	    (message-invis-spec (plist-get msg :message-invis-spec))
> >> +	    (invis-spec (make-symbol "notmuch-part-region")))
> >> +
> >> +	(overlay-put overlay 'invisible (list invis-spec message-invis-spec))
> >
> > Non-trivial buffer invisibility specs are really bad for performance
> > (Emacs' renderer does the obvious O(n^2) thing when rendering a buffer
> > with an invisibility spec).  Unfortunately, because of notmuch-wash and
> > the way overlays with trivial 'invisible properties combine with
> > overlays with list-type 'invisible properties combine, I don't think it
> > can be avoided.  But if we get rid of buffer invisibility specs from
> > notmuch-wash, this code can also get much simpler.
> 
> How do you plan to get rid of the invisibility properties from notmuch
> wash? 

Not the invisibility properties.  The invisibility specs.  The
performance impact and difficult composition comes from using buffer
invisibility specs, but the 'invisible text property can be simply nil
or t.  These are just as easy to manipulate as the generated
invisibility symbols we use all over the place (you just manipulate
the property directly instead of the buffer invisibility spec).  They
also compose like you'd want (if any overlay over some text has
'invisible set to t, then that text is invisible).

> >> +	(overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
> >
> > This will leave the "(not shown)" in the part header if isearch unfolds
> > the part.
> >
> > Do we even want isearch to unfold parts?  It's not clear we do for
> > multipart/alternative.  If we do, probably the right thing is something
> > like
> 
> I don't think we want to search hidden bodyparts so I just deleted this
> line.
> 
> >
> > (overlay-put overlay 'notmuch-show-part-button button)
> > (overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)
> >
> > (defun notmuch-show-part-isearch-open (overlay)
> >   (notmuch-show-toggle-invisible-part-action
> >    (overlay-get overlay 'notmuch-show-part-button)))
> >
> >> +	(overlay-put overlay 'priority 10)
> >> +	(overlay-put overlay 'type "part")
> >> +	;; Now we have to add invis-spec to every overlay this
> >> +	;; overlay contains, otherwise these inner overlays will
> >> +	;; override this one.
> >
> > Interesting.  In the simple case of using nil or t for 'invisible, the
> > specs do combine as one would expect, but you're right that, with a
> > non-trivial invisibility-spec, the highest priority overlay wins.  It's
> > too bad we don't know the depth of the part or we could just set the
> > overlay priority.  This is another thing that would go away if we didn't
> > use buffer invisibility-specs.
> 
> I don't think priorities get it right. As far as I could tell if you
> have two overlays at different priorities both with invisibility
> properties then the higher priority overlay decides visibility by
> itself: that is if it says invisible then the text is invisible, and if
> it says visible then the text is visible.

Yes.  I think I was confused when I claimed we could use priorities,
since I can't reconstruct how I thought we could do that.