unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / Atom feed
* [PATCH] Emacs: Fix notmuch-message-summary-face definition
@ 2020-08-16 11:10 Teemu Likonen
  2020-08-16 13:51 ` Jonas Bernoulli
  0 siblings, 1 reply; 4+ messages in thread
From: Teemu Likonen @ 2020-08-16 11:10 UTC (permalink / raw)
  To: notmuch; +Cc: Jonas Bernoulli

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Emacs: Fix notmuch-message-summary-face definition
  2020-08-16 11:10 [PATCH] Emacs: Fix notmuch-message-summary-face definition Teemu Likonen
@ 2020-08-16 13:51 ` Jonas Bernoulli
  2020-08-16 17:13   ` [PATCH v2] " Teemu Likonen
  0 siblings, 1 reply; 4+ messages in thread
From: Jonas Bernoulli @ 2020-08-16 13:51 UTC (permalink / raw)
  To: Teemu Likonen, notmuch

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] Emacs: Fix notmuch-message-summary-face definition
  2020-08-16 13:51 ` Jonas Bernoulli
@ 2020-08-16 17:13   ` Teemu Likonen
  2020-08-22 12:51     ` David Bremner
  0 siblings, 1 reply; 4+ messages in thread
From: Teemu Likonen @ 2020-08-16 17:13 UTC (permalink / raw)
  To: notmuch; +Cc: Jonas Bernoulli

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] Emacs: Fix notmuch-message-summary-face definition
  2020-08-16 17:13   ` [PATCH v2] " Teemu Likonen
@ 2020-08-22 12:51     ` David Bremner
  0 siblings, 0 replies; 4+ messages in thread
From: David Bremner @ 2020-08-22 12:51 UTC (permalink / raw)
  To: Teemu Likonen, notmuch; +Cc: Jonas Bernoulli

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-08-22 12:52 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-16 11:10 [PATCH] Emacs: Fix notmuch-message-summary-face definition Teemu Likonen
2020-08-16 13:51 ` Jonas Bernoulli
2020-08-16 17:13   ` [PATCH v2] " Teemu Likonen
2020-08-22 12:51     ` David Bremner

unofficial mirror of notmuch@notmuchmail.org

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/notmuch/0 notmuch/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 notmuch notmuch/ https://yhetil.org/notmuch \
		notmuch@notmuchmail.org
	public-inbox-index notmuch

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.mail.notmuch.general
	nntp://news.gmane.io/gmane.mail.notmuch.general


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git