bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Trent W. Buck]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

sieve-mode as at Emacs 28.1 defines its colors ab initio.
This means they have to be themed by hand.
I think sieve-mode should inherit from font-lock-*-face,
as most other things do.

Here is a quick-and-dirty version I knocked out for personal use:

    (custom-set-faces
     '(sieve-control-commands ((t :inherit font-lock-builtin-face)))
     '(sieve-action-commands  ((t :inherit font-lock-function-name-face)))
     '(sieve-test-commands    ((t :inherit font-lock-variable-name-face)))
     '(sieve-tagged-arguments ((t :inherit font-lock-type-face))))


[-- Attachment #2: before.png --]
[-- Type: image/png, Size: 71715 bytes --]

[-- Attachment #3: after.png --]
[-- Type: image/png, Size: 39600 bytes --]

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Eli Zaretskii]

> From: "Trent W. Buck" <trentbuck@gmail.com>
> Date: Wed, 22 Mar 2023 16:41:07 +1100
> 
> sieve-mode as at Emacs 28.1 defines its colors ab initio.

And there's nothing wrong with that, IMO.  Quite a few other modes do
the same.

> This means they have to be themed by hand.

Some modes already do.

> I think sieve-mode should inherit from font-lock-*-face,
> as most other things do.

I'm not sure I agree.  These faces are with us for a long time, so
changing their defaults now brings disadvantages: it could cause
someone dislike the result.

Does anyone else have an opinion about this?

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Gregory Heytings]

>> I think sieve-mode should inherit from font-lock-*-face, as most other 
>> things do.
>
> I'm not sure I agree.  These faces are with us for a long time, so 
> changing their defaults now brings disadvantages: it could cause someone 
> dislike the result.
>
> Does anyone else have an opinion about this?
>

I agree with both Trent and you.

Modes should inherit from font-lock-*-faces, unless there's a good reason 
not to inherit from these faces.  Otherwise customizing the visual 
appearance of Emacs is a nightmare.  This is, by the way, made clear in 
(elisp)Major Mode Conventions: "Each face that the mode defines should, if 
possible, inherit from an existing Emacs face.  See Basic Faces, and see 
Faces for Font Lock."

But the sieve-* faces have existed for twenty years, so it's now too late 
to change them.

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Jim Porter]

On 3/22/2023 1:10 PM, Gregory Heytings wrote:
> But the sieve-* faces have existed for twenty years, so it's now too 
> late to change them.

I think it might actually be ok to change this: the face attributes for 
each of the 'sieve-*' faces closely correspond to one of the 
'font-lock-*-face' faces. They're not *identical*, but the differences 
are very slight.

The mapping is:

   sieve-control-commands -> font-lock-builtin-face
   sieve-action-commands  -> font-lock-function-name-face
   sieve-test-commands    -> font-lock-constant-face
   sieve-tagged-arguments -> font-lock-keyword-face

(Note that this is a different mapping from Trent's proposal.)

If we mapped the 'sieve-*' faces like that, I doubt anyone using the 
default theme would notice. Likewise, anyone using a theme that 
customizes the 'sieve-*' faces wouldn't notice, since they're already 
explicitly-themed.

The only users who would see an impact would be those using non-default 
themes that *didn't* have definitions for 'sieve-*' faces, and they'll 
arguably have improved visuals: with the change, 'sieve-mode' will look 
consistently-themed with the rest of Emacs.

Since there's such a close correspondence to the font-lock faces, I'd 
(weakly) advocate for inheriting from them.

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Trent W. Buck]

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On Wed 22 Mar 2023 14:22:45 -0700, Jim Porter wrote:
> On 3/22/2023 1:10 PM, Gregory Heytings wrote:
> > But the sieve-* faces have existed for twenty years, so it's now too
> > late to change them.
>
> I think it might actually be ok to change this: the face attributes for each
> of the 'sieve-*' faces closely correspond to one of the 'font-lock-*-face'
> faces. They're not *identical*, but the differences are very slight.
>
> The mapping is:
>
>   sieve-control-commands -> font-lock-builtin-face
>   sieve-action-commands  -> font-lock-function-name-face
>   sieve-test-commands    -> font-lock-constant-face
>   sieve-tagged-arguments -> font-lock-keyword-face
>
> (Note that this is a different mapping from Trent's proposal.)

For the record, I'm fine with this mapping.
I only just started using sieve seriously yesterday, so
my initial mapping was a wild-ass guess.

> If we mapped the 'sieve-*' faces like that, I doubt anyone using the default
> theme would notice. Likewise, anyone using a theme that customizes the
> 'sieve-*' faces wouldn't notice, since they're already explicitly-themed.

I agree.

> The only users who would see an impact would be those using non-default
> themes that *didn't* have definitions for 'sieve-*' faces, and they'll
> arguably have improved visuals: with the change, 'sieve-mode' will look
> consistently-themed with the rest of Emacs.

I agree.

I don't think "I opened foo.sieve in Emacs 20 twenty years ago, and
now I opened foo.sieve in Emacs 30 today, and the colors aren't identical, and now I'm mad"
is a good argument for never changing the defaults.

Emacs changes the defaults like that ALL THE TIME.
If they piss me off, I just patch back in the old behaviour and forget about it.
Choice examples:

  ;; Colour comment bodies in console mode.  I tried to get this changed upstream,
  ;; but RMS decreed that it was a feature as (apparently) red is difficult to
  ;; read on eight-colour terminals.  Hopefully a circular dependency is OK...
  ;; FFS, RMS, just configure your terminal, e.g. printf '\033]P1CC0000\033]P9EF2929'
  (eval-after-load "font-lock"
    '(if (< emacs-major-version 24)
         (put 'font-lock-comment-face 'face-alias 'font-lock-comment-delimiter-face)
       ;; UPDATE: it's not OK in Emacs 24.  For now, just assume an 8-color tty.
       (set-face-attribute 'font-lock-comment-face nil :foreground "red")))

  ;; Undo stupid feature where ^M means ^J and ^J means ^M.
  ;; Fixes copy-paste between terminals without bracketed paste mode (i.e. ALL of them).
  ;; http://emacsredux.com/blog/2014/01/19/a-peek-at-emacs-24-dot-4-auto-indentation-by-default/
  (when (fboundp 'electric-indent-mode)
    (electric-indent-mode -1))

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Eli Zaretskii]

> Date: Thu, 23 Mar 2023 12:05:24 +1100
> From: "Trent W. Buck" <trentbuck@gmail.com>
> Cc: Gregory Heytings <gregory@heytings.org>, Eli Zaretskii <eliz@gnu.org>,
> 	62370@debbugs.gnu.org
> 
> I don't think "I opened foo.sieve in Emacs 20 twenty years ago, and
> now I opened foo.sieve in Emacs 30 today, and the colors aren't identical, and now I'm mad"
> is a good argument for never changing the defaults.

From my POV, it _is_ a good argument.  Especially when the defaults
change in a feature that is easily customizable, and so basically you
are asking to impose your preferences, which are easily catered to by
your local customizations, on everyone else.

> Emacs changes the defaults like that ALL THE TIME.

That's not true.  We do that rarely and sparingly, precisely for the
reasons you consider unimportant.

> If they piss me off, I just patch back in the old behaviour and forget about it.

You can do the same with the current defaults, so why are you asking
us to change them?

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Trent W. Buck]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Thu 23 Mar 2023 08:58:09 +0200, Eli Zaretskii wrote:
> > Date: Thu, 23 Mar 2023 12:05:24 +1100
> > From: "Trent W. Buck" <trentbuck@gmail.com>
> > Cc: Gregory Heytings <gregory@heytings.org>, Eli Zaretskii <eliz@gnu.org>,
> >     62370@debbugs.gnu.org
> >
> > I don't think "I opened foo.sieve in Emacs 20 twenty years ago, and
> > now I opened foo.sieve in Emacs 30 today, and the colors aren't identical, and now I'm mad"
> > is a good argument for never changing the defaults.
>
> From my POV, it _is_ a good argument.  Especially when the defaults
> change in a feature that is easily customizable, and so basically you
> are asking to impose your preferences, which are easily catered to by
> your local customizations, on everyone else.
>
> > Emacs changes the defaults like that ALL THE TIME.
>
> That's not true.  We do that rarely and sparingly, precisely for the
> reasons you consider unimportant.
>
> > If they piss me off, I just patch back in the old behaviour and forget about it.
>
> You can do the same with the current defaults, so why are you asking
> us to change them?

I thought I was asking "make sieve-mode faces follow best current practice" (i.e. inherit from font-lock where appropriate), not
"change sieve-mode faces from Tim's arbitrary preference to Trent's arbitrary preference".

But I am not really interested in arguing this further, so let's just WONTFIX this ticket.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Eli Zaretskii]

> Date: Thu, 23 Mar 2023 18:03:47 +1100
> From: "Trent W. Buck" <trentbuck@gmail.com>
> Cc: jporterbugs@gmail.com, gregory@heytings.org, 62370@debbugs.gnu.org
> 
> But I am not really interested in arguing this further, so let's just WONTFIX this ticket.

I didn't yet make up my mind, I'm still interested in hearing
additional arguments for and against the change, provided that they
don't re-iterate what was already said here.

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Jim Porter]

On 3/22/2023 11:58 PM, Eli Zaretskii wrote:
>> Date: Thu, 23 Mar 2023 12:05:24 +1100
>> From: "Trent W. Buck" <trentbuck@gmail.com>
>> Cc: Gregory Heytings <gregory@heytings.org>, Eli Zaretskii <eliz@gnu.org>,
>> 	62370@debbugs.gnu.org
>>
>> I don't think "I opened foo.sieve in Emacs 20 twenty years ago, and
>> now I opened foo.sieve in Emacs 30 today, and the colors aren't identical, and now I'm mad"
>> is a good argument for never changing the defaults.
> 
>  From my POV, it _is_ a good argument.  Especially when the defaults
> change in a feature that is easily customizable, and so basically you
> are asking to impose your preferences, which are easily catered to by
> your local customizations, on everyone else.

I agree that we shouldn't be changing defaults arbitrarily, but as I 
found when looking at the face definitions, we could make this change in 
a way where it just simplifies the job for theme authors but doesn't 
cause a noticeable change in most cases.

Does that sound ok? If not, then I think we can just close this.

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Stefan Kangas]

Jim Porter <jporterbugs@gmail.com> writes:

> I agree that we shouldn't be changing defaults arbitrarily, but as I found when
> looking at the face definitions, we could make this change in a way where it
> just simplifies the job for theme authors but doesn't cause a noticeable change
> in most cases.
>
> Does that sound ok? If not, then I think we can just close this.

I think the solution that you proposed makes sense: it wouldn't break
anything, and it would make sieve-mode look better for many users.

ISTR that Philip was planning to look into improving our face
definitions, so I'm putting him in Cc in case he has any comments.

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Philip Kaludercic]

Stefan Kangas <stefankangas@gmail.com> writes:

> Jim Porter <jporterbugs@gmail.com> writes:
>
>> I agree that we shouldn't be changing defaults arbitrarily, but as I found when
>> looking at the face definitions, we could make this change in a way where it
>> just simplifies the job for theme authors but doesn't cause a noticeable change
>> in most cases.
>>
>> Does that sound ok? If not, then I think we can just close this.
>
> I think the solution that you proposed makes sense: it wouldn't break
> anything, and it would make sieve-mode look better for many users.
>
> ISTR that Philip was planning to look into improving our face
> definitions, so I'm putting him in Cc in case he has any comments.

It is true that I have been looking into proposing more "semantic"
faces for faces.el, but I don't have any comments on this proposal.

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Jim Porter]

On 9/3/2023 3:30 AM, Stefan Kangas wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> I agree that we shouldn't be changing defaults arbitrarily, but as I found when
>> looking at the face definitions, we could make this change in a way where it
>> just simplifies the job for theme authors but doesn't cause a noticeable change
>> in most cases.
>>
>> Does that sound ok? If not, then I think we can just close this.
> 
> I think the solution that you proposed makes sense: it wouldn't break
> anything, and it would make sieve-mode look better for many users.

I tried my proposal out just to be safe and it does actually change the 
appearance in a few cases with the default theme, but I think it's for 
the better:

1. If you're using a color TTY, sieve-mode picks some very simple colors 
("blue", "magenta", and "cyan"). Normally that's fine, since it uses 
whatever your terminal's color palette uses for those basic ANSI colors. 
However, if you have a terminal that can display 24-bit colors, sieve's 
choices become really bad: it's no longer "the blue you've configured 
for your terminal theme that looks nice", but just regular blue 
(#0000ee), which on my terminal is almost unreadable.

2. The font-lock faces also feature slight tweaks to their colors when 
your system supports at least 88 colors. This is only really noticeable 
if you look closely though.

As such, I've pushed this to the master branch as f08684ab39d. Closing 
this now. (Of course, if this *does* cause some problems, we can revisit 
this.)

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Eli Zaretskii]

> Date: Mon, 4 Sep 2023 21:43:25 -0700
> Cc: "Trent W. Buck" <trentbuck@gmail.com>,
>  Philip Kaludercic <philipk@posteo.net>, Eli Zaretskii <eliz@gnu.org>,
>  gregory@heytings.org, 62370-done@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> As such, I've pushed this to the master branch as f08684ab39d. Closing 
> this now. (Of course, if this *does* cause some problems, we can revisit 
> this.)

Please in the future allow more time for the "interested parties" to
respond (and maybe even ask them directly).  At the beginning of this
discussion I asked to hear more opinions, after expressing my own
hesitation, so it would be prudent to let me chime in and say whether
I'm okay with the change before installing the changes right after
Stefan said he agreed with you...

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Jim Porter]

On 9/5/2023 5:01 AM, Eli Zaretskii wrote:
>> Date: Mon, 4 Sep 2023 21:43:25 -0700
>> Cc: "Trent W. Buck" <trentbuck@gmail.com>,
>>   Philip Kaludercic <philipk@posteo.net>, Eli Zaretskii <eliz@gnu.org>,
>>   gregory@heytings.org, 62370-done@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> As such, I've pushed this to the master branch as f08684ab39d. Closing
>> this now. (Of course, if this *does* cause some problems, we can revisit
>> this.)
> 
> Please in the future allow more time for the "interested parties" to
> respond (and maybe even ask them directly).  At the beginning of this
> discussion I asked to hear more opinions, after expressing my own
> hesitation, so it would be prudent to let me chime in and say whether
> I'm okay with the change before installing the changes right after
> Stefan said he agreed with you...

Will do, sorry about that. If you'd prefer that I revert this, just let 
me know (or you can; whichever is fine by me).

bug#62370: 28.1; sieve-mode: faces should inherit from font-lock-X-face faces

[Eli Zaretskii]

> Date: Tue, 5 Sep 2023 08:52:50 -0700
> Cc: trentbuck@gmail.com, philipk@posteo.net, gregory@heytings.org,
>  stefankangas@gmail.com, 62370-done@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 9/5/2023 5:01 AM, Eli Zaretskii wrote:
> >> Date: Mon, 4 Sep 2023 21:43:25 -0700
> >> Cc: "Trent W. Buck" <trentbuck@gmail.com>,
> >>   Philip Kaludercic <philipk@posteo.net>, Eli Zaretskii <eliz@gnu.org>,
> >>   gregory@heytings.org, 62370-done@debbugs.gnu.org
> >> From: Jim Porter <jporterbugs@gmail.com>
> >>
> >> As such, I've pushed this to the master branch as f08684ab39d. Closing
> >> this now. (Of course, if this *does* cause some problems, we can revisit
> >> this.)
> > 
> > Please in the future allow more time for the "interested parties" to
> > respond (and maybe even ask them directly).  At the beginning of this
> > discussion I asked to hear more opinions, after expressing my own
> > hesitation, so it would be prudent to let me chime in and say whether
> > I'm okay with the change before installing the changes right after
> > Stefan said he agreed with you...
> 
> Will do, sorry about that. If you'd prefer that I revert this, just let 
> me know (or you can; whichever is fine by me).

No need to revert, no.  Let's see if there are any complaints from
people who use this mode.