On Fri, Feb 17, 2012 at 13:00, Austin Clements <amdragon@mit.edu> wrote: > One general comment that affects a lot of things in this patch: I > think you should use the same JSON parsing settings that > notmuch-query-get-threads uses. Besides consistency and more > opportunities for code reuse, using lists instead of vectors for JSON > arrays will simplify a lot of this code without any drawbacks. I pretty much agree. The only reason I stuck with alists was, as you mention below, to be compatible with certain mail functions. Given the things you've pointed out, I think the small hassle of making those work with plists is worthwhile, so I'll give it a go. Clarification on a couple of things follow, otherwise I'll make all these changes for the next version. >> + (goto-char (point-max))) >> + >> + (let ((from (cdr (assq 'From original-headers))) >> + (date (cdr (assq 'Date original-headers))) >> + (start (point))) >> + >> + (insert "From: " from "\n") >> + (insert "Date: " date "\n\n") > > Sorry; I'm having trouble following the diff. What are the inserts > for? The function message-cite-original cites an original message, which is in the marked region. It assumes the headers of the original message will be part of the marked region, but the only ones it actually uses are From and Date. This could probably use a comment in the code. >> + (push-mark) > > It's unfortunate that message-cite-original depends on the mark. > Since you're about to push the mark for the user anyway, maybe this > should be a set-mark so that only one mark gets pushed? Probably the right thing to do. >> + (goto-char start) >> + ;; Quote the original message according to the user's configured style. >> + (message-cite-original)))) > > message-cite-original-without-signature? Perhaps it should be configurable (notmuch-reply-cite-function or somesuch). I believe message-cite-original matches the behavior of the old reply, which didn't strip signatures, but I don't have strong feelings either way. >> + (push-mark) > > Is message-cite-original guaranteed to leave point in a reasonable > place for this or should we create our own marker above (probably > after the if re-search-backward..) and use it here to get point to the > right place? In my experience, it leaves the point at the end of the quoted region, but the documentation doesn't make any guarantees. Probably safer to set a marker.