bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages

[Dmitry Gutov]

On 09.12.2019 14:59, Eli Zaretskii wrote:

> > but I wonder how they would interact with a long-running test
> > session.
>
> Not sure I understand: each test runs in a separate session.  What am
> I missing?

Never mind then. I was not aware.

> > Running them in an interactive session was tricky as well because
> > visiting any file, even in 'emacs -Q', automatically leads to
> > diff-mode.el being loaded, and so (should-not (featurep 'diff-mode))
> > fails right away.
>
> I guess you loaded the tests as a .el file?

Exactly. How else do you run the tests interactively?

I visit the buffer containing the tests, call eval-buffer, then M-x ert.

> > They also rely on the existing themes, the definitions of which will change.
>
> I wanted to avoid creating dummy themes just for this test, but if
> you'd like to do that, feel free.

I'm saying they will break once we remove the now-unnecessary bits from 
the existing themes. It's not like we should keep them just to keep 
these tests passing.

> > In addition to it, I'd like to revert the part of 64687872f6 that
> > changed the bundled themed (etc/themes/*). Is that okay?
>
> Fine with me, but if you do that, you will _have_ to add a special
> theme, or else we won't be able to test some of the features, because
> no theme will set the :extend attribute.

Right.

> > diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
> > index fa81b2e953..df6f07496e 100644
> > --- a/doc/lispref/display.texi
> > +++ b/doc/lispref/display.texi
> > @@ -2499,9 +2499,9 @@ Face Attributes
> >   @code{nil} to not use this face for the space between the end of the
> >   line and the edge of the window.  When Emacs merges several faces for
> >   displaying the empty space beyond end of line, only those faces with
> > -@code{:extend} non-@code{nil} will be merged.  By default, only
> > -@code{region} and @code{hl-line} faces have this attribute set to
> > -@code{t}.
> > +@code{:extend} non-@code{nil} will be merged.  This attribute is
> > +different from the others in that when a theme doesn't define it for a
> > +face, the value from the default spec is inherited.
>
> Why did you lose the sentence that starts with "By default"?

Because it's been untrue for some time. More faces than just region and 
hl-line have this attribute set. I have put the (hopefully) full list in 
NEWS now, but it's getting ridiculous. Certainly not something to 
mention in the manual.

> > +This attribute behaves specially when theme definitions are applied:
> > +if the theme doesn't specify its value for a face, the value from the
> > +default spec is used.
>
> "Its value" is ambiguous, suggest to say "an explicit value" instead.

I don't see the distinction, but ok.

> Also, "default spec" is somewhat unclear.  I would suggest "original
> face definition by @code{defface}" and add a cross-reference to
> "Defining Faces", where defface is described.

Ok.

> >                          Consequently, themes usually shouldn't touch
> > +this attribute at all.
>
> I don't think we should say that, it sounds like a guideline, which it
> isn't.  We should either remove it, or make it just something to
> consider, by saying "...shouldn't set this attribute, unless the theme
> has a good reason to do so."

Why not a guideline? A recommendation to avoid duplication and 
unnecessary incompatibility with older Emacs is a good thing.

In any case, the second option sounds good.

> > -    ;; defface spec entirely (rather than inheriting from it).  If
> > -    ;; there was no spec applicable to FRAME, apply the defface spec
> > -    ;; as well as any applicable X resources.
> > +    ;; defface spec entirely rather than inheriting from it, with the
> > +    ;; exception of the :extend attribute (which is inherited).
>
> Please keep the original wording by including "rather than inheriting
> from it" in parentheses.

Like this?

  defface spec entirely (rather than inheriting from it), with the
  exception of the :extend attribute (which is inherited).