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

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon 

Using the new JSON reply format allows emacs to quote HTML parts
nicely by first parsing them with w3m, then quoting them. This is
very useful for users who regularly receive HTML-only email.

The behavior for messages that contain plain text parts should be
unchanged, except that an additional quoted line is added to the end
of the reply message.  The test has been updated to reflect this.
---
 emacs/notmuch-mua.el |   62 +++--
 test/emacs   |1 +
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 7114e48..7f894cb 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -19,6 +19,7 @@
 ;;
 ;; Authors: David Edmondson 
 
+(require 'json)
 (require 'message)
 
 (require 'notmuch-lib)
@@ -71,27 +72,62 @@ list."
(push header message-hidden-headers)))
notmuch-mua-hidden-headers))
 
+(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
+(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
 (defun notmuch-mua-reply (query-string &optional sender)
-  (let (headers
+  (let (reply
+   original
+   headers
body
-   (args '("reply")))
+   (args '("reply" "--format=json")))
 (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)))
+
+;; 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
+  (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)))
+(setq body (concat body "\n"))
+   
 ;; If sender is non-nil, set the From: header to its value.
 (when sender
   (mail-header-set 'from sender headers))
diff --git a/test/emacs b/test/emacs
index a06c223..fe501da 100755
--- 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 -, Notmuch Test Suite  
wrote:
 > This is a test that messages are sent via SMTP
+> 
 EOF
 test_expect_equal_file OUTPUT EXPECTED
 
-- 
1.7.5.4

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Adam Wolfe Gordon]

From: Adam Wolfe Gordon 

Using the new JSON reply format allows emacs to quote HTML parts
nicely by first parsing them with w3m, then quoting them. This is
very useful for users who regularly receive HTML-only email.

The behavior for messages that contain plain text parts should be
unchanged, except that an additional quoted line is added to the end
of the reply message.  The test has been updated to reflect this.
---
 emacs/notmuch-mua.el |   62 +++--
 test/emacs   |1 +
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
index 7114e48..7f894cb 100644
--- a/emacs/notmuch-mua.el
+++ b/emacs/notmuch-mua.el
@@ -19,6 +19,7 @@
 ;;
 ;; Authors: David Edmondson 

+(require 'json)
 (require 'message)

 (require 'notmuch-lib)
@@ -71,27 +72,62 @@ list."
(push header message-hidden-headers)))
notmuch-mua-hidden-headers))

+(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
+(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
 (defun notmuch-mua-reply (query-string &optional sender)
-  (let (headers
+  (let (reply
+   original
+   headers
body
-   (args '("reply")))
+   (args '("reply" "--format=json")))
 (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)))
+
+;; 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
+  (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)))
+(setq body (concat body "\n"))
+   
 ;; If sender is non-nil, set the From: header to its value.
 (when sender
   (mail-header-set 'from sender headers))
diff --git a/test/emacs b/test/emacs
index a06c223..fe501da 100755
--- 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 -, Notmuch Test Suite  wrote:
 > This is a test that messages are sent via SMTP
+> 
 EOF
 test_expect_equal_file OUTPUT EXPECTED

-- 
1.7.5.4

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

[Aaron Ecay]

Adam,

One comment below.

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
wrote:
> From: Adam Wolfe Gordon 
> 
> Using the new JSON reply format allows emacs to quote HTML parts
> nicely by first parsing them with w3m, then quoting them. This is
> very useful for users who regularly receive HTML-only email.
> 
> The behavior for messages that contain plain text parts should be
> unchanged, except that an additional quoted line is added to the end
> of the reply message.  The test has been updated to reflect this.
> ---
>  emacs/notmuch-mua.el |   62 +++--
>  test/emacs   |1 +
>  2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 7114e48..7f894cb 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -19,6 +19,7 @@
>  ;;
>  ;; Authors: David Edmondson 
>  
> +(require 'json)
>  (require 'message)
>  
>  (require 'notmuch-lib)
> @@ -71,27 +72,62 @@ list."
>   (push header message-hidden-headers)))
>   notmuch-mua-hidden-headers))
>  
> +(defun w3m-region (start end)) ;; From `w3m.el'.

What is the purpose of the above line?  If it is to make the compiler
aware of the function, you should use ?declare-function? instead.  Defun
will erase the original definition of the w3m-region function.

-- 
Aaron Ecay

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

[David Edmondson]

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
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?
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Adam Wolfe Gordon]

On Sun, Jan 8, 2012 at 18:27, Aaron Ecay  wrote:
>> +(defun w3m-region (start end)) ;; From `w3m.el'.
>
> What is the purpose of the above line? ?If it is to make the compiler
> aware of the function, you should use ?declare-function? instead. ?Defun
> will erase the original definition of the w3m-region function.

Indeed, it's to make the compiler aware of the function.  I followed
the example of notmuch-show.el (defvar w3m-current-buffer, line 391),
but it sounds like declare-function is a better way to accomplish
this.  I haven't written a whole lot of emacs lisp, so thanks for the
tip.

Given David's comments about requiring w3m instead it might be moot,
but if not I'll make this change for the next version.

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

[David Edmondson]

On Mon, 9 Jan 2012 19:10:48 -0700, Adam Wolfe Gordon  wrote:
> > Using w3m means that you should `require' it. What happens when a user
> > doesn't have it? (Either the elisp or the command.)
> 
> This was my initial thought, but when I looked at notmuch-show.el,
> which uses w3m features, I noticed that it doesn't have a require.  To
> be clear, this patch requires w3m.el (not just the w3m binary), which
> I don't think anything else in notmuch does.
> 
> In the previous version I had a customize variable specifying whether
> to quote HTML parts, which meant that if the user could set the
> customize variable to false and everything would work without w3m.el.
> I'd like not to introduce a new prerequisite, so if there's a way to
> make w3m.el optional that would be my preference.  Can you provide
> some guidance on this?

My suggestion would be to move the `html to text' functionality to a new
.el, as we might have more than one way to achieve it (emacs 24 has
`shr', for example).

Have `notmuch-mua.el' use a generically named function to perform the
transformation from html to text and that function should determine the
best way to achieve it.

Testing for `w3m.el' is relatively easy (`require' it and check for
error). Testing for `w3m' itself can be done using some code similar to
`notmuch-address-locate-command' (search the list - it's not yet
integrated), which is itself just copied from w3m (and should end up in
`notmuch-lib.el').
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Adam Wolfe Gordon]

Hi David,

Thanks for the review.  Most of the things you've suggested are easy
changes, and I think obvious improvements, so I'll change them for the
next version.  A bit of discussion on the more involved things below:

On Mon, Jan 9, 2012 at 01:50, David Edmondson  wrote:
> On Sun, ?8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
> 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?

Indeed, it could, I just hadn't thought of it.  I'll do this for the
next version.

> Using w3m means that you should `require' it. What happens when a user
> doesn't have it? (Either the elisp or the command.)

This was my initial thought, but when I looked at notmuch-show.el,
which uses w3m features, I noticed that it doesn't have a require.  To
be clear, this patch requires w3m.el (not just the w3m binary), which
I don't think anything else in notmuch does.

In the previous version I had a customize variable specifying whether
to quote HTML parts, which meant that if the user could set the
customize variable to false and everything would work without w3m.el.
I'd like not to introduce a new prerequisite, so if there's a way to
make w3m.el optional that would be my preference.  Can you provide
some guidance on this?

-- 
Adam Wolfe Gordon

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

[Dmitry Kurochkin]

Hi Adam.

On Mon, 9 Jan 2012 19:10:48 -0700, Adam Wolfe Gordon  wrote:
> Hi David,
> 
> Thanks for the review.  Most of the things you've suggested are easy
> changes, and I think obvious improvements, so I'll change them for the
> next version.  A bit of discussion on the more involved things below:
> 
> On Mon, Jan 9, 2012 at 01:50, David Edmondson  wrote:
> > On Sun, ?8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  > 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?
> 
> Indeed, it could, I just hadn't thought of it.  I'll do this for the
> next version.
> 
> > Using w3m means that you should `require' it. What happens when a user
> > doesn't have it? (Either the elisp or the command.)
> 
> This was my initial thought, but when I looked at notmuch-show.el,
> which uses w3m features, I noticed that it doesn't have a require.  To
> be clear, this patch requires w3m.el (not just the w3m binary), which
> I don't think anything else in notmuch does.
> 
> In the previous version I had a customize variable specifying whether
> to quote HTML parts, which meant that if the user could set the
> customize variable to false and everything would work without w3m.el.
> I'd like not to introduce a new prerequisite, so if there's a way to
> make w3m.el optional that would be my preference.  Can you provide
> some guidance on this?
> 

I did not follow the rest of the discussion, so sorry if I missed
something obvious.  But why can't we render HTML parts in replies the
same way we do in notmuch-show (using `mm-display-part')?  That should
not introduce a w3m.el requirement, would use the same renderer as
configured for show and hence would produce consistent output in show
and reply.

Regards,
  Dmitry

> -- 
> Adam Wolfe Gordon
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[David Edmondson]

On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin  wrote:
> I did not follow the rest of the discussion, so sorry if I missed
> something obvious.  But why can't we render HTML parts in replies the
> same way we do in notmuch-show (using `mm-display-part')?  That should
> not introduce a w3m.el requirement, would use the same renderer as
> configured for show and hence would produce consistent output in show
> and reply.

Agreed, that would be good. It could be done as a second stage, though.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Tomi Ollila]

On Thu, 12 Jan 2012 12:07:40 +, David Edmondson  wrote:
> On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin  gmail.com> wrote:
> > I did not follow the rest of the discussion, so sorry if I missed
> > something obvious.  But why can't we render HTML parts in replies the
> > same way we do in notmuch-show (using `mm-display-part')?  That should
> > not introduce a w3m.el requirement, would use the same renderer as
> > configured for show and hence would produce consistent output in show
> > and reply.
> 
> Agreed, that would be good. It could be done as a second stage, though.

For the record. I built emacs 23.3 from source and after installation
by default there is no w3m module available.

Related thing: by default the value of (emacs variable)
mm-text-html-renderer is resolved as follows (IIRC):

   if w3m module can be loaded value will be 'w3m
   if no w3m is available but w3m binary exists, value will be 'w3m-standalone
   else it gets some other value (if any).

Tomi

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

[David Edmondson]

On Thu, 12 Jan 2012 15:36:07 +0200, Tomi Ollila  wrote:
> On Thu, 12 Jan 2012 12:07:40 +, David Edmondson  wrote:
> > On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin  > gmail.com> wrote:
> > > I did not follow the rest of the discussion, so sorry if I missed
> > > something obvious.  But why can't we render HTML parts in replies the
> > > same way we do in notmuch-show (using `mm-display-part')?  That should
> > > not introduce a w3m.el requirement, would use the same renderer as
> > > configured for show and hence would produce consistent output in show
> > > and reply.
> > 
> > Agreed, that would be good. It could be done as a second stage, though.
> 
> For the record. I built emacs 23.3 from source and after installation
> by default there is no w3m module available.

Currently it's broken for everyone. With the proposed changes it will be
fixed for some people. Further improvements will, of course, be
possible.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Adam Wolfe Gordon]

Hi Dmitry,

On Thu, Jan 12, 2012 at 01:33, Dmitry Kurochkin
 wrote:
> I did not follow the rest of the discussion, so sorry if I missed
> something obvious. ?But why can't we render HTML parts in replies the
> same way we do in notmuch-show (using `mm-display-part')? ?That should
> not introduce a w3m.el requirement, would use the same renderer as
> configured for show and hence would produce consistent output in show
> and reply.

Thanks for the suggestion - I've implemented this now and I think it's
definitely the best way to go.  New patches coming soon with all the
suggested changes.

-- 
Adam Wolfe Gordon

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

[Aaron Ecay]

Adam,

One comment below.

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
wrote:
> From: Adam Wolfe Gordon 
> 
> Using the new JSON reply format allows emacs to quote HTML parts
> nicely by first parsing them with w3m, then quoting them. This is
> very useful for users who regularly receive HTML-only email.
> 
> The behavior for messages that contain plain text parts should be
> unchanged, except that an additional quoted line is added to the end
> of the reply message.  The test has been updated to reflect this.
> ---
>  emacs/notmuch-mua.el |   62 +++--
>  test/emacs   |1 +
>  2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 7114e48..7f894cb 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -19,6 +19,7 @@
>  ;;
>  ;; Authors: David Edmondson 
>  
> +(require 'json)
>  (require 'message)
>  
>  (require 'notmuch-lib)
> @@ -71,27 +72,62 @@ list."
>   (push header message-hidden-headers)))
>   notmuch-mua-hidden-headers))
>  
> +(defun w3m-region (start end)) ;; From `w3m.el'.

What is the purpose of the above line?  If it is to make the compiler
aware of the function, you should use ‘declare-function’ instead.  Defun
will erase the original definition of the w3m-region function.

-- 
Aaron Ecay
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[David Edmondson]

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
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

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

[Adam Wolfe Gordon]

On Sun, Jan 8, 2012 at 18:27, Aaron Ecay  wrote:
>> +(defun w3m-region (start end)) ;; From `w3m.el'.
>
> What is the purpose of the above line?  If it is to make the compiler
> aware of the function, you should use ‘declare-function’ instead.  Defun
> will erase the original definition of the w3m-region function.

Indeed, it's to make the compiler aware of the function.  I followed
the example of notmuch-show.el (defvar w3m-current-buffer, line 391),
but it sounds like declare-function is a better way to accomplish
this.  I haven't written a whole lot of emacs lisp, so thanks for the
tip.

Given David's comments about requiring w3m instead it might be moot,
but if not I'll make this change for the next version.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[David Edmondson]

On Mon, 9 Jan 2012 19:10:48 -0700, Adam Wolfe Gordon  wrote:
> > Using w3m means that you should `require' it. What happens when a user
> > doesn't have it? (Either the elisp or the command.)
> 
> This was my initial thought, but when I looked at notmuch-show.el,
> which uses w3m features, I noticed that it doesn't have a require.  To
> be clear, this patch requires w3m.el (not just the w3m binary), which
> I don't think anything else in notmuch does.
> 
> In the previous version I had a customize variable specifying whether
> to quote HTML parts, which meant that if the user could set the
> customize variable to false and everything would work without w3m.el.
> I'd like not to introduce a new prerequisite, so if there's a way to
> make w3m.el optional that would be my preference.  Can you provide
> some guidance on this?

My suggestion would be to move the `html to text' functionality to a new
.el, as we might have more than one way to achieve it (emacs 24 has
`shr', for example).

Have `notmuch-mua.el' use a generically named function to perform the
transformation from html to text and that function should determine the
best way to achieve it.

Testing for `w3m.el' is relatively easy (`require' it and check for
error). Testing for `w3m' itself can be done using some code similar to
`notmuch-address-locate-command' (search the list - it's not yet
integrated), which is itself just copied from w3m (and should end up in
`notmuch-lib.el').


pgpB2tf7cE23L.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Adam Wolfe Gordon]

Hi David,

Thanks for the review.  Most of the things you've suggested are easy
changes, and I think obvious improvements, so I'll change them for the
next version.  A bit of discussion on the more involved things below:

On Mon, Jan 9, 2012 at 01:50, David Edmondson  wrote:
> On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
> 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?

Indeed, it could, I just hadn't thought of it.  I'll do this for the
next version.

> Using w3m means that you should `require' it. What happens when a user
> doesn't have it? (Either the elisp or the command.)

This was my initial thought, but when I looked at notmuch-show.el,
which uses w3m features, I noticed that it doesn't have a require.  To
be clear, this patch requires w3m.el (not just the w3m binary), which
I don't think anything else in notmuch does.

In the previous version I had a customize variable specifying whether
to quote HTML parts, which meant that if the user could set the
customize variable to false and everything would work without w3m.el.
I'd like not to introduce a new prerequisite, so if there's a way to
make w3m.el optional that would be my preference.  Can you provide
some guidance on this?

-- 
Adam Wolfe Gordon
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Dmitry Kurochkin]

Hi Adam.

On Mon, 9 Jan 2012 19:10:48 -0700, Adam Wolfe Gordon  wrote:
> Hi David,
> 
> Thanks for the review.  Most of the things you've suggested are easy
> changes, and I think obvious improvements, so I'll change them for the
> next version.  A bit of discussion on the more involved things below:
> 
> On Mon, Jan 9, 2012 at 01:50, David Edmondson  wrote:
> > On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon  
> > 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?
> 
> Indeed, it could, I just hadn't thought of it.  I'll do this for the
> next version.
> 
> > Using w3m means that you should `require' it. What happens when a user
> > doesn't have it? (Either the elisp or the command.)
> 
> This was my initial thought, but when I looked at notmuch-show.el,
> which uses w3m features, I noticed that it doesn't have a require.  To
> be clear, this patch requires w3m.el (not just the w3m binary), which
> I don't think anything else in notmuch does.
> 
> In the previous version I had a customize variable specifying whether
> to quote HTML parts, which meant that if the user could set the
> customize variable to false and everything would work without w3m.el.
> I'd like not to introduce a new prerequisite, so if there's a way to
> make w3m.el optional that would be my preference.  Can you provide
> some guidance on this?
> 

I did not follow the rest of the discussion, so sorry if I missed
something obvious.  But why can't we render HTML parts in replies the
same way we do in notmuch-show (using `mm-display-part')?  That should
not introduce a w3m.el requirement, would use the same renderer as
configured for show and hence would produce consistent output in show
and reply.

Regards,
  Dmitry

> -- 
> Adam Wolfe Gordon
> ___
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[David Edmondson]

On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin 
 wrote:
> I did not follow the rest of the discussion, so sorry if I missed
> something obvious.  But why can't we render HTML parts in replies the
> same way we do in notmuch-show (using `mm-display-part')?  That should
> not introduce a w3m.el requirement, would use the same renderer as
> configured for show and hence would produce consistent output in show
> and reply.

Agreed, that would be good. It could be done as a second stage, though.


pgpU0vzV9rMEx.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Tomi Ollila]

On Thu, 12 Jan 2012 12:07:40 +, David Edmondson  wrote:
> On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin 
>  wrote:
> > I did not follow the rest of the discussion, so sorry if I missed
> > something obvious.  But why can't we render HTML parts in replies the
> > same way we do in notmuch-show (using `mm-display-part')?  That should
> > not introduce a w3m.el requirement, would use the same renderer as
> > configured for show and hence would produce consistent output in show
> > and reply.
> 
> Agreed, that would be good. It could be done as a second stage, though.

For the record. I built emacs 23.3 from source and after installation
by default there is no w3m module available.

Related thing: by default the value of (emacs variable)
mm-text-html-renderer is resolved as follows (IIRC):

   if w3m module can be loaded value will be 'w3m
   if no w3m is available but w3m binary exists, value will be 'w3m-standalone
   else it gets some other value (if any).

Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[David Edmondson]

On Thu, 12 Jan 2012 15:36:07 +0200, Tomi Ollila  wrote:
> On Thu, 12 Jan 2012 12:07:40 +, David Edmondson  wrote:
> > On Thu, 12 Jan 2012 12:33:58 +0400, Dmitry Kurochkin 
> >  wrote:
> > > I did not follow the rest of the discussion, so sorry if I missed
> > > something obvious.  But why can't we render HTML parts in replies the
> > > same way we do in notmuch-show (using `mm-display-part')?  That should
> > > not introduce a w3m.el requirement, would use the same renderer as
> > > configured for show and hence would produce consistent output in show
> > > and reply.
> > 
> > Agreed, that would be good. It could be done as a second stage, though.
> 
> For the record. I built emacs 23.3 from source and after installation
> by default there is no w3m module available.

Currently it's broken for everyone. With the proposed changes it will be
fixed for some people. Further improvements will, of course, be
possible.


pgpM4Iy20CnZo.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Adam Wolfe Gordon]

Hi Dmitry,

On Thu, Jan 12, 2012 at 01:33, Dmitry Kurochkin
 wrote:
> I did not follow the rest of the discussion, so sorry if I missed
> something obvious.  But why can't we render HTML parts in replies the
> same way we do in notmuch-show (using `mm-display-part')?  That should
> not introduce a w3m.el requirement, would use the same renderer as
> configured for show and hence would produce consistent output in show
> and reply.

Thanks for the suggestion - I've implemented this now and I think it's
definitely the best way to go.  New patches coming soon with all the
suggested changes.

-- 
Adam Wolfe Gordon
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch