On Sun, 8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notm...@xvx.ca> wrote: > +(defun w3m-region (start end)) ;; From `w3m.el'. > +(defun notmuch-mua-quote-part (part) > + (with-temp-buffer > + (insert part) > + (message-mode) > + (fill-region (point-min) (point-max)) > + (goto-char (point-min)) > + (perform-replace "^" "> " nil t nil) > + (set-buffer-modified-p nil) > + (buffer-substring (point-min) (point-max))))
Couldn't all of this be done directly in the reply buffer?
> +(defun notmuch-mua-parse-html-part (part)
> + (with-temp-buffer
> + (insert part)
> + (w3m-region (point-min) (point-max))
> + (set-buffer-modified-p nil)
> + (buffer-substring (point-min) (point-max))))
Blank lines between functions are the house style (as much as there is
such a thing).
Using w3m means that you should `require' it. What happens when a user
doesn't have it? (Either the elisp or the command.)
> (defun notmuch-mua-reply (query-string &optional sender)
> - (let (headers
> + (let (reply
> + original
> + headers
> body
> - (args '("reply")))
> + (args '("reply" "--format=json")))
Initialised variables are generally shown before uninitialised. I know
that you didn't do this 'wrong' and that it's an unrelated change, but
it should be fixed. (To the people who say that should be a separate
patch I say 'meh' - life is too short.)
> (if notmuch-show-process-crypto
> (setq args (append args '("--decrypt"))))
> (setq args (append args (list query-string)))
> - ;; This make assumptions about the output of `notmuch reply', but
> - ;; really only that the headers come first followed by a blank
> - ;; line and then the body.
> + ;; Get the reply object as JSON, and parse it into an elisp object.
> (with-temp-buffer
> (apply 'call-process (append (list notmuch-command nil (list t t) nil)
> args))
> (goto-char (point-min))
> - (if (re-search-forward "^$" nil t)
> - (save-excursion
> - (save-restriction
> - (narrow-to-region (point-min) (point))
> - (goto-char (point-min))
> - (setq headers (mail-header-extract)))))
> - (forward-line 1)
> - (setq body (buffer-substring (point) (point-max))))
> + (setq reply (aref (json-read) 0)))
> +
> + ;; Get the list of headers
> + (setq headers (cdr (assq 'headers (assq 'reply reply))))
> + ;; Construct the body of the reply.
> + (setq original (cdr (assq 'original reply)))
The scope of (at least) `headers' and `original' could be tightened by
moving them down to the following `let' (and converting it to `let*').
> +
> + ;; Start with the prelude, based on the headers of the original message.
> + (let ((original-headers (cdr (assq 'headers original))))
> + (setq body (format "On %s, %s wrote:\n"
> + (cdr (assq 'date original-headers))
> + (cdr (assq 'from original-headers)))))
> +
> + ;; Extract the body parts and construct a reasonable quoted body.
> + (let* ((body-parts (cdr (assq 'body original)))
> + (find-parts (lambda (type) (delq nil (mapcar (lambda (part)
> + (if (string= (cdr
> (assq 'content-type part)) type)
> + (cdr (assq
> 'content part))))
> + body-parts))))
`find-parts' might be useful in other places. Maybe add it to notmuch-lib.el?
> + (plain-parts (apply find-parts '("text/plain")))
> + (html-parts (apply find-parts '("text/html"))))
> +
> + (if (not (null plain-parts))
> + (mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part
> part)))) plain-parts)
> + (mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part
> (notmuch-mua-parse-html-part part))))) html-parts)))
If you have an 'else' clause, why test '(if (not ..' ?
> + (setq body (concat body "\n"))
> +
If it already ends with a carriage return, why do this?
pgpsOtNZqY0I0.pgp
Description: PGP signature
_______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
