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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 8 Dec 2019 23:20:52 +0200
> 
> > OK, I've now reviewed all the callers of face-spec-recalc, and all of
> > its callers' callers, and wrote a bunch of tests to make sure that we
> > don't break anything (or at least anything important).
> 
> Thank you. That's pretty comprehensive. I would suggest to install those 
> tests

Done.

> 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?

> 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?  They are normally loaded
as .elc, which doesn't load diff-mode, and load-theme doesn't activate
diff-mode either.  In any case, the tests as committed all pass for
me, including interactively.

But it would be safer to replace the faces with ediff-* faces, I
agree.

> 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.

> > The tests in
> > the patch below all pass for the current code on master, and include a
> > couple of comments where the changes to implicitly inherit :extend by
> > themes are supposed to change the expected result.  If after applying
> > your patch all the tests still pass, both in -batch and in an
> > interactive session, then I think we are good to go (after adding the
> > necessary documentation and NEWS entry).
> 
> They do! If by "still pass" you mean the version of these tests where 
> the expected values are replaced with the values from "should be" comments.

Yes.

> All right, how does the attached patch look?

Looks good, see below.

> 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.

> 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"?

> +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.

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.

>                         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."

> -    ;; 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.

Thanks.