GNU bug report logs - #37806
27.0.50; Need to "extend" face-remap.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 37806 in the body.
You can then email your comments to 37806 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Need to "extend" face-remap.el
Date: Fri, 18 Oct 2019 11:14:09 +0200

[Message part 1 (text/plain, inline)]

0. emacs -Q
1. C-x C-+ (i.e. M-x text-scale-adjust)
2. M-x variable-pitch-mode or M-x buffer-face-mode
=> Args out of range: [nil :family :foundry :swidth :height :weight :slant :underline :inverse :foreground ...], 19

The error happens because the vector of face attributes defined in
face-remap.el is missing the recently added :extend attribute, so it is
too short.  This also breaks the MELPA package charmap.el (that's where
I hit the error).  The patch below fixes this, though perhaps now would
be a good time to do what the comment above the definition of the vector
says: "This variable should probably be defined in C code where the
actual definitions are available."  Or is this simple fix good enough?

(The vector is also missing the :distant-foreground attribute, so the
patch adds that as well.  This absence happened to be innocuous because
the first element of the vector is nil, in order to make the attribute
indices match those of the enum lface_attribute_index defined in
dispextern.h, so the vector was long enough; but after the addition of
the :extend attribute, it wasn't anymore (only the indices of the vector
are used in face-remap.el).)

In GNU Emacs 27.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2019-10-18 built on strobe-lfs84
Repository revision: 2d13a3f68d4724af52e47675bedf60709c7b5171
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Linux From Scratch

[face-remap.diff (text/x-patch, inline)]

diff --git a/lisp/face-remap.el b/lisp/face-remap.el
index 5cdecb92ee..8e565264fe 100644
--- a/lisp/face-remap.el
+++ b/lisp/face-remap.el
@@ -69,7 +69,7 @@ internal-lisp-face-attributes
   [nil
    :family :foundry :swidth :height :weight :slant :underline :inverse
    :foreground :background :stipple :overline :strike :box
-   :font :inherit :fontset :vector])
+   :font :inherit :fontset :distant :extend :vector])

 (defun face-attrs-more-relative-p (attrs1 attrs2)
   "Return true if ATTRS1 contains a greater number of relative

Message #8 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>, Ergus <spacibba <at> aol.com>
Cc: 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sat, 19 Oct 2019 20:33:33 +0300

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Date: Fri, 18 Oct 2019 11:14:09 +0200
> 
> 0. emacs -Q
> 1. C-x C-+ (i.e. M-x text-scale-adjust)
> 2. M-x variable-pitch-mode or M-x buffer-face-mode
> => Args out of range: [nil :family :foundry :swidth :height :weight :slant :underline :inverse :foreground ...], 19
> 
> The error happens because the vector of face attributes defined in
> face-remap.el is missing the recently added :extend attribute, so it is
> too short.  This also breaks the MELPA package charmap.el (that's where
> I hit the error).  The patch below fixes this, though perhaps now would
> be a good time to do what the comment above the definition of the vector
> says: "This variable should probably be defined in C code where the
> actual definitions are available."  Or is this simple fix good enough?
> 
> (The vector is also missing the :distant-foreground attribute, so the
> patch adds that as well.  This absence happened to be innocuous because
> the first element of the vector is nil, in order to make the attribute
> indices match those of the enum lface_attribute_index defined in
> dispextern.h, so the vector was long enough; but after the addition of
> the :extend attribute, it wasn't anymore (only the indices of the vector
> are used in face-remap.el).)
> 
> In GNU Emacs 27.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
>  of 2019-10-18 built on strobe-lfs84
> Repository revision: 2d13a3f68d4724af52e47675bedf60709c7b5171
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
> System Description: Linux From Scratch
> 
> 
> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
> index 5cdecb92ee..8e565264fe 100644
> --- a/lisp/face-remap.el
> +++ b/lisp/face-remap.el
> @@ -69,7 +69,7 @@ internal-lisp-face-attributes
>    [nil
>     :family :foundry :swidth :height :weight :slant :underline :inverse
>     :foreground :background :stipple :overline :strike :box
> -   :font :inherit :fontset :vector])
> +   :font :inherit :fontset :distant :extend :vector])
> 
>  (defun face-attrs-more-relative-p (attrs1 attrs2)
>    "Return true if ATTRS1 contains a greater number of relative

Thanks.

Jimmy, could you please take a look?

Message #11 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Ergus <spacibba <at> aol.com>, 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sun, 20 Oct 2019 14:21:50 +0200

On Sat, 19 Oct 2019 20:33:33 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Date: Fri, 18 Oct 2019 11:14:09 +0200
>>
>> 0. emacs -Q
>> 1. C-x C-+ (i.e. M-x text-scale-adjust)
>> 2. M-x variable-pitch-mode or M-x buffer-face-mode
>> => Args out of range: [nil :family :foundry :swidth :height :weight :slant :underline :inverse :foreground ...], 19
>>
>> The error happens because the vector of face attributes defined in
>> face-remap.el is missing the recently added :extend attribute, so it is
>> too short.  This also breaks the MELPA package charmap.el (that's where
>> I hit the error).  The patch below fixes this, though perhaps now would
>> be a good time to do what the comment above the definition of the vector
>> says: "This variable should probably be defined in C code where the
>> actual definitions are available."  Or is this simple fix good enough?
>>
>> (The vector is also missing the :distant-foreground attribute, so the
>> patch adds that as well.  This absence happened to be innocuous because
>> the first element of the vector is nil, in order to make the attribute
>> indices match those of the enum lface_attribute_index defined in
>> dispextern.h, so the vector was long enough; but after the addition of
>> the :extend attribute, it wasn't anymore (only the indices of the vector
>> are used in face-remap.el).)
>>
>> In GNU Emacs 27.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
>>  of 2019-10-18 built on strobe-lfs84
>> Repository revision: 2d13a3f68d4724af52e47675bedf60709c7b5171
>> Repository branch: master
>> Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
>> System Description: Linux From Scratch
>>
>>
>> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
>> index 5cdecb92ee..8e565264fe 100644
>> --- a/lisp/face-remap.el
>> +++ b/lisp/face-remap.el
>> @@ -69,7 +69,7 @@ internal-lisp-face-attributes
>>    [nil
>>     :family :foundry :swidth :height :weight :slant :underline :inverse
>>     :foreground :background :stipple :overline :strike :box
>> -   :font :inherit :fontset :vector])
>> +   :font :inherit :fontset :distant :extend :vector])
>>
>>  (defun face-attrs-more-relative-p (attrs1 attrs2)
>>    "Return true if ATTRS1 contains a greater number of relative
>
> Thanks.
>
> Jimmy, could you please take a look?

This bug (and bug#37824, which is the same) isn't due to the addition of
the :extend attribute per se, but to that addition increasing the length
of lface_attribute_index, which makes it necessary to adjust the length
of internal-lisp-face-attributes, only the indices of which
face-remap.el actually uses, as I noted above.  Indeed, the following
patch also fixes this bug:

diff --git a/lisp/face-remap.el b/lisp/face-remap.el
index 5cdecb92ee..e429752df9 100644
--- a/lisp/face-remap.el
+++ b/lisp/face-remap.el
@@ -66,7 +66,7 @@
 ;; definitions are available.
 ;;
 (defvar internal-lisp-face-attributes
-  [nil
+  [nil nil
    :family :foundry :swidth :height :weight :slant :underline :inverse
    :foreground :background :stipple :overline :strike :box
    :font :inherit :fontset :vector])

But then we might as well just use a vector with all nil (or arbitrary)
elements whose length is LFACE_VECTOR_SIZE (maybe that's even how the
comment in face-remap.el quoted above could be understood).

The simplest fix is to just add the missing attributes, as my first
patch does, but then on any future changes to lface_vector_size
internal-lisp-face-attributes will have to be correspondingly adjusted.

Steve Berman

Message #14 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Ergus <spacibba <at> aol.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sun, 20 Oct 2019 16:39:30 +0200

On Sun, Oct 20, 2019 at 02:21:50PM +0200, Stephen Berman wrote:
>On Sat, 19 Oct 2019 20:33:33 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman <at> gmx.net>
>>> Date: Fri, 18 Oct 2019 11:14:09 +0200
>>>
>>> 0. emacs -Q
>>> 1. C-x C-+ (i.e. M-x text-scale-adjust)
>>> 2. M-x variable-pitch-mode or M-x buffer-face-mode
>>> => Args out of range: [nil :family :foundry :swidth :height :weight :slant :underline :inverse :foreground ...], 19
>>>
>>> The error happens because the vector of face attributes defined in
>>> face-remap.el is missing the recently added :extend attribute, so it is
>>> too short.  This also breaks the MELPA package charmap.el (that's where
>>> I hit the error).  The patch below fixes this, though perhaps now would
>>> be a good time to do what the comment above the definition of the vector
>>> says: "This variable should probably be defined in C code where the
>>> actual definitions are available."  Or is this simple fix good enough?
>>>
>>> (The vector is also missing the :distant-foreground attribute, so the
>>> patch adds that as well.  This absence happened to be innocuous because
>>> the first element of the vector is nil, in order to make the attribute
>>> indices match those of the enum lface_attribute_index defined in
>>> dispextern.h, so the vector was long enough; but after the addition of
>>> the :extend attribute, it wasn't anymore (only the indices of the vector
>>> are used in face-remap.el).)
>>>
>>> In GNU Emacs 27.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
>>>  of 2019-10-18 built on strobe-lfs84
>>> Repository revision: 2d13a3f68d4724af52e47675bedf60709c7b5171
>>> Repository branch: master
>>> Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
>>> System Description: Linux From Scratch
>>>
>>>
>>> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
>>> index 5cdecb92ee..8e565264fe 100644
>>> --- a/lisp/face-remap.el
>>> +++ b/lisp/face-remap.el
>>> @@ -69,7 +69,7 @@ internal-lisp-face-attributes
>>>    [nil
>>>     :family :foundry :swidth :height :weight :slant :underline :inverse
>>>     :foreground :background :stipple :overline :strike :box
>>> -   :font :inherit :fontset :vector])
>>> +   :font :inherit :fontset :distant :extend :vector])
>>>
>>>  (defun face-attrs-more-relative-p (attrs1 attrs2)
>>>    "Return true if ATTRS1 contains a greater number of relative
>>
>> Thanks.
>>
>> Jimmy, could you please take a look?
>
>This bug (and bug#37824, which is the same) isn't due to the addition of
>the :extend attribute per se, but to that addition increasing the length
>of lface_attribute_index, which makes it necessary to adjust the length
>of internal-lisp-face-attributes, only the indices of which
>face-remap.el actually uses, as I noted above.  Indeed, the following
>patch also fixes this bug:
>
>diff --git a/lisp/face-remap.el b/lisp/face-remap.el
>index 5cdecb92ee..e429752df9 100644
>--- a/lisp/face-remap.el
>+++ b/lisp/face-remap.el
>@@ -66,7 +66,7 @@
> ;; definitions are available.
> ;;
> (defvar internal-lisp-face-attributes
>-  [nil
>+  [nil nil
>    :family :foundry :swidth :height :weight :slant :underline :inverse
>    :foreground :background :stipple :overline :strike :box
>    :font :inherit :fontset :vector])
>
>But then we might as well just use a vector with all nil (or arbitrary)
>elements whose length is LFACE_VECTOR_SIZE (maybe that's even how the
>comment in face-remap.el quoted above could be understood).
>
>The simplest fix is to just add the missing attributes, as my first
>patch does, but then on any future changes to lface_vector_size
>internal-lisp-face-attributes will have to be correspondingly adjusted.
>
>Steve Berman

Yes the problem is just the size of the vector, because actually the
elements in it are wrong as they are not as defined anywhere else.

The proper fix must be to declare the vector in the C side using the
real symbols like:

Lisp_Object internal-lisp-face-attributes[LFACE_VECTOR_SIZE] = {
           Qnil QCfamily, QCfoundry, QCwidth QCheight,
           QCweight, QCslant, QCunderline, QCinverse_video,
           QCforeground, QCbackground, QCstipple, QCoverline,
           QCstrike_through, QCbox, QCfont, QCinherit, QCfontset,
           QCdistant_foreground, QCextend
           };

But using makevector or a similar api from C. This vector can be used in
some other parts in the code where we are duplicating code now, but this
will require a refactor I don't know if it is desired.

So I will submit the simplest fix now and if Eli gives me a green light
I will implement the better version.

Message #17 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Ergus <spacibba <at> aol.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stephen Berman <stephen.berman <at> gmx.net>, 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sun, 20 Oct 2019 17:00:32 +0200

On Sat, Oct 19, 2019 at 08:33:33PM +0300, Eli Zaretskii wrote:
>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Date: Fri, 18 Oct 2019 11:14:09 +0200
>>
>> 0. emacs -Q
>> 1. C-x C-+ (i.e. M-x text-scale-adjust)
>> 2. M-x variable-pitch-mode or M-x buffer-face-mode
>> => Args out of range: [nil :family :foundry :swidth :height :weight :slant :underline :inverse :foreground ...], 19
>>
>> The error happens because the vector of face attributes defined in
>> face-remap.el is missing the recently added :extend attribute, so it is
>> too short.  This also breaks the MELPA package charmap.el (that's where
>> I hit the error).  The patch below fixes this, though perhaps now would
>> be a good time to do what the comment above the definition of the vector
>> says: "This variable should probably be defined in C code where the
>> actual definitions are available."  Or is this simple fix good enough?
>>
>> (The vector is also missing the :distant-foreground attribute, so the
>> patch adds that as well.  This absence happened to be innocuous because
>> the first element of the vector is nil, in order to make the attribute
>> indices match those of the enum lface_attribute_index defined in
>> dispextern.h, so the vector was long enough; but after the addition of
>> the :extend attribute, it wasn't anymore (only the indices of the vector
>> are used in face-remap.el).)
>>
>> In GNU Emacs 27.0.50 (build 17, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
>>  of 2019-10-18 built on strobe-lfs84
>> Repository revision: 2d13a3f68d4724af52e47675bedf60709c7b5171
>> Repository branch: master
>> Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
>> System Description: Linux From Scratch
>>
>>
>> diff --git a/lisp/face-remap.el b/lisp/face-remap.el
>> index 5cdecb92ee..8e565264fe 100644
>> --- a/lisp/face-remap.el
>> +++ b/lisp/face-remap.el
>> @@ -69,7 +69,7 @@ internal-lisp-face-attributes
>>    [nil
>>     :family :foundry :swidth :height :weight :slant :underline :inverse
>>     :foreground :background :stipple :overline :strike :box
>> -   :font :inherit :fontset :vector])
>> +   :font :inherit :fontset :distant :extend :vector])
>>
>>  (defun face-attrs-more-relative-p (attrs1 attrs2)
>>    "Return true if ATTRS1 contains a greater number of relative
>
>Thanks.
>
>Jimmy, could you please take a look?

Hi:

I just pushed a fix for this.

BTW: I don't know how to close a bug report. It is not in the CONTRIBUTE
file.

Message #20 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ergus <spacibba <at> aol.com>
Cc: stephen.berman <at> gmx.net, 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sun, 20 Oct 2019 18:05:31 +0300

> Date: Sun, 20 Oct 2019 16:39:30 +0200
> From: Ergus <spacibba <at> aol.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 37806 <at> debbugs.gnu.org
> 
> The proper fix must be to declare the vector in the C side using the
> real symbols like:
> 
> Lisp_Object internal-lisp-face-attributes[LFACE_VECTOR_SIZE] = {
>             Qnil QCfamily, QCfoundry, QCwidth QCheight,
>             QCweight, QCslant, QCunderline, QCinverse_video,
>             QCforeground, QCbackground, QCstipple, QCoverline,
>             QCstrike_through, QCbox, QCfont, QCinherit, QCfontset,
>             QCdistant_foreground, QCextend
>             };
> 
> But using makevector or a similar api from C.

Yes, and face-remap.el even says so.

You can "steal" the way this is done from coding.c, starting around
line 11635 (but there should be no need to staticpro a variable that
is visible from Lisp, unlike Vcoding_category_table).

Thanks.

Message #23 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ergus <spacibba <at> aol.com>
Cc: stephen.berman <at> gmx.net, 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sun, 20 Oct 2019 18:12:58 +0300

> Date: Sun, 20 Oct 2019 17:00:32 +0200
> From: Ergus <spacibba <at> aol.com>
> Cc: Stephen Berman <stephen.berman <at> gmx.net>, 37806 <at> debbugs.gnu.org
> 
> I just pushed a fix for this.

Thanks.

> BTW: I don't know how to close a bug report. It is not in the CONTRIBUTE
> file.

See admin/notes/bugtracker.

Message #28 received at 37806-done <at> debbugs.gnu.org (full text, mbox):

From: Ergus <spacibba <at> aol.com>
To: 37806-done <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Sun, 20 Oct 2019 21:58:29 +0200

Fixed in commit: 270ca8c2735cfc7935b9e4d0b770a55138493e

Message #31 received at 37806 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: spacibba <at> aol.com
Cc: stephen.berman <at> gmx.net, 37806 <at> debbugs.gnu.org
Subject: Re: bug#37806: 27.0.50; Need to "extend" face-remap.el
Date: Mon, 21 Oct 2019 10:20:00 +0300

> Date: Sun, 20 Oct 2019 18:05:31 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: stephen.berman <at> gmx.net, 37806 <at> debbugs.gnu.org
> 
> > Date: Sun, 20 Oct 2019 16:39:30 +0200
> > From: Ergus <spacibba <at> aol.com>
> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 37806 <at> debbugs.gnu.org
> > 
> > The proper fix must be to declare the vector in the C side using the
> > real symbols like:
> > 
> > Lisp_Object internal-lisp-face-attributes[LFACE_VECTOR_SIZE] = {
> >             Qnil QCfamily, QCfoundry, QCwidth QCheight,
> >             QCweight, QCslant, QCunderline, QCinverse_video,
> >             QCforeground, QCbackground, QCstipple, QCoverline,
> >             QCstrike_through, QCbox, QCfont, QCinherit, QCfontset,
> >             QCdistant_foreground, QCextend
> >             };
> > 
> > But using makevector or a similar api from C.
> 
> Yes, and face-remap.el even says so.
> 
> You can "steal" the way this is done from coding.c, starting around
> line 11635 (but there should be no need to staticpro a variable that
> is visible from Lisp, unlike Vcoding_category_table).
> 
> > So I will submit the simplest fix now and if Eli gives me a green light
> > I will implement the better version.

Jimmy, in case it was unclear, my comments above is the "green light"
you asked for implementing a better version in xfaces.c.  We should
have done that cleanup long ago, IMO.

Thanks.

Previous Next

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