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

[Jonas Bernoulli <jonas@bernoul.li>]

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