From: Tomi Ollila <tomi.ollila@iki.fi>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org,
	Austin Clements <aclements@csail.mit.edu>
Subject: Re: [PATCH v5 0/6] emacs: show: lazy handling of hidden parts
In-Reply-To: <1370840233-23258-1-git-send-email-markwalters1009@gmail.com>
References: <1370840233-23258-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.15.2+181~gfb53171 (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Mon, 10 Jun 2013 18:22:45 +0300
Message-ID: <m2fvwq6ldm.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Mon, Jun 10 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> This is version 5 of this patch set. Version 4 is at
> id:1370074547-24677-1-git-send-email-markwalters1009@gmail.com.
>
> This version fixes the two bugs pointed out in Austin's review
> id:20130610023952.GC22196@mit.edu.
>
> I decided to add the :notmuch-part text property separately from
> create-overlays as they have slightly different regions (one includes
> the button one does not) and are applied at slightly different times
> (we do not create overlays for lazy parts).
>
> I was not sure whether we created two overlays for hidden lazy parts
> in v4 (it might not have done as the part was empty and we do not
> create overlays for empty parts) but that is stupidly fragile. Thus we
> explicitly do not create overlays for lazy parts until they are
> inserted.
>
> The diff from v4 is below.

As far as I understood the code looked good. I also checked lazy
rendering by using w3m-standalone as mm-text-html-renderer and by
writing a wrapper w3m which appended command line to a file before
running w3m and then `tail -f`d the file...

... and w3m was run at the time [text/html (hidden)] was pressed.

So, +1

> Best wishes
>
> Mark

Tomi

PS: anyone interested the command lines were:

-dump -T text/html -I UTF-8 -O UTF-8
-dump -T text/html -I UTF-8 -O UTF-8
-dump -T text/html -I iso-8859-1 -O iso-8859-1

(based on original encoding, emacs then did conversions when needed)

> Mark Walters (6):
>   emacs: show: fake wash parts are handled at insert-bodypart level
>   emacs: show: move the insertion of the header button to the top level
>   emacs: show: pass button to create-overlays
>   emacs: show: modify the way hidden state is recorded.
>   emacs: show move addition of :notmuch-part to separate function
>   emacs: show: implement lazy hidden part handling
>
>  emacs/notmuch-show.el |  241 ++++++++++++++++++++++++++++++-------------------
>  1 files changed, 146 insertions(+), 95 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index b963859..d030ea3 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -817,6 +817,27 @@ message at DEPTH in the current thread."
>      ;; Return true if we created an overlay.
>      t))
>  
> +(defun notmuch-show-record-part-information (part beg end)
> +  "Store PART as a text property from BEG to END"
> +
> +  ;; Record part information.  Since we already inserted subparts,
> +  ;; don't override existing :notmuch-part properties.
> +  (notmuch-map-text-property beg end :notmuch-part
> +			     (lambda (v) (or v part)))
> +  ;; Make :notmuch-part front sticky and rear non-sticky so it stays
> +  ;; applied to the beginning of each line when we indent the
> +  ;; message.  Since we're operating on arbitrary renderer output,
> +  ;; watch out for sticky specs of t, which means all properties are
> +  ;; front-sticky/rear-nonsticky.
> +  (notmuch-map-text-property beg end 'front-sticky
> +			     (lambda (v) (if (listp v)
> +					     (pushnew :notmuch-part v)
> +					   v)))
> +  (notmuch-map-text-property beg end 'rear-nonsticky
> +			     (lambda (v) (if (listp v)
> +					     (pushnew :notmuch-part v)
> +					   v))))
> +
>  (defun notmuch-show-lazy-part (part-args button)
>    ;; Insert the lazy part after the button for the part. We would just
>    ;; move to the start of the new line following the button and insert
> @@ -843,6 +864,9 @@ message at DEPTH in the current thread."
>  	(indent-rigidly part-beg part-end depth))
>        (goto-char part-end)
>        (delete-char 1)
> +      (notmuch-show-record-part-information (second part-args)
> +					    (button-start button)
> +					    part-end)
>        ;; Create the overlay. If the lazy-part turned out to be empty/not
>        ;; showable this returns nil.
>        (notmuch-show-create-part-overlays button part-beg part-end))))
> @@ -876,27 +900,13 @@ If HIDE is non-nil then initially hide this part."
>      ;; Ensure that the part ends with a carriage return.
>      (unless (bolp)
>        (insert "\n"))
> -    (notmuch-show-create-part-overlays button content-beg (point))
> -    (when hide
> +    ;; We do not create the overlay for hidden (lazy) parts until
> +    ;; they are inserted.
> +    (if (not hide)
> +	(notmuch-show-create-part-overlays button content-beg (point))
>        (save-excursion
>  	(notmuch-show-toggle-part-invisibility button)))
> -    ;; Record part information.  Since we already inserted subparts,
> -    ;; don't override existing :notmuch-part properties.
> -    (notmuch-map-text-property beg (point) :notmuch-part
> -			       (lambda (v) (or v part)))
> -    ;; Make :notmuch-part front sticky and rear non-sticky so it stays
> -    ;; applied to the beginning of each line when we indent the
> -    ;; message.  Since we're operating on arbitrary renderer output,
> -    ;; watch out for sticky specs of t, which means all properties are
> -    ;; front-sticky/rear-nonsticky.
> -    (notmuch-map-text-property beg (point) 'front-sticky
> -			       (lambda (v) (if (listp v)
> -					       (pushnew :notmuch-part v)
> -					     v)))
> -    (notmuch-map-text-property beg (point) 'rear-nonsticky
> -			       (lambda (v) (if (listp v)
> -					       (pushnew :notmuch-part v)
> -					     v)))))
> +    (notmuch-show-record-part-information part beg (point))))
>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
>
>
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch