Re: [PATCH v2 4/4] emacs: Use the new JSON reply format.

[David Edmondson <dme@dme.org>]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

Much nicer now that it uses the mm stuff.

On Mon, 16 Jan 2012 11:13:23 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> wrote:
> +(defun find-parts (parts type)

Sorry for being a nuisance - this needs a name that indicates that it
relates to notmuch. How about `notmuch-parts-filter-by-type'?

> +  "Return a list of message parts with the given type"
> +  (delq nil (mapcar (lambda (part)
> +		      (if (string= (cdr (assq 'content-type part)) type)
> +			  (cdr (assq 'content part))))
> +		    parts)))

'(delq nil ...)' can more readably be implemented using something like:

       (loop for part in parts
             if (string= (cdr (assq 'content-type part)) type)
             collect (cdr (assq 'content part)))

(untested).

> +(defun notmuch-mua-insert-part-quoted (part)
> +  (save-restriction
> +    (narrow-to-region (point) (point))
> +    (insert part)
> +    (goto-char (point-min))
> +    (perform-replace "^" "> " nil t nil)

Narrowing to '(point) (point)' seems a bit weird and using
`perform-replace' is discouraged in programs. It would be more normal to
use a loop of `re-search-forward' and `replace-match' with a limit after
the insertion. Something like:

  (let ((start (point))
	limit)
    (insert part)
    (setq limit (point))
    (goto-char start)
    (while (re-search-forward "^" limit t)
      (replace-match "> "))

    ...

(untested).

> +    (insert "\n")
> +    (set-buffer-modified-p nil)))
      
Is this newline always required? Is it the cause of the spurious blank
line down below?

> +(defun notmuch-mua-parse-html-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (let ((handle (mm-make-handle (current-buffer) (list "text/html")))
> +	  (end-of-orig (point-max)))
> +      (mm-display-part handle)
> +      (kill-region (point-min) end-of-orig)
> +      (fill-region (point-min) (point-max))
> +      (buffer-substring (point-min) (point-max)))))

`kill-region' will save content in the kill ring. Was that intended?
(Maybe `delete-region' instead?)

>  (defun notmuch-mua-reply (query-string &optional sender reply-all)
> ...
> +      (insert (format "On %s, %s wrote:\n"
> +		      (cdr (assq 'date original-headers))
> +		      (cdr (assq 'from original-headers))))

I wonder whether emacs should be regenerating this or not. I'm okay with
it, but previous discussion was that it should remain the responsibility
of the CLI.

> +      (if (null plain-parts)
> +	  (mapc (lambda (part) (notmuch-mua-insert-part-quoted (notmuch-mua-parse-html-part part))) html-parts)
> +	(mapc (lambda (part) (notmuch-mua-insert-part-quoted part)) plain-parts))

Flip the 'then' and 'else' clauses to get rid of the `null'?

> --- a/test/emacs
> +++ b/test/emacs
> @@ -270,6 +270,7 @@ Fcc: $(pwd)/mail/sent
>  --text follows this line--
>  On 01 Jan 2000 12:00:00 -0000, Notmuch Test Suite <test_suite@notmuchmail.org> wrote:
>  > This is a test that messages are sent via SMTP
> +> 

It would be good if you could get rid of this trailing blank line.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]