[PATCH] Emacs: Fix notmuch-message-summary-face definition

[PATCH] Emacs: Fix notmuch-message-summary-face definition

[Teemu Likonen]

Emacs face definition forms are either

    ((DISPLAY . PLIST)
     (DISPLAY . PLIST))

or

    ((DISPLAY PLIST)   ;For backward compatibility.
     (DISPLAY PLIST))

Commit a2388bc56e55da5d5695816818274f8a84b0ed92 (2020-08-08) follows
neither of the correct formats. It defines:

    `((((class color) (background light))
       ,@(and (>= emacs-major-version 27) '(:extend t))
       (:background "#f0f0f0"))
      (((class color) (background dark))
       ,@(and (>= emacs-major-version 27) '(:extend t))
       (:background "#303030")))

which produces:

    ((DISPLAY
      :extend t (:background "#f0f0f0"))
     (DISPLAY
      :extend t (:background "#303030")))

And that is wrong format.

This change fixes the face definition form to produce:

    ((DISPLAY
      (:background "#f0f0f0" :extend t))
     (DISPLAY
      (:background "#303030" :extend t)))

which follows the (DISPLAY PLIST) format (see above).
---
 emacs/notmuch.el | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index babddbb6..16227b5c 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -274,11 +274,9 @@ there will be called at other points of notmuch execution."
 
 (defface notmuch-message-summary-face
   `((((class color) (background light))
-     ,@(and (>= emacs-major-version 27) '(:extend t))
-     (:background "#f0f0f0"))
+     (:background "#f0f0f0" ,@(if (>= emacs-major-version 27) '(:extend t))))
     (((class color) (background dark))
-     ,@(and (>= emacs-major-version 27) '(:extend t))
-     (:background "#303030")))
+     (:background "#303030" ,@(if (>= emacs-major-version 27) '(:extend t)))))
   "Face for the single-line message summary in notmuch-show-mode."
   :group 'notmuch-show
   :group 'notmuch-faces)
-- 
2.20.1

Re: [PATCH] Emacs: Fix notmuch-message-summary-face definition

[Jonas Bernoulli]

Teemu Likonen <tlikonen@iki.fi> writes:

> Emacs face definition forms are either
>
>     ((DISPLAY . PLIST)
>      (DISPLAY . PLIST))
>
> or
>
>     ((DISPLAY PLIST)   ;For backward compatibility.
>      (DISPLAY PLIST))
>
> Commit a2388bc56e55da5d5695816818274f8a84b0ed92 (2020-08-08) follows
> neither of the correct formats. It defines:
>
>     `((((class color) (background light))
>        ,@(and (>= emacs-major-version 27) '(:extend t))
>        (:background "#f0f0f0"))
>       (((class color) (background dark))
>        ,@(and (>= emacs-major-version 27) '(:extend t))
>        (:background "#303030")))
>
> which produces:
>
>     ((DISPLAY
>       :extend t (:background "#f0f0f0"))
>      (DISPLAY
>       :extend t (:background "#303030")))
>
> And that is wrong format.

You are right.

Sorry about this.  It happened because in every other package I patched
so far the new format was used and I didn't notice that this wasn't the
case here.

> This change fixes the face definition form to produce:
>
>     ((DISPLAY
>       (:background "#f0f0f0" :extend t))
>      (DISPLAY
>       (:background "#303030" :extend t)))
>
> which follows the (DISPLAY PLIST) format (see above).
> ---
>  emacs/notmuch.el | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index babddbb6..16227b5c 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -274,11 +274,9 @@ there will be called at other points of notmuch execution."
>  
>  (defface notmuch-message-summary-face
>    `((((class color) (background light))
> -     ,@(and (>= emacs-major-version 27) '(:extend t))
> -     (:background "#f0f0f0"))
> +     (:background "#f0f0f0" ,@(if (>= emacs-major-version 27) '(:extend t))))
>      (((class color) (background dark))
> -     ,@(and (>= emacs-major-version 27) '(:extend t))
> -     (:background "#303030")))
> +     (:background "#303030" ,@(if (>= emacs-major-version 27) '(:extend t)))))
>    "Face for the single-line message summary in notmuch-show-mode."
>    :group 'notmuch-show
>    :group 'notmuch-faces)
> -- 
> 2.20.1

I would recommend that you
- switch to using the new format
- keep the `:extend' setting on its own line
- keep the `:extend' at the beginning of the list
- use `and' instead of `if' because
  - it is better to use `when' instead of `if' when
    there is no ELSE part
  - it is better to use `and' instead of `when` when
    the form is about the returned value, not some
    side-effect


Best regards,
Jonas

[PATCH v2] Emacs: Fix notmuch-message-summary-face definition

[Teemu Likonen]

Emacs face definition forms are either

    ((DISPLAY . PLIST)
     (DISPLAY . PLIST))

or

    ((DISPLAY PLIST)   ;For backward compatibility.
     (DISPLAY PLIST))

Commit a2388bc56e55da5d5695816818274f8a84b0ed92 (2020-08-08) follows
neither of the correct formats. It defines:

    `((((class color) (background light))
       ,@(and (>= emacs-major-version 27) '(:extend t))
       (:background "#f0f0f0"))
      (((class color) (background dark))
       ,@(and (>= emacs-major-version 27) '(:extend t))
       (:background "#303030")))

which produces:

    ((DISPLAY
      :extend t (:background "#f0f0f0"))
     (DISPLAY
      :extend t (:background "#303030")))

And that is wrong format.

This change fixes the face definition form to produce:

    ((DISPLAY
      :extend t :background "#f0f0f0")
     (DISPLAY
      :extend t :background "#303030"))

which follows the (DISPLAY . PLIST) format (see above).
---
 emacs/notmuch.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


* 2020-08-16 15:51:14+02, Jonas Bernoulli wrote:

> I would recommend that you
> - switch to using the new format
> - keep the `:extend' setting on its own line
> - keep the `:extend' at the beginning of the list

The new format is the only meaningful change but OK.

> - use `and' instead of `if' because
>   - it is better to use `when' instead of `if' when
>     there is no ELSE part

I disagree with that. I think IF is more about return values and WHEN
about longer code with side effects.

>   - it is better to use `and' instead of `when` when
>     the form is about the returned value, not some
>     side-effect

To me AND is more like multiple condition for "if all the forms are
non-nil" and IF is more about return values. Obviously they are
techinally the same.

Nevertheless, I changed my IF's to AND's so there is now the smallest
possible diff in this version.


diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index babddbb6..04123595 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -275,10 +275,10 @@ there will be called at other points of notmuch execution."
 (defface notmuch-message-summary-face
   `((((class color) (background light))
      ,@(and (>= emacs-major-version 27) '(:extend t))
-     (:background "#f0f0f0"))
+     :background "#f0f0f0")
     (((class color) (background dark))
      ,@(and (>= emacs-major-version 27) '(:extend t))
-     (:background "#303030")))
+     :background "#303030"))
   "Face for the single-line message summary in notmuch-show-mode."
   :group 'notmuch-show
   :group 'notmuch-faces)
-- 
2.20.1

Re: [PATCH v2] Emacs: Fix notmuch-message-summary-face definition

[David Bremner]

Teemu Likonen <tlikonen@iki.fi> writes:

>
> This change fixes the face definition form to produce:
>
>     ((DISPLAY
>       :extend t :background "#f0f0f0")
>      (DISPLAY
>       :extend t :background "#303030"))
>
> which follows the (DISPLAY . PLIST) format (see above).

applied to release and master

d