From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <aclements@csail.mit.edu>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/3] emacs: show: add overlays for each part
In-Reply-To: <87txrtnssb.fsf@awakening.csail.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>
User-Agent: Notmuch/0.14+155~g7edfdc3 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Thu, 13 Dec 2012 08:54:22 +0000
Message-ID: <87obhy72o1.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi 

Many thanks for the review: I will post a new version shortly. Some
comments inline below:

On Tue, 11 Dec 2012, Austin Clements <aclements@csail.mit.edu> wrote:
> On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>> This make notmuch-show-insert-bodypart add an overlay for any
>
> s/make/makes/
>
>> non-trivial part with a button header (currently the first text/plain
>> part does not have a button). At this point the overlay is available
>> to the button but there is no action using it yet.
>>
>> In addition a not-shown variable which is used to request the part be
>
> not-shown is really an argument (I found this confusing).
>
>> hidden by default down to the overlay but this is not acted on yet.
>> ---
>>  emacs/notmuch-show.el |   62 +++++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 48 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index f8ce037..3215ebc 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -569,10 +569,9 @@ message at DEPTH in the current thread."
>>      ;; should be chosen if there are more than one that match?
>>      (mapc (lambda (inner-part)
>>  	    (let ((inner-type (plist-get inner-part :content-type)))
>> -	      (if (or notmuch-show-all-multipart/alternative-parts
>> -		      (string= chosen-type inner-type))
>> -		  (notmuch-show-insert-bodypart msg inner-part depth)
>> -		(notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))
>> +	      (notmuch-show-insert-bodypart msg inner-part depth
>> +					    (not (or notmuch-show-all-multipart/alternative-parts
>
> Since notmuch-show-all-multipart/alternative-parts was basically a hack
> around our poor multipart/alternative support, I think this series (or a
> follow up patch) should change its default to nil or even eliminate it
> entirely.

I have added a patch at the end setting this to nil by default.

>> +						     (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? 

>> +	(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.

>
>> +	(mapc (lambda (inner)
>
> I would use a (dolist (inner (overlays-in part-beg end)) ...) here.
> Seems a little more readable.
>
>> +		(when (and (>= (overlay-start inner) part-beg)
>> +			   (<= (overlay-end inner) end))
>> +		  (overlay-put inner 'invisible
>> +			       (cons invis-spec (overlay-get inner 'invisible)))))
>> +	      (overlays-in part-beg end))
>> +
>> +	(button-put button 'invisibility-spec invis-spec)
>> +	(button-put button 'overlay overlay))
>> +      (goto-char (point-max)))))
>
> This goto-char seems oddly out of place, since it has nothing to do with
> overlay creation.  Was it supposed to be here instead of in
> notmuch-show-insert-bodypart?  Is it even necessary?

It was suppose to be in notmuch-show-insert-body-part. It was necessary
but I have wrapped the toggle-button call in the third patch and now it
does not seem necessary.

It is possible that the toggle-button function should not move point: if
that got changed the save-excursion here could be removed. (I will
discuss this in my reply to your comments on the next patch).

Best wishes 

Mark

>
>> +
>> +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)
>
> Same comment about not-shown.  (Also in the commit message.)
>
>> +  "Insert the body part PART at depth DEPTH in the current thread.
>> +
>> +If not-shown is TRUE then initially hide this part."
>
> s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/
>
>>    (let ((content-type (downcase (plist-get part :content-type)))
>> -	(nth (plist-get part :id)))
>> -    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type))
>> -  ;; Some of the body part handlers leave point somewhere up in the
>> -  ;; part, so we make sure that we're down at the end.
>> -  (goto-char (point-max))
>> -  ;; Ensure that the part ends with a carriage return.
>> -  (unless (bolp)
>> -    (insert "\n")))
>> +	(nth (plist-get part :id))
>> +	(beg (point)))
>> +
>> +    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type)
>> +    ;; Some of the body part handlers leave point somewhere up in the
>> +    ;; part, so we make sure that we're down at the end.
>> +    (goto-char (point-max))
>> +    ;; Ensure that the part ends with a carriage return.
>> +    (unless (bolp)
>> +      (insert "\n"))
>> +    (notmuch-show-insert-part-overlays msg beg (point) not-shown)))
>>  
>>  (defun notmuch-show-insert-body (msg body depth)
>>    "Insert the body BODY at depth DEPTH in the current thread."
>> -- 
>> 1.7.9.1