[PATCH v3 0/5] emacs: show: lazy handling of hidden parts

[PATCH v3 0/5] emacs: show: lazy handling of hidden parts

[Mark Walters]

This is a substantially reworked version of
id:1369555061-21361-1-git-send-email-markwalters1009@gmail.com
attempting to answer the (very constructive) criticism of Austin.

The changes are: move the insert-part-header call from the
insert-part-text/plain function to insert-bodypart in line with all
the other part insertion functions. 

The other change in patch 2 is that we no longer need to pass
declared-type down to the part insertion functions so we remove this
argument.

The actual invisibility code has been substantially reworked. There
were two main aims: to remove the insertion of dummy text and to make
sure that toggling a lazy-part which cannot be rendered automatically
calls the default action for such a part (e.g. saving or viewing).

I have split this code into 3 pieces: two pieces of simple code
movement as preparation and then the new code to do the actual
lazy part handling.

All tests pass and everything I can think of to try works (viewing
pdf, lazy rendered html, lazy rendered pdf, toggling all types,
repeated toggling) but there are a lot of cases so testing is
especially helpful.

Best wishes

Mark





Mark Walters (5):
  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: implement lazy hidden part handling

 emacs/notmuch-show.el |  183 +++++++++++++++++++++++++++++--------------------
 1 file changed, 107 insertions(+), 76 deletions(-)

-- 
1.7.10.4

[PATCH v3 1/5] emacs: show: fake wash parts are handled at insert-bodypart level

[Mark Walters]

Earlier patches have moved the handling of wash fake inline patch
parts to insert-bodypart so we can drop the function
notmuch-show-insert-part-inline-patch-fake-part
---
 emacs/notmuch-show.el |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index b0a8d8a..fd14469 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -809,10 +809,6 @@ message at DEPTH in the current thread."
 		nil))
 	  nil))))
 
-;; Handler for wash generated inline patch fake parts.
-(defun notmuch-show-insert-part-inline-patch-fake-part (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type))
-
 (defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
   ;; text/html handler to work around bugs in renderers and our
   ;; invisibile parts code. In particular w3m sets up a keymap which
-- 
1.7.10.4

[PATCH v3 2/5] emacs: show: move the insertion of the header button to the top level

[Mark Walters]

Previously each of the part insertion handlers inserted the part
button themselves. Move this up into
notmuch-show-insert-bodypart. Since a small number of the handlers
modify the button (the encryption/signature ones) we need to pass the
header button as an argument into the individual part insertion
handlers. However, the declared-type argument was only used for the
text for the part buttons we can now omit it.

The patch is large but mostly simple. The only things of note are that
we let the text/plain handler applies notmuch-wash to the whole part
including the part button (otherwise rendering is slightly changed and
tests fail).
---
 emacs/notmuch-show.el |   97 ++++++++++++++++++++++++-------------------------
 1 file changed, 47 insertions(+), 50 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index fd14469..be3aeac 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -572,8 +572,7 @@ message at DEPTH in the current thread."
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
 	  (plist-get part :content)))
 
-(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-header nth declared-type content-type nil)
+(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth button)
   (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))
 	(inner-parts (plist-get part :content))
 	(start (point)))
@@ -650,8 +649,7 @@ message at DEPTH in the current thread."
 	  content-type)
       nil)))
 
-(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-header nth declared-type content-type nil)
+(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth button)
   (let ((inner-parts (plist-get part :content))
 	(start (point)))
 
@@ -670,16 +668,15 @@ message at DEPTH in the current thread."
       (indent-rigidly start (point) 1)))
   t)
 
-(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type)
-  (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil)))
-    (button-put button 'face 'notmuch-crypto-part-header)
-    ;; add signature status button if sigstatus provided
-    (if (plist-member part :sigstatus)
-	(let* ((from (notmuch-show-get-header :From msg))
-	       (sigstatus (car (plist-get part :sigstatus))))
-	  (notmuch-crypto-insert-sigstatus-button sigstatus from))
-      ;; if we're not adding sigstatus, tell the user how they can get it
-      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))
+(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth button)
+  (button-put button 'face 'notmuch-crypto-part-header)
+  ;; add signature status button if sigstatus provided
+  (if (plist-member part :sigstatus)
+      (let* ((from (notmuch-show-get-header :From msg))
+	     (sigstatus (car (plist-get part :sigstatus))))
+	(notmuch-crypto-insert-sigstatus-button sigstatus from))
+    ;; if we're not adding sigstatus, tell the user how they can get it
+    (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))
 
   (let ((inner-parts (plist-get part :content))
 	(start (point)))
@@ -692,20 +689,19 @@ message at DEPTH in the current thread."
       (indent-rigidly start (point) 1)))
   t)
 
-(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type)
-  (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil)))
-    (button-put button 'face 'notmuch-crypto-part-header)
-    ;; add encryption status button if encstatus specified
-    (if (plist-member part :encstatus)
-	(let ((encstatus (car (plist-get part :encstatus))))
-	  (notmuch-crypto-insert-encstatus-button encstatus)
-	  ;; add signature status button if sigstatus specified
-	  (if (plist-member part :sigstatus)
-	      (let* ((from (notmuch-show-get-header :From msg))
-		     (sigstatus (car (plist-get part :sigstatus))))
-		(notmuch-crypto-insert-sigstatus-button sigstatus from))))
-      ;; if we're not adding encstatus, tell the user how they can get it
-      (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")))
+(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth button)
+  (button-put button 'face 'notmuch-crypto-part-header)
+  ;; add encryption status button if encstatus specified
+  (if (plist-member part :encstatus)
+      (let ((encstatus (car (plist-get part :encstatus))))
+	(notmuch-crypto-insert-encstatus-button encstatus)
+	;; add signature status button if sigstatus specified
+	(if (plist-member part :sigstatus)
+	    (let* ((from (notmuch-show-get-header :From msg))
+		   (sigstatus (car (plist-get part :sigstatus))))
+	      (notmuch-crypto-insert-sigstatus-button sigstatus from))))
+    ;; if we're not adding encstatus, tell the user how they can get it
+    (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))
 
   (let ((inner-parts (plist-get part :content))
 	(start (point)))
@@ -718,8 +714,7 @@ message at DEPTH in the current thread."
       (indent-rigidly start (point) 1)))
   t)
 
-(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-header nth declared-type content-type nil)
+(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth button)
   (let ((inner-parts (plist-get part :content))
 	(start (point)))
     ;; Show all of the parts.
@@ -731,8 +726,7 @@ message at DEPTH in the current thread."
       (indent-rigidly start (point) 1)))
   t)
 
-(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-header nth declared-type content-type nil)
+(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth button)
   (let* ((message (car (plist-get part :content)))
 	 (body (car (plist-get message :body)))
 	 (start (point)))
@@ -753,12 +747,13 @@ message at DEPTH in the current thread."
       (indent-rigidly start (point) 1)))
   t)
 
-(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type)
-  (let ((start (point)))
-    ;; If this text/plain part is not the first part in the message,
-    ;; insert a header to make this clear.
-    (if (> nth 1)
-	(notmuch-show-insert-part-header nth declared-type content-type (plist-get part :filename)))
+(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth button)
+  ;; For backward compatibility we want to apply the text/plain hook
+  ;; to the whole of the part including the part button if there is
+  ;; one.
+  (let ((start (if button
+		   (button-start button)
+		 (point))))
     (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))
     (save-excursion
       (save-restriction
@@ -766,8 +761,7 @@ message at DEPTH in the current thread."
 	(run-hook-with-args 'notmuch-show-insert-text/plain-hook msg depth))))
   t)
 
-(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-header nth declared-type content-type (plist-get part :filename))
+(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth button)
   (insert (with-temp-buffer
 	    (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto))
 	    ;; notmuch-get-bodypart-content provides "raw", non-converted
@@ -790,8 +784,8 @@ message at DEPTH in the current thread."
   t)
 
 ;; For backwards compatibility.
-(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type)
-  (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type))
+(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth button)
+  (notmuch-show-insert-part-text/calendar msg part content-type nth depth button))
 
 (defun notmuch-show-get-mime-type-of-application/octet-stream (part)
   ;; If we can deduce a MIME type from the filename of the attachment,
@@ -809,7 +803,7 @@ message at DEPTH in the current thread."
 		nil))
 	  nil))))
 
-(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
+(defun notmuch-show-insert-part-text/html (msg part content-type nth depth button)
   ;; text/html handler to work around bugs in renderers and our
   ;; invisibile parts code. In particular w3m sets up a keymap which
   ;; "leaks" outside the invisible region and causes strange effects
@@ -817,11 +811,10 @@ message at DEPTH in the current thread."
   ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map
   ;; remains).
   (let ((mm-inline-text-html-with-w3m-keymap nil))
-    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type)))
+    (notmuch-show-insert-part-*/* msg part content-type nth depth button)))
 
-(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
+(defun notmuch-show-insert-part-*/* (msg part content-type nth depth button)
   ;; This handler _must_ succeed - it is the handler of last resort.
-  (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
   (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto)
   t)
 
@@ -844,13 +837,13 @@ message at DEPTH in the current thread."
 
 ;; \f
 
-(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type)
+(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth button)
   (let ((handlers (notmuch-show-handlers-for content-type)))
     ;; Run the content handlers until one of them returns a non-nil
     ;; value.
     (while (and handlers
 		(not (condition-case err
-			 (funcall (car handlers) msg part content-type nth depth declared-type)
+			 (funcall (car handlers) msg part content-type nth depth button)
 		       (error (progn
 				(insert "!!! Bodypart insert error: ")
 				(insert (error-message-string err))
@@ -878,6 +871,7 @@ message at DEPTH in the current thread."
   "Insert the body part PART at depth DEPTH in the current thread.
 
 If HIDE is non-nil then initially hide this part."
+
   (let* ((content-type (downcase (plist-get part :content-type)))
 	 (mime-type (or (and (string= content-type "application/octet-stream")
 			     (notmuch-show-get-mime-type-of-application/octet-stream part))
@@ -885,9 +879,12 @@ If HIDE is non-nil then initially hide this part."
 			     "text/x-diff")
 			content-type))
 	 (nth (plist-get part :id))
-	 (beg (point)))
+	 (beg (point))
+	 ;; We omit the part button for the first (or only) part if this is text/plain.
+	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
+		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))
 
-    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type)
+    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
     ;; 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))
-- 
1.7.10.4

[PATCH v3 3/5] emacs: show: pass button to create-overlays

[Mark Walters]

Now that the bodypart code has the button we can pass that to
create-overlays and simplify that.
---
 emacs/notmuch-show.el |   26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index be3aeac..66cc3a5 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -851,21 +851,19 @@ message at DEPTH in the current thread."
       (setq handlers (cdr handlers))))
   t)
 
-(defun notmuch-show-create-part-overlays (msg beg end hide)
+(defun notmuch-show-create-part-overlays (button beg end hide)
   "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. We
-    ;; also need to check that the button is a genuine part button not
-    ;; a notmuch-wash button.
-    (when (and button (/= part-beg end) (button-get button :base-label))
-      (button-put button 'overlay (make-overlay part-beg end))
-      ;; We toggle the button for hidden parts as that gets the
+
+  ;; If there is no button (i.e., the part is text/plain and the first
+  ;; part) or if the part has no content then we don't make the part
+  ;; toggleable.
+  (when (and button (/= beg end))
+    (button-put button 'overlay (make-overlay beg end))
+    ;; We toggle the button for hidden parts as that gets the
       ;; button label right.
       (save-excursion
 	(when hide
-	  (notmuch-show-toggle-part-invisibility button))))))
+	  (notmuch-show-toggle-part-invisibility button)))))
 
 (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
   "Insert the body part PART at depth DEPTH in the current thread.
@@ -879,10 +877,10 @@ If HIDE is non-nil then initially hide this part."
 			     "text/x-diff")
 			content-type))
 	 (nth (plist-get part :id))
-	 (beg (point))
 	 ;; We omit the part button for the first (or only) part if this is text/plain.
 	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
-		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))
+		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
+	 (beg (point)))
 
     (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
     ;; Some of the body part handlers leave point somewhere up in the
@@ -891,7 +889,7 @@ 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 msg beg (point) hide)))
+    (notmuch-show-create-part-overlays button beg (point) hide)))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
-- 
1.7.10.4

Re: [PATCH v3 3/5] emacs: show: pass button to create-overlays

[Austin Clements]

On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Now that the bodypart code has the button we can pass that to
> create-overlays and simplify that.
> ---
>  emacs/notmuch-show.el |   26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index be3aeac..66cc3a5 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -851,21 +851,19 @@ message at DEPTH in the current thread."
>        (setq handlers (cdr handlers))))
>    t)
>  
> -(defun notmuch-show-create-part-overlays (msg beg end hide)
> +(defun notmuch-show-create-part-overlays (button beg end hide)
>    "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. We
> -    ;; also need to check that the button is a genuine part button not
> -    ;; a notmuch-wash button.
> -    (when (and button (/= part-beg end) (button-get button :base-label))
> -      (button-put button 'overlay (make-overlay part-beg end))
> -      ;; We toggle the button for hidden parts as that gets the
> +
> +  ;; If there is no button (i.e., the part is text/plain and the first
> +  ;; part) or if the part has no content then we don't make the part
> +  ;; toggleable.
> +  (when (and button (/= beg end))
> +    (button-put button 'overlay (make-overlay beg end))
> +    ;; We toggle the button for hidden parts as that gets the
>        ;; button label right.
>        (save-excursion
>  	(when hide
> -	  (notmuch-show-toggle-part-invisibility button))))))
> +	  (notmuch-show-toggle-part-invisibility button)))))

I might just be mis-following the diff here, but shouldn't the above get
re-indented?

>  
>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>    "Insert the body part PART at depth DEPTH in the current thread.
> @@ -879,10 +877,10 @@ If HIDE is non-nil then initially hide this part."
>  			     "text/x-diff")
>  			content-type))
>  	 (nth (plist-get part :id))
> -	 (beg (point))
>  	 ;; We omit the part button for the first (or only) part if this is text/plain.
>  	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
> -		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))
> +		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
> +	 (beg (point)))

Was this swap necessary?

>  
>      (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
>      ;; Some of the body part handlers leave point somewhere up in the
> @@ -891,7 +889,7 @@ 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 msg beg (point) hide)))
> +    (notmuch-show-create-part-overlays button beg (point) hide)))
>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
> -- 
> 1.7.10.4

Re: [PATCH v3 3/5] emacs: show: pass button to create-overlays

[Mark Walters]

> On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>> Now that the bodypart code has the button we can pass that to
>> create-overlays and simplify that.
>> ---
>>  emacs/notmuch-show.el |   26 ++++++++++++--------------
>>  1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index be3aeac..66cc3a5 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -851,21 +851,19 @@ message at DEPTH in the current thread."
>>        (setq handlers (cdr handlers))))
>>    t)
>>  
>> -(defun notmuch-show-create-part-overlays (msg beg end hide)
>> +(defun notmuch-show-create-part-overlays (button beg end hide)
>>    "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. We
>> -    ;; also need to check that the button is a genuine part button not
>> -    ;; a notmuch-wash button.
>> -    (when (and button (/= part-beg end) (button-get button :base-label))
>> -      (button-put button 'overlay (make-overlay part-beg end))
>> -      ;; We toggle the button for hidden parts as that gets the
>> +
>> +  ;; If there is no button (i.e., the part is text/plain and the first
>> +  ;; part) or if the part has no content then we don't make the part
>> +  ;; toggleable.
>> +  (when (and button (/= beg end))
>> +    (button-put button 'overlay (make-overlay beg end))
>> +    ;; We toggle the button for hidden parts as that gets the
>>        ;; button label right.
>>        (save-excursion
>>  	(when hide
>> -	  (notmuch-show-toggle-part-invisibility button))))))
>> +	  (notmuch-show-toggle-part-invisibility button)))))
>
> I might just be mis-following the diff here, but shouldn't the above get
> re-indented?

I think you are right: it got a bit messed up when I tried to split up
the patch. I don't think it matters much as these lines get deleted in
the next patch. Incidentally is there a good git work flow for fixing up
something like this where you just want to change the midpoint of a
series?

>
>>  
>>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>>    "Insert the body part PART at depth DEPTH in the current thread.
>> @@ -879,10 +877,10 @@ If HIDE is non-nil then initially hide this part."
>>  			     "text/x-diff")
>>  			content-type))
>>  	 (nth (plist-get part :id))
>> -	 (beg (point))
>>  	 ;; We omit the part button for the first (or only) part if this is text/plain.
>>  	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
>> -		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))
>> +		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
>> +	 (beg (point)))
>
> Was this swap necessary?

This is needed: inserting a part header moves point and this means that
point is at the start of the part content rather than the start of the
part button.

Best wishes

Mark

>
>>  
>>      (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
>>      ;; Some of the body part handlers leave point somewhere up in the
>> @@ -891,7 +889,7 @@ 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 msg beg (point) hide)))
>> +    (notmuch-show-create-part-overlays button beg (point) hide)))
>>  
>>  (defun notmuch-show-insert-body (msg body depth)
>>    "Insert the body BODY at depth DEPTH in the current thread."
>> -- 
>> 1.7.10.4

Re: [PATCH v3 3/5] emacs: show: pass button to create-overlays

[Austin Clements]

Quoth Mark Walters on Jun 01 at 12:08 am:
> 
> > On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> >> Now that the bodypart code has the button we can pass that to
> >> create-overlays and simplify that.
> >> ---
> >>  emacs/notmuch-show.el |   26 ++++++++++++--------------
> >>  1 file changed, 12 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> >> index be3aeac..66cc3a5 100644
> >> --- a/emacs/notmuch-show.el
> >> +++ b/emacs/notmuch-show.el
> >> @@ -851,21 +851,19 @@ message at DEPTH in the current thread."
> >>        (setq handlers (cdr handlers))))
> >>    t)
> >>  
> >> -(defun notmuch-show-create-part-overlays (msg beg end hide)
> >> +(defun notmuch-show-create-part-overlays (button beg end hide)
> >>    "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. We
> >> -    ;; also need to check that the button is a genuine part button not
> >> -    ;; a notmuch-wash button.
> >> -    (when (and button (/= part-beg end) (button-get button :base-label))
> >> -      (button-put button 'overlay (make-overlay part-beg end))
> >> -      ;; We toggle the button for hidden parts as that gets the
> >> +
> >> +  ;; If there is no button (i.e., the part is text/plain and the first
> >> +  ;; part) or if the part has no content then we don't make the part
> >> +  ;; toggleable.
> >> +  (when (and button (/= beg end))
> >> +    (button-put button 'overlay (make-overlay beg end))
> >> +    ;; We toggle the button for hidden parts as that gets the
> >>        ;; button label right.
> >>        (save-excursion
> >>  	(when hide
> >> -	  (notmuch-show-toggle-part-invisibility button))))))
> >> +	  (notmuch-show-toggle-part-invisibility button)))))
> >
> > I might just be mis-following the diff here, but shouldn't the above get
> > re-indented?
> 
> I think you are right: it got a bit messed up when I tried to split up
> the patch. I don't think it matters much as these lines get deleted in
> the next patch. Incidentally is there a good git work flow for fixing up
> something like this where you just want to change the midpoint of a
> series?

Yeah, I just came across that.  If you want to fix it up, personally I
would use git rebase -i and mark this patch as "edit", then amend this
commit to fix the indentation, continue the rebase, and clean up the
inevitable rebase conflict when it replays the next patch.

> >
> >>  
> >>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
> >>    "Insert the body part PART at depth DEPTH in the current thread.
> >> @@ -879,10 +877,10 @@ If HIDE is non-nil then initially hide this part."
> >>  			     "text/x-diff")
> >>  			content-type))
> >>  	 (nth (plist-get part :id))
> >> -	 (beg (point))
> >>  	 ;; We omit the part button for the first (or only) part if this is text/plain.
> >>  	 (button (unless (and (string= mime-type "text/plain") (<= nth 1))
> >> -		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename)))))
> >> +		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
> >> +	 (beg (point)))
> >
> > Was this swap necessary?
> 
> This is needed: inserting a part header moves point and this means that
> point is at the start of the part content rather than the start of the
> part button.

Ah, okay.

> Best wishes
> 
> Mark
> 
> >
> >>  
> >>      (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
> >>      ;; Some of the body part handlers leave point somewhere up in the
> >> @@ -891,7 +889,7 @@ 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 msg beg (point) hide)))
> >> +    (notmuch-show-create-part-overlays button beg (point) hide)))
> >>  
> >>  (defun notmuch-show-insert-body (msg body depth)
> >>    "Insert the body BODY at depth DEPTH in the current thread."

[PATCH v3 4/5] emacs: show: modify the way hidden state is recorded.

[Mark Walters]

Previously, whether a part was hidden or shown was recorded in the
invisibility/visibility of the part overlay. Since we are going to
have lazily rendered parts with no overlay store the hidden/shown
state in the part button itself.

Additionally, in preparation for the invisible part handling move the
actual hiding of the hidden parts to insert-bodypart from
create-part-overlays.

Finally, we will need to know whether a part-insertion has done
anything (it won't if the invisible part cannot be displayed by emacs)
so we slightly rejig the code order in
notmuch-show-toggle-part-invisibility to make it easier for the
function to set an appropriate return value.
---
 emacs/notmuch-show.el |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 66cc3a5..89199e8 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -495,6 +495,7 @@ message at DEPTH in the current thread."
 	   (concat "[ " base-label " ]")
 	   :base-label base-label
 	   :type 'notmuch-show-part-button-type
+	   :notmuch-part-hidden nil
 	   :notmuch-part nth
 	   :notmuch-filename name
 	   :notmuch-content-type content-type))
@@ -555,18 +556,20 @@ message at DEPTH in the current thread."
   (let* ((button (or button (button-at (point))))
 	 (overlay (button-get button 'overlay)))
     (when overlay
-      (let* ((show (overlay-get overlay 'invisible))
+      (let* ((show (button-get button :notmuch-part-hidden))
 	     (new-start (button-start button))
 	     (button-label (button-get button :base-label))
 	     (old-point (point))
 	     (inhibit-read-only t))
-	(overlay-put overlay 'invisible (not show))
+	;; Toggle the button itself.
+	(button-put button :notmuch-part-hidden (not show))
 	(goto-char new-start)
 	(insert "[ " button-label (if show " ]" " (hidden) ]"))
 	(let ((old-end (button-end button)))
 	  (move-overlay button new-start (point))
 	  (delete-region (point) old-end))
-	(goto-char (min old-point (1- (button-end button))))))))
+	(goto-char (min old-point (1- (button-end button))))
+	(overlay-put overlay 'invisible (not show))))))
 
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
@@ -851,7 +854,7 @@ message at DEPTH in the current thread."
       (setq handlers (cdr handlers))))
   t)
 
-(defun notmuch-show-create-part-overlays (button beg end hide)
+(defun notmuch-show-create-part-overlays (button beg end)
   "Add an overlay to the part between BEG and END"
 
   ;; If there is no button (i.e., the part is text/plain and the first
@@ -859,11 +862,9 @@ message at DEPTH in the current thread."
   ;; toggleable.
   (when (and button (/= beg end))
     (button-put button 'overlay (make-overlay beg end))
-    ;; We toggle the button for hidden parts as that gets the
-      ;; button label right.
-      (save-excursion
-	(when hide
-	  (notmuch-show-toggle-part-invisibility button)))))
+    ;; Return true if we created an overlay.
+    t))
+
 
 (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
   "Insert the body part PART at depth DEPTH in the current thread.
@@ -889,7 +890,9 @@ 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 beg (point) hide)))
+    (notmuch-show-create-part-overlays button beg (point))
+    (when hide
+      (notmuch-show-toggle-part-invisibility button))))
 
 (defun notmuch-show-insert-body (msg body depth)
   "Insert the body BODY at depth DEPTH in the current thread."
-- 
1.7.10.4

Re: [PATCH v3 4/5] emacs: show: modify the way hidden state is recorded.

[Austin Clements]

On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Previously, whether a part was hidden or shown was recorded in the
> invisibility/visibility of the part overlay. Since we are going to
> have lazily rendered parts with no overlay store the hidden/shown
> state in the part button itself.
>
> Additionally, in preparation for the invisible part handling move the
> actual hiding of the hidden parts to insert-bodypart from
> create-part-overlays.
>
> Finally, we will need to know whether a part-insertion has done
> anything (it won't if the invisible part cannot be displayed by emacs)
> so we slightly rejig the code order in
> notmuch-show-toggle-part-invisibility to make it easier for the
> function to set an appropriate return value.
> ---
>  emacs/notmuch-show.el |   23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 66cc3a5..89199e8 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -495,6 +495,7 @@ message at DEPTH in the current thread."
>  	   (concat "[ " base-label " ]")
>  	   :base-label base-label
>  	   :type 'notmuch-show-part-button-type
> +	   :notmuch-part-hidden nil
>  	   :notmuch-part nth
>  	   :notmuch-filename name
>  	   :notmuch-content-type content-type))
> @@ -555,18 +556,20 @@ message at DEPTH in the current thread."
>    (let* ((button (or button (button-at (point))))
>  	 (overlay (button-get button 'overlay)))
>      (when overlay
> -      (let* ((show (overlay-get overlay 'invisible))
> +      (let* ((show (button-get button :notmuch-part-hidden))
>  	     (new-start (button-start button))
>  	     (button-label (button-get button :base-label))
>  	     (old-point (point))
>  	     (inhibit-read-only t))
> -	(overlay-put overlay 'invisible (not show))
> +	;; Toggle the button itself.
> +	(button-put button :notmuch-part-hidden (not show))
>  	(goto-char new-start)
>  	(insert "[ " button-label (if show " ]" " (hidden) ]"))
>  	(let ((old-end (button-end button)))
>  	  (move-overlay button new-start (point))
>  	  (delete-region (point) old-end))
> -	(goto-char (min old-point (1- (button-end button))))))))
> +	(goto-char (min old-point (1- (button-end button))))
> +	(overlay-put overlay 'invisible (not show))))))
>  
>  (defun notmuch-show-multipart/*-to-list (part)
>    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
> @@ -851,7 +854,7 @@ message at DEPTH in the current thread."
>        (setq handlers (cdr handlers))))
>    t)
>  
> -(defun notmuch-show-create-part-overlays (button beg end hide)
> +(defun notmuch-show-create-part-overlays (button beg end)
>    "Add an overlay to the part between BEG and END"
>  
>    ;; If there is no button (i.e., the part is text/plain and the first
> @@ -859,11 +862,9 @@ message at DEPTH in the current thread."
>    ;; toggleable.
>    (when (and button (/= beg end))
>      (button-put button 'overlay (make-overlay beg end))
> -    ;; We toggle the button for hidden parts as that gets the
> -      ;; button label right.
> -      (save-excursion
> -	(when hide
> -	  (notmuch-show-toggle-part-invisibility button)))))
> +    ;; Return true if we created an overlay.
> +    t))
> +

Ah, I see the mis-indentation was only temporary.

>  
>  (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
>    "Insert the body part PART at depth DEPTH in the current thread.
> @@ -889,7 +890,9 @@ 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 beg (point) hide)))
> +    (notmuch-show-create-part-overlays button beg (point))
> +    (when hide
> +      (notmuch-show-toggle-part-invisibility button))))

This lost the save-excursion around
notmuch-show-toggle-part-invisibility.  I'm surprised that works, given
how notmuch-show-toggle-part-invisibility moves point, but then again, I
can believe that the planets happen to align here because you're hiding
a part and that part happens to go to (point-max).  Might be worth a
comment or putting back the save-excursion just so people don't have to
think about it.

>  
>  (defun notmuch-show-insert-body (msg body depth)
>    "Insert the body BODY at depth DEPTH in the current thread."
> -- 
> 1.7.10.4

[PATCH v3 5/5] emacs: show: implement lazy hidden part handling

[Mark Walters]

This adds the actual code to do the lazy insertion of hidden parts.

We use a memory inefficient but simple method: when we come to insert
the part if it is hidden we just store all of the arguments to the
part insertion function as a button property. This means when we want
to show the part we can just resume where we left off.

One thing is that we can't tell if a lazy part will produce text until
we try to render it so when unhiding a part we check to see if it
rendered; if not we invoke the default part handler (e.g. an external
viewer).
---
 emacs/notmuch-show.el |   51 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 89199e8..4999b94 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -550,12 +550,14 @@ message at DEPTH in the current thread."
     (let ((handle (mm-make-handle (current-buffer) (list content-type))))
       (mm-pipe-part handle))))
 
-;; This is taken from notmuch-wash: maybe it should be unified?
 (defun notmuch-show-toggle-part-invisibility (&optional button)
   (interactive)
   (let* ((button (or button (button-at (point))))
-	 (overlay (button-get button 'overlay)))
-    (when overlay
+	 (overlay (button-get button 'overlay))
+	 (lazy-part (button-get button :notmuch-lazy-part)))
+    ;; We have a part to toggle if there is an overlay or if there is a lazy part.
+    ;; If neither is present we cannot toggle the part so we just return nil.
+    (when (or overlay lazy-part)
       (let* ((show (button-get button :notmuch-part-hidden))
 	     (new-start (button-start button))
 	     (button-label (button-get button :base-label))
@@ -569,7 +571,15 @@ message at DEPTH in the current thread."
 	  (move-overlay button new-start (point))
 	  (delete-region (point) old-end))
 	(goto-char (min old-point (1- (button-end button))))
-	(overlay-put overlay 'invisible (not show))))))
+	;; Return nil if there is a lazy-part, it is empty, and we are
+	;; trying to show it.  In all other cases return t.
+	(if lazy-part
+	    (when show
+	      (button-put button :notmuch-lazy-part nil)
+	      (notmuch-show-lazy-part lazy-part button))
+	  ;; else there must be an overlay.
+	  (overlay-put overlay 'invisible (not show))
+	  t)))))
 
 (defun notmuch-show-multipart/*-to-list (part)
   (mapcar (lambda (inner-part) (plist-get inner-part :content-type))
@@ -865,6 +875,27 @@ message at DEPTH in the current thread."
     ;; Return true if we created an overlay.
     t))
 
+(defun notmuch-show-lazy-part (part-args button)
+  ;; Insert the lazy part after the button for the part.
+  (save-excursion
+    (goto-char (1+ (button-end button)))
+    (let* ((inhibit-read-only t)
+	   ;; We need to use markers for the start and end of the part
+	   ;; because the part insertion functions do not guarantee
+	   ;; to leave point at the end of the part.
+	   (part-beg (copy-marker (point) nil))
+	   (part-end (copy-marker (point) t))
+	   ;; We have to save the depth as we can't find the depth
+	   ;; when narrowed.
+	   (depth (notmuch-show-get-depth)))
+      (save-restriction
+	(narrow-to-region part-beg part-end)
+	(delete-region part-beg part-end)
+	(apply #'notmuch-show-insert-bodypart-internal part-args)
+	(indent-rigidly part-beg part-end depth))
+      ;; 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))))
 
 (defun notmuch-show-insert-bodypart (msg part depth &optional hide)
   "Insert the body part PART at depth DEPTH in the current thread.
@@ -883,7 +914,11 @@ If HIDE is non-nil then initially hide this part."
 		   (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))
 	 (beg (point)))
 
-    (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
+    (if (not hide)
+        (notmuch-show-insert-bodypart-internal msg part mime-type nth depth button)
+      (button-put button :notmuch-lazy-part
+                  (list msg part mime-type nth depth button)))
+
     ;; 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))
@@ -2002,8 +2037,10 @@ the user (see `notmuch-show-stash-mlarchive-link-alist')."
 (defun notmuch-show-part-button-default (&optional button)
   (interactive)
   (let ((button (or button (button-at (point)))))
-    (if (button-get button 'overlay)
-	(notmuch-show-toggle-part-invisibility button)
+    ;; Try to toggle the part, if that fails then call the default
+    ;; action. The toggle fails if the part has no emacs renderable
+    ;; content.
+    (unless (notmuch-show-toggle-part-invisibility button)
       (notmuch-show-part-button-internal button notmuch-show-part-button-default-action))))
 
 (defun notmuch-show-part-button-save (&optional button)
-- 
1.7.10.4

Re: [PATCH v3 0/5] emacs: show: lazy handling of hidden parts

[Austin Clements]

LGTM other than the tiny things I mentioned.  As Mark says, there are a
lot of paths through this code (though he did a good job of minimizing
them), so it'll need some testing.  Of course, what better place to test
than git master?

As a heads up, I think this will have a trivial conflict with
id:"1369876428-13537-4-git-send-email-amdragon@mit.edu", which is
currently in the ready queue.

On Fri, 31 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> This is a substantially reworked version of
> id:1369555061-21361-1-git-send-email-markwalters1009@gmail.com
> attempting to answer the (very constructive) criticism of Austin.
>
> The changes are: move the insert-part-header call from the
> insert-part-text/plain function to insert-bodypart in line with all
> the other part insertion functions. 
>
> The other change in patch 2 is that we no longer need to pass
> declared-type down to the part insertion functions so we remove this
> argument.
>
> The actual invisibility code has been substantially reworked. There
> were two main aims: to remove the insertion of dummy text and to make
> sure that toggling a lazy-part which cannot be rendered automatically
> calls the default action for such a part (e.g. saving or viewing).
>
> I have split this code into 3 pieces: two pieces of simple code
> movement as preparation and then the new code to do the actual
> lazy part handling.
>
> All tests pass and everything I can think of to try works (viewing
> pdf, lazy rendered html, lazy rendered pdf, toggling all types,
> repeated toggling) but there are a lot of cases so testing is
> especially helpful.
>
> Best wishes
>
> Mark
>
>
>
>
>
> Mark Walters (5):
>   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: implement lazy hidden part handling
>
>  emacs/notmuch-show.el |  183 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 107 insertions(+), 76 deletions(-)
>
> -- 
> 1.7.10.4