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

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

[Andrey Orst]

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

From: Andrey Orst <andreyorst@gmail.com>
To: bug-gnu-emacs@gnu.org
Subject: 27.0.50; new :extend attribute broke visuals of all themes and
 other packages
--text follows this line--

Somewhat last checkout from master brought the change of face
attributes, adding new `:extend` attribute, which make all themes, and
packages like Magit display weirdly.  By this I mean that before the
change, some faces were set up to extend highlighting beyond EOL, but
now all of those faces are not doing this.  I've first reported this to
the theme package I'm using:
https://github.com/hlissner/emacs-doom-themes/issues/342 but I think
that this should be handled by emacs itself, because if not it will
result in the duplicated or extra code in themes fro different Emacs
versions.  This reddit post has some screenshots of what I mean:
https://www.reddit.com/r/emacs/comments/diahh1/emacs_27_update_changed_how_highlighted_lines/

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.12,
cairo version 1.17.3)
 of 2019-10-15 built on v5-572g
Repository revision: 6ac99ebb3f623c64379f5c6811f1cdeb6ecac7da
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12005000
System Description: Arch Linux

Recent messages:
Loading /home/andreyorst/.emacs.d/custom.el (source)...done
Loading /home/andreyorst/.emacs.d/.disabled.el (source)...done
Turning on magit-auto-revert-mode...done
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --mandir=/usr/share/man --with-gameuser=:games
 --with-sound=alsa --with-modules --without-gconf --without-gsettings
 --enable-link-time-optimization --with-x-toolkit=gtk3 --without-xaw3d
 --without-m17n-flt --with-cairo --without-compress-install
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -flto=jobserver
 -fuse-linker-plugin -fuse-ld=gold' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GLIB NOTIFY INOTIFY ACL
GNUTLS LIBXML2 FREETYPE HARFBUZZ LIBOTF ZLIB TOOLKIT_SCROLL_BARS GTK3
X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Treemacs

Minor modes in effect:
  eldoc-box-hover-at-point-mode: t
  global-tab-line-mode: t
  company-quickhelp-mode: t
  company-quickhelp-local-mode: t
  company-flx-mode: t
  global-company-mode: t
  gcmh-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  ivy-mode: t
  minions-mode: t
  eyebrowse-mode: t
  global-magit-file-mode: t
  magit-auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  treemacs-filewatch-mode: t
  treemacs-follow-mode: t
  treemacs-git-mode: deferred
  treemacs-fringe-indicator-mode: t
  hl-line-mode: t
  doom-modeline-mode: t
  solaire-global-mode: t
  override-global-mode: t
  savehist-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  window-divider-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug sendmail counsel xdg swiper vc-git
eldoc-box face-remap tab-line company-files company-capf
company-quickhelp pos-tip company-flx company init vlf-setup gcmh ediff
ediff-merg ediff-mult ediff-wind ediff-diff ediff-help ediff-init
ediff-util undo-tree ivy flx delsel colir color ivy-overlay flymake-proc
flymake compile warnings minions eyebrowse treemacs-magit
magit-submodule magit-obsolete magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func magit-diff
smerge-mode diff diff-mode magit-core magit-autorevert autorevert
magit-margin magit-transient magit-process magit-mode transient
git-commit magit-git magit-section magit-utils crm log-edit message rmc
puny format-spec rfc822 mml mml-sec epa derived epg epg-config gnus-util
rmail rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr mailabbrev mail-utils gmm-utils mailheader pcvs-util add-log
with-editor async-bytecomp async shell pcomplete comint ansi-color
server treemacs treemacs-compatibility treemacs-mode treemacs-interface
treemacs-extensions treemacs-persistence treemacs-mouse-interface
treemacs-tag-follow-mode hydra lv treemacs-filewatch-mode treemacs-tags
imenu xref project filenotify treemacs-follow-mode treemacs-rendering
treemacs-async treemacs-faces treemacs-icons treemacs-workspaces
treemacs-dom treemacs-visuals treemacs-fringe-indicator pulse
treemacs-themes treemacs-core-utils pfuture ace-window avy ring hl-line
treemacs-macros pcase inline ht treemacs-customization doom-modeline
doom-modeline-segments doom-modeline-env doom-modeline-core shrink-path
f s dash solaire-mode disp-table doom-themes-ext-org doom-one-theme
doom-themes doom-themes-base all-the-icons all-the-icons-faces
data-material data-weathericons data-octicons data-fileicons
data-faicons data-alltheicons memoize cmake-mode thingatpt rx toml-mode
conf-mode align display-line-numbers doc-view jka-compr image-mode exif
dired dired-loaddefs cl-extra help-mode use-package use-package-ensure
use-package-delight use-package-diminish use-package-bind-key bind-key
easy-mmode use-package-core finder-inf info package easymenu browse-url
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv savehist advice edmacro kmacro cl-loaddefs
cl-lib tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting font-render-setting cairo move-toolbar gtk
x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 382073 282221)
 (symbols 48 27088 10)
 (strings 32 116208 28128)
 (string-bytes 1 3260176)
 (vectors 16 44709)
 (vector-slots 8 729458 136124)
 (floats 8 818 1234)
 (intervals 56 706 639)
 (buffers 1000 13))

[-- Attachment #2: Type: text/html, Size: 8185 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Wed, 16 Oct 2019 10:00:38 +0300
> 
> Somewhat last checkout from master brought the change of face
> attributes, adding new `:extend` attribute, which make all themes, and
> packages like Magit display weirdly.  By this I mean that before the
> change, some faces were set up to extend highlighting beyond EOL, but
> now all of those faces are not doing this.  I've first reported this to
> the theme package I'm using:
> https://github.com/hlissner/emacs-doom-themes/issues/342 but I think
> that this should be handled by emacs itself, because if not it will
> result in the duplicated or extra code in themes fro different Emacs
> versions.  This reddit post has some screenshots of what I mean:
> https://www.reddit.com/r/emacs/comments/diahh1/emacs_27_update_changed_how_highlighted_lines/

The screenshots you posted don't clearly explain the problem.  Some of
them seem actually identical before and after the change, and with
others I don't think I see the problem.

So please explain what exactly is incorrect or "weird" in the visual
appearance after the change.  Specifically, why the faces in question
need to be extended past EOL?

Thanks.

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

[Andrey Orst]

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

these faces are forming visual interface, e.g. hunks in Magit are
rectangular regions with background color for entire width of the window
that can be folded. Code blocks in org mode are, ahem, blocks. Now those
blocks doesn't have anything like block visually.

On Wed, Oct 16, 2019, 10:53 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Andrey Orst <andreyorst@gmail.com>
> > Date: Wed, 16 Oct 2019 10:00:38 +0300
> >
> > Somewhat last checkout from master brought the change of face
> > attributes, adding new `:extend` attribute, which make all themes, and
> > packages like Magit display weirdly.  By this I mean that before the
> > change, some faces were set up to extend highlighting beyond EOL, but
> > now all of those faces are not doing this.  I've first reported this to
> > the theme package I'm using:
> > https://github.com/hlissner/emacs-doom-themes/issues/342 but I think
> > that this should be handled by emacs itself, because if not it will
> > result in the duplicated or extra code in themes fro different Emacs
> > versions.  This reddit post has some screenshots of what I mean:
> >
> https://www.reddit.com/r/emacs/comments/diahh1/emacs_27_update_changed_how_highlighted_lines/
>
> The screenshots you posted don't clearly explain the problem.  Some of
> them seem actually identical before and after the change, and with
> others I don't think I see the problem.
>
> So please explain what exactly is incorrect or "weird" in the visual
> appearance after the change.  Specifically, why the faces in question
> need to be extended past EOL?
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 2346 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Wed, 16 Oct 2019 11:12:44 +0300
> Cc: 37774@debbugs.gnu.org, Ergus <spacibba@aol.com>
> 
> these faces are forming visual interface, e.g. hunks in Magit are rectangular regions with background color for
> entire width of the window that can be folded. Code blocks in org mode are, ahem, blocks. Now those blocks
> doesn't have anything like block visually.

So you are saying that you don't like the new appearance?  The Subject
says "broke visuals", which sounds like a much more serious problem.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> So you are saying that you don't like the new appearance?  The Subject
> says "broke visuals", which sounds like a much more serious problem.

It's not a question of aesthetics, but of usability.  For example, the
third party magit gets harder to use if the faces aren't adapted to
use the new extend property.

I don't know if there is any benefit to the new behaviour.  Is it just
stylistic?

Best regards,
Stefan Kangas

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 16 Oct 2019 16:20:46 +0200
> Cc: Andrey Orst <andreyorst@gmail.com>, Ergus <spacibba@aol.com>, 37774@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So you are saying that you don't like the new appearance?  The Subject
> > says "broke visuals", which sounds like a much more serious problem.
> 
> It's not a question of aesthetics, but of usability.  For example, the
> third party magit gets harder to use if the faces aren't adapted to
> use the new extend property.

Can you tell the details about "harder"?  (I don't use Magit.)

> I don't know if there is any benefit to the new behaviour.  Is it just
> stylistic?

It was extensively discussed on the emacs-devel list prior to
implementation, and the motivation was given there.  I recommend
reading those discussions.

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

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> So please explain what exactly is incorrect or "weird" in the visual
> appearance after the change.  Specifically, why the faces in question
> need to be extended past EOL?

At least with Helm, Magit and Ediff higlighting often looks very
unfamiliar.  I guess very often the original purpose had been to
highlight full visible lines.  Was it intentional to change all of that,
or only a side effect?


Michael.

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

[martin rudalics]

 > At least with Helm, Magit and Ediff higlighting often looks very
 > unfamiliar.  I guess very often the original purpose had been to
 > highlight full visible lines.  Was it intentional to change all of that,
 > or only a side effect?

A side-effect.

martin

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

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Andrey Orst <andreyorst@gmail.com>,  Ergus <spacibba@aol.com>,
>   37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 10:59:51 +0200
> 
> At least with Helm, Magit and Ediff higlighting often looks very
> unfamiliar.  I guess very often the original purpose had been to
> highlight full visible lines.  Was it intentional to change all of that,
> or only a side effect?

It was intentional, meaning the assumption was that extending the face
past the last character on the line makes little sense in general.
IOW, preventing the extension for most faces was the main point of the
change.

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

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> It was intentional, meaning the assumption was that extending the face
> past the last character on the line makes little sense in general.
> IOW, preventing the extension for most faces was the main point of the
> change.

Ok.  But it obviously created a lot of fallout to fix.

Michael.

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

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: andreyorst@gmail.com,  spacibba@aol.com,  37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 13:38:43 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It was intentional, meaning the assumption was that extending the face
> > past the last character on the line makes little sense in general.
> > IOW, preventing the extension for most faces was the main point of the
> > change.
> 
> Ok.  But it obviously created a lot of fallout to fix.

Not clear yet, at least not to me.  But it's possible.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Eli and Martin:

I have seen these reports and also the ones in reddit. Do you think that
we should/must/can do anything about?



On Wed, Oct 16, 2019 at 10:53:21AM +0300, Eli Zaretskii wrote:
>> From: Andrey Orst <andreyorst@gmail.com>
>> Date: Wed, 16 Oct 2019 10:00:38 +0300
>>
>> Somewhat last checkout from master brought the change of face
>> attributes, adding new `:extend` attribute, which make all themes, and
>> packages like Magit display weirdly.  By this I mean that before the
>> change, some faces were set up to extend highlighting beyond EOL, but
>> now all of those faces are not doing this.  I've first reported this to
>> the theme package I'm using:
>> https://github.com/hlissner/emacs-doom-themes/issues/342 but I think
>> that this should be handled by emacs itself, because if not it will
>> result in the duplicated or extra code in themes fro different Emacs
>> versions.  This reddit post has some screenshots of what I mean:
>> https://www.reddit.com/r/emacs/comments/diahh1/emacs_27_update_changed_how_highlighted_lines/
>
>The screenshots you posted don't clearly explain the problem.  Some of
>them seem actually identical before and after the change, and with
>others I don't think I see the problem.
>
>So please explain what exactly is incorrect or "weird" in the visual
>appearance after the change.  Specifically, why the faces in question
>need to be extended past EOL?
>
>Thanks.

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

[Andrey Orst]

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

> So you are saying that you don't like the new appearance?  The Subject
> says "broke visuals", which sounds like a much more serious problem.

Well, "broke" may be wrong term, here, but lot of themes and packages
crafted
in a way to display things like that, and now all of those things displayed
accordingly
to a new setting, which in turn means that:

a) package maintainers should update *all* their packages to look like
before the change, and
b) maybe Emacs could treat `nil` here as "do not affect", and specify
symbols to set this to different
   settings, like `:extend t` or `:extend 'EOL`, and `:extend 'noextend` to
disable. Though, I do not
   know how code was changed, so maybe there's no way to treat `nil` as "do
not affect".

On Wed, Oct 16, 2019 at 2:10 PM Ergus <spacibba@aol.com> wrote:

> Hi Eli and Martin:
>
> I have seen these reports and also the ones in reddit. Do you think that
> we should/must/can do anything about?
>
>
>
> On Wed, Oct 16, 2019 at 10:53:21AM +0300, Eli Zaretskii wrote:
> >> From: Andrey Orst <andreyorst@gmail.com>
> >> Date: Wed, 16 Oct 2019 10:00:38 +0300
> >>
> >> Somewhat last checkout from master brought the change of face
> >> attributes, adding new `:extend` attribute, which make all themes, and
> >> packages like Magit display weirdly.  By this I mean that before the
> >> change, some faces were set up to extend highlighting beyond EOL, but
> >> now all of those faces are not doing this.  I've first reported this to
> >> the theme package I'm using:
> >> https://github.com/hlissner/emacs-doom-themes/issues/342 but I think
> >> that this should be handled by emacs itself, because if not it will
> >> result in the duplicated or extra code in themes fro different Emacs
> >> versions.  This reddit post has some screenshots of what I mean:
> >>
> https://www.reddit.com/r/emacs/comments/diahh1/emacs_27_update_changed_how_highlighted_lines/
> >
> >The screenshots you posted don't clearly explain the problem.  Some of
> >them seem actually identical before and after the change, and with
> >others I don't think I see the problem.
> >
> >So please explain what exactly is incorrect or "weird" in the visual
> >appearance after the change.  Specifically, why the faces in question
> >need to be extended past EOL?
> >
> >Thanks.
>


-- 
Best regards,
Andrey Listopadov

[-- Attachment #2: Type: text/html, Size: 3572 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Wed, 16 Oct 2019 14:17:27 +0300
> Cc: Eli Zaretskii <eliz@gnu.org>, 37774@debbugs.gnu.org
> 
> > So you are saying that you don't like the new appearance?  The Subject
> > says "broke visuals", which sounds like a much more serious problem.
> 
> Well, "broke" may be wrong term, here, but lot of themes and packages crafted
> in a way to display things like that, and now all of those things displayed accordingly
> to a new setting, which in turn means that:
> 
> a) package maintainers should update *all* their packages to look like before the change, and

Are you saying that _all_ the faces will have to be modified to make
them extended?  IOW, are you saying that this feature is wrong with
most or all of the faces?

The assumption behind this feature was that the absolute majority of
faces don't need to be extended.  If you say this is wrong, can you
show enough examples to back up that?

> b) maybe Emacs could treat `nil` here as "do not affect", and specify symbols to set this to different
>    settings, like `:extend t` or `:extend 'EOL`, and `:extend 'noextend` to disable. Though, I do not
>    know how code was changed, so maybe there's no way to treat `nil` as "do not affect".

Let's first find out how many faces would need to be modified to adapt
to this feature, and only after that discuss the details of the
solution(s).

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

[Andrey Orst]

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

> Are you saying that _all_ the faces will have to be modified to make
> them extended?  IOW, are you saying that this feature is wrong with
> most or all of the faces?

I don't know about /all/ faces, but I have experienced a lot of visual
changes
when using `doom-one' theme provided by `doom-themes' package paired
with at least these packages: magit, ediff, solaire-mode, org-mode.

> The assumption behind this feature was that the absolute majority of
> faces don't need to be extended.  If you say this is wrong, can you
> show enough examples to back up that?

I understand this, and maybe package maintainers should adopt the change
but since Emacs doesn't ignore unknown attributes, this may result in a lot
of
extra code in order to support both pre-27 Emacs, and 27+ Emacs to make
different versions look consistently.

On Wed, Oct 16, 2019 at 2:41 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Andrey Orst <andreyorst@gmail.com>
> > Date: Wed, 16 Oct 2019 14:17:27 +0300
> > Cc: Eli Zaretskii <eliz@gnu.org>, 37774@debbugs.gnu.org
> >
> > > So you are saying that you don't like the new appearance?  The Subject
> > > says "broke visuals", which sounds like a much more serious problem.
> >
> > Well, "broke" may be wrong term, here, but lot of themes and packages
> crafted
> > in a way to display things like that, and now all of those things
> displayed accordingly
> > to a new setting, which in turn means that:
> >
> > a) package maintainers should update *all* their packages to look like
> before the change, and
>
> Are you saying that _all_ the faces will have to be modified to make
> them extended?  IOW, are you saying that this feature is wrong with
> most or all of the faces?
>
> The assumption behind this feature was that the absolute majority of
> faces don't need to be extended.  If you say this is wrong, can you
> show enough examples to back up that?
>
> > b) maybe Emacs could treat `nil` here as "do not affect", and specify
> symbols to set this to different
> >    settings, like `:extend t` or `:extend 'EOL`, and `:extend 'noextend`
> to disable. Though, I do not
> >    know how code was changed, so maybe there's no way to treat `nil` as
> "do not affect".
>
> Let's first find out how many faces would need to be modified to adapt
> to this feature, and only after that discuss the details of the
> solution(s).
>


-- 
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 3434 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Wed, 16 Oct 2019 15:08:53 +0300
> Cc: 37774@debbugs.gnu.org
> 
> > The assumption behind this feature was that the absolute majority of
> > faces don't need to be extended.  If you say this is wrong, can you
> > show enough examples to back up that?
> 
> I understand this, and maybe package maintainers should adopt the change

The assumption was that they won't need to adapt, because users will
want most faces should not to be extended.  If indeed many faces need
adaptation, then I do see a problem of backward compatibility; but
that wasn't the assumption.

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

[Eli Zaretskii]

> Date: Wed, 16 Oct 2019 13:10:04 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Andrey Orst <andreyorst@gmail.com>, 37774@debbugs.gnu.org
> 
> I have seen these reports and also the ones in reddit. Do you think that
> we should/must/can do anything about?

Maybe, I'm not yet sure I understand the magnitude of the problem.
Let's see where this discussion leads us.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Yes, internally it won't produce any issue (I am using it since I
implemented it the first time), but some external packages will need to
update some of their faces...; and others could remove some of the hacks
they implemented to emulate the functionality this change provides.

Do we have something in defface that we can "recommend" to conditionally
specify this attribute when version >= 27 only (maybe a syntax sugar)?

In any case maybe we need to add a recommendation in NEWS about how to
update.

On Wed, Oct 16, 2019 at 02:42:09PM +0300, Eli Zaretskii wrote:
>> Date: Wed, 16 Oct 2019 13:10:04 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: Andrey Orst <andreyorst@gmail.com>, 37774@debbugs.gnu.org
>>
>> I have seen these reports and also the ones in reddit. Do you think that
>> we should/must/can do anything about?
>
>Maybe, I'm not yet sure I understand the magnitude of the problem.
>Let's see where this discussion leads us.

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

[Andrey Orst]

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

> In any case maybe we need to add a recommendation in NEWS about how to
> update.

This would be nice to have, because some old packages may not receive
updates
and users would have to deal with it.

On Wed, Oct 16, 2019 at 4:18 PM Ergus <spacibba@aol.com> wrote:
>
> Yes, internally it won't produce any issue (I am using it since I
> implemented it the first time), but some external packages will need to
> update some of their faces...; and others could remove some of the hacks
> they implemented to emulate the functionality this change provides.
>
> Do we have something in defface that we can "recommend" to conditionally
> specify this attribute when version >= 27 only (maybe a syntax sugar)?
>
> In any case maybe we need to add a recommendation in NEWS about how to
> update.
>
> On Wed, Oct 16, 2019 at 02:42:09PM +0300, Eli Zaretskii wrote:
> >> Date: Wed, 16 Oct 2019 13:10:04 +0200
> >> From: Ergus <spacibba@aol.com>
> >> Cc: Andrey Orst <andreyorst@gmail.com>, 37774@debbugs.gnu.org
> >>
> >> I have seen these reports and also the ones in reddit. Do you think
that
> >> we should/must/can do anything about?
> >
> >Maybe, I'm not yet sure I understand the magnitude of the problem.
> >Let's see where this discussion leads us.



--
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 1731 bytes --]

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wed, Oct 16, 2019 at 04:33:08PM +0300, Andrey Orst wrote:
>> In any case maybe we need to add a recommendation in NEWS about how to
>> update.
>
>This would be nice to have, because some old packages may not receive
>updates
>and users would have to deal with it.
>

In any case I have been looking around and this is not really critical
cause only few active/extended packages will be directly affected for
this now. If there are some inactive-old-unmaintained package you know
will be affected and it is probably unmaintained, then probably we can
contact the author, make a pull request or in the best case, recommend
other with same functionality, but more active-maintained-supported.

Magit counsel-swiper and Helm, for example, I am pretty sure they will
receive the update immediately once we publish the recommended way to do
so without breaking backward compatibility.



>On Wed, Oct 16, 2019 at 4:18 PM Ergus <spacibba@aol.com> wrote:
>>
>> Yes, internally it won't produce any issue (I am using it since I
>> implemented it the first time), but some external packages will need to
>> update some of their faces...; and others could remove some of the hacks
>> they implemented to emulate the functionality this change provides.
>>
>> Do we have something in defface that we can "recommend" to conditionally
>> specify this attribute when version >= 27 only (maybe a syntax sugar)?
>>
>> In any case maybe we need to add a recommendation in NEWS about how to
>> update.
>>
>> On Wed, Oct 16, 2019 at 02:42:09PM +0300, Eli Zaretskii wrote:
>> >> Date: Wed, 16 Oct 2019 13:10:04 +0200
>> >> From: Ergus <spacibba@aol.com>
>> >> Cc: Andrey Orst <andreyorst@gmail.com>, 37774@debbugs.gnu.org
>> >>
>> >> I have seen these reports and also the ones in reddit. Do you think
>that
>> >> we should/must/can do anything about?
>> >
>> >Maybe, I'm not yet sure I understand the magnitude of the problem.
>> >Let's see where this discussion leads us.
>
>
>
>--
>Best regards,
>Andrey Orst

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

[Eli Zaretskii]

> Date: Wed, 16 Oct 2019 15:18:13 +0200
> From: Ergus <spacibba@aol.com>
> Cc: andreyorst@gmail.com, 37774@debbugs.gnu.org
> 
> Do we have something in defface that we can "recommend" to conditionally
> specify this attribute when version >= 27 only (maybe a syntax sugar)?

(if (>= emacs-major-version 27)
    (defface foo...) ; for Emacs 27 and later
  (defface foo...)   ; for Emacs 26 and older

> In any case maybe we need to add a recommendation in NEWS about how to
> update.

Do you really think the above needs to be in NEWS?

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

[Andrey Orst]

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

> (if (>= emacs-major-version 27)
>     (defface foo...) ; for Emacs 27 and later
>   (defface foo...)   ; for Emacs 26 and older

This results in lots of code duplication. Maybe it's better to:

(defface foo...)
(when (>= emacs-major-version 27)
  (set-face-attribute foo... :extend t))

I'm not elisp expert by any means.
-- 
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 737 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Wed, 16 Oct 2019 21:13:23 +0300
> Cc: Ergus <spacibba@aol.com>, 37774@debbugs.gnu.org
> 
> (defface foo...)
> (when (>= emacs-major-version 27)
>   (set-face-attribute foo... :extend t))

Yes, that could work as well.

My pint is that making defface version-dependent is very easy.

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

[Kévin Le Gouguec]

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

Andrey Orst <andreyorst@gmail.com> writes:

> (defface foo...)
> (when (>= emacs-major-version 27)
>   (set-face-attribute foo... :extend t))

Unless I'm mistaken, this has the disadvantage of making Custom confused
as to who changed the face, i.e. M-x customize-face RET foo... RET will
say that the face was "CHANGED outside Customize".  For all intents and
purposes a package author may wish the face to be described as
"STANDARD" as long as no theming or user customization has been applied.

Maybe some backquote trickery may help?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: magit-extend-demo.patch --]
[-- Type: text/x-patch, Size: 666 bytes --]

--- magit-diff.el.bkp	2019-10-17 20:29:21.771892709 +0200
+++ magit-diff.el	2019-10-17 20:53:47.927829447 +0200
@@ -509,12 +509,14 @@
   :group 'magit-faces)
 
 (defface magit-diff-hunk-heading
-  '((((class color) (background light))
+  `((((class color) (background light))
      :background "grey80"
-     :foreground "grey30")
+     :foreground "grey30"
+     ,@(unless (version<= emacs-version "27") '(:extend t)))
     (((class color) (background dark))
      :background "grey25"
-     :foreground "grey70"))
+     :foreground "grey70"
+     ,@(unless (version<= emacs-version "27") '(:extend t))))
   "Face for diff hunk headings."
   :group 'magit-faces)
 

[-- Attachment #3: Type: text/plain, Size: 212 bytes --]


(I did not look for a way to factor out the (:extend t) into a leading
(default …) clause, but there ought to be a way to do it.)

This method should be usable by theme authors too, crufty as it looks…

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

[Kévin Le Gouguec]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> +     ,@(unless (version<= emacs-version "27") '(:extend t)))

(I really meant version< here, although version<= works because 27.0.50
is greater than 27)

(It might be less confusing to spell it as
    ,@(when (>= emacs-major-version 27) '(:extend t))
…)

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Do we have something in defface that we can "recommend" to conditionally
>> specify this attribute when version >= 27 only (maybe a syntax sugar)?
>
> (if (>= emacs-major-version 27)
>     (defface foo...) ; for Emacs 27 and later
>   (defface foo...)   ; for Emacs 26 and older

I believe it is more complex. There might be situations foo shall not
extend to EOL, and other situations it should.

The latter case is when foo is used in a "rectangular" context as
explained. You need to introduce a second face foo-extend, and you must
replace all uses of foo by foo-extend where needed.

Best regards, Michael.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Thu, Oct 17, 2019 at 01:36:26PM +0200, Michael Albinus wrote:
>Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Do we have something in defface that we can "recommend" to conditionally
>>> specify this attribute when version >= 27 only (maybe a syntax sugar)?
>>
>> (if (>= emacs-major-version 27)
>>     (defface foo...) ; for Emacs 27 and later
>>   (defface foo...)   ; for Emacs 26 and older
>
>I believe it is more complex. There might be situations foo shall not
>extend to EOL, and other situations it should.
>
>The latter case is when foo is used in a "rectangular" context as
>explained. You need to introduce a second face foo-extend, and you must
>replace all uses of foo by foo-extend where needed.
>
>Best regards, Michael.

Sorry, what do you mean by "rectangular" context?

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Ergus <spacibba@aol.com>,  andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Thu, 17 Oct 2019 13:36:26 +0200
> 
> I believe it is more complex. There might be situations foo shall not
> extend to EOL, and other situations it should.
> 
> The latter case is when foo is used in a "rectangular" context as
> explained.

What is a "rectangular context"?  I'm not sure I understand the exact
meaning of that.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> I believe it is more complex. There might be situations foo shall not
>> extend to EOL, and other situations it should.
>>
>> The latter case is when foo is used in a "rectangular" context as
>> explained.
>
> What is a "rectangular context"?  I'm not sure I understand the exact
> meaning of that.

I mean the use cases shown by Andrey Orst. Maybe "rectangular context"
is wrong wording; I thought it is obvious what's meant.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: andreyorst@gmail.com,  spacibba@aol.com,  37774@debbugs.gnu.org
> Date: Thu, 17 Oct 2019 18:18:23 +0200
> 
> > What is a "rectangular context"?  I'm not sure I understand the exact
> > meaning of that.
> 
> I mean the use cases shown by Andrey Orst. Maybe "rectangular context"
> is wrong wording; I thought it is obvious what's meant.

Ah, okay.  I thought you had more to add to that.

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

[Juri Linkov]

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

> I have seen these reports and also the ones in reddit. Do you think that
> we should/must/can do anything about?

Two major problems:

1. Backward-compatibility problem:

I had to spend significant time investigating why the region face broke
recently, and discovered that customized faces in custom-set-faces need
to be updated.  Soon I tired fixing their customizations one by one manually,
so I wrote a function that automatically fixes all faces.  I wonder
how all other users are supposed to get out of a similar situation.

Moreover, the problem is wider than personal customization
and affects hundreds of existing themes.

2. Conceptual problem:

We need to think again what this change was intended to fix?

All faces could be divided into two more-less equally large groups:

a. faces with distinct foreground that highlight text properties,
they include mostly font-lock faces, underline faces, and so on;

b. faces with distinct background that highlight blocks of text,
such as the region face, diff hunk faces, etc.

As I see the change was meant to fix only the problem that relates to
faces with distinct foreground, because indeed underlines extended
to the window edge look very ugly.  So the change should affect
only faces with distinct foreground.

But faces for multi-line regions with a distinct background color
require to look like rectangular blocks.

This screenshot demonstrates how badly broken these blocks are now
in diff-mode that it makes harder to read diffs:


[-- Attachment #2: diff-extend-eol.png --]
[-- Type: image/png, Size: 57666 bytes --]

[-- Attachment #3: Type: text/plain, Size: 76 bytes --]


And this shows how they looked like rectangular blocks before the change:


[-- Attachment #4: diff-extend-edge.png --]
[-- Type: image/png, Size: 57641 bytes --]

[-- Attachment #5: Type: text/plain, Size: 383 bytes --]


Frankly speaking, this is not great too because long stretches are ugly.
Ideally to be more nice-looking, background colors in such faces should be
extended to the column defined e.g. by display-fill-column-indicator-column.

Here's an edited image to demonstrate an ideal look of such background faces
(note how the mode-line is wider, and blocks are narrower than window width):


[-- Attachment #6: diff-extend-column.png --]
[-- Type: image/png, Size: 57683 bytes --]

[-- Attachment #7: Type: text/plain, Size: 208 bytes --]


So what would pacify the current situation is to extend to eol
only foreground colors.  But background colors should be extended to
some predefined fixed column such as fill-column to have a look of blocks.

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

[Andrey Orst]

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

> Here's an edited image to demonstrate an ideal look of such background
faces
> (note how the mode-line is wider, and blocks are narrower than window
width):

I like the unedited example a bit more, especially if we're talking about
side by side diffs, like ediff.
I think that this may be controlled by a variable named something like
`trim-eol-face-at-fill-column'

On Wed, Oct 16, 2019 at 8:36 PM Juri Linkov <juri@linkov.net> wrote:
>
> > I have seen these reports and also the ones in reddit. Do you think that
> > we should/must/can do anything about?
>
> Two major problems:
>
> 1. Backward-compatibility problem:
>
> I had to spend significant time investigating why the region face broke
> recently, and discovered that customized faces in custom-set-faces need
> to be updated.  Soon I tired fixing their customizations one by one
manually,
> so I wrote a function that automatically fixes all faces.  I wonder
> how all other users are supposed to get out of a similar situation.
>
> Moreover, the problem is wider than personal customization
> and affects hundreds of existing themes.
>
> 2. Conceptual problem:
>
> We need to think again what this change was intended to fix?
>
> All faces could be divided into two more-less equally large groups:
>
> a. faces with distinct foreground that highlight text properties,
> they include mostly font-lock faces, underline faces, and so on;
>
> b. faces with distinct background that highlight blocks of text,
> such as the region face, diff hunk faces, etc.
>
> As I see the change was meant to fix only the problem that relates to
> faces with distinct foreground, because indeed underlines extended
> to the window edge look very ugly.  So the change should affect
> only faces with distinct foreground.
>
> But faces for multi-line regions with a distinct background color
> require to look like rectangular blocks.
>
> This screenshot demonstrates how badly broken these blocks are now
> in diff-mode that it makes harder to read diffs:
>
>
> And this shows how they looked like rectangular blocks before the change:
>
>
> Frankly speaking, this is not great too because long stretches are ugly.
> Ideally to be more nice-looking, background colors in such faces should be
> extended to the column defined e.g. by
display-fill-column-indicator-column.
>
> Here's an edited image to demonstrate an ideal look of such background
faces
> (note how the mode-line is wider, and blocks are narrower than window
width):
>
>
> So what would pacify the current situation is to extend to eol
> only foreground colors.  But background colors should be extended to
> some predefined fixed column such as fill-column to have a look of blocks.



--
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 3188 bytes --]

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

[Juri Linkov]

>> Here's an edited image to demonstrate an ideal look of such background faces
>> (note how the mode-line is wider, and blocks are narrower than window width):
>
> I like the unedited example a bit more, especially if we're talking about side by side diffs, like ediff.

I agree that in side by side ediffs, face background colors should
extend to the window edge, to better match diff colors between windows.

> I think that this may be controlled by a variable named something like `trim-eol-face-at-fill-column'

Or with an optional value for the face attribute, e.g. :extend 'column.
But this is an additional feature.  The main task is to get background colors back
to extend to the window edge.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 16 Oct 2019 21:18:06 +0300
> Cc: 37774@debbugs.gnu.org
> 
> The main task is to get background colors back to extend to the
> window edge.

If that's the main task, then the easiest way is just to revert all
the changes that introduced that feature.

(It's a pity that the long discussion of this before the development
started went without any such objections from the people who are
regulars on emacs-devel.)

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

[Juri Linkov]

>> The main task is to get background colors back to extend to the
>> window edge.
>
> If that's the main task, then the easiest way is just to revert all
> the changes that introduced that feature.

This is a useful feature, but for backward-compatibility perhaps
it should be optional, i.e. the meaning of ':extend t' could be
reversed, then we could find and fix all default faces where
this new feature is needed.

> (It's a pity that the long discussion of this before the development
> started went without any such objections from the people who are
> regulars on emacs-devel.)

I had no objections because I thought that the discussed feature is opt-in,
i.e. I thought that optional ':extend t' means extend to EOL.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 22:52:35 +0300
> 
> This is a useful feature, but for backward-compatibility perhaps
> it should be optional

We could do that, but then the feature will mostly make no sense.

> i.e. the meaning of ':extend t' could be reversed, then we could
> find and fix all default faces where this new feature is needed.

Based on what I saw until now, we will never be able to agree on what
faces need this.

> > (It's a pity that the long discussion of this before the development
> > started went without any such objections from the people who are
> > regulars on emacs-devel.)
> 
> I had no objections because I thought that the discussed feature is opt-in,

The main reason for the discussion was that the extension is annoying
and in most cases should be disabled.  That's how the discussion
started.  That's what other applications do.

> i.e. I thought that optional ':extend t' means extend to EOL.

That's exactly what was implemented.

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

[Eli Zaretskii]

> Date: Wed, 16 Oct 2019 21:54:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: andreyorst@gmail.com, 37774@debbugs.gnu.org
> 
> (It's a pity that the long discussion of this before the development
> started went without any such objections from the people who are
> regulars on emacs-devel.)

Btw, please take into account that what that change caused is that
Emacs now behaves like other applications in this regard.

I'm quite sure that at least some of the protests are caused by people
being accustomed to the previous display, and the stark difference
that the new display produces.  But if you are using other
applications, the new display should be very familiar to you.

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

[Juri Linkov]

>> (It's a pity that the long discussion of this before the development
>> started went without any such objections from the people who are
>> regulars on emacs-devel.)
>
> Btw, please take into account that what that change caused is that
> Emacs now behaves like other applications in this regard.

I don't know what applications behave the same.  I tried different
editors that I could find (namely LibreOffice Writer and xed)
and all they extend highlighting of the selected region
to the window right edge, not to EOL.

Also I looked how other applications extend diff blocks, and e.g.
GitLab extends diff background colors to the window right edge,
not to EOL, for example,
https://github.com/emacs-mirror/emacs/commit/3d6075e3ee8c447f8974b37007a1b1ae1af8917c

However, no other application extends underlines to the window edge
as Emacs used to do, this was a plain bug that this change fixed.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 23:14:22 +0300
> 
> >> (It's a pity that the long discussion of this before the development
> >> started went without any such objections from the people who are
> >> regulars on emacs-devel.)
> >
> > Btw, please take into account that what that change caused is that
> > Emacs now behaves like other applications in this regard.
> 
> I don't know what applications behave the same.

Word processors.

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

[Andrey Orst]

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

> Word processors.

Emacs is so much more than a word processor.

The thing is, that all your thoughts are seem correct if
we're speaking about text. Though,in Emacs there are many
packages that are used not for text editing, but for
interactions with different tools, and such change breaks
well established user interface. Examples were already
provided: Helm, Magit, Ediff, and there are also a lot of
user themes which provide more modern look to Emacs by using
this feature, which then affects packages that these themes
try to configure, e.g. doom-themes can configure Treemacs
package to look more like a true UI element rather than
buffer with text, that supposed to be interacted because it
just have some icons.

I do agree that this change is aimed to provide native way
of extending highlighting beyond EOL without relying on
hacks. And I think it's a good intention.I just disagree
with how it was forced in a way that it broke visuals of
many external packages that provide UI elements via
highlighting.

--
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 1188 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Thu, 17 Oct 2019 11:52:46 +0300
> Cc: 37774@debbugs.gnu.org
> 
> > Word processors.
> 
> Emacs is so much more than a word processor.
> 
> The thing is, that all your thoughts are seem correct if
> we're speaking about text. Though,in Emacs there are many
> packages that are used not for text editing, but for
> interactions with different tools, and such change breaks
> well established user interface. Examples were already
> provided: Helm, Magit, Ediff

I don't think I understand your point.  All the packages you mentioned
display text.

> I do agree that this change is aimed to provide native way
> of extending highlighting beyond EOL without relying on
> hacks. And I think it's a good intention.I just disagree
> with how it was forced in a way that it broke visuals of
> many external packages that provide UI elements via
> highlighting.

I still don't see why "broke visuals" is what it did.  I happen to
think that the new appearance is not worse, and sometimes better than
the old one.

And this feature was discussed at length before implementing, so it
isn't like it came out of the blue.

How about if you try using this feature for a week or so, and see if
you become accustomed to it nonetheless?

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

[Andrey Orst]

[-- Attachment #1.1: Type: text/plain, Size: 2130 bytes --]

> I don't think I understand your point.  All the packages you mentioned
> display text.

Everything in Emacs is text, let's not pretend that every buffer is
used for editing, because it's not.

Magit is an interface to Git. It displays visual information about
repository. It displays hunks. Hunks are text, but in Magit we're
supposed to interact with hunks as with interface items. We can fold
those and unfold hunks. We can stage and stash hunks. We can open hunk
in Ediff. I wonder what would you say if in some GUI app interface
suddenly becomes as long as the text in it and goes from block shape
to teared down cardboard shape.

Though here's an example why old behavior is better, and it's based on
other apps, since I see that you value this argument, by referring to
external word processors. Emacs has a built in merge tool: Ediff, and
it is also used for diffing buffers side by side, and it's way more
natural to see extended highlighting, as it is done in other merge
tools outside Emacs.

Meld:
[image: image.png]

Emacs:
[image: image.png]

> I still don't see why "broke visuals" is what it did.  I happen to
> think that the new appearance is not worse, and sometimes better than
> the old one.

Perhaps you're the only one who do not see it. The key words here are
/some times/, and trust me, I see it as /rare times/ and not as /most
of the times/.

> And this feature was discussed at length before implementing, so it
> isn't like it came out of the blue.

I'm a Emacs user, and I'm not associated with development by any
means. I just updated my Emacs, as I do every day, and spotted the
change that seem to look like a bug. I asked on the web, and was
suggested to post a bug report about it.

> How about if you try using this feature for a week or so, and see if
> you become accustomed to it nonetheless?

I was already using this feature for a while and see how wrong it is
for my workflow of using Ediff and Magit alone. There's also Org mode
that was themed in a way that I can see different sections separated
by beyond EOL highlighting and source blocks were blocks.

--
Best regards,
Andrey Orst

[-- Attachment #1.2: Type: text/html, Size: 2537 bytes --]

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

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

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Thu, 17 Oct 2019 12:20:36 +0300
> Cc: 37774@debbugs.gnu.org
> 
> Though here's an example why old behavior is better, and it's based on
> other apps, since I see that you value this argument, by referring to
> external word processors. Emacs has a built in merge tool: Ediff, and
> it is also used for diffing buffers side by side, and it's way more
> natural to see extended highlighting, as it is done in other merge
> tools outside Emacs.

What would happen in your Ediff example if the face for added lines
were customized to have a distinct foreground color, leaving the
background color at its default?  That's what "git diff" shows on a
terminal.

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

[Andrey Orst]

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

Git diff is an inline diff. It doesn't need changes to be aligned side by
side,
and doesn't need changes to be linked as in side by side diff tools like,
meld, and diff-viewers in Intellij Idea[1], VSCode[2], Sublime Merge[3],
Smartgit[4],

[1]
https://www.oreilly.com/library/view/intellij-idea-essentials/9781784396930/graphics/6930OT_03_44.jpg
[2] https://www.meziantou.net/assets/result1.png?v=2c344912
[3] https://edge.alluremedia.com.au/m/l/2018/09/smerge.png
[4]
https://www.syntevo.com/assets/images/products/smartgit/opener-481720cb.png

and even when we looking at inline diff,
most GUI tools use extended highlighters,
because it is easy to understand:

sublime merge: https://i.ytimg.com/vi/ZrdkEBJV660/maxresdefault.jpg
Magit: https://magit.vc/screenshots/commit-top.png

Consider looking at how magit supposed to look in official screenshots here,
and how it looks after the change.
https://emacsair.me/2017/09/01/magit-walk-through/
--
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 1668 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Thu, 17 Oct 2019 16:40:49 +0300
> Cc: 37774@debbugs.gnu.org
> 
> Git diff is an inline diff. It doesn't need changes to be aligned side by side,
> and doesn't need changes to be linked as in side by side diff tools like,
> meld, and diff-viewers in Intellij Idea[1], VSCode[2], Sublime Merge[3], Smartgit[4],
> 
> [1] https://www.oreilly.com/library/view/intellij-idea-essentials/9781784396930/graphics/6930OT_03_44.jpg
> [2] https://www.meziantou.net/assets/result1.png?v=2c344912
> [3] https://edge.alluremedia.com.au/m/l/2018/09/smerge.png
> [4] https://www.syntevo.com/assets/images/products/smartgit/opener-481720cb.png
> 
> and even when we looking at inline diff,
> most GUI tools use extended highlighters,
> because it is easy to understand:
> 
> sublime merge: https://i.ytimg.com/vi/ZrdkEBJV660/maxresdefault.jpg
> Magit: https://magit.vc/screenshots/commit-top.png
> 
> Consider looking at how magit supposed to look in official screenshots here,
> and how it looks after the change. https://emacsair.me/2017/09/01/magit-walk-through/

So maybe a few more faces need to be extended, at least optionally.
The issue that was brought up here was, AFAIU, that this feature
breaks many features so badly that it needs to be reverted or at least
turned off by default.  I have yet to see some sound arguments for
that.

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

[Andrey Orst]

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

> So maybe a few more faces need to be extended, at least optionally.
> The issue that was brought up here was, AFAIU, that this feature
> breaks many features so badly that it needs to be reverted or at least
> turned off by default.  I have yet to see some sound arguments for
> that.

Perhaps we need to see what developers of packages think about this change?

--
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 535 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Thu, 17 Oct 2019 17:13:00 +0300
> Cc: 37774@debbugs.gnu.org
> 
> > So maybe a few more faces need to be extended, at least optionally.
> > The issue that was brought up here was, AFAIU, that this feature
> > breaks many features so badly that it needs to be reverted or at least
> > turned off by default.  I have yet to see some sound arguments for
> > that.
> 
> Perhaps we need to see what developers of packages think about this change?

Everybody who has an opinion and arguments to back that up is invited
to speak up here.  TIA.

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

[Dmitry Gutov]

On 17.10.2019 11:59, Eli Zaretskii wrote:
> I still don't see why "broke visuals" is what it did.  I happen to
> think that the new appearance is not worse, and sometimes better than
> the old one.

I'm pretty sure that the claim that the new feature broke things is 
valid. It's my reaction as well, and just this morning I got a message 
from another long-time Emacs user (and a package author) asking if I had 
any idea what could have broken the region's display in the master build.

Given your conservative stance on many issues in the past, I'm frankly 
surprised.

In any case, while I haven't looked deep into this issue, I'm under 
impression that it should be possible to both retain the "rectangular" 
behavior many here are accustomed to, and fix the original pain point 
that brought this feature into existence.

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

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 17 Oct 2019 17:02:41 +0300
> 
> I'm pretty sure that the claim that the new feature broke things is 
> valid. It's my reaction as well, and just this morning I got a message 
> from another long-time Emacs user (and a package author) asking if I had 
> any idea what could have broken the region's display in the master build.

Thanks for offering your opinion, but could you please elaborate about
the breakage?

> Given your conservative stance on many issues in the past, I'm frankly 
> surprised.

It should perhaps tell you something important about the change.

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

[Dmitry Gutov]

On 17.10.2019 19:20, Eli Zaretskii wrote:
> Thanks for offering your opinion, but could you please elaborate about
> the breakage?

Same as mentioned by others: the newlines in a region are highlighted 
differently than before.

 > It should perhaps tell you something important about the change.

That is this case you're willing to weather the storm of complaints from 
users that dislike change, even purely visual like this one?

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 17 Oct 2019 19:46:16 +0300
> 
>  > It should perhaps tell you something important about the change.
> 
> That is this case you're willing to weather the storm of complaints from 
> users that dislike change, even purely visual like this one?

No, that I consider this change a no-brainer and not really a
significant change at all.

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

[martin rudalics]

 >> Btw, please take into account that what that change caused is that
 >> Emacs now behaves like other applications in this regard.
 >
 > I don't know what applications behave the same.  I tried different
 > editors that I could find (namely LibreOffice Writer and xed)
 > and all they extend highlighting of the selected region
 > to the window right edge, not to EOL.

I miss you here.  Emacs now by default also extends the region to the
right window edge.  OTOH both Mozilla and Thunderbird here extend the
region to EOL only.

 > Also I looked how other applications extend diff blocks, and e.g.
 > GitLab extends diff background colors to the window right edge,
 > not to EOL, for example,
 > https://github.com/emacs-mirror/emacs/commit/3d6075e3ee8c447f8974b37007a1b1ae1af8917c

With Firefox these diffs are boxed in a subarea of the Firefox window.
They do not start or extend at the window edges and text in these
boxes is static, can neither overflow into a newline nor be broken.

But I think that our (e)diff blocks are affected by the change and all
their face settings probably have to change, as well as tables and
listings.

martin

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

[Dmitry Gutov]

On 17.10.2019 11:25, martin rudalics wrote:
>  > Also I looked how other applications extend diff blocks, and e.g.
>  > GitLab extends diff background colors to the window right edge,
>  > not to EOL, for example,
>  > 
> https://github.com/emacs-mirror/emacs/commit/3d6075e3ee8c447f8974b37007a1b1ae1af8917c 
> 
> 
> With Firefox these diffs are boxed in a subarea of the Firefox window.
> They do not start or extend at the window edges and text in these
> boxes is static, can neither overflow into a newline nor be broken.

I think we can think of this "subarea" as directly corresponding to an 
Emacs window for the purposes of this discussion, when we think how 
diff-mode should display things.

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

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 17 Oct 2019 17:08:39 +0300
> 
> > With Firefox these diffs are boxed in a subarea of the Firefox window.
> > They do not start or extend at the window edges and text in these
> > boxes is static, can neither overflow into a newline nor be broken.
> 
> I think we can think of this "subarea" as directly corresponding to an 
> Emacs window for the purposes of this discussion, when we think how 
> diff-mode should display things.

If you select text in Firefox, what do you see? does it extend the
selection to the edge of the window?

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

[Dmitry Gutov]

On 17.10.2019 19:29, Eli Zaretskii wrote:
> If you select text in Firefox, what do you see? does it extend the
> selection to the edge of the window?

It does not.

After considering it a bit, I might like Firefox-like behavior for the 
region personally. Even so, it might be safer to only offer such option 
and not change it by default.

This example was about how diff-mode behavior did/should look, though.

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 17 Oct 2019 19:50:12 +0300
> 
> On 17.10.2019 19:29, Eli Zaretskii wrote:
> > If you select text in Firefox, what do you see? does it extend the
> > selection to the edge of the window?
> 
> It does not.
> 
> After considering it a bit, I might like Firefox-like behavior for the 
> region personally.

That's what happened to me as well.  So I think people who are
claiming it's a breaking change might try running with the change for
a week or so, perhaps they will change their minds.

> Even so, it might be safer to only offer such option and not change
> it by default.

We did, for the 'region' face.

> This example was about how diff-mode behavior did/should look, though.

We could consider individual faces for making them extend by default.
But there's a more general claim in this bug report: that the change
will screw many unbundled packages out there; if that is true,
changing some faces in core is not a solution.

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

[Dmitry Gutov]

On 17.10.2019 20:23, Eli Zaretskii wrote:

>> After considering it a bit, I might like Firefox-like behavior for the
>> region personally.
> 
> That's what happened to me as well.  So I think people who are
> claiming it's a breaking change might try running with the change for
> a week or so, perhaps they will change their minds.

FWIW my friend is adamant in his dislike, however. I'm sure there will 
be others.

But that's of no import, considering we'll make sure the 'region' face 
has the :extend property set to t even when a third-party theme is used, 
right?

>> Even so, it might be safer to only offer such option and not change
>> it by default.
> 
> We did, for the 'region' face.

Speaking of... it might be just my opinion, but it feels like whether a 
face background should extend to the edge of the screen is more of a 
structural quality, like a personal choice, and not something that 
themes (being color palettes) should define or redefine.

So maybe I would pick a different mechanism instead of a face attribute. 
E.g. just a property on the face's symbol name. Then it won't be 
affected by custom-set-faces either way.

Or another idea: split it into extend-foreground and extend-background. 
As someone remarked in this thread already, extend-foreground can safely 
default to nil, and we can set extend-background to t by default, for 
maximum backward compatibility.

>> This example was about how diff-mode behavior did/should look, though.
> 
> We could consider individual faces for making them extend by default.
> But there's a more general claim in this bug report: that the change
> will screw many unbundled packages out there; if that is true,
> changing some faces in core is not a solution.

Magit and Org will probably take the brunt of the change.

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

[Dmitry Gutov]

On 17.10.2019 20:23, Eli Zaretskii wrote:
> But there's a more general claim in this bug report: that the change
> will screw many unbundled packages out there; if that is true,
> changing some faces in core is not a solution.

Yeah, so: what's the plan for Magit?

Will the new version of it have to

(if <emacs version is...>
     (defface ...)
   (defface ...)

?

Essentially copying the definition of each affected face?

I'm not sure it can use set-face-attribute here, because if this 
attribute is allowed to be overridden by a theme, packages should 
respect that.

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 20 Oct 2019 23:07:03 +0300
> 
> On 17.10.2019 20:23, Eli Zaretskii wrote:
> > But there's a more general claim in this bug report: that the change
> > will screw many unbundled packages out there; if that is true,
> > changing some faces in core is not a solution.
> 
> Yeah, so: what's the plan for Magit?

I don't know, as I don't have a clear idea what faces there are
affected and why.  I hoped someone, preferably the Magit developers,
would describe that in enough detail to understand the situation.

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

[Dmitry Gutov]

On 21.10.2019 9:10, Eli Zaretskii wrote:

>> Yeah, so: what's the plan for Magit?
> 
> I don't know, as I don't have a clear idea what faces there are
> affected and why.  I hoped someone, preferably the Magit developers,
> would describe that in enough detail to understand the situation.

The list of faces has been posted here already:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#233

As apparent from their names, most of them are used in a Diff output 
buffer, similar to our diff-mode faces. With the same motivation to 
:extend them.

In anticipation of your next question: no, they don't inherit from any 
of the diff-mode faces.

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 23 Oct 2019 15:47:23 +0300
> 
> > I don't know, as I don't have a clear idea what faces there are
> > affected and why.  I hoped someone, preferably the Magit developers,
> > would describe that in enough detail to understand the situation.
> 
> The list of faces has been posted here already:
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#233

AFAIU, that's a list of faces one particular user decided to customize
to have them extended.  It's a far cry from the list of faces that
actually need to be extended, lest some important functionality will
suffer.  IOW, we need some rationale for each face, so that we could
consider that and decide whether or not to extend each one by default.

Besides, some of those in the list were already changed.

If too many faces in unbundled packages indeed need to change in that
way, we should consider additional measures.  That's why we need good
reasons for extending each face, not just "because they were before"
or because people were used to see them extended.

> As apparent from their names, most of them are used in a Diff output 
> buffer, similar to our diff-mode faces.

Most, but not all.  And I'm not yet convinced that every face with
"diff" in it must indeed be extended, we need to see examples of their
display, and we need to talk about that.

> In anticipation of your next question: no, they don't inherit from any 
> of the diff-mode faces.

I know.

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

[Dmitry Gutov]

On 23.10.2019 18:39, Eli Zaretskii wrote:

>>> I don't know, as I don't have a clear idea what faces there are
>>> affected and why.  I hoped someone, preferably the Magit developers,
>>> would describe that in enough detail to understand the situation.
>>
>> The list of faces has been posted here already:
>>
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#233
> 
> AFAIU, that's a list of faces one particular user decided to customize
> to have them extended.  It's a far cry from the list of faces that
> actually need to be extended, lest some important functionality will
> suffer.  IOW, we need some rationale for each face, so that we could
> consider that and decide whether or not to extend each one by default.

Magit's maintainer will decide for each face, sure.

But I don't really see much a difference between having 2 and 20 faces 
that will need to be updated, if it's within one package.

Even if it's just 2, do we have a recommended way to write their 
definitions in third-party packages in a way that's compatible with 
Emacs 26?

> If too many faces in unbundled packages indeed need to change in that
> way, we should consider additional measures.  That's why we need good
> reasons for extending each face, not just "because they were before"
> or because people were used to see them extended.

Those are not the worst reasons, though.

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 23 Oct 2019 19:12:26 +0300
> 
> >> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#233
> > 
> > AFAIU, that's a list of faces one particular user decided to customize
> > to have them extended.  It's a far cry from the list of faces that
> > actually need to be extended, lest some important functionality will
> > suffer.  IOW, we need some rationale for each face, so that we could
> > consider that and decide whether or not to extend each one by default.
> 
> Magit's maintainer will decide for each face, sure.

I don't mind if package maintainers want to make that decision by
themselves, but if that is the case, I don't think there's anything
left to do for this bug report?  I though some action will be required
from us, that's why I asked all those questions.

> But I don't really see much a difference between having 2 and 20 faces 
> that will need to be updated, if it's within one package.

It's a difference between a small number and a very large number.
Theoretically, someone could argue that a change that requires to
modify lots of faces shouldn't be so unconditional, or shouldn't be
the default, or should have a "fire escape", or something to that
effect.  But if people don't mind changing their faces, then such
fears have no basis, and we are good with what we have.

> Even if it's just 2, do we have a recommended way to write their 
> definitions in third-party packages in a way that's compatible with 
> Emacs 26?

The best way is to inherit from some suitable parent face, I think.

> > If too many faces in unbundled packages indeed need to change in that
> > way, we should consider additional measures.  That's why we need good
> > reasons for extending each face, not just "because they were before"
> > or because people were used to see them extended.
> 
> Those are not the worst reasons, though.

Not sure I understand in what sens did you use "the worst" here.

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

[Dmitry Gutov]

On 23.10.2019 21:04, Eli Zaretskii wrote:

>>> AFAIU, that's a list of faces one particular user decided to customize
>>> to have them extended.  It's a far cry from the list of faces that
>>> actually need to be extended, lest some important functionality will
>>> suffer.  IOW, we need some rationale for each face, so that we could
>>> consider that and decide whether or not to extend each one by default.
>>
>> Magit's maintainer will decide for each face, sure.
> 
> I don't mind if package maintainers want to make that decision by
> themselves, but if that is the case, I don't think there's anything
> left to do for this bug report?  I though some action will be required
> from us, that's why I asked all those questions.

We should define and document a "migration path", e.g. say what a 
package author should do if they have a face which needs to be extended, 
preferably without breaking compatibility with Emacs 26.

>> But I don't really see much a difference between having 2 and 20 faces
>> that will need to be updated, if it's within one package.
> 
> It's a difference between a small number and a very large number.
> Theoretically, someone could argue that a change that requires to
> modify lots of faces shouldn't be so unconditional, or shouldn't be
> the default, or should have a "fire escape", or something to that
> effect.  But if people don't mind changing their faces, then such
> fears have no basis, and we are good with what we have.

A "fire escape" would depend on a user's config, right? I don't like the 
sound of that approach, personally.

>> Even if it's just 2, do we have a recommended way to write their
>> definitions in third-party packages in a way that's compatible with
>> Emacs 26?
> 
> The best way is to inherit from some suitable parent face, I think.

A lot of face don't inherit from anything on purpose. Anyway, I've 
pinged Magit's maintainer, let's see what he says.

>>> If too many faces in unbundled packages indeed need to change in that
>>> way, we should consider additional measures.  That's why we need good
>>> reasons for extending each face, not just "because they were before"
>>> or because people were used to see them extended.
>>
>> Those are not the worst reasons, though.
> 
> Not sure I understand in what sens did you use "the worst" here.

"People were used to ..." is basically 99% of the arguments that were 
given in all past discussions for not changing defaults to be more 
"modern" or whatever. And there's some merit to that.

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 23 Oct 2019 23:28:50 +0300
> 
> > I don't mind if package maintainers want to make that decision by
> > themselves, but if that is the case, I don't think there's anything
> > left to do for this bug report?  I though some action will be required
> > from us, that's why I asked all those questions.
> 
> We should define and document a "migration path", e.g. say what a 
> package author should do if they have a face which needs to be extended, 
> preferably without breaking compatibility with Emacs 26.

We can do that in NEWS, if what's already there is not clear enough.

> > It's a difference between a small number and a very large number.
> > Theoretically, someone could argue that a change that requires to
> > modify lots of faces shouldn't be so unconditional, or shouldn't be
> > the default, or should have a "fire escape", or something to that
> > effect.  But if people don't mind changing their faces, then such
> > fears have no basis, and we are good with what we have.
> 
> A "fire escape" would depend on a user's config, right? I don't like the 
> sound of that approach, personally.

"Fire escape" in this context means a way to get the old behavior
without inordinately too much work on the part of the user.

> A lot of face don't inherit from anything on purpose. Anyway, I've 
> pinged Magit's maintainer, let's see what he says.

Thanks.

> >>> If too many faces in unbundled packages indeed need to change in that
> >>> way, we should consider additional measures.  That's why we need good
> >>> reasons for extending each face, not just "because they were before"
> >>> or because people were used to see them extended.
> >>
> >> Those are not the worst reasons, though.
> > 
> > Not sure I understand in what sens did you use "the worst" here.
> 
> "People were used to ..." is basically 99% of the arguments that were 
> given in all past discussions for not changing defaults to be more 
> "modern" or whatever. And there's some merit to that.

No, in past discussions people usually also brought up
functionality-related arguments, not just that they are used to the
old behavior.

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

[Kévin Le Gouguec]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> Yeah, so: what's the plan for Magit?
>
> Will the new version of it have to
>
> (if <emacs version is...>
>     (defface ...)
>   (defface ...)
>
> ?

> We should define and document a "migration path", e.g. say what a
> package author should do if they have a face which needs to be
> extended, preferably without breaking compatibility with Emacs 26.

Earlier in the thread[1] I suggested this method to avoid the duplicate
defface issue:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: demo.patch --]
[-- Type: text/x-diff, Size: 656 bytes --]

--- magit-diff.el.bkp	2019-10-23 17:02:13.340410735 +0200
+++ magit-diff.el	2019-10-24 17:54:58.769446997 +0200
@@ -509,12 +509,14 @@
   :group 'magit-faces)
 
 (defface magit-diff-hunk-heading
-  '((((class color) (background light))
+  `((((class color) (background light))
      :background "grey80"
-     :foreground "grey30")
+     :foreground "grey30"
+     ,@(when (>= emacs-major-version 27) '(:extend t)))
     (((class color) (background dark))
      :background "grey25"
-     :foreground "grey70"))
+     :foreground "grey70"
+     ,@(when (>= emacs-major-version 27) '(:extend t))))
   "Face for diff hunk headings."
   :group 'magit-faces)
 

[-- Attachment #3: Type: text/plain, Size: 324 bytes --]


Would this be an acceptable migration path?  Magit requires Emacs≥25.1
if I am not mistaken; I don't know how portable this solution would be.

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#212
    AFAICT no-one outright rejected this idea; I apologize if I missed
    someone pointing out shortcomings.

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

[Juri Linkov]

> I miss you here.  Emacs now by default also extends the region to the
> right window edge.

Emacs doesn't extend the region to the right window edge when the region
face was already customized, and has no "extend t" in the init file.

>> Also I looked how other applications extend diff blocks, and e.g.
>> GitLab extends diff background colors to the window right edge,
>> not to EOL, for example,
>> https://github.com/emacs-mirror/emacs/commit/3d6075e3ee8c447f8974b37007a1b1ae1af8917c
>
> With Firefox these diffs are boxed in a subarea of the Firefox window.
> They do not start or extend at the window edges and text in these
> boxes is static, can neither overflow into a newline nor be broken.

This is why I proposed to limit these boxes to some fixed column
like fill-column.

> But I think that our (e)diff blocks are affected by the change and all
> their face settings probably have to change, as well as tables and
> listings.

Yes, (e)diff face settings have to change, but actually I discovered
that diff-refined faces don't need to extend to the window edge,
because they don't form a block, they are word-based.

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

[Andrey Orst]

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

> Yes, (e)diff face settings have to change, but actually I discovered
> that diff-refined faces don't need to extend to the window edge,
> because they don't form a block, they are word-based.

Maybe I misunderstood you, but Ediff shows blocks of changes, plus changes
inside that block. And the block should be extended to window edge, because
of side by side nature

[-- Attachment #2: Type: text/html, Size: 523 bytes --]

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Fri, 18 Oct 2019 01:22:16 +0300
> 
> > I miss you here.  Emacs now by default also extends the region to the
> > right window edge.
> 
> Emacs doesn't extend the region to the right window edge when the region
> face was already customized, and has no "extend t" in the init file.

I proposed a fix for that.

> > With Firefox these diffs are boxed in a subarea of the Firefox window.
> > They do not start or extend at the window edges and text in these
> > boxes is static, can neither overflow into a newline nor be broken.
> 
> This is why I proposed to limit these boxes to some fixed column
> like fill-column.

This is not currently workable, because we cannot extend faces on
pixel granularity, and extending them on column granularity will
produce ugly jagged display with variable-pitch fonts, or even if
font-lock uses bold or italic variants for some faces used by the
major mode whose files are diff'ed.

> > But I think that our (e)diff blocks are affected by the change and all
> > their face settings probably have to change, as well as tables and
> > listings.
> 
> Yes, (e)diff face settings have to change, but actually I discovered
> that diff-refined faces don't need to extend to the window edge,
> because they don't form a block, they are word-based.

I agree.  I think the number of faces that might need to include
:extend is very small.

But we still have the broader issue of unbundled packages out there.
It was mentioned a few times, but there's no detailed information
regarding that, and so it's unclear whether just changing a few more
core faces will allow us to solve this issue and move on.

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

[Juri Linkov]

>> > But I think that our (e)diff blocks are affected by the change and all
>> > their face settings probably have to change, as well as tables and
>> > listings.
>>
>> Yes, (e)diff face settings have to change, but actually I discovered
>> that diff-refined faces don't need to extend to the window edge,
>> because they don't form a block, they are word-based.
>
> I agree.  I think the number of faces that might need to include
> :extend is very small.

So I added :extend to diff faces, except word-based refinement faces.

Also I considered adding :extend to multi-line isearch matches,
but in fact yanking in isearch is word-based such as C-w,
so maybe the current default is fine.  Or do you think it's important
to extend highlighting of matched empty lines beyond EOL
to make them more noticeable?  Then we'll need to extend
matching of empty like also for lazy-highlight, hi-lock, occur faces.

Additional question: since now in multi-line Info references faces don't
extend beyond EOL by default, could the following hack to be removed
from info.el:

              ;; For multiline ref, unfontify newline and surrounding whitespace
              (save-excursion
                (goto-char rbeg)
                (save-match-data
                  (while (re-search-forward "\\s-*\n\\s-*" rend t nil)
                    (remove-text-properties (match-beginning 0)
                                            (match-end 0)
                                            '(font-lock-face t)))))

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: rudalics@gmx.at,  andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Sat, 19 Oct 2019 23:53:47 +0300
> 
> So I added :extend to diff faces, except word-based refinement faces.

Thanks.

> Also I considered adding :extend to multi-line isearch matches,
> but in fact yanking in isearch is word-based such as C-w,
> so maybe the current default is fine.  Or do you think it's important
> to extend highlighting of matched empty lines beyond EOL
> to make them more noticeable?  Then we'll need to extend
> matching of empty like also for lazy-highlight, hi-lock, occur faces.

I think we should add :extend only if there's little doubt about its
necessity.  So let's wait with the Isearch faces until we are sure.

> Additional question: since now in multi-line Info references faces don't
> extend beyond EOL by default, could the following hack to be removed
> from info.el:
> 
>               ;; For multiline ref, unfontify newline and surrounding whitespace
>               (save-excursion
>                 (goto-char rbeg)
>                 (save-match-data
>                   (while (re-search-forward "\\s-*\n\\s-*" rend t nil)
>                     (remove-text-properties (match-beginning 0)
>                                             (match-end 0)
>                                             '(font-lock-face t)))))

Yes, I think so, but maybe leave this code in place conditioned by the
relevant face being extended, in case users customize them?

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

[Juri Linkov]

>> Additional question: since now in multi-line Info references faces don't
>> extend beyond EOL by default, could the following hack to be removed
>> from info.el:
>>
>>               ;; For multiline ref, unfontify newline and surrounding whitespace
>>               (save-excursion
>>                 (goto-char rbeg)
>>                 (save-match-data
>>                   (while (re-search-forward "\\s-*\n\\s-*" rend t nil)
>>                     (remove-text-properties (match-beginning 0)
>>                                             (match-end 0)
>>                                             '(font-lock-face t)))))
>
> Yes, I think so, but maybe leave this code in place conditioned by the
> relevant face being extended, in case users customize them?

I noticed that this code is still needed because ':extend nil'
still highlights the final newline at EOL, but this code removes
highlighting from newlines.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: rudalics@gmx.at,  andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Sun, 20 Oct 2019 18:42:35 +0300
> 
> >>               (save-excursion
> >>                 (goto-char rbeg)
> >>                 (save-match-data
> >>                   (while (re-search-forward "\\s-*\n\\s-*" rend t nil)
> >>                     (remove-text-properties (match-beginning 0)
> >>                                             (match-end 0)
> >>                                             '(font-lock-face t)))))
> >
> > Yes, I think so, but maybe leave this code in place conditioned by the
> > relevant face being extended, in case users customize them?
> 
> I noticed that this code is still needed because ':extend nil'
> still highlights the final newline at EOL, but this code removes
> highlighting from newlines.

Right.

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

[Juri Linkov]

>>> > But I think that our (e)diff blocks are affected by the change and all
>>> > their face settings probably have to change, as well as tables and
>>> > listings.
>>>
>>> Yes, (e)diff face settings have to change, but actually I discovered
>>> that diff-refined faces don't need to extend to the window edge,
>>> because they don't form a block, they are word-based.
>>
>> I agree.  I think the number of faces that might need to include
>> :extend is very small.
>
> So I added :extend to diff faces, except word-based refinement faces.

I forgot to add :extend to dynamically generated vc-annotate faces,
now fixed.  I wonder how many such tricky cases still remain undiscovered.

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

[martin rudalics]

 >> I miss you here.  Emacs now by default also extends the region to the
 >> right window edge.
 >
 > Emacs doesn't extend the region to the right window edge when the region
 > face was already customized, and has no "extend t" in the init file.

But when the region face was already customized we hardly talk about
the default any more.

 >> With Firefox these diffs are boxed in a subarea of the Firefox window.
 >> They do not start or extend at the window edges and text in these
 >> boxes is static, can neither overflow into a newline nor be broken.
 >
 > This is why I proposed to limit these boxes to some fixed column
 > like fill-column.

The point I wanted to make is that the diffs shown in that Firefox
window are static, you can't modify them.  Hence it's easy to produce
them with a once calculated, fixed column, even based on a variable
pitch font.  (Which, BTW, is an exaggeration - try to show that page
in the non-unified, side-by-side style, shrink the Firefox frame and
look at the barely readable mixture of line truncation and breaking.)

With Emacs, things are different.  When you ediff two buffers, you can
modify their contents any which way you want and any highlighting has
to adapt accordingly.  Thus any rectangular block concept based on a
previously established fixed column will break at least here.

 >> But I think that our (e)diff blocks are affected by the change and all
 >> their face settings probably have to change, as well as tables and
 >> listings.
 >
 > Yes, (e)diff face settings have to change, but actually I discovered
 > that diff-refined faces don't need to extend to the window edge,
 > because they don't form a block, they are word-based.

I wouldn't consider blocks, boxes or rectangles specially in the
present context.  None of these should, by design, automatically
extend to a window edge.  The fact that they did until Ergus pushed
his patch rather hints at a design shortcoming that, however,
installed itself in the minds of many users (including yours truly).

What we should consider specially IMHO are lines in certain, specific
environments like the above mentioned side-by-side windows used by
ediff for comparing two buffers.  There, having a highlight in one
window extend to the edge will ease passing to the corresponding line
in the other window (provided we can keep these lines in synch in the
first place - we'd urgently need code for that).

And in listings that may contain empty lines, indicating such a line
with a highlighting that expands to the edge it will probably make it
easier to re-focus a user's attention when scrolling that window.
Just as we do for 'hl-line' but without enabling it everywhere.

Still, most of these customization are merely a matter of taste and I
can't yet see the breakage I personally expected when a few weeks ago
I urged Ergus to install his patch ASAP.

martin

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

[Dmitry Gutov]

On 18.10.2019 11:25, martin rudalics wrote:
> Still, most of these customization are merely a matter of taste and I
> can't yet see the breakage I personally expected when a few weeks ago
> I urged Ergus to install his patch ASAP.

The whole faces subsystem is about matters of taste. That doesn't mean 
that its functionality is not important.

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

[Andrey Orst]

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

Though this may be not all faces that I need to fix, but at first glance,
everything that I use now looks as before the update. I am sure that I will
keep adding faces to the lists, because I know that I've missed many of
those in a places I do not remember right now. These are the faces I've
changed:

(when (>= emacs-major-version 27)
  (with-eval-after-load 'org
    (dolist (face '(org-block
                    org-block-begin-line
                    org-block-end-line
                    org-level-1))
      (set-face-attribute face nil :extend t)))
  (with-eval-after-load 'magit
    (dolist (face '(magit-diff-hunk-heading
                    magit-diff-hunk-heading-highlight
                    magit-diff-hunk-heading-selection
                    magit-diff-hunk-region
                    magit-diff-lines-heading
                    magit-diff-lines-boundary
                    magit-diff-conflict-heading
                    magit-diff-added
                    magit-diff-removed
                    magit-diff-our
                    magit-diff-base
                    magit-diff-their
                    magit-diff-context
                    magit-diff-added-highlight
                    magit-diff-removed-highlight
                    magit-diff-our-highlight
                    magit-diff-base-highlight
                    magit-diff-their-highlight
                    magit-diff-context-highlight
                    magit-diff-whitespace-warning
                    magit-diffstat-added
                    magit-diffstat-removed
                    magit-section-heading
                    magit-section-heading-selection
                    magit-section-highlight
                    magit-section-secondary-heading
                    magit-diff-file-heading
                    magit-diff-file-heading-highlight
                    magit-diff-file-heading-selection))
      (set-face-attribute face nil :extend t)))
  (with-eval-after-load 'ediff
    (dolist (face '(ediff-current-diff-A
                    ediff-current-diff-Ancestor
                    ediff-current-diff-B
                    ediff-current-diff-C
                    ediff-even-diff-A
                    ediff-even-diff-Ancestor
                    ediff-even-diff-B
                    ediff-even-diff-C
                    ediff-fine-diff-A
                    ediff-fine-diff-Ancestor
                    ediff-fine-diff-B
                    ediff-fine-diff-C
                    ediff-odd-diff-A
                    ediff-odd-diff-Ancestor
                    ediff-odd-diff-B
                    ediff-odd-diff-C))
      (set-face-attribute face nil :extend t)))
  (with-eval-after-load 'hl-line
    (set-face-attribute 'hl-line nil :extend t)))

This is now a part of my configuration of doom-themes package.

--
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 3642 bytes --]

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

[Dmitry Gutov]

On 16.10.2019 23:14, Juri Linkov wrote:
>>> (It's a pity that the long discussion of this before the development
>>> started went without any such objections from the people who are
>>> regulars on emacs-devel.)
>>
>> Btw, please take into account that what that change caused is that
>> Emacs now behaves like other applications in this regard.
> 
> I don't know what applications behave the same.  I tried different
> editors that I could find (namely LibreOffice Writer and xed)
> and all they extend highlighting of the selected region
> to the window right edge, not to EOL.
> 
> Also I looked how other applications extend diff blocks, and e.g.
> GitLab extends diff background colors to the window right edge,
> not to EOL, for example,
> https://github.com/emacs-mirror/emacs/commit/3d6075e3ee8c447f8974b37007a1b1ae1af8917c
> 
> However, no other application extends underlines to the window edge
> as Emacs used to do, this was a plain bug that this change fixed.

+1

Which at least disproves the claim that Emacs didn't "behave like other 
applications" previously, and that it does with the new change.

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

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 17 Oct 2019 16:56:23 +0300
> 
> > However, no other application extends underlines to the window edge
> > as Emacs used to do, this was a plain bug that this change fixed.
> 
> +1
> 
> Which at least disproves the claim that Emacs didn't "behave like other 
> applications" previously, and that it does with the new change.

It doesn't disprove it, it makes it less strong.  Because some
applications definitely behave like Emacs does now, they even behave
like that with selected text, something we didn't change in Emacs.

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

[Dmitry Gutov]

On 16.10.2019 21:54, Eli Zaretskii wrote:

> (It's a pity that the long discussion of this before the development
> started went without any such objections from the people who are
> regulars on emacs-devel.)

Please try to recall highly non-specific subject name of that thread.

There are a lot of new messages in emacs-devel every day during recent 
weeks (and in the bug tracker as well), and skipping long-running 
threads talking about specialized matters is only natural.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Andrey Orst <andreyorst@gmail.com>,
>   37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 20:27:40 +0300
> 
> 1. Backward-compatibility problem:
> 
> I had to spend significant time investigating why the region face broke
> recently, and discovered that customized faces in custom-set-faces need
> to be updated.

I'm not sure I understand: the region face is defined to be extended
beyond EOL.  How does custom-set-faces enter this picture, and why did
you need to do anything about the customized faces?

> Soon I tired fixing their customizations one by one manually,

Which other faces needed to be "fixed", how, and why?

> All faces could be divided into two more-less equally large groups:
> 
> a. faces with distinct foreground that highlight text properties,
> they include mostly font-lock faces, underline faces, and so on;
> 
> b. faces with distinct background that highlight blocks of text,
> such as the region face, diff hunk faces, etc.

Why are you talking only about the colors?  face extension is not only
about colors, it's about other attributes as well: underline,
strike-through, box, etc.  You list underline with foreground color,
but they are not the same as color, especially not when face extension
is concerned.  They actually behave more like background colors.

And then there are faces with both foreground and background colors.

> As I see the change was meant to fix only the problem that relates to
> faces with distinct foreground, because indeed underlines extended
> to the window edge look very ugly.  So the change should affect
> only faces with distinct foreground.

That wasn't the intent.  the intent was explicitly to cause the change
in background color and underline/strikethough/etc. attributes--those
which show in the face extension.  Foreground color doesn't show in
face extension.

> This screenshot demonstrates how badly broken these blocks are now
> in diff-mode that it makes harder to read diffs:

I'm sorry, but I don't see why it is broken or hard to read.

> Ideally to be more nice-looking, background colors in such faces should be
> extended to the column defined e.g. by display-fill-column-indicator-column.

That would be ugly if the line's text extends beyond the fill-column,
no?  Also, it would look even uglier with variable-pitch fonts.

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

[Juri Linkov]

>> 1. Backward-compatibility problem:
>>
>> I had to spend significant time investigating why the region face broke
>> recently, and discovered that customized faces in custom-set-faces need
>> to be updated.
>
> I'm not sure I understand: the region face is defined to be extended
> beyond EOL.  How does custom-set-faces enter this picture, and why did
> you need to do anything about the customized faces?

The region face customized long ago in the init file
has no ':extend t' face attribute, e.g.

(custom-set-faces
 '(region ((((class color) (background light)) (:background "gray90"))))

>> Soon I tired fixing their customizations one by one manually,
>
> Which other faces needed to be "fixed", how, and why?

All diff faces and faces that have a distinct background color
like 'comint-highlight-input' (should extend to window edge
to help locating visually the command line in shell buffers),
'org-block' (because it highlights code blocks), 'xref-file-header'
for the same reason as diff faces, i.e. faces that highlights blocks.

> Why are you talking only about the colors?  face extension is not only
> about colors, it's about other attributes as well: underline,
> strike-through, box, etc.  You list underline with foreground color,
> but they are not the same as color, especially not when face extension
> is concerned.  They actually behave more like background colors.

Yes, this new feature is useful for all these face attributes
to extend them to EOL.  The only exception is background colors.

All complaints are only about extending background colors to EOL.
So the change could apply to all face attributes except background colors.

Only other attributes should be extended to EOL, because when such face
attributes like underline and strike-through are displayed over
an empty space beyond EOL, this looks ugly.

> And then there are faces with both foreground and background colors.

Actually the distinction is not so simple: even some background colors
need to extend to EOL, such as when used in combination with the 'box'
face attributes, because when a button takes two lines, extending
the button box face to the window edge looks ugly.

>> As I see the change was meant to fix only the problem that relates to
>> faces with distinct foreground, because indeed underlines extended
>> to the window edge look very ugly.  So the change should affect
>> only faces with distinct foreground.
>
> That wasn't the intent.  the intent was explicitly to cause the change
> in background color and underline/strikethough/etc. attributes--those
> which show in the face extension.  Foreground color doesn't show in
> face extension.
>
>> This screenshot demonstrates how badly broken these blocks are now
>> in diff-mode that it makes harder to read diffs:
>
> I'm sorry, but I don't see why it is broken or hard to read.

Because there is no distinctive rectangular header anymore,
and no diff hunk blocks.

>> Ideally to be more nice-looking, background colors in such faces should be
>> extended to the column defined e.g. by display-fill-column-indicator-column.
>
> That would be ugly if the line's text extends beyond the fill-column,
> no?  Also, it would look even uglier with variable-pitch fonts.

Extending to the fill-column could be an optional feature.
It won't work with variable-pitch fonts the same way as
filling to fill-column doesn't work with variable-pitch fonts.
But if some line's text will extend beyond the fill-column
with fixed-pitch fonts, this even could help to find long lines
(like in whitespace-mode).

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: spacibba@aol.com,  andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 22:46:55 +0300
> 
> > I'm not sure I understand: the region face is defined to be extended
> > beyond EOL.  How does custom-set-faces enter this picture, and why did
> > you need to do anything about the customized faces?
> 
> The region face customized long ago in the init file
> has no ':extend t' face attribute, e.g.
> 
> (custom-set-faces
>  '(region ((((class color) (background light)) (:background "gray90"))))

So maybe we should modify custom-set-faces to preserve the :extend
attribute?  Would that solve the problem?

> >> Soon I tired fixing their customizations one by one manually,
> >
> > Which other faces needed to be "fixed", how, and why?
> 
> All diff faces and faces that have a distinct background color
> like 'comint-highlight-input' (should extend to window edge
> to help locating visually the command line in shell buffers),
> 'org-block' (because it highlights code blocks), 'xref-file-header'
> for the same reason as diff faces, i.e. faces that highlights blocks.

I don't think I agree.  I'm not convinced by the reasons, and I find
the new appearance not worse (and sometimes better) than the old.

I think the objections are mostly because of the surprising new
appearance.

> All complaints are only about extending background colors to EOL.

We've been discussing this only for a day.  So whether all the
complaints are about the background remains to be seen.  It could be
because most of our faces only specify colors, for example.

> >> This screenshot demonstrates how badly broken these blocks are now
> >> in diff-mode that it makes harder to read diffs:
> >
> > I'm sorry, but I don't see why it is broken or hard to read.
> 
> Because there is no distinctive rectangular header anymore,
> and no diff hunk blocks.

Sorry, I don't think I follow: how do you mean there's no distinctive
header and no diff hunk blocks?  I see them quite clearly.

> >> Ideally to be more nice-looking, background colors in such faces should be
> >> extended to the column defined e.g. by display-fill-column-indicator-column.
> >
> > That would be ugly if the line's text extends beyond the fill-column,
> > no?  Also, it would look even uglier with variable-pitch fonts.
> 
> Extending to the fill-column could be an optional feature.

But above you mention it as the default.  If it's an option, then it
cannot be a solution to the problems we are discussing.

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

[Juri Linkov]

>> > I'm not sure I understand: the region face is defined to be extended
>> > beyond EOL.  How does custom-set-faces enter this picture, and why did
>> > you need to do anything about the customized faces?
>> 
>> The region face customized long ago in the init file
>> has no ':extend t' face attribute, e.g.
>> 
>> (custom-set-faces
>>  '(region ((((class color) (background light)) (:background "gray90"))))
>
> So maybe we should modify custom-set-faces to preserve the :extend
> attribute?  Would that solve the problem?

I don't know how feasible this is.  This looks like a hack.

>> All diff faces and faces that have a distinct background color
>> like 'comint-highlight-input' (should extend to window edge
>> to help locating visually the command line in shell buffers),
>> 'org-block' (because it highlights code blocks), 'xref-file-header'
>> for the same reason as diff faces, i.e. faces that highlights blocks.
>
> I don't think I agree.  I'm not convinced by the reasons, and I find
> the new appearance not worse (and sometimes better) than the old.

I find the new appearance better too in most cases, but not
for background colors.

>> Because there is no distinctive rectangular header anymore,
>> and no diff hunk blocks.
>
> Sorry, I don't think I follow: how do you mean there's no distinctive
> header and no diff hunk blocks?  I see them quite clearly.

I meant a rectangular header like in other applications.

>> Extending to the fill-column could be an optional feature.
>
> But above you mention it as the default.  If it's an option, then it
> cannot be a solution to the problems we are discussing.

Extending to fill-column could be optional.  Extending to window edge
could be default for faces with distinct background colors.  Extending to
EOL could be default for all other faces.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wed, Oct 16, 2019 at 11:23:14PM +0300, Juri Linkov wrote:
>>> > I'm not sure I understand: the region face is defined to be extended
>>> > beyond EOL.  How does custom-set-faces enter this picture, and why did
>>> > you need to do anything about the customized faces?
>>>
>>> The region face customized long ago in the init file
>>> has no ':extend t' face attribute, e.g.
>>>
>>> (custom-set-faces
>>>  '(region ((((class color) (background light)) (:background "gray90"))))
>>
>> So maybe we should modify custom-set-faces to preserve the :extend
>> attribute?  Would that solve the problem?
>
>I don't know how feasible this is.  This looks like a hack.
>
Actually it will be a transition workaround than could be removed in
emacs 28 for those who don't want (or know) how to update manually
because they made it with the interface.

But will break the case when the user explicitly wants a non extensible
region face and set that in the custom-set-face section in his init.

>>> All diff faces and faces that have a distinct background color
>>> like 'comint-highlight-input' (should extend to window edge
>>> to help locating visually the command line in shell buffers),
>>> 'org-block' (because it highlights code blocks), 'xref-file-header'
>>> for the same reason as diff faces, i.e. faces that highlights blocks.
>>
>> I don't think I agree.  I'm not convinced by the reasons, and I find
>> the new appearance not worse (and sometimes better) than the old.
>
>I find the new appearance better too in most cases, but not
>for background colors.
>
90% of the application/usability of this is actually background color
and the ability to control that after eol. We cannot (and conceptually
shouldn't in my opinion) discriminate some face attributes from others.

>>> Because there is no distinctive rectangular header anymore,
>>> and no diff hunk blocks.
>>
>> Sorry, I don't think I follow: how do you mean there's no distinctive
>> header and no diff hunk blocks?  I see them quite clearly.
>
>I meant a rectangular header like in other applications.
>
>>> Extending to the fill-column could be an optional feature.
>>
>> But above you mention it as the default.  If it's an option, then it
>> cannot be a solution to the problems we are discussing.
>
>Extending to fill-column could be optional.  Extending to window edge
>could be default for faces with distinct background colors.  Extending to
>EOL could be default for all other faces.

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

[Eli Zaretskii]

> Date: Wed, 16 Oct 2019 22:37:40 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, andreyorst@gmail.com,
> 	37774@debbugs.gnu.org
> 
> >>> (custom-set-faces
> >>>  '(region ((((class color) (background light)) (:background "gray90"))))
> >>
> >> So maybe we should modify custom-set-faces to preserve the :extend
> >> attribute?  Would that solve the problem?
> >
> >I don't know how feasible this is.  This looks like a hack.
> >
> Actually it will be a transition workaround than could be removed in
> emacs 28 for those who don't want (or know) how to update manually
> because they made it with the interface.
> 
> But will break the case when the user explicitly wants a non extensible
> region face and set that in the custom-set-face section in his init.

If we implement this only when the custom-set-face does NOT include an
explicit :extend setting, then there will be no breakage, I think.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: spacibba@aol.com,  andreyorst@gmail.com,  37774@debbugs.gnu.org
> Date: Wed, 16 Oct 2019 23:23:14 +0300
> 
> >> (custom-set-faces
> >>  '(region ((((class color) (background light)) (:background "gray90"))))
> >
> > So maybe we should modify custom-set-faces to preserve the :extend
> > attribute?  Would that solve the problem?
> 
> I don't know how feasible this is.  This looks like a hack.

Why do you think it's a hack?

> > I don't think I agree.  I'm not convinced by the reasons, and I find
> > the new appearance not worse (and sometimes better) than the old.
> 
> I find the new appearance better too in most cases, but not
> for background colors.

I'm talking specifically about background colors: I find the
appearance not worse and sometimes better.

> >> Because there is no distinctive rectangular header anymore,
> >> and no diff hunk blocks.
> >
> > Sorry, I don't think I follow: how do you mean there's no distinctive
> > header and no diff hunk blocks?  I see them quite clearly.
> 
> I meant a rectangular header like in other applications.

But if the faces are customized to have a distinct foreground color,
instead of background, you will see exactly what you see now with the
background color.  The terminal display of "git diff" and other
similar commands does precisely that, and we don't seem to mind the
fact that the colors don't align at the end of each line.

> Extending to fill-column could be optional.  Extending to window edge
> could be default for faces with distinct background colors.  Extending to
> EOL could be default for all other faces.

How is this supposed to work, given that any face can be customized to
use either of these attributes, or any combination thereof?

Once again: other applications do what we do now, and users don't seem
to mind.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wed, Oct 16, 2019 at 11:03:15PM +0300, Eli Zaretskii wrote:
>> From: Juri Linkov <juri@linkov.net>
>> Cc: spacibba@aol.com,  andreyorst@gmail.com,  37774@debbugs.gnu.org
>> Date: Wed, 16 Oct 2019 22:46:55 +0300
>>
>> > I'm not sure I understand: the region face is defined to be extended
>> > beyond EOL.  How does custom-set-faces enter this picture, and why did
>> > you need to do anything about the customized faces?
>>
>> The region face customized long ago in the init file
>> has no ':extend t' face attribute, e.g.
>>
>> (custom-set-faces
>>  '(region ((((class color) (background light)) (:background "gray90"))))
>
>So maybe we should modify custom-set-faces to preserve the :extend
>attribute?  Would that solve the problem?
>
>> >> Soon I tired fixing their customizations one by one manually,
>> >
>> > Which other faces needed to be "fixed", how, and why?
>>
>> All diff faces and faces that have a distinct background color
>> like 'comint-highlight-input' (should extend to window edge
>> to help locating visually the command line in shell buffers),
>> 'org-block' (because it highlights code blocks), 'xref-file-header'
>> for the same reason as diff faces, i.e. faces that highlights blocks.
>
>I don't think I agree.  I'm not convinced by the reasons, and I find
>the new appearance not worse (and sometimes better) than the old.
>
>I think the objections are mostly because of the surprising new
>appearance.
>
Agree. I also asked in the emacs telegram group and in general many
people prefer that "the selection looks like in vim"

>> All complaints are only about extending background colors to EOL.
>
>We've been discussing this only for a day.  So whether all the
>complaints are about the background remains to be seen.  It could be
>because most of our faces only specify colors, for example.
>
The mode maintainers (like diff mode) will update their mode's faces if
they find that more convenient.

>> >> This screenshot demonstrates how badly broken these blocks are now
>> >> in diff-mode that it makes harder to read diffs:
>> >
>> > I'm sorry, but I don't see why it is broken or hard to read.
>>
>> Because there is no distinctive rectangular header anymore,
>> and no diff hunk blocks.
>
>Sorry, I don't think I follow: how do you mean there's no distinctive
>header and no diff hunk blocks?  I see them quite clearly.
>
>> >> Ideally to be more nice-looking, background colors in such faces should be
>> >> extended to the column defined e.g. by display-fill-column-indicator-column.
>> >
>> > That would be ugly if the line's text extends beyond the fill-column,
>> > no?  Also, it would look even uglier with variable-pitch fonts.
>>
>> Extending to the fill-column could be an optional feature.
>
>But above you mention it as the default.  If it's an option, then it
>cannot be a solution to the problems we are discussing.

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

[Dmitry Gutov]

On 16.10.2019 23:29, Ergus via Bug reports for GNU Emacs, the Swiss army 
knife of text editors wrote:
>> I don't think I agree.  I'm not convinced by the reasons, and I find
>> the new appearance not worse (and sometimes better) than the old.
>>
>> I think the objections are mostly because of the surprising new
>> appearance.
>>
> Agree. I also asked in the emacs telegram group and in general many
> people prefer that "the selection looks like in vim"

I think it's a valid preference, and it'll be nice if people can 
configure their Emacs to do that.

BTW, if we want to adopt the appearance of Vim or Firefox for the region 
display, we should probably avoid highlighting the extra character at 
the end of the line, the one that corresponds to newline apparently.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Wed, Oct 16, 2019 at 10:46:55PM +0300, Juri Linkov wrote:
>>> 1. Backward-compatibility problem:
>>>
>>> I had to spend significant time investigating why the region face broke
>>> recently, and discovered that customized faces in custom-set-faces need
>>> to be updated.
>>
>> I'm not sure I understand: the region face is defined to be extended
>> beyond EOL.  How does custom-set-faces enter this picture, and why did
>> you need to do anything about the customized faces?
>
>The region face customized long ago in the init file
>has no ':extend t' face attribute, e.g.
>
>(custom-set-faces
> '(region ((((class color) (background light)) (:background "gray90"))))
>
When moving to a new emacs version (with early init and other tweaks) it
will be recommended to update some details in the init file any way. 

>>> Soon I tired fixing their customizations one by one manually,
>>
>> Which other faces needed to be "fixed", how, and why?
>
>All diff faces and faces that have a distinct background color
>like 'comint-highlight-input' (should extend to window edge
>to help locating visually the command line in shell buffers),
>'org-block' (because it highlights code blocks), 'xref-file-header'
>for the same reason as diff faces, i.e. faces that highlights blocks.
>
For sure some other faces will be corrected once we find they are
"broken" or they work better with ":extend t". Org mode is a very active
package so the authors will correct this once they get emacs 27 (or we
contact them). Actually some of them follow this mailing list.

>> Why are you talking only about the colors?  face extension is not only
>> about colors, it's about other attributes as well: underline,
>> strike-through, box, etc.  You list underline with foreground color,
>> but they are not the same as color, especially not when face extension
>> is concerned.  They actually behave more like background colors.
>
>Yes, this new feature is useful for all these face attributes
>to extend them to EOL.  The only exception is background colors.
>
>All complaints are only about extending background colors to EOL.
>So the change could apply to all face attributes except background colors.
>
I think this will be inconsistent with the own intention if the feature,
the problems it fixes and the criteria to distinguish and determine what
to extend and what not. Remember we are dealing with merged faces too.

>Only other attributes should be extended to EOL, because when such face
>attributes like underline and strike-through are displayed over
>an empty space beyond EOL, this looks ugly.
>
In the discussion previous to the implementation we agreed to give the
freedom to the user to extend or not. Actually we choose a more complex
design to assert give this freedom to the user. Baybe the user wants to
highlight the region using underline, and adjust that to the EOL or
not... now he can. 

>> And then there are faces with both foreground and background colors.
>
>Actually the distinction is not so simple: even some background colors
>need to extend to EOL, such as when used in combination with the 'box'
>face attributes, because when a button takes two lines, extending
>the button box face to the window edge looks ugly.
>
This depends of the use case; so we won't have a criteria that will fit
all the cases. 

>>> As I see the change was meant to fix only the problem that relates to
>>> faces with distinct foreground, because indeed underlines extended
>>> to the window edge look very ugly.  So the change should affect
>>> only faces with distinct foreground.
>>
>> That wasn't the intent.  the intent was explicitly to cause the change
>> in background color and underline/strikethough/etc. attributes--those
>> which show in the face extension.  Foreground color doesn't show in
>> face extension.
>>
>>> This screenshot demonstrates how badly broken these blocks are now
>>> in diff-mode that it makes harder to read diffs:
>>
>> I'm sorry, but I don't see why it is broken or hard to read.
>
>Because there is no distinctive rectangular header anymore,
>and no diff hunk blocks.
>
This will be fixed in 2 minutes once we have a set of faces we should
update. But also this will be a matter of preference. 

>>> Ideally to be more nice-looking, background colors in such faces should be
>>> extended to the column defined e.g. by display-fill-column-indicator-column.
>>
>> That would be ugly if the line's text extends beyond the fill-column,
>> no?  Also, it would look even uglier with variable-pitch fonts.
>
>Extending to the fill-column could be an optional feature.
>It won't work with variable-pitch fonts the same way as
>filling to fill-column doesn't work with variable-pitch fonts.
>But if some line's text will extend beyond the fill-column
>with fixed-pitch fonts, this even could help to find long lines
>(like in whitespace-mode).

This will be not implemented for now; it will add too many corner cases
I'm not sure it worth the effort to solve them just for an exceptional
aesthetic use case. Maybe for emacs 28 ;p

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

[Andrey Orst]

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

Sorry anyone if I disturbed your personal email addresses, I didn't
understand
how to reply there and thought that cc to 37774@debbugs.gnu.org would do
the trick. I see that my messages don't appear at the bug report page, so
I'll
send those back as a single e-mail. Still don't understand where I should
properly
reply.

> So please explain what exactly is incorrect or "weird" in the visual
> appearance after the change.  Specifically, why the faces in question
> need to be extended past EOL?

These faces are forming visual interface, e.g. hunks in Magit are
rectangular
regions with background color for entire width of the window that can be
folded.
Code blocks in org mode are, ahem, blocks. Now those blocks doesn't have
anything like block visually.

> So you are saying that you don't like the new appearance?  The Subject
> says "broke visuals", which sounds like a much more serious problem.

Well, "broke" may be wrong term, here, but lot of themes and packages
crafted
in a way to display things like that, and now all of those things displayed
accordingly
to a new setting, which in turn means that:

a) package maintainers should update *all* their packages to look like
before the change, and
b) maybe Emacs could treat `nil` here as "do not affect", and specify
symbols to set this to different
   settings, like `:extend t` or `:extend 'EOL`, and `:extend 'noextend` to
disable. Though, I do not
   know how code was changed, so maybe there's no way to treat `nil` as "do
not affect".

-- 
Best regards,
Andrey Orst

[-- Attachment #2: Type: text/html, Size: 2108 bytes --]

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

[Eli Zaretskii]

> From: Andrey Orst <andreyorst@gmail.com>
> Date: Wed, 16 Oct 2019 14:33:37 +0300
> 
> I see that my messages don't appear at the bug report page

They do, so there's nothing wrong with how you reply.  Having the bug
address in the CC list is the right way.

There's no need to repeat any of your messages.

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

[Jonas Bernoulli]

Dmitry keeps urging me to comment here, so I am doing that even though
I don't feel like I understand this enough yet to not make a fool of
myself.  Oh well, if you insist.

My main concern--and, due to what I believe to be bugs in the current
implementation, I haven't been able to verify whether that is a valid
concerns--is that each and every theme that customizes a face that was
defined to `extend' beyond eol will have to redo that configuration.

If this is really so, then it will take years until all themes have been
adjusted and through these years users will keep failing bug reports
about broken looks of Magit and other packages.  I do not look forward
to that.

Then again maybe I am wrong about that. However I have glanced over some
things that sound like `extend' is getting some special treatment that
does not apply to other attributes.  If that is so then I would consider
that a mistake and a strong indicator that maybe another approach should
be found that does not require any special treatment.

IMO going with a `noextend' attribute instead of `extend' would be that
alternative approach.  Even if `extend' does not require any special
treatment and even if it does not require each and every theme to be
adjusted.  Again, I don't know whether there is any special treatment
and whether themes have to be adjusted.

(It should be clear by now that I am not so happy that Dmitry kept
urging me to comment here.)

Okay then lets move to the bugs that I have found, possibly.  Maybe they
are not bugs and I have just done something stupid without realizing it.
Anyways...

I believe that sometimes a face extends beyond eol even though there is
nothing (no explicit `:extend t` nor any `:inherit' what-so-ever) that
tells it to do so.  Maybe it does make a difference whether the face is
used by an overlay or not.  Or maybe that is completely irrelevant.
Other faces however do not extend past eol and I am unable to see how
these faces differ from the faces that do.

You can verify that
(1) by making sure no theme is in use
(2) opening a Magit diff
(3) note how most faces extend beyond eol
(4) look at the definition of these faces and notice that there is
    nothing that tells those faces to extend beyond eol.  Yet they
    do.

Such faces include for example `magit-section-highlight' and
`magit-diff-added'.

A counter example is `magit-diff-file-heading-highlight'.  That does
not extend and I don't see how it is different from the other faces
that I mentioned.

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

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Thu, 31 Oct 2019 17:06:17 +0100
> 
> I believe that sometimes a face extends beyond eol even though there is
> nothing (no explicit `:extend t` nor any `:inherit' what-so-ever) that
> tells it to do so.  Maybe it does make a difference whether the face is
> used by an overlay or not.  Or maybe that is completely irrelevant.
> Other faces however do not extend past eol and I am unable to see how
> these faces differ from the faces that do.
> 
> You can verify that
> (1) by making sure no theme is in use
> (2) opening a Magit diff
> (3) note how most faces extend beyond eol
> (4) look at the definition of these faces and notice that there is
>     nothing that tells those faces to extend beyond eol.  Yet they
>     do.
> 
> Such faces include for example `magit-section-highlight' and
> `magit-diff-added'.
> 
> A counter example is `magit-diff-file-heading-highlight'.  That does
> not extend and I don't see how it is different from the other faces
> that I mentioned.

Is there an easier way of reproducing this than installing Magit and
actually using it?  Could you perhaps come up with a much simpler
recipe that just defines a face like Magit does?

The feature we are talking about is implemented in the display code,
whereas Magit is a large and complex package.  Mapping what one sees
in Magit back to the display code is not an easy job, so anything you
can do to ease that will be appreciated.

Thanks.

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

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:
> Is there an easier way of reproducing this than installing Magit and
> actually using it?  Could you perhaps come up with a much simpler
> recipe that just defines a face like Magit does?

Sorry for the delay.  I have finally reproduced some of the unexpected
behavior without using Magit.

Yank the following in some buffer like *scratch*.

------
(defface testing '((t (:background "LightYellow"))) "DOC")

(overlay-put (make-overlay (point-min) (point-max) nil t)
             'face ; or 'font-lock-face
             'testing)

(remove-overlays (point-min) (point-max))

(put-text-property (point-min) (point-max) 'font-lock-face 'testing)
------

Evaluate just the first two forms and notice that the `testing' face is
used beyond eol.  At that point I assummed I had narrowed down the issue
to overlays, but then I evaluated the next two forms and that resulted
in the same issue (the comment and strings used different faces, but
that is due to font-lock using two fontification phases I believe).

I have not yet looked into why Magit's diff faces extend beyond eol even
though I didn't tell them too, but the above is so surprising to me that
I will wait to hear back from you about that first before investigating
any further.

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

[martin rudalics]

One surprising effect with recent master is that with emacs -Q
evaluating

(custom-set-faces
  '(font-lock-comment-face ((((class color) (background light)) (:background "Beige" :foreground "Black")))))

makes the comment background extend to the end of the window.  This
means that unless I explicitly supply :extend nil, the behavior is as
with Emacs 26.  Is that intended?

martin

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Wed, 6 Nov 2019 18:06:04 +0100
> 
> One surprising effect with recent master is that with emacs -Q
> evaluating
> 
> (custom-set-faces
>   '(font-lock-comment-face ((((class color) (background light)) (:background "Beige" :foreground "Black")))))
> 
> makes the comment background extend to the end of the window.  This
> means that unless I explicitly supply :extend nil, the behavior is as
> with Emacs 26.  Is that intended?

No, of course it isn't intended.

Jimmy, are you looking into this?  It sounds like there's a difference
with processing the :extend attribute when its value is 'unspecified'
and when it's nil.  Because just adding

  (set-face-extend 'FOO nil)

for the faces named by Jonas and Martin makes the problem go away, and
the faces aren't extended, as I'd expect.

Let me know if you need help in debugging or resolving this.

Thanks.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

On Thu, Nov 07, 2019 at 03:58:02PM +0200, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org
>> From: martin rudalics <rudalics@gmx.at>
>> Date: Wed, 6 Nov 2019 18:06:04 +0100
>>
>> One surprising effect with recent master is that with emacs -Q
>> evaluating
>>
>> (custom-set-faces
>>   '(font-lock-comment-face ((((class color) (background light)) (:background "Beige" :foreground "Black")))))
>>
>> makes the comment background extend to the end of the window.  This
>> means that unless I explicitly supply :extend nil, the behavior is as
>> with Emacs 26.  Is that intended?
>
>No, of course it isn't intended.
>
>Jimmy, are you looking into this?  It sounds like there's a difference
>with processing the :extend attribute when its value is 'unspecified'
>and when it's nil.  Because just adding
>
>  (set-face-extend 'FOO nil)
>
>for the faces named by Jonas and Martin makes the problem go away, and
>the faces aren't extended, as I'd expect.
>
>Let me know if you need help in debugging or resolving this.
>
>Thanks.

Hi:

Please try the attached patch. (I'm in a network where I can't use git
now.)

Best,
Ergus

[-- Attachment #2: test.patch --]
[-- Type: text/plain, Size: 1599 bytes --]

diff --git a/src/xfaces.c b/src/xfaces.c
index 3806fa90e2..ff4e2796f2 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -2063,7 +2063,10 @@ merge_face_vectors (struct window *w, struct frame *f,
   eassert (attr_filter <  LFACE_VECTOR_SIZE);
 
   /* When FROM sets attr_filter to nil explicitly we don't merge it.  */
-  if (attr_filter > 0 && NILP(from[attr_filter]))
+  if (attr_filter > 0 && (NILP(from[attr_filter])
+                          || (UNSPECIFIEDP(from[attr_filter])
+			      && (NILP (from[LFACE_INHERIT_INDEX])
+			          || UNSPECIFIEDP (from[LFACE_INHERIT_INDEX])))))
     return;
 
   /* If FROM inherits from some other faces, merge their attributes into
@@ -2082,7 +2085,7 @@ merge_face_vectors (struct window *w, struct frame *f,
       else if (UNSPECIFIEDP (from[attr_filter])) /* FROM don't specify filter */
 	{
 	  Lisp_Object tmp[LFACE_VECTOR_SIZE];
-	  memcpy (tmp, to, LFACE_VECTOR_SIZE * sizeof *tmp);
+	  memcpy (tmp, to, LFACE_VECTOR_SIZE * sizeof(*tmp));
 
 	  merge_face_ref (w, f, from[LFACE_INHERIT_INDEX],
 	                  tmp, false, named_merge_points, attr_filter);
@@ -2177,7 +2180,8 @@ merge_named_face (struct window *w,
 		     && !UNSPECIFIEDP(from[attr_filter]))
                  || (!NILP(from[attr_filter])  /* Filter, unspecified, but inherited.  */
 		     && UNSPECIFIEDP(from[attr_filter])
-		     && !NILP (from[LFACE_INHERIT_INDEX]))))
+		     && !NILP (from[LFACE_INHERIT_INDEX])
+		     && !UNSPECIFIEDP (from[LFACE_INHERIT_INDEX]))))
         merge_face_vectors (w, f, from, to, named_merge_points, attr_filter);
 
       return ok;

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

[martin rudalics]

 > Please try the attached patch. (I'm in a network where I can't use git
 > now.)

Works as expected with this patch.

Many thanks, martin

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Thu, Nov 07, 2019 at 06:10:40PM +0100, martin rudalics wrote:
>> Please try the attached patch. (I'm in a network where I can't use git
>> now.)
>
>Works as expected with this patch.
>
>Many thanks, martin

Please push it for me.

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

[martin rudalics]

 > Please push it for me.

Pushed now.  Please have a look.

One more question: Are we sure that we want to show the background
(underline, ...) on the newline character when we do not extend?  It
certainly provides the information that the character is covered by
the property or overlay at that position and also fits well when the
cursor is positioned on it.  Yet it appears somewhat clumsy.

martin

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

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, jonas@bernoul.li, 37774@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 8 Nov 2019 10:20:22 +0100
> 
> One more question: Are we sure that we want to show the background
> (underline, ...) on the newline character when we do not extend?  It
> certainly provides the information that the character is covered by
> the property or overlay at that position and also fits well when the
> cursor is positioned on it.  Yet it appears somewhat clumsy.

More clumsy than it was before installing that feature?

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

[martin rudalics]

 > More clumsy than it was before installing that feature?

No.  But line breaks in underlined text appear strange (as I noticed
when looking at Bug#38038).

martin

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

[Eli Zaretskii]

> Cc: spacibba@aol.com, jonas@bernoul.li, 37774@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 8 Nov 2019 19:26:38 +0100
> 
>  > More clumsy than it was before installing that feature?
> 
> No.  But line breaks in underlined text appear strange (as I noticed
> when looking at Bug#38038).

OK, but let's see what we get about this new feature during the
pretest.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On November 8, 2019 10:20:22 AM GMT+01:00, martin rudalics <rudalics@gmx.at> wrote:
> > Please push it for me.
>
>Pushed now.  Please have a look.
>
>One more question: Are we sure that we want to show the background
>(underline, ...) on the newline character when we do not extend?  It
>certainly provides the information that the character is covered by
>the property or overlay at that position and also fits well when the
>cursor is positioned on it.  Yet it appears somewhat clumsy.
>
>martin


It is useful, for example, when not extending the region face and the region contains empty lines. 
It is actually the main reason for it I think.

Ergus

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[martin rudalics]

 > It is useful, for example, when not extending the region face and
 > the region contains empty lines.  It is actually the main reason for
 > it I think.

OK.  Non-extended highlighting a line might be another reason then.

martin

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

[Kévin Le Gouguec]

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

martin rudalics <rudalics@gmx.at> writes:

>> Please push it for me.
>
> Pushed now.  Please have a look.

(For context: this was commit 8232325337 "Handle case where a face's
:extend attribute is unspecified (Bug#37774)".)

I think there might have been a regression since this commit: in-between
1c29ba0340 (2019-11-17) and 21790e5473 (2019-12-05), something caused
the backgrounds to no longer extend for faces with unspecified :extend
inheriting faces with :extend t.

The attached theme sets :extend t for diff-{added,removed}, and sets
:inherit (diff-…) for ediff-fine-diff-{A,B}.


[-- Attachment #2: extend-inherit-theme.el --]
[-- Type: application/emacs-lisp, Size: 926 bytes --]

[-- Attachment #3: Type: text/plain, Size: 299 bytes --]


From emacs -Q, running the following code:

    (add-to-list 'custom-theme-load-path default-directory)
    (load-theme 'extend-inherit t)
    (ediff-files "/usr/share/common-licenses/LGPL-2" "/usr/share/common-licenses/LGPL-2.1")

… gives the following results with commit 1c29ba0340:


[-- Attachment #4: bug-37774-inherit-1c29ba0340.png --]
[-- Type: image/png, Size: 168676 bytes --]

[-- Attachment #5: Type: text/plain, Size: 59 bytes --]


… and the following results with commit 21790e5473:


[-- Attachment #6: bug-37774-inherit-21790e5473.png --]
[-- Type: image/png, Size: 168897 bytes --]

[-- Attachment #7: Type: text/plain, Size: 406 bytes --]


Is this deliberate?  I don't think I read anything in this thread
suggesting it; apologies if I missed something.

If it's a bug, maybe this is due to recent changes in xfaces.c?

    git log --oneline 1c29ba0340..21790e5473 -- src/xfaces.c 
    d4515f3cab Support ':extend' in faces defined by list of key/value pairs
    19aecd340b Fix face merging when some have :extend non-nil and some are inherited

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

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: Ergus <spacibba@aol.com>,  Eli Zaretskii <eliz@gnu.org>,
>   jonas@bernoul.li,  37774@debbugs.gnu.org
> Date: Thu, 05 Dec 2019 17:48:22 +0100
> 
> I think there might have been a regression since this commit: in-between
> 1c29ba0340 (2019-11-17) and 21790e5473 (2019-12-05), something caused
> the backgrounds to no longer extend for faces with unspecified :extend
> inheriting faces with :extend t.

Right.

> (custom-theme-set-faces
>  'extend-inherit
>  '(default ((t (:background "black" :foreground "gainsboro"))))
>  '(highlight ((t (:background "gray10"))))
>  '(diff-added ((t (:background "#12222f" :extend t))))
>  '(diff-removed ((t (:background "#2f1e00" :extend t))))
>  '(diff-refine-added ((t (:background "#1b3347"))))
>  '(diff-refine-removed ((t (:background "#472e00"))))
>  '(ediff-current-diff-A ((t (:inherit (diff-removed)))))

Why are you using values for :inherit that are lists of one element?
Why not just ":inherit FACE"?

But yes, this is a bug: the value of :inherit can legitimately be a
list.  Should be fixed now, thanks.

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

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

> Why are you using values for :inherit that are lists of one element?
> Why not just ":inherit FACE"?

… You can just ":inherit FACE"?

More seriously, there are no good reason AFAIR, some faces in my theme
inherit from multiple elements; maybe I took the habit of always making
lists out of uniformity 🤷

> But yes, this is a bug: the value of :inherit can legitimately be a
> list.  Should be fixed now, thanks.

Can confirm, thanks!

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

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: rudalics@gmx.at,  spacibba@aol.com,  jonas@bernoul.li,
>   37774@debbugs.gnu.org
> Date: Thu, 05 Dec 2019 20:05:01 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Why are you using values for :inherit that are lists of one element?
> > Why not just ":inherit FACE"?
> 
> … You can just ":inherit FACE"?

From the ELisp manual:

  ‘:inherit’
       The name of a face from which to inherit attributes, or a list of
       face names.  [...] If a list of faces is used, attributes from
       faces earlier in the list override those from later faces.

> > But yes, this is a bug: the value of :inherit can legitimately be a
> > list.  Should be fixed now, thanks.
> 
> Can confirm, thanks!

Thanks for testing.

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

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi:

I just saw this message and I was going to look into it but I see that
there is a today's commit related with the extend feature.

Eli did you already fixed this? Or is it still pending?



On Thu, Dec 05, 2019 at 05:48:22PM +0100, K??vin Le Gouguec wrote:
>martin rudalics <rudalics@gmx.at> writes:
>
>>> Please push it for me.
>>
>> Pushed now.  Please have a look.
>
>(For context: this was commit 8232325337 "Handle case where a face's
>:extend attribute is unspecified (Bug#37774)".)
>
>I think there might have been a regression since this commit: in-between
>1c29ba0340 (2019-11-17) and 21790e5473 (2019-12-05), something caused
>the backgrounds to no longer extend for faces with unspecified :extend
>inheriting faces with :extend t.
>
>The attached theme sets :extend t for diff-{added,removed}, and sets
>:inherit (diff-???) for ediff-fine-diff-{A,B}.
>


>
>From emacs -Q, running the following code:
>
>    (add-to-list 'custom-theme-load-path default-directory)
>    (load-theme 'extend-inherit t)
>    (ediff-files "/usr/share/common-licenses/LGPL-2" "/usr/share/common-licenses/LGPL-2.1")
>
>??? gives the following results with commit 1c29ba0340:
>


>
>??? and the following results with commit 21790e5473:
>


>
>Is this deliberate?  I don't think I read anything in this thread
>suggesting it; apologies if I missed something.
>
>If it's a bug, maybe this is due to recent changes in xfaces.c?
>
>    git log --oneline 1c29ba0340..21790e5473 -- src/xfaces.c
>    d4515f3cab Support ':extend' in faces defined by list of key/value pairs
>    19aecd340b Fix face merging when some have :extend non-nil and some are inherited

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

[Eli Zaretskii]

> Date: Thu, 5 Dec 2019 19:22:46 +0100
> From: Ergus <spacibba@aol.com>
> Cc: martin rudalics <rudalics@gmx.at>, Eli Zaretskii <eliz@gnu.org>,
> 	jonas@bernoul.li, 37774@debbugs.gnu.org
> 
> I just saw this message and I was going to look into it but I see that
> there is a today's commit related with the extend feature.
> 
> Eli did you already fixed this? Or is it still pending?

I hope I fixed this, yes.

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

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 37774@debbugs.gnu.org
> Date: Wed, 06 Nov 2019 17:26:12 +0100
> 
> ------
> (defface testing '((t (:background "LightYellow"))) "DOC")
> 
> (overlay-put (make-overlay (point-min) (point-max) nil t)
>              'face ; or 'font-lock-face
>              'testing)
> 
> (remove-overlays (point-min) (point-max))
> 
> (put-text-property (point-min) (point-max) 'font-lock-face 'testing)
> ------

Thanks.

> I have not yet looked into why Magit's diff faces extend beyond eol even
> though I didn't tell them too

Crystal ball says that Magit faces which inherit from some core faces
behave differently than those which don't inherit.

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

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jonas Bernoulli <jonas@bernoul.li>
>> Cc: 37774@debbugs.gnu.org
>> Date: Wed, 06 Nov 2019 17:26:12 +0100
>>
>> ------
>> (defface testing '((t (:background "LightYellow"))) "DOC")
>>
>> (overlay-put (make-overlay (point-min) (point-max) nil t)
>>              'face ; or 'font-lock-face
>>              'testing)
>>
>> (remove-overlays (point-min) (point-max))
>>
>> (put-text-property (point-min) (point-max) 'font-lock-face 'testing)
>> ------
>
> Thanks.
>
>> I have not yet looked into why Magit's diff faces extend beyond eol even
>> though I didn't tell them too
>
> Crystal ball says that Magit faces which inherit from some core faces
> behave differently than those which don't inherit.

Turns out it was the same issue as the one above.  (Magit's diff faces
do not inherit from diff faces that led to too many regressions when
themes changed diff faces without thinking of the somewhat different
needs of Magit.)

Okay then moving on to telling many Magit faces to :extend...
I'll be back.

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

[Jonas Bernoulli]

> I'll be back.

Currently if a package sets `:extend t` for some face, then that has no
effect if some theme modifies that face without explicitly repeating the
`:extend t'.

Lets use `hl-line' as an example.  Enable `hl-line-mode' and visit the
definition of the `hl-line' face.  You will notice that it `:extend t'
and that the highlighting indeed extends to the window edge.

Then enable any theme and notice how the highlighting no longer extends
to the edge of the window.

That's because the theme does something like:
  '(hl-line ((t (:background "lightgrey"))))
as opposed to
  '(hl-line ((t (:background "lightgrey" :extend t))))

I mentioned this elsewhere and Dmitry said that this is not how it is
supposed to work and if it did work that way then that would be a bug.

He also mentioned that this had been discussed here but I have been
reading this issue from the top while listening to all the way to
"The End" of the The Very Best Of The Doors and I still have not read
anything about something being done to prevent the need to repeat the
extend setting.  Message #104 mentions a variation of this issue, but
so far I haven't gotten to a message saying "okay lets add a hack to
deal with this" yet, so I figured I would ask:

Should it be unnecessary that each and every theme does:
-  '(hl-line ((t (:background "lightgrey"))))
+  '(hl-line ((t (:background "lightgrey" :extend t))))
?

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

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 37774@debbugs.gnu.org
> Date: Mon, 11 Nov 2019 20:03:45 +0100
> 
> Then enable any theme and notice how the highlighting no longer extends
> to the edge of the window.
> 
> That's because the theme does something like:
>   '(hl-line ((t (:background "lightgrey"))))
> as opposed to
>   '(hl-line ((t (:background "lightgrey" :extend t))))
> 
> I mentioned this elsewhere and Dmitry said that this is not how it is
> supposed to work and if it did work that way then that would be a bug.
> 
> He also mentioned that this had been discussed here but I have been
> reading this issue from the top while listening to all the way to
> "The End" of the The Very Best Of The Doors and I still have not read
> anything about something being done to prevent the need to repeat the
> extend setting.  Message #104 mentions a variation of this issue, but
> so far I haven't gotten to a message saying "okay lets add a hack to
> deal with this" yet, so I figured I would ask:
> 
> Should it be unnecessary that each and every theme does:
> -  '(hl-line ((t (:background "lightgrey"))))
> +  '(hl-line ((t (:background "lightgrey" :extend t))))
> ?

How is :extend different from any other face attribute?

The documentation of custom-theme-set-faces says that FACE should be a
face spec, like in defface.  And the latter does override all the
attributes, unless it uses :inherit.

So I'm not unsure why you expected something else.  AFAIU, we should
now modify all the themes that come with Emacs to use :extend for
those faces whose default spec does.

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

[Dmitry Gutov]

On 14.11.2019 13:33, Eli Zaretskii wrote:
>> Should it be unnecessary that each and every theme does:
>> -  '(hl-line ((t (:background "lightgrey"))))
>> +  '(hl-line ((t (:background "lightgrey" :extend t))))
>> ?
> How is :extend different from any other face attribute?
> 
> The documentation of custom-theme-set-faces says that FACE should be a
> face spec, like in defface.  And the latter does override all the
> attributes, unless it uses :inherit.
> 
> So I'm not unsure why you expected something else.

*I* expected that going by your messages here and here:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#104

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#131

Had I been mistaken?

Then the backward compatibility problem is going to be bigger than I 
thought. That's too bad. And my apologies to Jonas.

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

[Eli Zaretskii]

> Cc: 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 14 Nov 2019 16:14:16 +0200
> 
> > How is :extend different from any other face attribute?
> > 
> > The documentation of custom-theme-set-faces says that FACE should be a
> > face spec, like in defface.  And the latter does override all the
> > attributes, unless it uses :inherit.
> > 
> > So I'm not unsure why you expected something else.
> 
> *I* expected that going by your messages here and here:
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#104
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#131

That was about custom-set-faces, not custom-theme-set-faces.  The
former is a function we write into the user files, so it's hard to
expect anyone to insert :extend there.  And it was only a question, to
which I still don't have an answer (the issue of user face
customizations somehow stopped being discussed).

custom-theme-set-faces is different: it's code written by theme
authors, so we could expect them to cater to :extend.

> Then the backward compatibility problem is going to be bigger than I 
> thought. That's too bad. And my apologies to Jonas.

We are still discussing, so I see no need for apologies.

If the backward compatibility (or, rather, transparent DWIM-ish
operation) is the overriding consideration, then you are actually
saying that any face attribute we will introduce in the future will
have to be treated the same?  IOW, we will have to "inherit" it from
the default face definition even if :inherit was not specified?  If
so, how does a theme refuse to "inherit" in this way?

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

[Dmitry Gutov]

On 14.11.2019 16:41, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Thu, 14 Nov 2019 16:14:16 +0200
>>
>>> How is :extend different from any other face attribute?
>>>
>>> The documentation of custom-theme-set-faces says that FACE should be a
>>> face spec, like in defface.  And the latter does override all the
>>> attributes, unless it uses :inherit.
>>>
>>> So I'm not unsure why you expected something else.
>>
>> *I* expected that going by your messages here and here:
>>
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#104
>>
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37774#131
> 
> That was about custom-set-faces, not custom-theme-set-faces.  The
> former is a function we write into the user files, so it's hard to
> expect anyone to insert :extend there.

Okay, I didn't catch the distinction.

> custom-theme-set-faces is different: it's code written by theme
> authors, so we could expect them to cater to :extend.

Lots of themes out there, though. Lots of said authors. Who will have to 
do a version check and the splat-unquote thing, every one of them. I 
think that's pretty bad.

>> Then the backward compatibility problem is going to be bigger than I
>> thought. That's too bad. And my apologies to Jonas.
> 
> We are still discussing, so I see no need for apologies.
> 
> If the backward compatibility (or, rather, transparent DWIM-ish
> operation) is the overriding consideration, then you are actually
> saying that any face attribute we will introduce in the future will
> have to be treated the same?

I don't know what attributes we will introduce, and whether the default 
values will be a departure from the previous behavior like this one is.

But please note that having it a face attribute was your choice (or 
maybe Ergus's). I suggested using a symbol property instead. Though I 
was admittedly late to the party. Doing it in this way would side-step a 
number of questions like the ones you just asked.

> IOW, we will have to "inherit" it from
> the default face definition even if :inherit was not specified?  If
> so, how does a theme refuse to "inherit" in this way?

By setting it to an explicit nil value? Since the default is known, this 
should have the exact same effect.

BTW, does this scheme have a chance of working at all? IIUC, 
custom-theme-set-faces also handles faces which are not defined at all 
(yet), so inheriting an attribute from its default value might not be 
too easy.

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

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 15 Nov 2019 00:42:40 +0200
> 
> > custom-theme-set-faces is different: it's code written by theme
> > authors, so we could expect them to cater to :extend.
> 
> Lots of themes out there, though. Lots of said authors. Who will have to 
> do a version check and the splat-unquote thing, every one of them. I 
> think that's pretty bad.

It's not bad, because IMO most faces don't need to be changed at all.
That Jonas and others went ahead and decided to change almost all of
them is IMO a mistake.

> > If the backward compatibility (or, rather, transparent DWIM-ish
> > operation) is the overriding consideration, then you are actually
> > saying that any face attribute we will introduce in the future will
> > have to be treated the same?
> 
> I don't know what attributes we will introduce, and whether the default 
> values will be a departure from the previous behavior like this one is.

It doesn't matter if the default face definition uses that attribute,
does it?

> But please note that having it a face attribute was your choice (or 
> maybe Ergus's). I suggested using a symbol property instead. Though I 
> was admittedly late to the party. Doing it in this way would side-step a 
> number of questions like the ones you just asked.

Using a symbol property is extremely unclean, IMO, and would
unnecessarily complicate the face-merging code.

> > IOW, we will have to "inherit" it from
> > the default face definition even if :inherit was not specified?  If
> > so, how does a theme refuse to "inherit" in this way?
> 
> By setting it to an explicit nil value?

That does nothing if the :inherit is not explicit.  Unless, again, we
complicate the heck out of the face-merging code.

> BTW, does this scheme have a chance of working at all? IIUC, 
> custom-theme-set-faces also handles faces which are not defined at all 
> (yet), so inheriting an attribute from its default value might not be 
> too easy.

Probably not, and I said up front I didn't see how doing this will
fly.

I guess the best solution at this point is to require all themes to
make the appropriate changes.

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

[Dmitry Gutov]

On 15.11.2019 10:08, Eli Zaretskii wrote:

>> Lots of themes out there, though. Lots of said authors. Who will have to
>> do a version check and the splat-unquote thing, every one of them. I
>> think that's pretty bad.
> 
> It's not bad, because IMO most faces don't need to be changed at all.
> That Jonas and others went ahead and decided to change almost all of
> them is IMO a mistake.

I'm not sure which faces he changed exactly, but IIUC there were at 
least 5 faces that need to be changed (maybe more). And that would have 
to happen in every theme.

>>> If the backward compatibility (or, rather, transparent DWIM-ish
>>> operation) is the overriding consideration, then you are actually
>>> saying that any face attribute we will introduce in the future will
>>> have to be treated the same?
>>
>> I don't know what attributes we will introduce, and whether the default
>> values will be a departure from the previous behavior like this one is.
> 
> It doesn't matter if the default face definition uses that attribute,
> does it?

Well, it kind of does. At least, if the default value of the new 
attribute is in line with the previous behavior, most faces won't have 
to change. They *might* (or some of them might), but that would be on 
the authors of that code, and the new feature would trickle down 
gradually, like we usually do with Emacs.

>> But please note that having it a face attribute was your choice (or
>> maybe Ergus's). I suggested using a symbol property instead. Though I
>> was admittedly late to the party. Doing it in this way would side-step a
>> number of questions like the ones you just asked.
> 
> Using a symbol property is extremely unclean, IMO, and would
> unnecessarily complicate the face-merging code.

Fair enough.

Another option that had been voiced is to split the value into two 
attributes: :extend-foreground and :extend-background. Then we can set 
the default values for both an immediate improvement (:extend-foreground 
to nil) and maximum compatibility (:extend-background to t).

But that brings me to a question. I think whether the 'region' face has 
:extend-background to nil or not is a personal choice. Would the user 
have to fight and convince the author of the theme they are using to 
change that attribute? Or will it be easy to apply personal 
customization and call it a day?

> I guess the best solution at this point is to require all themes to
> make the appropriate changes.

Or that.

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

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 16 Nov 2019 01:50:22 +0200
> 
> >>> If the backward compatibility (or, rather, transparent DWIM-ish
> >>> operation) is the overriding consideration, then you are actually
> >>> saying that any face attribute we will introduce in the future will
> >>> have to be treated the same?
> >>
> >> I don't know what attributes we will introduce, and whether the default
> >> values will be a departure from the previous behavior like this one is.
> > 
> > It doesn't matter if the default face definition uses that attribute,
> > does it?
> 
> Well, it kind of does. At least, if the default value of the new 
> attribute is in line with the previous behavior, most faces won't have 
> to change.

I was talking about the case where the defface we have for that face
DOES use the new attribute.  In that case, the default value of the
attribute doesn't matter, since the defface uses some specific value,
and that will always be a non-default value.

IOW, whenever we introduce a new face attribute and use it to modify
the defface of a built-in face, this problem will pop up.

> Another option that had been voiced is to split the value into two 
> attributes: :extend-foreground and :extend-background.

But :extend is not just about colors, it is also about underline,
overline, strike-through, and box attributes.  In fact, the underline
attribute was an even more important one, because extending it looks
exceptionally ugly (we even had a few bug reports about that).

> But that brings me to a question. I think whether the 'region' face has 
> :extend-background to nil or not is a personal choice. Would the user 
> have to fight and convince the author of the theme they are using to 
> change that attribute? Or will it be easy to apply personal 
> customization and call it a day?

Why would using a theme need anything beyond a simple face
customization to modify :extend (or any other attribute)?  The author
of a theme can do what they think is best, but users can always
override that by customizing the face after loading the theme.

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

[Dmitry Gutov]

On 16.11.2019 10:09, Eli Zaretskii wrote:

>> Well, it kind of does. At least, if the default value of the new
>> attribute is in line with the previous behavior, most faces won't have
>> to change.
> 
> I was talking about the case where the defface we have for that face
> DOES use the new attribute.  In that case, the default value of the
> attribute doesn't matter, since the defface uses some specific value,
> and that will always be a non-default value.

My point is those should be more rare. Or, well, bring significant value 
somehow.

> IOW, whenever we introduce a new face attribute and use it to modify
> the defface of a built-in face, this problem will pop up.

Yes. But in some of those cases third-party faces would not have to be 
updated. And if the default doesn't change the behavior from the 
previous Emacs releases, they certainly wouldn't have to be updated 
right away.

>> Another option that had been voiced is to split the value into two
>> attributes: :extend-foreground and :extend-background.
> 
> But :extend is not just about colors, it is also about underline,
> overline, strike-through, and box attributes.  In fact, the underline
> attribute was an even more important one, because extending it looks
> exceptionally ugly (we even had a few bug reports about that).

I think the idea for this approach is to consider underline, overline, 
strike-through to be a part of foreground. Maybe box as well, I'm not sure.

>> But that brings me to a question. I think whether the 'region' face has
>> :extend-background to nil or not is a personal choice. Would the user
>> have to fight and convince the author of the theme they are using to
>> change that attribute? Or will it be easy to apply personal
>> customization and call it a day?
> 
> Why would using a theme need anything beyond a simple face
> customization to modify :extend (or any other attribute)?  The author
> of a theme can do what they think is best, but users can always
> override that by customizing the face after loading the theme.

OK, good. :-)

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

[Juri Linkov]

The release pretest deadline is quickly approaching, but it seems the
face-extend feature is still unfinished.

At least, I see that even though diff-refine-added and diff-refine-removed
faces have no :extend face attribute, but still extend to the window edge.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 24 Nov 2019 01:20:20 +0200
> 
> The release pretest deadline is quickly approaching, but it seems the
> face-extend feature is still unfinished.

Is there something else to be developed?  I wasn't aware of anything
that didn't get implemented already.

> At least, I see that even though diff-refine-added and diff-refine-removed
> faces have no :extend face attribute, but still extend to the window edge.

Was this reported somewhere?  If so, I guess I missed it.

Anyway, I think I see the problem, and I'm testing a solution.  But in
any case, starting a release cycle doesn't mean we will release Emacs
27.1 tomorrow, it just means we will start pretesting it, so there
will be plenty of time to find and fix bugs in its features.  We need
to consider only development of new features when we decide whether to
cut a release branch.

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

[Juanma Barranquero]

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

On Mon, Nov 25, 2019 at 4:30 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Is there something else to be developed?  I wasn't aware of anything
> that didn't get implemented already.

Near the end of this thread

https://lists.gnu.org/archive/html/emacs-devel/2019-10/msg00590.html

Ergus said:

Somehow this is related with something we discussed some time ago, about

the fact that we should call extend_face_to_end_of_line in the last line

of the buffer if not empty in some conditions (dfci is active for

example.)

Maybe you remember that we don't have the indicator for the last line,

which somehow we agreed must be corrected. In this case the problem is

the same: the extend_face... function is not called for the latest line

in the buffer but I didn't find a better condition to fix this (I didn't

try very hard either) But probably it just requires to extend a

condition in an if and part of this problem will be fixed (the case for

the last line at least)

There are some conditions in the display_line function to not call

extend_face_to... when the line ends at ZV, fixing this condition we

should be done right?


Was this resolved / fixed?

[-- Attachment #2: Type: text/html, Size: 2592 bytes --]

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

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 26 Nov 2019 00:45:56 +0100
> Cc: Juri Linkov <juri@linkov.net>, 37774@debbugs.gnu.org
> 
> > Is there something else to be developed?  I wasn't aware of anything
> > that didn't get implemented already.
> 
> Near the end of this thread
> 
> https://lists.gnu.org/archive/html/emacs-devel/2019-10/msg00590.html 
> 
> Ergus said:
> 
>  Somehow this is related with something we discussed some time ago, about
> the fact that we should call extend_face_to_end_of_line in the last line
> of the buffer if not empty in some conditions (dfci is active for
> example.)
> Maybe you remember that we don't have the indicator for the last line,
> which somehow we agreed must be corrected. In this case the problem is
> the same: the extend_face... function is not called for the latest line
> in the buffer but I didn't find a better condition to fix this (I didn't
> try very hard either) But probably it just requires to extend a
> condition in an if and part of this problem will be fixed (the case for
> the last line at least)
> There are some conditions in the display_line function to not call
> extend_face_to... when the line ends at ZV, fixing this condition we
> should be done right?
> 
> Was this resolved / fixed?

I responded to that in that discussion.  Basically, I don't think this
is a problem, certainly not one that needs to prevent us from starting
the pretest.

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

[Dmitry Gutov]

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

On 24.11.2019 1:20, Juri Linkov wrote:
> The release pretest deadline is quickly approaching, but it seems the
> face-extend feature is still unfinished.
> 
> At least, I see that even though diff-refine-added and diff-refine-removed
> faces have no :extend face attribute, but still extend to the window edge.

Indeed. I'm attaching a screenshot to illustrate.

BTW, the diff-context needs ':extend t' as well. But that's of little 
importance since as soon as I load a custom theme, whatever defaults 
were there don't seem to matter.

[-- Attachment #2: Screenshot from 2019-11-25 02-24-14.png --]
[-- Type: image/png, Size: 111618 bytes --]

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

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 25 Nov 2019 02:29:26 +0200
> 
> > At least, I see that even though diff-refine-added and diff-refine-removed
> > faces have no :extend face attribute, but still extend to the window edge.
> 
> Indeed. I'm attaching a screenshot to illustrate.

Thanks.  Face merging didn't work correctly with faces some of which
have :extend non-nil and some don't, when face inheritance was
involved.

Should be fixed now.

> BTW, the diff-context needs ':extend t' as well.

Feel free to make that change (although when did you last see a
context diff?).

> But that's of little importance since as soon as I load a custom
> theme, whatever defaults were there don't seem to matter.

We need to modify all the themes we provide to specify :extend for
faces where we do that by default.  It seems there's no way around
that, since the semantics of custom-theme-set-faces is clearly to
reset all face attributes to 'unspecified' before applying the face
spec, so keeping some attributes from the default face spec is out of
the question, unfortunately.  It's clear that the faces stuff was not
designed to accommodate addition of attributes easily.

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

[Dmitry Gutov]

On 25.11.2019 18:00, Eli Zaretskii wrote:

> Should be fixed now.

Thanks.

>> BTW, the diff-context needs ':extend t' as well.
> 
> Feel free to make that change (although when did you last see a
> context diff?).

It's not for context diffs, it's for context around the changes in 
unified diffs as well. Notice the gray background on the screenshot.

>> But that's of little importance since as soon as I load a custom
>> theme, whatever defaults were there don't seem to matter.
> 
> We need to modify all the themes we provide to specify :extend for
> faces where we do that by default.  It seems there's no way around
> that, since the semantics of custom-theme-set-faces is clearly to
> reset all face attributes to 'unspecified' before applying the face
> spec, so keeping some attributes from the default face spec is out of
> the question, unfortunately.  It's clear that the faces stuff was not
> designed to accommodate addition of attributes easily.

Seems like it's a consequence of the implementation strategy. There were 
a couple of others that had been proposed (splitting the attribute in 
two, with different default values) or using a symbol property.

You said the latter would complicate the face merging code (which makes 
sense) and "is extremely unclean". I have to take you at your word here. 
But it would relieve the theme authors of having to support this 
attribute explicitly.

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

[Eli Zaretskii]

> Cc: juri@linkov.net, 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 26 Nov 2019 01:50:23 +0200
> 
> On 25.11.2019 18:00, Eli Zaretskii wrote:
> 
> > Feel free to make that change (although when did you last see a
> > context diff?).
> 
> It's not for context diffs, it's for context around the changes in 
> unified diffs as well. Notice the gray background on the screenshot.

Ah, okay.  Still, feel free to change its :extend attribute.

> > We need to modify all the themes we provide to specify :extend for
> > faces where we do that by default.  It seems there's no way around
> > that, since the semantics of custom-theme-set-faces is clearly to
> > reset all face attributes to 'unspecified' before applying the face
> > spec, so keeping some attributes from the default face spec is out of
> > the question, unfortunately.  It's clear that the faces stuff was not
> > designed to accommodate addition of attributes easily.
> 
> Seems like it's a consequence of the implementation strategy. There were 
> a couple of others that had been proposed (splitting the attribute in 
> two, with different default values) or using a symbol property.

Splitting into two attributes won't help here (quite the contrary,
since we'd have to deal with 2 new attributes).  As for the symbol
property suggestion, see below.

> You said the latter would complicate the face merging code (which makes 
> sense) and "is extremely unclean". I have to take you at your word here. 

You don't have to take my word for it, the code is there to read and
make up your own mind.

Basically, on the C level each Lisp face is represented by an array of
its attributes, where each array element holds the value of the
corresponding attribute.  Once this array is computed, we can (and do)
manipulate just the array, and for all practical purposes can forget
about the face's symbol.  Using a symbol property would then need to
keep the symbol around at all times, which is inconvenient and would
make the code ugly.

But even if we'd overcome this annoyance, how do you specify this
property for a face like below?

   '(:inherit foo :background "green" :underline "red")

There's no symbol to put the property on.  Do we say that such
anonymous faces cannot support this attribute?  Unclean.

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

[Dmitry Gutov]

On 26.11.2019 19:43, Eli Zaretskii wrote:

>> It's not for context diffs, it's for context around the changes in
>> unified diffs as well. Notice the gray background on the screenshot.
> 
> Ah, okay.  Still, feel free to change its :extend attribute.

OK, done. But it feels pretty useless, considering any theme where this 
would affect something would have to repeat the 'extend t' definition 
anyway. And this would work without the change I just made anyway.

Too bad we don't have a way to affect all themes at once here.

>>> We need to modify all the themes we provide to specify :extend for
>>> faces where we do that by default.  It seems there's no way around
>>> that, since the semantics of custom-theme-set-faces is clearly to
>>> reset all face attributes to 'unspecified' before applying the face
>>> spec, so keeping some attributes from the default face spec is out of
>>> the question, unfortunately.  It's clear that the faces stuff was not
>>> designed to accommodate addition of attributes easily.
>>
>> Seems like it's a consequence of the implementation strategy. There were
>> a couple of others that had been proposed (splitting the attribute in
>> two, with different default values) or using a symbol property.
> 
> Splitting into two attributes won't help here (quite the contrary,
> since we'd have to deal with 2 new attributes).

That depends on the end goal. If we were aiming to keep the previous 
behavior almost entirely, but avoid extending things like underline over 
newlines, there are two default values for the two proposed attributes 
that would satisfy almost everybody. And the people who would prefer not 
to extend the region face background over newlines would apply that in 
their init scripts (ditto for other faces).

Sounds like a win-win, avoiding the necessity to update all themes, 
first- and third-party ones. Also it seems like it's close to the way we 
usually introduce far-reaching changes in Emacs. But it's one option.

> As for the symbol
> property suggestion, see below.

See below as well (option two, in my opinion).

>> You said the latter would complicate the face merging code (which makes
>> sense) and "is extremely unclean". I have to take you at your word here.
> 
> You don't have to take my word for it, the code is there to read and
> make up your own mind.

Forgive my ignorance, but xfaces.c is 7K lines long. It will be easier 
to speak high-level, but please correct whatever misconceptions I may bring.

> Basically, on the C level each Lisp face is represented by an array of
> its attributes, where each array element holds the value of the
> corresponding attribute.  Once this array is computed, we can (and do)
> manipulate just the array, and for all practical purposes can forget
> about the face's symbol.  Using a symbol property would then need to
> keep the symbol around at all times, which is inconvenient and would
> make the code ugly.

I don't think so. Once the symbol is gone, whatever left is just the 
value. So when this array is computed, the function doing that would 
merge the faces attributes with whatever attributes are specified using 
the alternative symbol property.

To be more exact, the current face attributes are also assigned to a 
symbol property (face-defface-spec). We can add another property: 
face-default-spec, which would contain attributes that should apply to 
the face unless explicitly overridden in face-defface-spec. It could 
even be set by a new defface keyword instead of plain 'put', but that's 
a minor concern IMO. (Option two ends here).

This seems to be the easiest way to go around the long-established 
behavior that custom-theme-set-faces overwrites the face attributes 
(instead of trying to merge them). We could do in the other way, but it 
would require changes in both custom-theme-set-faces and defface, as 
well as some other functions I imagine, and either a whitelist of 
attributes that would always be retained unless overridden. Or a 
wholesale change to retain all attributes by default. I might like the 
last option personally, but it would be a major breaking change. Still, 
all themes could be updated to account for it while keeping 
compatibility with older Emacs with little trouble (which is harder to 
do with the current :extend situation).

> But even if we'd overcome this annoyance, how do you specify this
> property for a face like below?
> 
>     '(:inherit foo :background "green" :underline "red")
> 
> There's no symbol to put the property on.  Do we say that such
> anonymous faces cannot support this attribute?  Unclean.

See above. Just add ':extend t' (or nil) to the end of the value.

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: Mon, 2 Dec 2019 02:05:10 +0200
> 
>     feel free to change its :extend attribute.
> 
> OK, done. But it feels pretty useless, considering any theme where this would affect something would have to repeat the 'extend t' definition anyway.

That's true for themes, but what about face customization by users
using the likes of set-face-background?  For them, we should have the
attribute even when the default face definition is empty.

>         Seems like it's a consequence of the implementation strategy. There were
>         a couple of others that had been proposed (splitting the attribute in
>         two, with different default values) or using a symbol property.
>
>     Splitting into two attributes won't help here (quite the contrary,
>     since we'd have to deal with 2 new attributes).
>
> That depends on the end goal. If we were aiming to keep the previous behavior almost entirely, but avoid extending things like underline over newlines, there are two default values for the two proposed attributes that would satisfy almost everybody. And the people who would prefer not to extend the region face background over newlines would apply that in their init scripts (ditto for other faces). 

The context of this part was how themes customize faces, it was not
about users customizing the faces directly.  In the context of themes,
having two attributes would not have helped, because themes will have
to be changed anyway.

As for the other aspect of this: the idea was that almost all faces do
not need to be extended, something that no default will succeed to do
silently.

>     Basically, on the C level each Lisp face is represented by an array of
>     its attributes, where each array element holds the value of the
>     corresponding attribute.  Once this array is computed, we can (and do)
>     manipulate just the array, and for all practical purposes can forget
>     about the face's symbol.  Using a symbol property would then need to
>     keep the symbol around at all times, which is inconvenient and would
>     make the code ugly.
>
> I don't think so. Once the symbol is gone, whatever left is just the value. So when this array is computed, the function doing that would merge the faces attributes with whatever attributes are specified using the alternative symbol property. 

The array is computed only once, whereas merging happens many times
and in different places.  So we cannot compute the value of an
attribute only once, because its value depends on what other faces are
being merged, on whether their :extend attribute is set. and on
whether the particular merging process cares about :extend.

> To be more exact, the current face attributes are also assigned to a symbol property (face-defface-spec). We can add another property: face-default-spec, which would contain attributes that should apply to the face unless explicitly overridden in face-defface-spec. It could even be set by a new defface keyword instead of plain 'put', but that's a minor concern IMO. (Option two ends here).
> 
> This seems to be the easiest way to go around the long-established behavior that custom-theme-set-faces overwrites the face attributes (instead of trying to merge them). We could do in the other way, but it would require changes in both custom-theme-set-faces and defface, as well as some other functions I imagine, and either a whitelist of attributes that would always be retained unless overridden. Or a wholesale change to retain all attributes by default. I might like the last option personally, but it would be a major breaking change. Still, all themes could be updated to account for it while keeping compatibility with older Emacs with little trouble (which is harder to do with the current :extend situation).

I don't think I understand your proposal in concrete terms, and you
didn't read the code to check your proposal against the actual
implementation, so this is a sure way to misunderstandings and talking
past each other.  I can only say that face-defface-spec and other
properties of face symbols are used by custom.el on the Lisp level,
whereas we were talking about what happens on the C level.  On the C
level, face merging creates an unnamed face with its attributes
computed by the merging process, and that process currently takes the
:extend attribute into account.  Any proposal to use face symbol's
properties instead will have to explain how those properties are
communicated to the merging process.

>     But even if we'd overcome this annoyance, how do you specify this
>     property for a face like below?
> 
>         '(:inherit foo :background "green" :underline "red")
> 
>     There's no symbol to put the property on.  Do we say that such
>     anonymous faces cannot support this attribute?  Unclean.
> 
> See above. Just add ':extend t' (or nil) to the end of the value.

So you propose to have both symbol property _and_ a face attribute?

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

[Dmitry Gutov]

On 02.12.2019 18:21, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Mon, 2 Dec 2019 02:05:10 +0200
>>
>>      feel free to change its :extend attribute.
>>
>> OK, done. But it feels pretty useless, considering any theme where this would affect something would have to repeat the 'extend t' definition anyway.
> 
> That's true for themes, but what about face customization by users
> using the likes of set-face-background?  For them, we should have the
> attribute even when the default face definition is empty.

Fair enough.

> As for the other aspect of this: the idea was that almost all faces do
> not need to be extended, something that no default will succeed to do
> silently.

Whose idea was it?

I think we can all agree about not extending foreground and related 
attributes, but extending background is a long-standing behavior. So it 
would make sense to introduce non-extending backgrounds only in select 
faces and gradually. I think that's the general expectation in the Emacs 
community. But we don't have to do this, we can go another way.

>>      Basically, on the C level each Lisp face is represented by an array of
>>      its attributes, where each array element holds the value of the
>>      corresponding attribute.  Once this array is computed, we can (and do)
>>      manipulate just the array, and for all practical purposes can forget
>>      about the face's symbol.  Using a symbol property would then need to
>>      keep the symbol around at all times, which is inconvenient and would
>>      make the code ugly.
>>
>> I don't think so. Once the symbol is gone, whatever left is just the value. So when this array is computed, the function doing that would merge the faces attributes with whatever attributes are specified using the alternative symbol property.
> 
> The array is computed only once, whereas merging happens many times
> and in different places.  So we cannot compute the value of an
> attribute only once, because its value depends on what other faces are
> being merged, on whether their :extend attribute is set. and on
> whether the particular merging process cares about :extend.

I'm not talking about face merging (*).

I'm talking about merging the attributes from the two properties (old 
and new) when computing the value of the array.

TBH, this is difficult to discuss. "Merging" and "array computation" are 
very generic terms.

(*) We shouldn't be talking about face merging at all because whatever 
happens after an "array" has been "computed" is opaque to defface, 
custom-theme-set-faces and custom-set-faces. So we should only discuss 
how "array" is computed, and what affects its resulting value, and not 
whatever happens next.

>> To be more exact, the current face attributes are also assigned to a symbol property (face-defface-spec). We can add another property: face-default-spec, which would contain attributes that should apply to the face unless explicitly overridden in face-defface-spec. It could even be set by a new defface keyword instead of plain 'put', but that's a minor concern IMO. (Option two ends here).
>>
>> This seems to be the easiest way to go around the long-established behavior that custom-theme-set-faces overwrites the face attributes (instead of trying to merge them). We could do in the other way, but it would require changes in both custom-theme-set-faces and defface, as well as some other functions I imagine, and either a whitelist of attributes that would always be retained unless overridden. Or a wholesale change to retain all attributes by default. I might like the last option personally, but it would be a major breaking change. Still, all themes could be updated to account for it while keeping compatibility with older Emacs with little trouble (which is harder to do with the current :extend situation).
> 
> I don't think I understand your proposal in concrete terms, and you
> didn't read the code to check your proposal against the actual
> implementation, so this is a sure way to misunderstandings and talking
> past each other.

You haven't pointed at any code to read. So if this email doesn't help 
reach clarity, could you give some pointers?

> I can only say that face-defface-spec and other
> properties of face symbols are used by custom.el on the Lisp level,
> whereas we were talking about what happens on the C level.  On the C
> level, face merging creates an unnamed face with its attributes
> computed by the merging process, and that process currently takes the
> :extend attribute into account.  Any proposal to use face symbol's
> properties instead will have to explain how those properties are
> communicated to the merging process.

Like I said, the new property I suggested is a way to go around having 
to change custom-theme-set-faces in an incompatible way. We would create 
a second "namespace" for face attributes that wouldn't be overwritten.

>>      But even if we'd overcome this annoyance, how do you specify this
>>      property for a face like below?
>>
>>          '(:inherit foo :background "green" :underline "red")
>>
>>      There's no symbol to put the property on.  Do we say that such
>>      anonymous faces cannot support this attribute?  Unclean.
>>
>> See above. Just add ':extend t' (or nil) to the end of the value.
> 
> So you propose to have both symbol property _and_ a face attribute?

Yes. Sorry for not making this clear. Since you outlined the "unnamed 
face value" situation, I've had to amend the idea a little (**). 
Further, since the :extend attribute would still be used, this wouldn't 
require too many changes on the C level.

The new symbol property could be used for different attributes, but it 
seems like :extend needs it the most.

(**) Although we could go back to my previous suggestion of only setting 
"extend" via symbol property. That would still require the new :extend 
attribute, but it could remain hidden from the user and the Lisp code.

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: Tue, 3 Dec 2019 02:01:10 +0200
> 
> > As for the other aspect of this: the idea was that almost all faces do
> > not need to be extended, something that no default will succeed to do
> > silently.
> 
> Whose idea was it?

It's the main idea behind the feature.  Without it, the feature makes
no sense at all.

> I think we can all agree about not extending foreground and related 
> attributes, but extending background is a long-standing behavior. So it 
> would make sense to introduce non-extending backgrounds only in select 
> faces and gradually. I think that's the general expectation in the Emacs 
> community. But we don't have to do this, we can go another way.

I don't think we should restart this discussion, we already had it.

> > The array is computed only once, whereas merging happens many times
> > and in different places.  So we cannot compute the value of an
> > attribute only once, because its value depends on what other faces are
> > being merged, on whether their :extend attribute is set. and on
> > whether the particular merging process cares about :extend.
> 
> I'm not talking about face merging (*).

This whole issue, and in fact the feature itself, is about face
merging, and only about it.  Emacs displays faces by merging
attributes from all of the possible sources of face information that
are in effect at a given buffer position.

> > I don't think I understand your proposal in concrete terms, and you
> > didn't read the code to check your proposal against the actual
> > implementation, so this is a sure way to misunderstandings and talking
> > past each other.
> 
> You haven't pointed at any code to read. So if this email doesn't help 
> reach clarity, could you give some pointers?

I suggest to start with the large comment at the beginning of
xfaces.c, and then proceed to read these functions:

  get_lface_attributes
  merge_face_vectors
  merge_named_face
  merge_face_ref
  internal-make-lisp-face
  internal-set-lisp-face-attribute

The last two should make it clear how defface makes a face with all of
the attributes.

> Like I said, the new property I suggested is a way to go around having 
> to change custom-theme-set-faces in an incompatible way. We would create 
> a second "namespace" for face attributes that wouldn't be overwritten.
> 
> >>      But even if we'd overcome this annoyance, how do you specify this
> >>      property for a face like below?
> >>
> >>          '(:inherit foo :background "green" :underline "red")
> >>
> >>      There's no symbol to put the property on.  Do we say that such
> >>      anonymous faces cannot support this attribute?  Unclean.
> >>
> >> See above. Just add ':extend t' (or nil) to the end of the value.
> > 
> > So you propose to have both symbol property _and_ a face attribute?
> 
> Yes. Sorry for not making this clear. Since you outlined the "unnamed 
> face value" situation, I've had to amend the idea a little (**). 
> Further, since the :extend attribute would still be used, this wouldn't 
> require too many changes on the C level.
> 
> The new symbol property could be used for different attributes, but it 
> seems like :extend needs it the most.

Sorry, still not clear.  Maybe providing examples of defining a face
to be extended, and then repeating the above in more detail with
references to the examples, would help in clearing the picture.

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

[Dmitry Gutov]

On 03.12.2019 18:21, Eli Zaretskii wrote:

>> I think we can all agree about not extending foreground and related
>> attributes, but extending background is a long-standing behavior. So it
>> would make sense to introduce non-extending backgrounds only in select
>> faces and gradually. I think that's the general expectation in the Emacs
>> community. But we don't have to do this, we can go another way.
> 
> I don't think we should restart this discussion, we already had it.

Fair enough.

>>> The array is computed only once, whereas merging happens many times
>>> and in different places.  So we cannot compute the value of an
>>> attribute only once, because its value depends on what other faces are
>>> being merged, on whether their :extend attribute is set. and on
>>> whether the particular merging process cares about :extend.
>>
>> I'm not talking about face merging (*).
> 
> This whole issue, and in fact the feature itself, is about face
> merging, and only about it.  Emacs displays faces by merging
> attributes from all of the possible sources of face information that
> are in effect at a given buffer position.

If the effect of the new symbol property is recorded in the array, 
whatever calculations happen thereafter using different arrays that 
represent faces (named or otherwise) should be orthogonal to my proposal.

>>> I don't think I understand your proposal in concrete terms, and you
>>> didn't read the code to check your proposal against the actual
>>> implementation, so this is a sure way to misunderstandings and talking
>>> past each other.
>>
>> You haven't pointed at any code to read. So if this email doesn't help
>> reach clarity, could you give some pointers?
> 
> I suggest to start with the large comment at the beginning of
> xfaces.c, and then proceed to read these functions:

Thank you.

>    get_lface_attributes

So apparently lface_from_face_name_no_resolve is the place where a face 
name turns into an plist-like array of attributes.

So we could, as a rough idea, look up the symbol property there and, if 
present, merge its contents with the return value.

I'm not quite sure of the purpose behind Vface_new_frame_defaults (vs. 
just using props on face symbols), but we could add a similar storage 
for "transient face spec". And in the frame structs too.

>    merge_face_vectors
>    merge_named_face
>    merge_face_ref
>    internal-make-lisp-face
>    internal-set-lisp-face-attribute

The last function might grow a new optional attribute: TRANSIENTP, if we 
want to be able to change such "transient" attributes at runtime.

To define "transient" here: these will be attributes that are not 
affected by custom-theme-set-faces unless explicitly mentioned in the 
corresponding specs. Which is what we had been discussing for :extend 
for quite a while.

>> The new symbol property could be used for different attributes, but it
>> seems like :extend needs it the most.
> 
> Sorry, still not clear.  Maybe providing examples of defining a face
> to be extended, and then repeating the above in more detail with
> references to the examples, would help in clearing the picture.

The new definition for diff-added would look like:

   (defface diff-added
     '((default
        :inherit diff-changed)
       (((class color) (min-colors 257) (background light))
        :background "#eeffee")
       (((class color) (min-colors 88) (background light))
        :background "#ddffdd")
       (((class color) (min-colors 88) (background dark))
        :background "#335533")
       (((class color))
        :foreground "green"))
     "`diff-mode' face used to highlight added lines.")

   (put 'diff-added 'face-transient-spec '((t :extend t)))

Or maybe like:

   (defface diff-added
     '((default
        :inherit diff-changed)
       (((class color) (min-colors 257) (background light))
        :background "#eeffee")
       (((class color) (min-colors 88) (background light))
        :background "#ddffdd")
       (((class color) (min-colors 88) (background dark))
        :background "#335533")
       (((class color))
        :foreground "green"))
     "`diff-mode' face used to highlight added lines."
     :transient '((t :extend t)))

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: Thu, 5 Dec 2019 03:44:22 +0200
> 
> The new definition for diff-added would look like:
> 
>    (defface diff-added
>      '((default
>         :inherit diff-changed)
>        (((class color) (min-colors 257) (background light))
>         :background "#eeffee")
>        (((class color) (min-colors 88) (background light))
>         :background "#ddffdd")
>        (((class color) (min-colors 88) (background dark))
>         :background "#335533")
>        (((class color))
>         :foreground "green"))
>      "`diff-mode' face used to highlight added lines.")
> 
>    (put 'diff-added 'face-transient-spec '((t :extend t)))

OK, and how will this work to countermand the problem with themes?

custom-theme-set-faces calls face-spec-set, which calls
face-spec-recalc, which starts by resetting all face attributes to
'unspecified'.  And the last 2 functions are general-purpose, not
specific to themes.

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

[Dmitry Gutov]

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

On 05.12.2019 17:47, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Thu, 5 Dec 2019 03:44:22 +0200
>>
>> The new definition for diff-added would look like:
>>
>>     (defface diff-added
>>       '((default
>>          :inherit diff-changed)
>>         (((class color) (min-colors 257) (background light))
>>          :background "#eeffee")
>>         (((class color) (min-colors 88) (background light))
>>          :background "#ddffdd")
>>         (((class color) (min-colors 88) (background dark))
>>          :background "#335533")
>>         (((class color))
>>          :foreground "green"))
>>       "`diff-mode' face used to highlight added lines.")
>>
>>     (put 'diff-added 'face-transient-spec '((t :extend t)))
> 
> OK, and how will this work to countermand the problem with themes?
> 
> custom-theme-set-faces calls face-spec-set, which calls
> face-spec-recalc, which starts by resetting all face attributes to
> 'unspecified'.  And the last 2 functions are general-purpose, not
> specific to themes.

Well, the idea was to use a different structure to store the "transient" 
attributes. That could be an extra symbol property, or an additional 
structure for storing faces attributes, in addition to 
default-frame-alist. But looking at the code now, it seems fairly clunky 
and crossing abstraction levels.

It's great that you mentioned face-spec-recalc. It looks just like the 
place to change, since both defface and theme definitions and 
customizations go through it.

We can implement in there a new kind of "face spec" along the lines of 
my previous description, or simply special-case the :extend attribute, 
and take it from the default spec. The latter option is implemented in 
the attached patch, which seems to work in my limited testing.

[-- Attachment #2: inherit-face-extend-spec.diff --]
[-- Type: text/x-patch, Size: 1908 bytes --]

diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..b3659605a2 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1669,21 +1669,29 @@ face-spec-recalc
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
 	(no-match-found 0)
-	face-attrs theme-face-applied)
+        face-attrs theme-face-applied
+        default-attrs themed-extend-attr)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
 	  (setq face-attrs (face-spec-choose (cadr elt) frame no-match-found))
 	  (unless (eq face-attrs no-match-found)
+            (unless themed-extend-attr
+              (setq themed-extend-attr (plist-member face-attrs :extend)))
 	    (face-spec-set-2 face frame face-attrs)
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
-    ;; 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).
+    ;;
+    ;; If there was no spec applicable to FRAME, apply the defface
+    ;; spec as well as any applicable X resources.
+    (setq default-attrs (face-spec-choose (face-default-spec face) frame))
     (unless theme-face-applied
-      (setq face-attrs (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame face-attrs)
+      (face-spec-set-2 face frame default-attrs)
       (make-face-x-resource-internal face frame))
+    (when (and theme-face-applied (not themed-extend-attr))
+      (let ((extend-p (plist-get default-attrs :extend)))
+        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame face-attrs)))
 

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: Fri, 6 Dec 2019 17:44:33 +0200
> 
> It's great that you mentioned face-spec-recalc. It looks just like the 
> place to change, since both defface and theme definitions and 
> customizations go through it.
> 
> We can implement in there a new kind of "face spec" along the lines of 
> my previous description, or simply special-case the :extend attribute, 
> and take it from the default spec. The latter option is implemented in 
> the attached patch, which seems to work in my limited testing.

Thanks, but is it clean enough to do such exemption for :extend?

And if we want to do this, why do it in face-spec-recalc and not in
custom-set-faces itself?  The latter will not risk producing
unintended consequences for callers of face-spec-recalc other than
custom-set-faces.

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

You slightly modified the syntax of the comment, probably a typo.

> +    (when (and theme-face-applied (not themed-extend-attr))
> +      (let ((extend-p (plist-get default-attrs :extend)))
> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
                                                     ^^^^^^^^^^^^
I think this should be extend-p instead, because the face's default
spec could legitimately say ":extend nil".  Right?

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

[Dmitry Gutov]

On 06.12.2019 18:18, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 6 Dec 2019 17:44:33 +0200
>>
>> It's great that you mentioned face-spec-recalc. It looks just like the
>> place to change, since both defface and theme definitions and
>> customizations go through it.
>>
>> We can implement in there a new kind of "face spec" along the lines of
>> my previous description, or simply special-case the :extend attribute,
>> and take it from the default spec. The latter option is implemented in
>> the attached patch, which seems to work in my limited testing.
> 
> Thanks, but is it clean enough to do such exemption for :extend?

I think so. Most importantly, it will save a lot of people the trouble 
of adapting to the current change, limiting the necessary efforts to 
just package authors (and Emacs core, of course).

Further, as you noted in https://debbugs.gnu.org/37774#404, :extend is 
somewhat special, in that we really want its values to be consistent, so 
the results look uniform. It's not the only attribute like that, but for 
others we have different way to ensure that, such as having a default 
face (where font face, height, etc, are usually customized, instead of 
doing that in all individual faces).

> And if we want to do this, why do it in face-spec-recalc and not in
> custom-set-faces itself?

Not the place to do that. custom-theme-set-faces saves the specs defined 
by the theme (or user customization) to the face's symbol property, and 
then leaves it to face-spec-recalc to combine and apply all the specs 
related to the face.

> The latter will not risk producing
> unintended consequences for callers of face-spec-recalc other than
> custom-set-faces.

I think the purpose of face-spec-recalc is clear enough. So I'd like to 
see at least one of those "unintended consequences".

>> -    ;; 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).
> 
> You slightly modified the syntax of the comment, probably a typo.

I inserted an empty line for clarity (IMHO), but I certainly do not 
insist on it. There would be other documentation changes required, too.

>> +    (when (and theme-face-applied (not themed-extend-attr))
>> +      (let ((extend-p (plist-get default-attrs :extend)))
>> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
>                                                       ^^^^^^^^^^^^
> I think this should be extend-p instead, because the face's default
> spec could legitimately say ":extend nil".  Right?

But that's the default value of that attribute. If faces didn't specify 
it either, there's nothing to change, right?

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: Fri, 6 Dec 2019 18:58:26 +0200
> 
> > Thanks, but is it clean enough to do such exemption for :extend?
> 
> I think so. Most importantly, it will save a lot of people the trouble 
> of adapting to the current change, limiting the necessary efforts to 
> just package authors (and Emacs core, of course).

Well, we will have to document this exemption prominently, then.

> > And if we want to do this, why do it in face-spec-recalc and not in
> > custom-set-faces itself?
> 
> Not the place to do that. custom-theme-set-faces saves the specs defined 
> by the theme (or user customization) to the face's symbol property, and 
> then leaves it to face-spec-recalc to combine and apply all the specs 
> related to the face.

Is the patch likely to change any behavior except that of
custom-theme-set-faces?  I'd like to limit it only to that use case,
so that we don't risk breaking anything else.

> I think the purpose of face-spec-recalc is clear enough. So I'd like to 
> see at least one of those "unintended consequences".

Let's try to avoid such consequences from the get-go, okay?

> >> +    (when (and theme-face-applied (not themed-extend-attr))
> >> +      (let ((extend-p (plist-get default-attrs :extend)))
> >> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
> >                                                       ^^^^^^^^^^^^
> > I think this should be extend-p instead, because the face's default
> > spec could legitimately say ":extend nil".  Right?
> 
> But that's the default value of that attribute.

No, the default is 'unspecified', which is different from nil, when
merging with a face that specifies :extend, and when inheriting.  A
theme that says ':extend nil' should override the default spec, unlike
'unspecified'.

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

[Dmitry Gutov]

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

On 06.12.2019 19:31, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 6 Dec 2019 18:58:26 +0200
>>
>>> Thanks, but is it clean enough to do such exemption for :extend?
>>
>> I think so. Most importantly, it will save a lot of people the trouble
>> of adapting to the current change, limiting the necessary efforts to
>> just package authors (and Emacs core, of course).
> 
> Well, we will have to document this exemption prominently, then.

I suppose so. Though I wouldn't consider it too important to most users, 
considering that thanks to this change in semantics, most people won't 
have to do a thing when upgrading to the new version of Emacs.

>>> And if we want to do this, why do it in face-spec-recalc and not in
>>> custom-set-faces itself?
>>
>> Not the place to do that. custom-theme-set-faces saves the specs defined
>> by the theme (or user customization) to the face's symbol property, and
>> then leaves it to face-spec-recalc to combine and apply all the specs
>> related to the face.
> 
> Is the patch likely to change any behavior except that of
> custom-theme-set-faces? 

Only the behavior of other its callers (direct and indirect). :-)

Such as enable-theme, face-set-after-frame-default, 
frame-set-background-mode and face-spec-set. I'm pretty sure all of 
these should behave consistently WRT this change.

>> I think the purpose of face-spec-recalc is clear enough. So I'd like to
>> see at least one of those "unintended consequences".
> 
> Let's try to avoid such consequences from the get-go, okay?

I'm "avoiding such consequences" by trying to make sure the change is in 
the correct function and that it makes sense within the context.

>>>> +    (when (and theme-face-applied (not themed-extend-attr))
>>>> +      (let ((extend-p (plist-get default-attrs :extend)))
>>>> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
>>>                                                        ^^^^^^^^^^^^
>>> I think this should be extend-p instead, because the face's default
>>> spec could legitimately say ":extend nil".  Right?
>>
>> But that's the default value of that attribute.
> 
> No, the default is 'unspecified', which is different from nil, when
> merging with a face that specifies :extend, and when inheriting.  A
> theme that says ':extend nil' should override the default spec, unlike
> 'unspecified'.

This distinction is handled in the second hunk of the patch where 
themed-extend-attr is calculated. If this attribute is not themed, there 
is no difference between nil and 'unspecified' (in the default spec).

Finally, the value 'unspecified' seems impossible to get from the 
attributes list this way. plist-get will simply return nil.

That said, I think I've found one valid scenario where this patch will 
do wrong: if the themed spec includes an :inherit directive, and the 
inherited face specifies (:extend nil). The current patch would 
inevitably ignore it and override with the value from the default spec.

So here's a second try (attached).

[-- Attachment #2: inherit-face-extend-spec-2.diff --]
[-- Type: text/x-patch, Size: 1807 bytes --]

diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..64cb76b4fa 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1669,7 +1669,7 @@ face-spec-recalc
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
 	(no-match-found 0)
-	face-attrs theme-face-applied)
+        default-attrs face-attrs theme-face-applied)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
 	  (setq face-attrs (face-spec-choose (cadr elt) frame no-match-found))
@@ -1677,13 +1677,19 @@ face-spec-recalc
 	    (face-spec-set-2 face frame face-attrs)
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
-    ;; 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).
+    ;;
+    ;; If there was no spec applicable to FRAME, apply the defface
+    ;; spec as well as any applicable X resources.
+    (setq default-attrs (face-spec-choose (face-default-spec face) frame))
     (unless theme-face-applied
-      (setq face-attrs (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame face-attrs)
+      (face-spec-set-2 face frame default-attrs)
       (make-face-x-resource-internal face frame))
+    (when (and theme-face-applied
+               (eq 'unspecified (face-attribute face :extend frame t)))
+      (let ((extend-p (plist-get default-attrs :extend)))
+        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame face-attrs)))
 

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: Sat, 7 Dec 2019 03:06:05 +0200
> 
> > Well, we will have to document this exemption prominently, then.
> 
> I suppose so. Though I wouldn't consider it too important to most users, 

It's important to developers of Lisp programs that manipulate faces.

> > Is the patch likely to change any behavior except that of
> > custom-theme-set-faces? 
> 
> Only the behavior of other its callers (direct and indirect). :-)
> 
> Such as enable-theme, face-set-after-frame-default, 
> frame-set-background-mode and face-spec-set. I'm pretty sure all of 
> these should behave consistently WRT this change.
> 
> >> I think the purpose of face-spec-recalc is clear enough. So I'd like to
> >> see at least one of those "unintended consequences".
> > 
> > Let's try to avoid such consequences from the get-go, okay?
> 
> I'm "avoiding such consequences" by trying to make sure the change is in 
> the correct function and that it makes sense within the context.

I meant to avoid such consequences by making sure those other callers
can never trigger this new processing of :extend.  Can we please do
that?  That will go a long way towards my agreement to have this code
in Emacs 27.

> >>>> +    (when (and theme-face-applied (not themed-extend-attr))
> >>>> +      (let ((extend-p (plist-get default-attrs :extend)))
> >>>> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
> >>>                                                        ^^^^^^^^^^^^
> >>> I think this should be extend-p instead, because the face's default
> >>> spec could legitimately say ":extend nil".  Right?
> >>
> >> But that's the default value of that attribute.
> > 
> > No, the default is 'unspecified', which is different from nil, when
> > merging with a face that specifies :extend, and when inheriting.  A
> > theme that says ':extend nil' should override the default spec, unlike
> > 'unspecified'.
> 
> This distinction is handled in the second hunk of the patch where 
> themed-extend-attr is calculated. If this attribute is not themed, there 
> is no difference between nil and 'unspecified' (in the default spec).

The use case I had in mind is this:

  . the theme doesn't specify :extend
  . the default spec for a face specifies ':extend nil'

In this case, after applying the theme, the face should have
':extend nil', implicitly "inherited" from the default spec.  Do you
agree?

> Finally, the value 'unspecified' seems impossible to get from the 
> attributes list this way. plist-get will simply return nil.

Exactly.  And when a face does specify a nil value for :extend, then
plist will return the list '(:extend nil), which is non-nil.

> That said, I think I've found one valid scenario where this patch will 
> do wrong: if the themed spec includes an :inherit directive, and the 
> inherited face specifies (:extend nil). The current patch would 
> inevitably ignore it and override with the value from the default spec.

Once again, I think this part:

> +    (when (and theme-face-applied
> +               (eq 'unspecified (face-attribute face :extend frame t)))
> +      (let ((extend-p (plist-get default-attrs :extend)))
> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
                                                     ^^^^^^^^^^^^
isn't right, because it seems to say that when the default face says
':extend nil', the face after applying a theme will have ':extend t',
which isn't TRT, IMO.

Thanks.

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

[Dmitry Gutov]

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

On 07.12.2019 9:53, Eli Zaretskii wrote:

>>> Well, we will have to document this exemption prominently, then.
>>
>> I suppose so. Though I wouldn't consider it too important to most users,
> 
> It's important to developers of Lisp programs that manipulate faces.

Yes, sure.

>> I'm "avoiding such consequences" by trying to make sure the change is in
>> the correct function and that it makes sense within the context.
> 
> I meant to avoid such consequences by making sure those other callers
> can never trigger this new processing of :extend.

Eli, what you're asking for would be actively harmful.

To make an analogy, when we're changing a core Emacs behavior, we 
shouldn't try to make it work only on Tuesdays. Even if the original bug 
reporter observed the problem on a Tuesday.

Can we please focus on the more pressing question: whether the proposed 
patch actually works, and does that reliably, or are there 
scenarios/configurations where it would do something unexpected.

>> This distinction is handled in the second hunk of the patch where
>> themed-extend-attr is calculated. If this attribute is not themed, there
>> is no difference between nil and 'unspecified' (in the default spec).
> 
> The use case I had in mind is this:
> 
>    . the theme doesn't specify :extend
>    . the default spec for a face specifies ':extend nil'
> 
> In this case, after applying the theme, the face should have
> ':extend nil', implicitly "inherited" from the default spec.  Do you
> agree?

Ok, I think I understand the distinction: it's for when a character has 
several faces specified for it.

Sure. It's easy to fix anyway.

>> Finally, the value 'unspecified' seems impossible to get from the
>> attributes list this way. plist-get will simply return nil.
> 
> Exactly.  And when a face does specify a nil value for :extend, then
> plist will return the list '(:extend nil), which is non-nil.

plist-member, you mean.

>> That said, I think I've found one valid scenario where this patch will
>> do wrong: if the themed spec includes an :inherit directive, and the
>> inherited face specifies (:extend nil). The current patch would
>> inevitably ignore it and override with the value from the default spec.
> 
> Once again, I think this part:
> 
>> +    (when (and theme-face-applied
>> +               (eq 'unspecified (face-attribute face :extend frame t)))
>> +      (let ((extend-p (plist-get default-attrs :extend)))
>> +        (and extend-p (face-spec-set-2 face frame '(:extend t)))))
>                                                       ^^^^^^^^^^^^
> isn't right, because it seems to say that when the default face says
> ':extend nil', the face after applying a theme will have ':extend t',
> which isn't TRT, IMO.

Updated patch attached.

[-- Attachment #2: inherit-face-extend-spec-3.diff --]
[-- Type: text/x-patch, Size: 1852 bytes --]

diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..0f31628f5f 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1669,7 +1669,7 @@ face-spec-recalc
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
 	(no-match-found 0)
-	face-attrs theme-face-applied)
+        default-attrs face-attrs theme-face-applied)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
 	  (setq face-attrs (face-spec-choose (cadr elt) frame no-match-found))
@@ -1677,13 +1677,20 @@ face-spec-recalc
 	    (face-spec-set-2 face frame face-attrs)
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
-    ;; 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).
+    ;;
+    ;; If there was no spec applicable to FRAME, apply the defface
+    ;; spec as well as any applicable X resources.
+    (setq default-attrs (face-spec-choose (face-default-spec face) frame))
     (unless theme-face-applied
-      (setq face-attrs (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame face-attrs)
+      (face-spec-set-2 face frame default-attrs)
       (make-face-x-resource-internal face frame))
+    (when (and theme-face-applied
+               (eq 'unspecified (face-attribute face :extend frame t)))
+      (let ((tail (plist-member default-attrs :extend)))
+        (and tail (face-spec-set-2 face frame
+                                   (list :extend (cadr tail))))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame face-attrs)))
 

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: Sat, 7 Dec 2019 19:06:08 +0200
> 
> > I meant to avoid such consequences by making sure those other callers
> > can never trigger this new processing of :extend.
> 
> Eli, what you're asking for would be actively harmful.
> 
> To make an analogy, when we're changing a core Emacs behavior, we 
> shouldn't try to make it work only on Tuesdays. Even if the original bug 
> reporter observed the problem on a Tuesday.

Sorry, I don't see the analogy.

We are not trying to change the core behavior, we are trying to change
how themes define faces, in a way that makes the :extend attribute be
implicitly inherited from the default spec of the face to the face
after theme's customizations.  All I want is to make sure no other
caller of face-spec-recalc, one that has nothing to do with themes
defining faces, picks up the change, because that would be unintended.
If you consider this incorrect or unjustified, please explain why.

> Can we please focus on the more pressing question: whether the proposed 
> patch actually works, and does that reliably, or are there 
> scenarios/configurations where it would do something unexpected.

We were considering only one scenario: that of a theme defining its
own face specs.  face-spec-recalc is called in other scenarios, but I
don't think we should consider them, because we don't want to change
the behavior in any of those other scenarios.

> >    . the theme doesn't specify :extend
> >    . the default spec for a face specifies ':extend nil'
> > 
> > In this case, after applying the theme, the face should have
> > ':extend nil', implicitly "inherited" from the default spec.  Do you
> > agree?
> 
> Ok, I think I understand the distinction: it's for when a character has 
> several faces specified for it.

Yes, it's important when merging with those other faces that are in
effect for displaying a character.

> +    (when (and theme-face-applied
> +               (eq 'unspecified (face-attribute face :extend frame t)))
> +      (let ((tail (plist-member default-attrs :extend)))
> +        (and tail (face-spec-set-2 face frame
> +                                   (list :extend (cadr tail))))))

This is OK, but why say

    (list :extend (cadr tail))

instead of just

    tail

?  Unless I'm missing something here, the value of 'tail' here should
be (:extend VAL), where VAL is either t or nil.  Right?

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

[Dmitry Gutov]

On 07.12.2019 19:53, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Sat, 7 Dec 2019 19:06:08 +0200
>>
>>> I meant to avoid such consequences by making sure those other callers
>>> can never trigger this new processing of :extend.
>>
>> Eli, what you're asking for would be actively harmful.
>>
>> To make an analogy, when we're changing a core Emacs behavior, we
>> shouldn't try to make it work only on Tuesdays. Even if the original bug
>> reporter observed the problem on a Tuesday.
> 
> Sorry, I don't see the analogy.
> 
> We are not trying to change the core behavior, we are trying to change
> how themes define faces, in a way that makes the :extend attribute be
> implicitly inherited from the default spec of the face to the face
> after theme's customizations.

We're really trying to change how a face's attributes are calculated 
based on its default spec, as well as enabled themes. There are 
different callers to face-spec-recalc because different user features 
require that re-calculation.

> All I want is to make sure no other
> caller of face-spec-recalc, one that has nothing to do with themes
> defining faces, picks up the change, because that would be unintended.
> If you consider this incorrect or unjustified, please explain why.

Here's some examples:

enable-theme needs that recalculation because a different set of themes 
is now in effect, and face attributes need to be updated.

face-set-after-frame-default needs that because a frame's parameters can 
affect how faces look as well.

frame-set-background-mode needs that to update how face specs are 
interpreted given the changed background mode.

All of these affect how a face spec is evaluated, which affects how 
theme specs and user specs apply to the face. Which should be able to 
change which spec the value of :extend is taken from.

Or, to look at it from another direction: if we create a special 
different version of face-spec-recalc purely for custom-theme-set-faces, 
and face-set-after-frame-default wouldn't use it, whatever changed logic 
we implement wouldn't apply to new frames.

>> Can we please focus on the more pressing question: whether the proposed
>> patch actually works, and does that reliably, or are there
>> scenarios/configurations where it would do something unexpected.
> 
> We were considering only one scenario: that of a theme defining its
> own face specs.

Right. But this scenario has different configurations. Like a themed 
spec can inherit from some other face (and the first face's default has 
':extend t' as well). I was wondering what's going to happen if the user 
customizes that other face to have ':extend t' or ':extend nil'. But my 
testing shows it behaves as expected.

> face-spec-recalc is called in other scenarios, but I
> don't think we should consider them, because we don't want to change
> the behavior in any of those other scenarios.

I'm pretty sure they'll be fine. Or if not, it'll likely be a bug 
somewhere else.

>> +    (when (and theme-face-applied
>> +               (eq 'unspecified (face-attribute face :extend frame t)))
>> +      (let ((tail (plist-member default-attrs :extend)))
>> +        (and tail (face-spec-set-2 face frame
>> +                                   (list :extend (cadr tail))))))
> 
> This is OK, but why say
> 
>      (list :extend (cadr tail))
> 
> instead of just
> 
>      tail
> 
> ?  Unless I'm missing something here, the value of 'tail' here should
> be (:extend VAL), where VAL is either t or nil.  Right?

I'm not sure :extend is always the last pair in the list.

ELISP> (plist-member '(:a 1 :b 2 :c 3) :b)
(:b 2 :c 3)

I could use map-elt, though. If it's allowed in faces.el.

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: Sat, 7 Dec 2019 20:55:31 +0200
> 
> > We are not trying to change the core behavior, we are trying to change
> > how themes define faces, in a way that makes the :extend attribute be
> > implicitly inherited from the default spec of the face to the face
> > after theme's customizations.
> 
> We're really trying to change how a face's attributes are calculated 
> based on its default spec, as well as enabled themes. There are 
> different callers to face-spec-recalc because different user features 
> require that re-calculation.
> 
> > All I want is to make sure no other
> > caller of face-spec-recalc, one that has nothing to do with themes
> > defining faces, picks up the change, because that would be unintended.
> > If you consider this incorrect or unjustified, please explain why.
> 
> Here's some examples:
> 
> enable-theme needs that recalculation because a different set of themes 
> is now in effect, and face attributes need to be updated.
> 
> face-set-after-frame-default needs that because a frame's parameters can 
> affect how faces look as well.
> 
> frame-set-background-mode needs that to update how face specs are 
> interpreted given the changed background mode.
> 
> All of these affect how a face spec is evaluated, which affects how 
> theme specs and user specs apply to the face. Which should be able to 
> change which spec the value of :extend is taken from.
> 
> Or, to look at it from another direction: if we create a special 
> different version of face-spec-recalc purely for custom-theme-set-faces, 
> and face-set-after-frame-default wouldn't use it, whatever changed logic 
> we implement wouldn't apply to new frames.

Our goal is to allow themes "inherit" the :extend attribute without
having to specify it in their face specs, unlike with other
attributes.  That's the only goal; we don't want :extend to behave
differently from other face attributes in any other context.

If you are saying that we cannot make this change apply only to face
definitions by themes, then it means we don't really understand what
we could break here, and then I don't think I want this change in
Emacs 27.  Sorry, it's too risky.

(I thought cus-face.el stores information in symbol properties that
enables it to apply the face attributes in a special way.  But I don't
consider myself an expert on these matters, so if you say we cannot
differentiate between general face definition and what themes do, so
be it.)

> I'm not sure :extend is always the last pair in the list.
> 
> ELISP> (plist-member '(:a 1 :b 2 :c 3) :b)
> (:b 2 :c 3)

You are right, your code is more correct.

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

[Dmitry Gutov]

On 07.12.2019 21:14, Eli Zaretskii wrote:

> Our goal is to allow themes "inherit" the :extend attribute without
> having to specify it in their face specs, unlike with other
> attributes.  That's the only goal;

But that's exactly what it does. This question is simply different from 
"does it only affect the function custom-theme-set-faces", as I have 
explained profusely.

> we don't want :extend to behave
> differently from other face attributes in any other context.

What other contexts do you have in mind? What *shouldn't* it do?

> If you are saying that we cannot make this change apply only to face
> definitions by themes,

What other face definitions are there? There's defface, of course, which 
we treat differently. And there are theme definitions (both third-party 
and "user theme").

set-face-attribute is not affected, in case you were worried about that.

> then it means we don't really understand what
> we could break here, and then I don't think I want this change in
> Emacs 27.  Sorry, it's too risky.

What about the existing risk of breaking every theme out there by doing 
nothing?

> (I thought cus-face.el stores information in symbol properties that
> enables it to apply the face attributes in a special way.

It does.

> But I don't
> consider myself an expert on these matters, so if you say we cannot
> differentiate between general face definition and what themes do, so
> be it.)

What's a "general face definition"?

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 02:42:42 +0200
> 
> On 07.12.2019 21:14, Eli Zaretskii wrote:
> 
> > Our goal is to allow themes "inherit" the :extend attribute without
> > having to specify it in their face specs, unlike with other
> > attributes.  That's the only goal;
> 
> But that's exactly what it does.

It does, but the implementation is too general, and might affect other
use cases.

> > we don't want :extend to behave
> > differently from other face attributes in any other context.
> 
> What other contexts do you have in mind?

Any context other than a theme defining a face.

> What *shouldn't* it do?

What we do with any other face attribute.

> > If you are saying that we cannot make this change apply only to face
> > definitions by themes,
> 
> What other face definitions are there? There's defface, of course, which 
> we treat differently. And there are theme definitions (both third-party 
> and "user theme").

All the other situations where face-spec-recalc is called.  You listed
at least some of them up-thread.

> > then it means we don't really understand what
> > we could break here, and then I don't think I want this change in
> > Emacs 27.  Sorry, it's too risky.
> 
> What about the existing risk of breaking every theme out there by doing 
> nothing?

If we don't have a safe solution, we will have to live with that risk,
unfortunately.

> > But I don't
> > consider myself an expert on these matters, so if you say we cannot
> > differentiate between general face definition and what themes do, so
> > be it.)
> 
> What's a "general face definition"?

Everything except theme definition of faces.

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

[Dmitry Gutov]

On 08.12.2019 5:32, Eli Zaretskii wrote:

>>> Our goal is to allow themes "inherit" the :extend attribute without
>>> having to specify it in their face specs, unlike with other
>>> attributes.  That's the only goal;
>>
>> But that's exactly what it does.
> 
> It does, but the implementation is too general, and might affect other
> use cases.

Other use cases of what? face-spec-respec is about applying theme and 
default faces specs.

>>> we don't want :extend to behave
>>> differently from other face attributes in any other context.
>>
>> What other contexts do you have in mind?
> 
> Any context other than a theme defining a face.
> 
>> What *shouldn't* it do?
> 
> What we do with any other face attribute.

Either I don't understand you here, or the patch obviously satisfies 
that criterion.

>>> If you are saying that we cannot make this change apply only to face
>>> definitions by themes,
>>
>> What other face definitions are there? There's defface, of course, which
>> we treat differently. And there are theme definitions (both third-party
>> and "user theme").
> 
> All the other situations where face-spec-recalc is called.  You listed
> at least some of them up-thread.

As I've explained, the other callers are all part of the system that 
makes sure theme specs (and defaults specs, of course) are applied 
correctly in various situations.

>>> then it means we don't really understand what
>>> we could break here, and then I don't think I want this change in
>>> Emacs 27.  Sorry, it's too risky.
>>
>> What about the existing risk of breaking every theme out there by doing
>> nothing?
> 
> If we don't have a safe solution, we will have to live with that risk,
> unfortunately.

We haven't even started the pretest yet. If there are bugs in this patch 
(unlikely, but always possible), we have time for people to see and 
report them.

>>> But I don't
>>> consider myself an expert on these matters, so if you say we cannot
>>> differentiate between general face definition and what themes do, so
>>> be it.)
>>
>> What's a "general face definition"?
> 
> Everything except theme definition of faces.

Please give an example.

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 12:39:06 +0200
> 
> > If we don't have a safe solution, we will have to live with that risk,
> > unfortunately.
> 
> We haven't even started the pretest yet. If there are bugs in this patch 
> (unlikely, but always possible), we have time for people to see and 
> report them.

Experience teaches up that quite a few problems, especially in subtle
areas, are discovered only after the release.  I guess it means that
the group of people who use the pretest is not representative enough.

> >>> But I don't
> >>> consider myself an expert on these matters, so if you say we cannot
> >>> differentiate between general face definition and what themes do, so
> >>> be it.)
> >>
> >> What's a "general face definition"?
> > 
> > Everything except theme definition of faces.
> 
> Please give an example.

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

Thanks.

diff --git a/test/lisp/faces-tests.el b/test/lisp/faces-tests.el
index f00c93cedc..7cba4b26eb 100644
--- a/test/lisp/faces-tests.el
+++ b/test/lisp/faces-tests.el
@@ -36,6 +36,26 @@
   ""
   :group 'faces--test)
 
+(defface faces--test-extend
+  '((t :extend t :background "blue"))
+  ""
+  :group 'faces--test)
+
+(defface faces--test-no-extend
+  '((t :extend nil :background "blue"))
+  ""
+  :group 'faces--test)
+
+(defface faces--test-inherit-extend
+  '((t :inherit (faces--test-extend faces--test2) :background "blue"))
+  ""
+  :group 'faces--test)
+
+(defface faces--test-inherit-no-extend
+  '((t :inherit (faces--test2 faces--test-no-extend) :background "blue"))
+  ""
+  :group 'faces--test)
+
 (ert-deftest faces--test-color-at-point ()
   (with-temp-buffer
     (insert (propertize "STRING" 'face '(faces--test2 faces--test1)))
@@ -69,5 +89,133 @@
   ;; face IDs to faces.
   (should (> (face-id 'faces--test1) (face-id 'tooltip))))
 
+(ert-deftest faces--test-extend ()
+  (should (equal (face-attribute 'faces--test-extend :extend) t))
+  (should (equal (face-attribute 'faces--test-no-extend :extend) nil))
+  (should (equal (face-attribute 'faces--test1 :extend) 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  )
+
+(ert-deftest faces--test-extend-with-themes ()
+  ;; Make sure the diff-mode faces are not defined.
+  (should-not (featurep 'diff-mode))
+  (defface diff-changed-face
+    '((t :extend t :weight bold))
+    "")
+  (defface diff-added
+    '((t :background "grey"))
+    "")
+  (defface diff-file-header-face
+    '((t :extend nil :foreground "cyan"))
+    "")
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) 'unspecified))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (load-theme 'manoj-dark t t)
+  (load-theme 'tsdh-light t t)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) 'unspecified))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (enable-theme 'manoj-dark)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) 'unspecified)) ; should be t
+  (should (equal (face-attribute 'diff-added :extend) t))
+  (should (equal (face-attribute 'diff-file-header-face :extend) 'unspecified)) ; should be nil
+  (defface faces--test-face3
+    '((t :inherit diff-added :weight bold))
+    "")
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+  (disable-theme 'manoj-dark)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) 'unspecified))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) 'unspecified))
+  (defface diff-indicator-changed
+    '((t (:weight bold :extend t)))
+    "")
+  (enable-theme 'tsdh-light)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) t))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (should (equal (face-attribute 'diff-indicator-changed :extend) 'unspecified)) ; should be t
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+  (frame-set-background-mode (selected-frame) 'dark)
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-extend :extend nil t) t))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend)
+                 'unspecified))
+  (should (equal (face-attribute 'faces--test-inherit-no-extend :extend nil t)
+                 nil))
+  (should (equal (face-attribute 'diff-changed-face :extend) t))
+  (should (equal (face-attribute 'diff-added :extend) t))
+  (should (equal (face-attribute 'diff-file-header-face :extend) nil))
+  (should (equal (face-attribute 'diff-indicator-changed :extend) 'unspecified)) ; should be t
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+  (or noninteractive
+      (let ((fr (make-frame)))
+        (should (equal (face-attribute 'faces--test-inherit-extend :extend fr)
+                       'unspecified))
+        (should (equal (face-attribute 'faces--test-inherit-extend :extend fr t)
+                       t))
+        (should (equal (face-attribute 'faces--test-inherit-no-extend
+                                       :extend fr)
+                       'unspecified))
+        (should (equal (face-attribute 'faces--test-inherit-no-extend
+                                       :extend fr t)
+                       nil))
+        (should (equal (face-attribute 'diff-changed-face :extend fr) t))
+        (should (equal (face-attribute 'diff-added :extend fr) t))
+        (should (equal (face-attribute 'diff-file-header-face :extend fr) nil))
+        (should (equal (face-attribute 'diff-indicator-changed :extend fr)
+                       'unspecified)) ; should be t
+        (should (equal (face-attribute 'faces--test-face3 :extend nil t) t))
+        ))
+  (disable-theme 'tsdh-light)
+  (should (equal (face-attribute 'diff-indicator-changed :extend) t))
+  (should (equal (face-attribute 'faces--test-face3 :extend nil t) 'unspecified))
+  (or noninteractive
+      (let ((fr (make-frame)))
+        (should (equal (face-attribute 'diff-changed-face :extend fr) t))
+        (should (equal (face-attribute 'diff-added :extend fr) 'unspecified))
+        (should (equal (face-attribute 'diff-file-header-face :extend fr) nil))
+        (should (equal (face-attribute 'diff-indicator-changed :extend fr) t))
+        (should (equal (face-attribute 'faces--test-face3 :extend nil t) 'unspecified))
+        ))
+  )
+
 (provide 'faces-tests)
 ;;; faces-tests.el ends here

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

[Dmitry Gutov]

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

On 08.12.2019 17:50, Eli Zaretskii wrote:

> Experience teaches up that quite a few problems, especially in subtle
> areas, are discovered only after the release.  I guess it means that
> the group of people who use the pretest is not representative enough.

Sure. But on the other hand, the number of users in the period before 
the pretest in even smaller.

> 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, but I wonder how they would interact with a long-running test 
session.

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.

They also rely on the existing themes, the definitions of which will change.

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

All right, how does the attached patch look?

In addition to it, I'd like to revert the part of 64687872f6 that 
changed the bundled themed (etc/themes/*). Is that okay?

[-- Attachment #2: inherit-face-extend-spec-4.diff --]
[-- Type: text/x-patch, Size: 4055 bytes --]

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.
 
 @end table
 
diff --git a/etc/NEWS b/etc/NEWS
index 28bcb720cd..3d0781c24c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -437,15 +437,16 @@ to 'completion-styles' or 'completion-category-overrides' to use it.
 The new face attribute ':extend' controls whether to use the face for
 displaying the empty space beyond end of line (EOL) till the edge of
 the window.  By default, this attribute is non-nil only for 'region',
-'secondary-selection', 'hl-line' and some faces of Diff and Ediff
-modes; any other face that crosses end of line will not affect the
-display of the empty space at EOL.  This is to make Emacs behave more
-like other GUI applications with respect to displaying faces that
-cross line boundaries.
-
-Themes that redefine faces should add a non-nil ':extend' attribute to
-the above-mentioned faces, to keep the behavior of the default face
-definitions.
+'secondary-selection', 'hl-line' and some faces of Diff, Ediff,
+LogView and SMerge modes; any other face that crosses end of line will
+not affect the display of the empty space at EOL.  This is to make
+Emacs behave more like other GUI applications with respect to
+displaying faces that cross line boundaries.
+
+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.  Consequently, themes usually shouldn't touch
+this attribute at all.
 
 ** Connection-local variables
 
diff --git a/lisp/faces.el b/lisp/faces.el
index dc5bcca760..0f31628f5f 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1669,7 +1669,7 @@ face-spec-recalc
   ;; `theme-face' records.
   (let ((theme-faces (get face 'theme-face))
 	(no-match-found 0)
-	face-attrs theme-face-applied)
+        default-attrs face-attrs theme-face-applied)
     (if theme-faces
 	(dolist (elt (reverse theme-faces))
 	  (setq face-attrs (face-spec-choose (cadr elt) frame no-match-found))
@@ -1677,13 +1677,20 @@ face-spec-recalc
 	    (face-spec-set-2 face frame face-attrs)
 	    (setq theme-face-applied t))))
     ;; If there was a spec applicable to FRAME, that overrides the
-    ;; 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).
+    ;;
+    ;; If there was no spec applicable to FRAME, apply the defface
+    ;; spec as well as any applicable X resources.
+    (setq default-attrs (face-spec-choose (face-default-spec face) frame))
     (unless theme-face-applied
-      (setq face-attrs (face-spec-choose (face-default-spec face) frame))
-      (face-spec-set-2 face frame face-attrs)
+      (face-spec-set-2 face frame default-attrs)
       (make-face-x-resource-internal face frame))
+    (when (and theme-face-applied
+               (eq 'unspecified (face-attribute face :extend frame t)))
+      (let ((tail (plist-member default-attrs :extend)))
+        (and tail (face-spec-set-2 face frame
+                                   (list :extend (cadr tail))))))
     (setq face-attrs (face-spec-choose (get face 'face-override-spec) frame))
     (face-spec-set-2 face frame face-attrs)))
 

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.

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

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: Mon, 9 Dec 2019 16:07:54 +0200
> 
> > I guess you loaded the tests as a .el file?
> 
> Exactly. How else do you run the tests interactively?

  M-x load-file RET foo.elc RET
  M-x ert-run-tests-interactively RET RET

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

We could say something like "'region' and some others".  Not saying
that some have this by default would be a mistake, IMO.

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

Yes, thanks.

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

[Dmitry Gutov]

On 09.12.2019 16:42, Eli Zaretskii wrote:
>> Cc: 37774@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Mon, 9 Dec 2019 16:07:54 +0200
>>
>>> I guess you loaded the tests as a .el file?
>>
>> Exactly. How else do you run the tests interactively?
> 
>    M-x load-file RET foo.elc RET
>    M-x ert-run-tests-interactively RET RET

Ok, thanks. It'll be better to use some safer faces, though. Or even 
define those inside the test file.

>>> 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.
> 
> We could say something like "'region' and some others".  Not saying
> that some have this by default would be a mistake, IMO.

The original wording implies an exhaustive list. How would you like it 
to be phrased now?

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: Mon, 9 Dec 2019 17:24:19 +0200
> 
> >    M-x load-file RET foo.elc RET
> >    M-x ert-run-tests-interactively RET RET
> 
> Ok, thanks. It'll be better to use some safer faces, though. Or even 
> define those inside the test file.

If we have a special test theme, we could use any face name we want.

> > We could say something like "'region' and some others".  Not saying
> > that some have this by default would be a mistake, IMO.
> 
> The original wording implies an exhaustive list. How would you like it 
> to be phrased now?

How about the below?

  "By default, only a small number of faces, notably, @code{region},
  have this attribute set."

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

[Dmitry Gutov]

On 09.12.2019 17:42, Eli Zaretskii wrote:

>> Ok, thanks. It'll be better to use some safer faces, though. Or even
>> define those inside the test file.
> 
> If we have a special test theme, we could use any face name we want.

True.

> How about the below?
> 
>    "By default, only a small number of faces, notably, @code{region},
>    have this attribute set."

Good.

I've pushed the changes now, with some minor changes in phrasing. Please 
take a look.

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

[Eli Zaretskii]

> Cc: 37774-done@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 10 Dec 2019 02:20:29 +0200
> 
> I've pushed the changes now, with some minor changes in phrasing. Please 
> take a look.

LGTM (except that you inadvertently filled the heading line in NEWS
together with the body; I fixed that).  Thanks.

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

[Juri Linkov]

0. emacs -Q
1. Eval: (progn (load-library "ldap") (customize-variable 'ldap-host-parameters-alist))
2. Click the button "INS".
3. Where are the input fields?  I don't see any.  Are they lost?
   These fields were visible in the previous version.
   Ah, this is the :extend fallout in action.

That means this patch is in order:

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 94ab938a22..aabb3be197 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -126,15 +126,16 @@ widget-mouse-face
 ;; background, at least on light-background TTYs.
 (defface widget-field '((((type tty))
 			 :background "yellow3"
-			 :foreground "black")
+			 :foreground "black"
+			 :extend t)
 			(((class grayscale color)
 			  (background light))
-			 :background "gray85")
+			 :background "gray85" :extend t)
 			(((class grayscale color)
 			  (background dark))
-			 :background "dim gray")
+			 :background "dim gray" :extend t)
 			(t
-			 :slant italic))
+			 :slant italic :extend t))
   "Face used for editable fields."
   :group 'widget-faces)
 

I wonder how many similar fields shrunk to the size of the cursor
still exist.  Maybe web forms in eww?

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  37774@debbugs.gnu.org
> Date: Thu, 12 Dec 2019 01:02:42 +0200
> 
> 0. emacs -Q
> 1. Eval: (progn (load-library "ldap") (customize-variable 'ldap-host-parameters-alist))
> 2. Click the button "INS".
> 3. Where are the input fields?  I don't see any.  Are they lost?
>    These fields were visible in the previous version.
>    Ah, this is the :extend fallout in action.
> 
> That means this patch is in order:

Thanks, please install.

> I wonder how many similar fields shrunk to the size of the cursor
> still exist.  Maybe web forms in eww?

I also wonder.  Patches to fix any breakage are welcome.

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

[Juri Linkov]

>> 0. emacs -Q
>> 1. Eval: (progn (load-library "ldap") (customize-variable 'ldap-host-parameters-alist))
>> 2. Click the button "INS".
>> 3. Where are the input fields?  I don't see any.  Are they lost?
>>    These fields were visible in the previous version.
>>    Ah, this is the :extend fallout in action.
>>
>> That means this patch is in order:
>
> Thanks, please install.

Installed.

>> I wonder how many similar fields shrunk to the size of the cursor
>> still exist.  Maybe web forms in eww?
>
> I also wonder.  Patches to fix any breakage are welcome.

Actually there is no problem in eww because eww-form-text
fills the input field with 40 spaces by default, so the
field is clearly recognizable, even in absence of :extend.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dgutov@yandex.ru,  37774@debbugs.gnu.org
> Date: Fri, 13 Dec 2019 01:44:17 +0200
> 
> >> 3. Where are the input fields?  I don't see any.  Are they lost?
> >>    These fields were visible in the previous version.
> >>    Ah, this is the :extend fallout in action.
> >>
> >> That means this patch is in order:
> >
> > Thanks, please install.
> 
> Installed.

Thanks.

> Actually there is no problem in eww because eww-form-text
> fills the input field with 40 spaces by default, so the
> field is clearly recognizable, even in absence of :extend.

Good to know, thanks for looking into that.

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

[Juri Linkov]

>>> BTW, the diff-context needs ':extend t' as well.
>> Feel free to make that change (although when did you last see a
>> context diff?).
>
> It's not for context diffs, it's for context around the changes in unified
> diffs as well. Notice the gray background on the screenshot.

diff-context by default is just '((t nil))
Where do you think to add ':extend t'?
To empty face definition?

>>> But that's of little importance since as soon as I load a custom
>>> theme, whatever defaults were there don't seem to matter.
>>
>> We need to modify all the themes we provide to specify :extend for
>> faces where we do that by default.  It seems there's no way around
>> that, since the semantics of custom-theme-set-faces is clearly to
>> reset all face attributes to 'unspecified' before applying the face
>> spec, so keeping some attributes from the default face spec is out of
>> the question, unfortunately.  It's clear that the faces stuff was not
>> designed to accommodate addition of attributes easily.

This means manually adding :extend to all files in etc/themes?

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

[Dmitry Gutov]

On 27.11.2019 23:30, Juri Linkov wrote:
>>>> BTW, the diff-context needs ':extend t' as well.
>>> Feel free to make that change (although when did you last see a
>>> context diff?).
>>
>> It's not for context diffs, it's for context around the changes in unified
>> diffs as well. Notice the gray background on the screenshot.
> 
> diff-context by default is just '((t nil))
> Where do you think to add ':extend t'?
> To empty face definition?

'((t (:extend t))), something like that?

Good point, though.

The theme I'm currently using has this for this face's definition:

   `(diff-context                   ((,class (:inherit highlight))))

(to inherit the color).

I'm not sure where it'll add the required attribute there. Or if.

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

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, 37774@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 28 Nov 2019 01:34:08 +0200
> 
> > diff-context by default is just '((t nil))
> > Where do you think to add ':extend t'?
> > To empty face definition?
> 
> '((t (:extend t))), something like that?

Yes.

> Good point, though.

If it is a good point, I guess I'm missing it.

> The theme I'm currently using has this for this face's definition:
> 
>    `(diff-context                   ((,class (:inherit highlight))))
> 
> (to inherit the color).
> 
> I'm not sure where it'll add the required attribute there.

Again, I don't think I understand the difficulty.  Suppose you wanted
to add an :underline attribute -- would you have a similar difficulty
then?

> Or if.

Up to you, of course.  I said long ago that IMO it is/was a mistake to
make so many Diff faces extend to EOL.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  37774@debbugs.gnu.org
> Date: Wed, 27 Nov 2019 23:30:04 +0200
> 
> >>> BTW, the diff-context needs ':extend t' as well.
> >> Feel free to make that change (although when did you last see a
> >> context diff?).
> >
> > It's not for context diffs, it's for context around the changes in unified
> > diffs as well. Notice the gray background on the screenshot.
> 
> diff-context by default is just '((t nil))
> Where do you think to add ':extend t'?
> To empty face definition?

I don't think I understand the question.  Are you asking how to do
that technically?  Or are you asking whether it makes sense?

> >> We need to modify all the themes we provide to specify :extend for
> >> faces where we do that by default.  It seems there's no way around
> >> that, since the semantics of custom-theme-set-faces is clearly to
> >> reset all face attributes to 'unspecified' before applying the face
> >> spec, so keeping some attributes from the default face spec is out of
> >> the question, unfortunately.  It's clear that the faces stuff was not
> >> designed to accommodate addition of attributes easily.
> 
> This means manually adding :extend to all files in etc/themes?

Yes.

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

[Eli Zaretskii]

> Date: Thu, 28 Nov 2019 17:16:24 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 37774@debbugs.gnu.org, dgutov@yandex.ru
> 
> > >> We need to modify all the themes we provide to specify :extend for
> > >> faces where we do that by default.  It seems there's no way around
> > >> that, since the semantics of custom-theme-set-faces is clearly to
> > >> reset all face attributes to 'unspecified' before applying the face
> > >> spec, so keeping some attributes from the default face spec is out of
> > >> the question, unfortunately.  It's clear that the faces stuff was not
> > >> designed to accommodate addition of attributes easily.
> > 
> > This means manually adding :extend to all files in etc/themes?
> 
> Yes.

I've now done that.

Two comments:

  . When adding the :extend attribute to a face, we should make sure
    all of that face's definitions have the same value of it, even if
    the default definition of the face for some 'class' of displays
    doesn't need it (e.g., because it specifies only the foreground
    color).  This is so that if users customize the face, the results
    will look uniform regardless of which face attributes they
    customize.  Otherwise, if the user customizes the background color
    or :underline or some other similar attribute, the appearance will
    be different from that on other classes of terminals, and that is
    baaaad...

  . Some of the themes we have in core customize faces defined by
    unbundled packages.  I didn't change the definitions of those
    faces; it's up to the respective package developers and/or users
    to come up and ask for such changes, if it turns out the packages
    added the :extend attribute and we didn't.

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

[Dmitry Gutov]

On 30.11.2019 13:35, Eli Zaretskii wrote:

>>> This means manually adding :extend to all files in etc/themes?
>>
>> Yes.
> 
> I've now done that.
> 
> Two comments:
> 
>    . When adding the :extend attribute to a face, we should make sure
>      all of that face's definitions have the same value of it, even if
>      the default definition of the face for some 'class' of displays
>      doesn't need it (e.g., because it specifies only the foreground
>      color).  This is so that if users customize the face, the results
>      will look uniform regardless of which face attributes they
>      customize.  Otherwise, if the user customizes the background color
>      or :underline or some other similar attribute, the appearance will
>      be different from that on other classes of terminals, and that is
>      baaaad...
> 
>    . Some of the themes we have in core customize faces defined by
>      unbundled packages.  I didn't change the definitions of those
>      faces; it's up to the respective package developers and/or users
>      to come up and ask for such changes, if it turns out the packages
>      added the :extend attribute and we didn't.

I think the "alternative property" would help both of these concerns 
with less effort (mental and physical) required from everybody.

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

[Dmitry Gutov]

On 31.10.2019 18:06, Jonas Bernoulli wrote:
> Dmitry keeps urging me to comment here, so I am doing that even though
> I don't feel like I understand this enough yet to not make a fool of
> myself.  Oh well, if you insist.

I make a fool of myself on a regular basis.

> IMO going with a `noextend' attribute instead of `extend' would be that
> alternative approach.  Even if `extend' does not require any special
> treatment and even if it does not require each and every theme to be
> adjusted.  Again, I don't know whether there is any special treatment
> and whether themes have to be adjusted.

Personally, I'd go with a simple symbol property instead of face 
attributes, so themes are unaffected either way. But we'd need good 
reasons to change the design now.

> (It should be clear by now that I am not so happy that Dmitry kept
> urging me to comment here.)

I appreciate you doing that anyway.