[PATCH] emacs: make the remaining faces configurable.

[PATCH] emacs: make the remaining faces configurable.

[Matt Armstrong]

I believe this moves all "anonymous" face specifications in notmuch
code into a configurable defface.
---
 NEWS                 |  8 ++++----
 emacs/notmuch-tag.el | 45 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/NEWS b/NEWS
index 3a9c8d3..2ff78e9 100644
--- a/NEWS
+++ b/NEWS
@@ -6,10 +6,10 @@ Emacs
 
 Face customization is easier
 
-  New faces `notmuch-search-flagged-face` and
-  `notmuch-search-unread-face` are used by default by
-  `notmuch-search-line-faces`. Customize `notmuch-faces` to modify
-  them.
+  New faces `notmuch-tag-unread`, `notmuch-tag-flagged`,
+  `notmuch-tag-deleted`, `notmuch-tag-added`,
+  `notmuch-search-flagged-face` and `notmuch-search-unread-face` are
+  now used by default. Customize `notmuch-faces` to modify them.
 
 Ruby Bindings
 -------------
diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index a3f0c52..ec3c964 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -56,9 +56,23 @@
 					  (string :tag "Custom")))
 			    (sexp :tag "Custom")))))
 
+(defface notmuch-tag-unread
+  '((t :foreground "red"))
+  "Default face used for the unread tag.
+
+Used in the default value of `notmuch-tag-formats`."
+  :group 'notmuch-faces)
+
+(defface notmuch-tag-flagged
+  '((t :foreground "blue"))
+  "Face used for the flagged tag.
+
+Used in the default value of `notmuch-tag-formats`."
+  :group 'notmuch-faces)
+
 (defcustom notmuch-tag-formats
-  '(("unread" (propertize tag 'face '(:foreground "red")))
-    ("flagged" (propertize tag 'face '(:foreground "blue"))
+  '(("unread" (propertize tag 'face 'notmuch-tag-unread))
+    ("flagged" (propertize tag 'face 'notmuch-tag-flagged)
      (notmuch-tag-format-image-data tag (notmuch-tag-star-icon))))
   "Custom formats for individual tags.
 
@@ -90,15 +104,17 @@ with images."
   :group 'notmuch-faces
   :type 'notmuch-tag-format-type)
 
+(defface notmuch-tag-deleted
+  '((((class color) (supports :strike-through)) :strike-through "red")
+    (t :inverse-video t))
+  "Face used to display deleted tags.
+
+Used in the default value of `notmuch-tag-deleted-formats`."
+  :group 'notmuch-faces)
+
 (defcustom notmuch-tag-deleted-formats
-  '(("unread" (notmuch-apply-face bare-tag
-				  (if (display-supports-face-attributes-p '(:strike-through "red"))
-				      '(:strike-through "red")
-				    '(:inverse-video t))))
-    (".*" (notmuch-apply-face tag
-			      (if (display-supports-face-attributes-p '(:strike-through "red"))
-				  '(:strike-through "red")
-				'(:inverse-video t)))))
+  '(("unread" (notmuch-apply-face bare-tag `notmuch-tag-deleted))
+    (".*" (notmuch-apply-face tag `notmuch-tag-deleted)))
   "Custom formats for tags when deleted.
 
 For deleted tags the formats in `notmuch-tag-formats` are applied
@@ -118,8 +134,15 @@ See `notmuch-tag-formats' for full documentation."
   :group 'notmuch-faces
   :type 'notmuch-tag-format-type)
 
+(defface notmuch-tag-added
+  '((t :underline "green"))
+  "Default face used for added tags.
+
+Used in the default value for `notmuch-tag-added-formats`."
+  :group 'notmuch-faces)
+
 (defcustom notmuch-tag-added-formats
-  '((".*" (notmuch-apply-face tag '(:underline "green"))))
+  '((".*" (notmuch-apply-face tag 'notmuch-tag-added)))
   "Custom formats for tags when added.
 
 For added tags the formats in `notmuch-tag-formats` are applied
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] emacs: make the remaining faces configurable.

[David Bremner]

Matt Armstrong <marmstrong@google.com> writes:

> I believe this moves all "anonymous" face specifications in notmuch
> code into a configurable defface.
> ---

pushed,

d

[PATCH] emacs: tag deleted face bugfix

[Mark Walters]

Commit d25d33ff cleaned up some of the tag face code. However, for the
face notmuch-tag-deleted it used the test

((class color) (supports :strike-through))

to decide whether to use red strikethrough or inverse-video (emacs in
a terminal typically doesn't support red strikethrough, but in X it does).

However, it seems that test often returns true even though red
strikethrough is not supported. This breaks the tag update code -- the
wrong thing is displayed to the user.

Thus we make the test explicitly more specific, changing the test to

((class color) (supports :strike-through "red"))
---

Tomi found this bug today, and narrowed it down to a recent notmuch
change. This seems to fix it, and the code now seems to work as
expected in terminals and in X. However I am not an expert on emacs
faces so there may be a better way.

Best wishes

Mark



emacs/notmuch-tag.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
index 644ce40..e59f148 100644
--- a/emacs/notmuch-tag.el
+++ b/emacs/notmuch-tag.el
@@ -137,7 +137,7 @@ with images."
   :type 'notmuch-tag-format-type)
 
 (defface notmuch-tag-deleted
-  '((((class color) (supports :strike-through)) :strike-through "red")
+  '((((class color) (supports :strike-through "red")) :strike-through "red")
     (t :inverse-video t))
   "Face used to display deleted tags.
 
-- 
2.1.4

Re: [PATCH] emacs: tag deleted face bugfix

[Tomi Ollila]

On Sun, Sep 18 2016, Mark Walters <markwalters1009@gmail.com> wrote:

> Commit d25d33ff cleaned up some of the tag face code. However, for the
> face notmuch-tag-deleted it used the test
>
> ((class color) (supports :strike-through))
>
> to decide whether to use red strikethrough or inverse-video (emacs in
> a terminal typically doesn't support red strikethrough, but in X it does).
>
> However, it seems that test often returns true even though red
> strikethrough is not supported. This breaks the tag update code -- the
> wrong thing is displayed to the user.
>
> Thus we make the test explicitly more specific, changing the test to
>
> ((class color) (supports :strike-through "red"))
> ---
>
> Tomi found this bug today, and narrowed it down to a recent notmuch
> change. This seems to fix it, and the code now seems to work as
> expected in terminals and in X. However I am not an expert on emacs
> faces so there may be a better way.

The change works for me and IMO it looks sensible

>-  '((((class color) (supports :strike-through)) :strike-through "red")
>+  '((((class color) (supports :strike-through "red")) :strike-through "red")

I tried to look documentation and grepped some emacs (lisp!) source to
verify that this fix is exactly as it should be, but could not find any.
Well, at least it looks better than what it used to be...

+1

Tomi



>
> Best wishes
>
> Mark
>
>
>
> emacs/notmuch-tag.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 644ce40..e59f148 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -137,7 +137,7 @@ with images."
>    :type 'notmuch-tag-format-type)
>  
>  (defface notmuch-tag-deleted
> -  '((((class color) (supports :strike-through)) :strike-through "red")
> +  '((((class color) (supports :strike-through "red")) :strike-through "red")
>      (t :inverse-video t))
>    "Face used to display deleted tags.
>  
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: tag deleted face bugfix

[Matt Armstrong]

(notmuch-apply-face tag
   (if (display-supports-face-attributes-p            '(:strike-through "red"))
       '(:strike-through "red")
             '(:inverse-video t)))

Mark Walters <markwalters1009@gmail.com> writes:

> Commit d25d33ff cleaned up some of the tag face code. However, for the
> face notmuch-tag-deleted it used the test
>
> ((class color) (supports :strike-through))
>
> to decide whether to use red strikethrough or inverse-video (emacs in
> a terminal typically doesn't support red strikethrough, but in X it does).
>
> However, it seems that test often returns true even though red
> strikethrough is not supported. This breaks the tag update code -- the
> wrong thing is displayed to the user.
>
> Thus we make the test explicitly more specific, changing the test to
>
> ((class color) (supports :strike-through "red"))
> ---
>
> Tomi found this bug today, and narrowed it down to a recent notmuch
> change. This seems to fix it, and the code now seems to work as
> expected in terminals and in X. However I am not an expert on emacs
> faces so there may be a better way.
>
> Best wishes
>
> Mark

Mark, thanks, the patch looks correct to me.  I apologize for the
regression.

My commit d25d33ff intended to adapt following literal face spec in the
call to notmuch-apply-face into a new face declared with defface:

(notmuch-apply-face
 tag (if (display-supports-face-attributes-p
          '(:strike-through "red"))
         '(:strike-through "red")
       '(:inverse-video t)))

Which would be what you've done in this patch:

(((class color) (supports :strike-through "red")) :strike-through "red")

I omitted the "red" portion of the supports string, which looks to be an
error.

Info docs for face attributes has:

‘:strike-through’
     Whether or not characters should be strike-through, and in what
     color.  The value is used like that of ‘:overline’.

This suggests that supplying a color is appropriate.





> emacs/notmuch-tag.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index 644ce40..e59f148 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -137,7 +137,7 @@ with images."
>    :type 'notmuch-tag-format-type)
>  
>  (defface notmuch-tag-deleted
> -  '((((class color) (supports :strike-through)) :strike-through "red")
> +  '((((class color) (supports :strike-through "red")) :strike-through "red")
>      (t :inverse-video t))
>    "Face used to display deleted tags.
>  
> -- 
> 2.1.4

Re: [PATCH] emacs: tag deleted face bugfix

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Commit d25d33ff cleaned up some of the tag face code. However, for the
> face notmuch-tag-deleted it used the test
>

pushed,

d