From: Austin Clements <amdragon@MIT.EDU>
To: Damien Cassou <damien.cassou@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/2] emacs: possibility to customize the rendering of tags
In-Reply-To: <1360162435-29506-3-git-send-email-damien.cassou@gmail.com>
References: <1360162435-29506-1-git-send-email-damien.cassou@gmail.com>
	<1360162435-29506-3-git-send-email-damien.cassou@gmail.com>
User-Agent: Notmuch/0.15+6~g7d4cb73 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Sat, 16 Mar 2013 00:10:05 -0400
Message-ID: <87sj3w56jm.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

(Decided I needed a brief break from working continuously.  I haven't
been following the mailing list at all, so I don't know if there's been
additional context relevant to this patch series, but this at least
appears to be the latest version.)

This is looking really good.  Just a few tiny comments below.

On Wed, 06 Feb 2013, Damien Cassou <damien.cassou@gmail.com> wrote:
> This patch extracts the rendering of tags in notmuch-show to
> the notmuch-tag file.
>
> This file introduces a `notmuch-tag-formats' variable that associates
> each tag to a particular format. This variable can be customized
> thanks to the work of Austin Clements. For example,
>
>   '(("unread" (propertize tag 'face '(:foreground "red")))
>     ("flagged" (notmuch-tag-format-image tag "star.svg")))
>
> associates a red foreground to the "unread" tag and a star picture to
> the "flagged" tag.
>
> Signed-off-by: Damien Cassou <damien.cassou@gmail.com>
> ---
>  emacs/notmuch-show.el |    6 +-
>  emacs/notmuch-tag.el  |  221 +++++++++++++++++++++++++++++++++++++++++++=
+++++-
>  emacs/notmuch.el      |    5 +-
>  3 files changed, 224 insertions(+), 8 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 1864dd1..bb4bd92 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -362,8 +362,7 @@ operation on the contents of the current buffer."
>      (if (re-search-forward "(\\([^()]*\\))$" (line-end-position) t)
>  	(let ((inhibit-read-only t))
>  	  (replace-match (concat "("
> -				 (propertize (mapconcat 'identity tags " ")
> -					     'face 'notmuch-tag-face)
> +				 (notmuch-tag-format-tags tags)
>  				 ")"))))))
>=20=20
>  (defun notmuch-clean-address (address)
> @@ -441,8 +440,7 @@ message at DEPTH in the current thread."
>  	    " ("
>  	    date
>  	    ") ("
> -	    (propertize (mapconcat 'identity tags " ")
> -			'face 'notmuch-tag-face)
> +	    (notmuch-tag-format-tags tags)
>  	    ")\n")
>      (overlay-put (make-overlay start (point)) 'face 'notmuch-message-sum=
mary-face)))
>=20=20
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 4fce3a9..2a64d48 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -1,5 +1,6 @@
>  ;; notmuch-tag.el --- tag messages within emacs
>  ;;
> +;; Copyright =C2=A9 Damien Cassou
>  ;; Copyright =C2=A9 Carl Worth
>  ;;
>  ;; This file is part of Notmuch.
> @@ -18,11 +19,229 @@
>  ;; along with Notmuch.  If not, see <http://www.gnu.org/licenses/>.
>  ;;
>  ;; Authors: Carl Worth <cworth@cworth.org>
> +;;          Damien Cassou <damien.cassou@gmail.com>
> +;;
> +;;; Code:
> +;;
>=20=20
> -(eval-when-compile (require 'cl))
> +(require 'cl)
>  (require 'crm)
>  (require 'notmuch-lib)
>=20=20
> +(defcustom notmuch-tag-formats
> +  '(("unread" (propertize tag 'face '(:foreground "red")))
> +    ("flagged" (notmuch-tag-format-image-data tag (notmuch-tag-star-icon=
))))
> +  "Custom formats for individual tags.
> +
> +This gives a list that maps from tag names to lists of formatting
> +expressions.  The car of each element gives a tag name and the
> +cdr gives a list of Elisp expressions that modify the tag.  If
> +the list is empty, the tag will simply be hidden.  Otherwise,
> +each expression will be evaluated in order: for the first
> +expression, the variable `tag' will be bound to the tag name; for
> +each later expression, the variable `tag' will be bound to the
> +result of the previous expression.  In this way, each expression
> +can build on the formatting performed by the previous expression.
> +The result of the last expression will displayed in place of the
> +tag.
> +
> +For example, to replace a tag with another string, simply use
> +that string as a formatting expression.  To change the foreground
> +of a tag to red, use the expression
> +  (propertize tag 'face '(:foreground \"red\"))
> +
> +See also `notmuch-tag-format-image', which can help replace tags
> +with images."
> +
> +  :group 'notmuch-search
> +  :group 'notmuch-show
> +  :type '(alist :key-type (string :tag "Tag")
> +		:extra-offset -3
> +		:value-type
> +		(radio :format "%v"
> +		       (const :tag "Hidden" nil)
> +		       (set :tag "Modified"
> +			    (string :tag "Display as")
> +			    (list :tag "Face" :extra-offset -4
> +				  (const :format "" :inline t
> +					 (propertize tag 'face))
> +				  (list :format "%v"
> +					(const :format "" quote)
> +					custom-face-edit))
> +			    (list :format "%v" :extra-offset -4
> +				  (const :format "" :inline t
> +					 (notmuch-tag-format-image-data tag))
> +				  (choice :tag "Image"
> +					  (const :tag "Star"
> +						 (notmuch-tag-star-icon))
> +					  (const :tag "Empty star"
> +						 (notmuch-tag-star-empty-icon))
> +					  (const :tag "Tag"
> +						 (notmuch-tag-tag-icon))
> +					  (string :tag "Custom")))
> +			    (sexp :tag "Custom")))))
> +
> +(defun notmuch-tag-format-image-data (tag data)
> +  "Replace TAG with image DATA, if available.
> +
> +This function returns a propertized string that will display image
> +DATA in place of TAG.This is designed for use in
> +`notmuch-tag-formats'.
> +
> +DATA is the content of an SVG picture (e.g., as returned by
> +`notmuch-tag-star-icon')."
> +  (propertize tag 'display
> +	      `(image :type svg
> +		      :data ,data
> +		      :ascent center
> +		      :mask heuristic)))
> +
> +(defun notmuch-tag-star-icon ()
> +  "Return SVG data representing a star icon.
> +This can be used with `notmuch-tag-format-image-data'."
> +  "<?xml version=3D\"1.0\" encoding=3D\"UTF-8\" standalone=3D\"no\"?>
> +<svg
> +   xmlns:dc=3D\"http://purl.org/dc/elements/1.1/\"
> +   xmlns:cc=3D\"http://creativecommons.org/ns#\"
> +   xmlns:rdf=3D\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\"
> +   xmlns:svg=3D\"http://www.w3.org/2000/svg\"
> +   xmlns=3D\"http://www.w3.org/2000/svg\"
> +   version=3D\"1.1\"
> +   width=3D\"16\"
> +   height=3D\"16\"
> +   id=3D\"svg2\">
> +  <defs
> +     id=3D\"defs4\" />
> +  <metadata
> +     id=3D\"metadata7\">
> +    <rdf:RDF>
> +      <cc:Work
> +         rdf:about=3D\"\">
> +        <dc:format>image/svg+xml</dc:format>
> +        <dc:type
> +           rdf:resource=3D\"http://purl.org/dc/dcmitype/StillImage\" />
> +        <dc:title></dc:title>
> +      </cc:Work>
> +    </rdf:RDF>
> +  </metadata>
> +  <g
> +     transform=3D\"translate(-242.81601,-315.59635)\"
> +     id=3D\"layer1\">
> +    <path
> +       d=3D\"m 290.25762,334.31206 -17.64143,-11.77975 -19.70508,7.85447=
 5.75171,-20.41814 -13.55925,-16.31348 21.19618,-0.83936 11.325,-17.93675 7=
.34825,19.89939 20.55849,5.22795 -16.65471,13.13786 z\"
> +       transform=3D\"matrix(0.2484147,-0.02623394,0.02623394,0.2484147,1=
74.63605,255.37691)\"
> +       id=3D\"path2985\"
> +       style=3D\"fill:#ffff00;fill-rule:evenodd;stroke:#000000;stroke-wi=
dth:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opacity:1\" />
> +  </g>
> +</svg>")

You could simplify these SVGs much further.  You can remove the defs and
metadata blocks, and then remove all of the xmlns:* properties (keeping
just the root namespace).  You can also remove the id properties.  I
think you can even remove the g, leaving only the path, since it's only
a translate and I'm pretty sure the origin doesn't matter.

> +
> +(defun notmuch-tag-star-empty-icon ()
> +  "Return SVG data representing an empty star icon.
> +This can be used with `notmuch-tag-format-image-data'."
> +  "<?xml version=3D\"1.0\" encoding=3D\"UTF-8\" standalone=3D\"no\"?>
> +<!-- Created with Inkscape (http://www.inkscape.org/) -->
> +
> +<svg
> +   xmlns:dc=3D\"http://purl.org/dc/elements/1.1/\"
> +   xmlns:cc=3D\"http://creativecommons.org/ns#\"
> +   xmlns:rdf=3D\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\"
> +   xmlns:svg=3D\"http://www.w3.org/2000/svg\"
> +   xmlns=3D\"http://www.w3.org/2000/svg\"
> +   version=3D\"1.1\"
> +   width=3D\"16\"
> +   height=3D\"16\"
> +   id=3D\"svg2\">
> +  <defs
> +     id=3D\"defs4\" />
> +  <metadata
> +     id=3D\"metadata7\">
> +    <rdf:RDF>
> +      <cc:Work
> +         rdf:about=3D\"\">
> +        <dc:format>image/svg+xml</dc:format>
> +        <dc:type
> +           rdf:resource=3D\"http://purl.org/dc/dcmitype/StillImage\" />
> +        <dc:title></dc:title>
> +      </cc:Work>
> +    </rdf:RDF>
> +  </metadata>
> +  <g
> +     transform=3D\"translate(-242.81601,-315.59635)\"
> +     id=3D\"layer1\">
> +    <path
> +       d=3D\"m 290.25762,334.31206 -17.64143,-11.77975 -19.70508,7.85447=
 5.75171,-20.41814 -13.55925,-16.31348 21.19618,-0.83936 11.325,-17.93675 7=
.34825,19.89939 20.55849,5.22795 -16.65471,13.13786 z\"
> +       transform=3D\"matrix(0.2484147,-0.02623394,0.02623394,0.2484147,1=
74.63605,255.37691)\"
> +       id=3D\"path2985\"
> +       style=3D\"fill:#d6d6d1;fill-opacity:1;fill-rule:evenodd;stroke:#0=
00000;stroke-width:1px;stroke-linecap:butt;stroke-linejoin:miter;stroke-opa=
city:1\" />
> +  </g>
> +</svg>")
> +
> +(defun notmuch-tag-tag-icon ()
> +  "Return SVG data representing a tag icon.
> +This can be used with `notmuch-tag-format-image-data'."
> +  "<?xml version=3D\"1.0\" encoding=3D\"UTF-8\" standalone=3D\"no\"?>
> +<!-- Created with Inkscape (http://www.inkscape.org/) -->
> +
> +<svg
> +   xmlns:dc=3D\"http://purl.org/dc/elements/1.1/\"
> +   xmlns:cc=3D\"http://creativecommons.org/ns#\"
> +   xmlns:rdf=3D\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\"
> +   xmlns:svg=3D\"http://www.w3.org/2000/svg\"
> +   xmlns=3D\"http://www.w3.org/2000/svg\"
> +   version=3D\"1.1\"
> +   width=3D\"16\"
> +   height=3D\"16\"
> +   id=3D\"svg3805\">
> +  <defs
> +     id=3D\"defs3807\" />
> +  <metadata
> +     id=3D\"metadata3810\">
> +    <rdf:RDF>
> +      <cc:Work
> +         rdf:about=3D\"\">
> +        <dc:format>image/svg+xml</dc:format>
> +        <dc:type
> +           rdf:resource=3D\"http://purl.org/dc/dcmitype/StillImage\" />
> +        <dc:title></dc:title>
> +      </cc:Work>
> +    </rdf:RDF>
> +  </metadata>
> +  <g
> +     transform=3D\"translate(0,-1036.3622)\"
> +     id=3D\"layer1\">
> +    <path
> +       d=3D\"m 0.44642857,1040.9336 12.50000043,0 2.700893,3.6161 -2.700=
893,3.616 -12.50000043,0 z\"
> +       id=3D\"rect4321\"
> +       style=3D\"fill:#ffff00;fill-opacity:1;fill-rule:evenodd;stroke:#0=
00000;stroke-width:0.25;stroke-linecap:butt;stroke-linejoin:miter;stroke-mi=
terlimit:4;stroke-opacity:1\" />
> +  </g>
> +</svg>")
> +
> +(defun notmuch-tag-get-format (tag)
> +  "Return the format for TAG in `notmuch-tag-formats'."

Hmm.  I read this as simply returning the format from
notmuch-tag-formats, like just the assoc, whereas really this *applies*
a format to tag.  Maybe notmuch-tag-format-tag?  This is really the
single tag equivalent of notmuch-tag-format-tags, so it would make sense
for the names to parallel each other.

> +  (let ((formats (assoc tag notmuch-tag-formats)))
> +    (cond
> +     ((null formats)		;; - Tag not in `notmuch-tag-formats',
> +      tag)			;; the format is then the tag itself.
> +     ((null (cdr formats))	;; - Tag was deliberately hidden,
> +      nil)			;; no format must be returned
> +     (t				;; - Tag was found and has formats,
> +      (let ((tag tag))		;; we must apply all the formats.
> +	(dolist (format (cdr formats) tag)
> +	  (setq tag (eval format))))))))
> +
> +(defun notmuch-tag-list-get-format (tags)

Is there a reason to separate this in to a separate function, rather
than inlining it into notmuch-tag-format-tags?  The reason I wonder is
that the function name doesn't seem very informative, which suggests
that it doesn't really exist as a separate concept outside of
notmuch-tag-format-tags.

> +  (mapconcat #'identity
> +	     ;; nil indicated that the tag was deliberately hidden
> +	     (delq nil (mapcar #'notmuch-tag-get-format tags))
> +	     " "))
> +
> +(defun notmuch-tag-format-tags (tags)
> +  "Return a string representing TAGS with their formats."
> +  (notmuch-combine-face-text-property-string
> +   (notmuch-tag-list-get-format tags)
> +   'notmuch-tag-face
> +   t))
> +
>  (defcustom notmuch-before-tag-hook nil
>    "Hooks that are run before tags of a message are modified.
>=20=20
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index c98a4fe..e58c51d 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -797,9 +797,8 @@ non-authors is found, assume that all of the authors =
match."
>      (notmuch-search-insert-authors format-string (plist-get result :auth=
ors)))
>=20=20
>     ((string-equal field "tags")
> -    (let ((tags-str (mapconcat 'identity (plist-get result :tags) " ")))
> -      (insert (propertize (format format-string tags-str)
> -			  'face 'notmuch-tag-face))))))
> +    (let ((tags (plist-get result :tags)))
> +      (insert (format format-string (notmuch-tag-format-tags tags)))))))
>=20=20
>  (defun notmuch-search-show-result (result &optional pos)
>    "Insert RESULT at POS or the end of the buffer if POS is null."
> --=20
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch