bug#73552: obsolete (and broken) face attribute :reverse-video

bug#73552: obsolete (and broken) face attribute :reverse-video

[Mattias Engdegård]

The face attribute keyword :reverse-video has been broken in defface for a long time, and it's not used very much, so I added a compiler warning and a notice to NEWS.

This keyword along with :bold and :italic have been obsolete for longer than that and aren't even mentioned in any documentation (on purpose) but in the internal face-and-display machinery they still partly work.

I'm not quite sure why :reverse-video doesn't work in defface. Demo, in *scratch*:

  (defface myface '((t :reverse-video t)) "my face")

  (insert (propertize "bon bon" 'font-lock-face 'myface) ?\n)
  => bon bon  ; not inverted

  (insert (propertize "non non" 'font-lock-face '(:reverse-video t)) ?\n)
  => non non  ; inverted

I first blamed a simple 14 years old mistake in `custom-fix-face-spec` but fixing that doesn't help, not sure why. But it means that we probably don't need to fix it, and indeed there are precious few packages using :reverse-video. One is make-mode.el; outside Emacs, I only found one (yaml-mode).

While :bold and :italic are obsolete as well, they are used a lot more (and actually seem to work) so I'm not warning about them now.

To summarise:
- Should we fix `custom-fix-face-spec`? Maybe, or just stop pretending that it handles :reverse-video.
- Should we bother to fix what other mechanism preventing :reverse-video from working? Probably no.
- Should we remove :reverse-video from the display machinery? Yes, but maybe not now?
- Should we remove :bold and :italic? Yes, but definitely not now.

bug#73552: obsolete (and broken) face attribute :reverse-video

[Gerd Möllmann]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> The face attribute keyword :reverse-video has been broken in defface
> for a long time, and it's not used very much, so I added a compiler
> warning and a notice to NEWS.
>
> This keyword along with :bold and :italic have been obsolete for
> longer than that and aren't even mentioned in any documentation (on
> purpose) but in the internal face-and-display machinery they still
> partly work.
>
> I'm not quite sure why :reverse-video doesn't work in defface. Demo, in *scratch*:
>
>   (defface myface '((t :reverse-video t)) "my face")
>
>   (insert (propertize "bon bon" 'font-lock-face 'myface) ?\n)
>   => bon bon  ; not inverted
>
>   (insert (propertize "non non" 'font-lock-face '(:reverse-video t)) ?\n)
>   => non non  ; inverted
>
> I first blamed a simple 14 years old mistake in `custom-fix-face-spec`
> but fixing that doesn't help, not sure why. But it means that we
> probably don't need to fix it, and indeed there are precious few
> packages using :reverse-video. One is make-mode.el; outside Emacs, I
> only found one (yaml-mode).
>
> While :bold and :italic are obsolete as well, they are used a lot more
> (and actually seem to work) so I'm not warning about them now.
>
> To summarise:
> - Should we fix `custom-fix-face-spec`? Maybe, or just stop pretending that it handles :reverse-video.
> - Should we bother to fix what other mechanism preventing :reverse-video from working? Probably no.
> - Should we remove :reverse-video from the display machinery? Yes, but maybe not now?
> - Should we remove :bold and :italic? Yes, but definitely not now.

FWIW I'm pretty sure that I added :italic, :bold, :reverse-video to what
later became Emacs 21 only for backwards compatibility with the old
pre-21 face implementation. At least on the surface, because 21 faces
work completely differently. (I also seem to remeber that reverse-video
was a synonym for inverse-video in the old code, maybe even
undocumented. Too lazy to look that up ATM, though :-).)

bug#73552: obsolete (and broken) face attribute :reverse-video

[Mattias Engdegård]

29 sep. 2024 kl. 13.31 skrev Gerd Möllmann <gerd.moellmann@gmail.com>:

>> I first blamed a simple 14 years old mistake in `custom-fix-face-spec`
>> but fixing that doesn't help, not sure why.

Having said that I naturally had to find the bug and it appears to be `face-spec-set-2` which only sets attributes that are present in `face-x-resources` for some reason.

> I'm pretty sure that I added :italic, :bold, :reverse-video to what
> later became Emacs 21 only for backwards compatibility with the old
> pre-21 face implementation. At least on the surface, because 21 faces
> work completely differently. (I also seem to remeber that reverse-video
> was a synonym for inverse-video in the old code, maybe even
> undocumented.

Thanks. Given Gerd's testimony, the lack of use and general brokenness, I propose we just give up on :reverse-video, perhaps removing it from xfaces.c.

bug#73552: obsolete (and broken) face attribute :reverse-video

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sun, 29 Sep 2024 14:35:01 +0200
> Cc: Emacs Bug Report <bug-gnu-emacs@gnu.org>,
>  Eli Zaretskii <eliz@gnu.org>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> 
> 29 sep. 2024 kl. 13.31 skrev Gerd Möllmann <gerd.moellmann@gmail.com>:
> 
> >> I first blamed a simple 14 years old mistake in `custom-fix-face-spec`
> >> but fixing that doesn't help, not sure why.
> 
> Having said that I naturally had to find the bug and it appears to be `face-spec-set-2` which only sets attributes that are present in `face-x-resources` for some reason.
> 
> > I'm pretty sure that I added :italic, :bold, :reverse-video to what
> > later became Emacs 21 only for backwards compatibility with the old
> > pre-21 face implementation. At least on the surface, because 21 faces
> > work completely differently. (I also seem to remeber that reverse-video
> > was a synonym for inverse-video in the old code, maybe even
> > undocumented.
> 
> Thanks. Given Gerd's testimony, the lack of use and general brokenness, I propose we just give up on :reverse-video, perhaps removing it from xfaces.c.

I'm not sure I understand the proposal: we remove :reverse-video, but
keep :inverse-video (which _is_ included in face-x-resources)?  Or did
you mean to remove :inverse-video as well?

bug#73552: obsolete (and broken) face attribute :reverse-video

[Mattias Engdegård]

29 sep. 2024 kl. 14.55 skrev Eli Zaretskii <eliz@gnu.org>:

> I'm not sure I understand the proposal: we remove :reverse-video, but
> keep :inverse-video (which _is_ included in face-x-resources)?  Or did
> you mean to remove :inverse-video as well?

Leave face-x-resources as it is. Keep :inverse-video.
Remove all references to :reverse-video everywhere (except the newly introduced warning).

bug#73552: obsolete (and broken) face attribute :reverse-video

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sun, 29 Sep 2024 15:41:53 +0200
> Cc: gerd.moellmann@gmail.com,
>  bug-gnu-emacs@gnu.org,
>  monnier@iro.umontreal.ca
> 
> 29 sep. 2024 kl. 14.55 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I'm not sure I understand the proposal: we remove :reverse-video, but
> > keep :inverse-video (which _is_ included in face-x-resources)?  Or did
> > you mean to remove :inverse-video as well?
> 
> Leave face-x-resources as it is. Keep :inverse-video.
> Remove all references to :reverse-video everywhere (except the newly introduced warning).

I think the use of :reverse-video in make-mode.el should be changed to
:inverse-video.  Otherwise, I'm okay with retiring this old and
half-supported alias.

bug#73552: obsolete (and broken) face attribute :reverse-video

[Stefan Kangas]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> I first blamed a simple 14 years old mistake in `custom-fix-face-spec`
> but fixing that doesn't help, not sure why. But it means that we
> probably don't need to fix it, and indeed there are precious few
> packages using :reverse-video. One is make-mode.el; outside Emacs, I
> only found one (yaml-mode).

I suggest fixing this in make-mode.el to avoid the warning.

> While :bold and :italic are obsolete as well, they are used a lot more
> (and actually seem to work) so I'm not warning about them now.

We could start by doing s/:bold t/:weight bold/ and
s/:italic t/:slant italic/ in our tree.

> To summarise:
> - Should we fix `custom-fix-face-spec`? Maybe, or just stop pretending that it handles :reverse-video.
> - Should we bother to fix what other mechanism preventing :reverse-video from working? Probably no.
> - Should we remove :reverse-video from the display machinery? Yes, but maybe not now?

It doesn't seem worth fixing :reverse-video at this point, indeed.

I suggest removing it, given that it's been broken for so long and that
we now have a warning instead.

> - Should we remove :bold and :italic? Yes, but definitely not now.

I'd propose adding obsoletion warnings for :bold and :italic.  I note
that they are not documented in the elisp manual.

bug#73552: obsolete (and broken) face attribute :reverse-video

[Mattias Engdegård]

29 sep. 2024 kl. 15.49 skrev Eli Zaretskii <eliz@gnu.org>:

> I think the use of :reverse-video in make-mode.el should be changed to
> :inverse-video.

Oh, certainly (compiling that file now yields a warning).

The few extant uses of :reverse-video discovered so far seem to be fall-back cases for non-colour displays, decidedly a rarity these days.

>  Otherwise, I'm okay with retiring this old and
> half-supported alias.

Thank you, now done.


29 sep. 2024 kl. 15.55 skrev Stefan Kangas <stefankangas@gmail.com>:

> We could start by doing s/:bold t/:weight bold/ and
> s/:italic t/:slant italic/ in our tree.

Yes, maybe later. At least these still work, and are widely used by external packages.

> I'd propose adding obsoletion warnings for :bold and :italic.  I note
> that they are not documented in the elisp manual.

Wr probably should but the compilation warnings only catch constant arguments to `defface`. Other uses (themes, font-lock, computed text properties, non-constant arguments to defface, etc) aren't caught; that would require a run-time warning.

Anyway, we're done with :reverse-video, thanks everyone!

bug#73552: obsolete (and broken) face attribute :reverse-video

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sun, 29 Sep 2024 18:22:42 +0200
> Cc: 73552-done@debbugs.gnu.org,
>  Gerd Möllmann <gerd.moellmann@gmail.com>,
>  Eli Zaretskii <eliz@gnu.org>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> 
> 29 sep. 2024 kl. 15.49 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> >  Otherwise, I'm okay with retiring this old and
> > half-supported alias.
> 
> Thank you, now done.

Thanks.  Should the warning in bytecomp.el be reworded to the effect
that :reverse-video is no longer supported?

bug#73552: obsolete (and broken) face attribute :reverse-video

[Mattias Engdegård]

29 sep. 2024 kl. 18.42 skrev Eli Zaretskii <eliz@gnu.org>:

> Should the warning in bytecomp.el be reworded to the effect
> that :reverse-video is no longer supported?

Yes, why not -- done.