Re: [PATCH] emacs: tag deleted face bugfix
Mark Walters writes: > Commit d25d33ff cleaned up some of the tag face code. However, for the > face notmuch-tag-deleted it used the test > pushed, d ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: tag deleted face bugfix
(notmuch-apply-face tag (if (display-supports-face-attributes-p'(:strike-through "red")) '(:strike-through "red") '(:inverse-video t))) Mark Walters 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 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] emacs: tag deleted face bugfix
On Sun, Sep 18 2016, Mark Walters 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 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] emacs: tag deleted face bugfix
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 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
