From: Aaron Ecay <aaronecay@gmail.com>
To: David Edmondson <dme@dme.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH] emacs: Improved printing support.
In-Reply-To: <1325599946-5936-1-git-send-email-dme@dme.org>
References: <1325240897-27525-1-git-send-email-dme@dme.org>
	<1325599946-5936-1-git-send-email-dme@dme.org>
User-Agent: Notmuch/0.11+62~ge41b0d3 (http://notmuchmail.org) Emacs/24.0.92.1
	(i386-apple-darwin10.8.0)
Date: Mon, 16 Jan 2012 16:27:06 -0500
Message-ID: <m2vcobbaol.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

I don=E2=80=99t have (easy access to) a printer, so I haven=E2=80=99t actua=
lly exercised
the printing functionality of this patch.

In general, the patch LGTM.  Specific comments below.

On Tue,  3 Jan 2012 14:12:26 +0000, David Edmondson <dme@dme.org> wrote:
> Add various functions to print notmuch messages and tie them together
> with a simple frontend. No keybinding is currently made to encourage
> paper saving.
> ---
>=20
> Fix the lack of shell quoting for muttprint. Rename the ps-print
> buffers to have a relevant name.
>=20
>  emacs/Makefile.local   |    3 +-
>  emacs/notmuch-print.el |   79 ++++++++++++++++++++++++++++++++++++++++++=
++++++
>  emacs/notmuch-show.el  |   51 +++++++++++++++++++++++++++++++
>  3 files changed, 132 insertions(+), 1 deletions(-)
>  create mode 100644 emacs/notmuch-print.el
>=20
> diff --git a/emacs/Makefile.local b/emacs/Makefile.local

[...]

> --- /dev/null
> +++ b/emacs/notmuch-print.el
> @@ -0,0 +1,79 @@
> +;; notmuch-print.el --- printing messages from notmuch.
> +;;
> +;; Copyright =C2=A9 David Edmondson
> +;;
> +;; This file is part of Notmuch.
> +;;
> +;; Notmuch is free software: you can redistribute it and/or modify it
> +;; under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +;;
> +;; Notmuch is distributed in the hope that it will be useful, but
> +;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +;; General Public License for more details.
> +;;
> +;; You should have received a copy of the GNU General Public License
> +;; along with Notmuch.  If not, see <http://www.gnu.org/licenses/>.
> +;;
> +;; Authors: David Edmondson <dme@dme.org>
> +
> +(defcustom notmuch-print-mechanism 'notmuch-print-lpr
> +  "How should printing be done?"
> +  :group 'notmuch
> +  :type '(choice
> +	  (function :tag "Use lpr" notmuch-print-lpr)
> +	  (function :tag "Use ps-print" notmuch-print-ps-print)
> +	  (function :tag "Use ps-print then evince" notmuch-print-ps-print/evin=
ce)
> +	  (function :tag "Use muttprint" notmuch-print-muttprint)
> +	  (function :tag "Use muttprint then evince" notmuch-print-muttprint/ev=
ince)
> +	  (function :tag "Using a custom function")))
> +
> +(defmacro notmuch-print-with-file (filename &rest body)
> +  `(with-temp-buffer
> +     (insert-file-contents ,filename)
> +     (set-buffer-modified-p nil)
> +     ,@body))
> +
> +(defun notmuch-print-lpr (filename msg)
> +  (notmuch-print-with-file filename
> +			   (lpr-buffer)))
> +
> +(defun notmuch-print-ps-print (filename msg)
> +  (let ((subject (plist-get (notmuch-show-get-prop :headers msg) :Subjec=
t)))
> +    (notmuch-print-with-file filename
> +			     (rename-buffer subject t)
> +			     (ps-print-buffer))))
> +
> +(defun notmuch-print-ps-print/evince (filename msg)
> +  (let ((ps-file (make-temp-file "notmuch"))
> +	(subject (plist-get (notmuch-show-get-prop :headers msg) :Subject)))
> +    (notmuch-print-with-file filename
> +			     (rename-buffer subject t)
> +			     (ps-print-buffer ps-file)
> +			     (async-shell-command (concat "evince " ps-file)))))
> +
> +(defun notmuch-print-muttprint (filename msg)
> +  (shell-command
> +   (concat "muttprint "
> +	   "--file " (shell-quote-argument filename) " "
> +	   ;; Show the tags.
> +	   "--printed-headers 'Date_To_From_CC_Newsgroups_*Subject*_/Tags/'")))
> +
> +(defun notmuch-print-muttprint/evince (filename msg)
> +  (let ((ps-file (make-temp-file "notmuch")))
> +    (call-process-shell-command
> +     (concat "muttprint "
> +	     "--file " (shell-quote-argument filename) " "
> +	     ;; Show the tags.
> +	     "--printed-headers 'Date_To_From_CC_Newsgroups_*Subject*_/Tags/' "
> +	     "--printer 'TO_FILE:" (shell-quote-argument ps-file) "'"))
> +    (async-shell-command (concat "evince " ps-file))))
> +
> +(defun notmuch-print-message (filename msg)
> +  (funcall notmuch-print-mechanism filename msg))
> +
> +;;

The above is just a stray comment line, right?

> +
> +(provide 'notmuch-print)
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5502efd..55ccfc5 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -34,6 +34,7 @@
>  (require 'notmuch-wash)
>  (require 'notmuch-mua)
>  (require 'notmuch-crypto)
> +(require 'notmuch-print)
>=20=20
>  (declare-function notmuch-call-notmuch-process "notmuch" (&rest args))
>  (declare-function notmuch-fontify-headers "notmuch" nil)
> @@ -186,6 +187,51 @@ indentation."
>        mm-handle (> (notmuch-count-attachments mm-handle) 1))))
>    (message "Done"))
>=20=20
> +(defun notmuch-show-with-message-as-text (fn)
> +  "Apply function `fn' to a text representation of the current
> +message."

I think the docstring should say what agrs get passed to =E2=80=98fn=E2=80=
=99.  Also,
function arguments should be referred to with capital letters, and the
first line should be a complete sentence.  I=E2=80=99d recommend =E2=80=9CA=
pply FN to
(...the rest unchanged)=E2=80=9D for brevity in the first line.

(For anyone who is curious, these and other conventions are documented
in section D.6 of the Elisp manual.)

> +
> +  ;; Remake the header to ensure that all information is available.
> +  (let* ((to (notmuch-show-get-to))
> +	 (cc (notmuch-show-get-cc))
> +	 (from (notmuch-show-get-from))
> +	 (subject (notmuch-show-get-subject))
> +	 (date (notmuch-show-get-date))
> +	 (tags (notmuch-show-get-tags))
> +	 (depth (notmuch-show-get-depth))
> +
> +	 (header (concat
> +		  "Subject: " subject "\n"
> +		  "To: " to "\n"
> +		  (if (not (string=3D cc ""))
> +		      (concat "Cc: " cc "\n")
> +		    "")
> +		  "From: " from "\n"
> +		  "Date: " date "\n"
> +		  (if tags
> +		      (concat "Tags: "
> +			      (mapconcat '(lambda (tag) tag) tags ", ") "\n")

#'identity instead of the lambda

> +		    "")))
> +	 (all (buffer-substring (notmuch-show-message-top)
> +				(notmuch-show-message-bottom)))
> +
> +	 (file (make-temp-file "notmuch")))
> +    (with-temp-file file
> +      (insert all)
> +      (indent-rigidly (point-min) (point-max) (- depth))
> +      ;; Remove the original header.
> +      (goto-char (point-min))
> +      (re-search-forward "^$" (point-max) nil)
> +      (delete-region (point-min) (point))
> +      (insert header))
> +    (funcall fn file (notmuch-show-get-message-properties))
> +    (delete-file file)))

Why does this function write to a file?  It seems that, of the print
methods, two (ps-print and lpr) don=E2=80=99t use the file at all.  One
(ps-print/evince) creates yet another file, without depending on the
first.  The other muttprint functions do use the file.  So:
- This function would be more general if it didn=E2=80=99t handle file-writ=
ing
  itself, but rather let each consumer of text-ified messages handle
  that if needed.
- It would be cleaner if print backends that don=E2=80=99t crucially depend=
 on
  the existence of a file didn=E2=80=99t create one at all.  If muttprint c=
an
  accept a message on stdin (it looks from the source like it can; it
  gives me some error message about Iconv Perl modules when I try to run
  it, so I can=E2=80=99t be sure), maybe writing to a file isn=E2=80=99t ne=
cessary at
  all.

> +
> +(defun notmuch-show-print-message ()
> +  "Print the current message."
> +  (interactive)
> +  (notmuch-show-with-message-as-text 'notmuch-print-message))
> +
>  (defun notmuch-show-fontify-header ()
>    (let ((face (cond
>  	       ((looking-at "[Tt]o:")
> @@ -760,6 +806,8 @@ current buffer, if possible."
>        (overlay-put headers-overlay 'priority 10))
>      (overlay-put (make-overlay body-start body-end) 'invisible message-i=
nvis-spec)
>=20=20
> +    (plist-put msg :depth depth)
> +
>      ;; Save the properties for this message. Currently this saves the
>      ;; entire message (augmented it with other stuff), which seems
>      ;; like overkill. We might save a reduced subset (for example, not
> @@ -1111,6 +1159,9 @@ Some useful entries are:
>  (defun notmuch-show-get-to ()
>    (notmuch-show-get-header :To))
>=20=20
> +(defun notmuch-show-get-depth ()
> +  (notmuch-show-get-prop :depth))
> +
>  (defun notmuch-show-set-tags (tags)
>    "Set the tags of the current message."
>    (notmuch-show-set-prop :tags tags)
> --=20
> 1.7.7.3
>=20
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

--=20
Aaron Ecay