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

Previous Next

Full log

View this message in rfc822 format

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 37774 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: bug#37774: 27.0.50; new :extend attribute broke visuals of all themes and other packages
Date: Mon, 9 Dec 2019 16:07:54 +0200

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.