From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/3] emacs: show: add overlays for each part
In-Reply-To: <1354663662-20524-3-git-send-email-markwalters1009@gmail.com>
References: <1354663662-20524-1-git-send-email-markwalters1009@gmail.com>
	<1354663662-20524-3-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.14+159~g6895fee (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Mon, 10 Dec 2012 22:59:00 -0500
Message-ID: <87txrtnssb.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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.

> +						     (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.

> +	(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

(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.

> +	(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?

> +
> +(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