bug#37806: 27.0.50; Need to "extend" face-remap.el

bug#37806: 27.0.50; Need to "extend" face-remap.el

[Stephen Berman]

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: face-remap patch --]
[-- Type: text/x-patch, Size: 549 bytes --]

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

bug#37806: 27.0.50; Need to "extend" face-remap.el

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@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?

bug#37806: 27.0.50; Need to "extend" face-remap.el

[Stephen Berman]

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

>> From: Stephen Berman <stephen.berman@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

bug#37806: 27.0.50; Need to "extend" face-remap.el

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

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@gnu.org> wrote:
>
>>> From: Stephen Berman <stephen.berman@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.

bug#37806: 27.0.50; Need to "extend" face-remap.el

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

On Sat, Oct 19, 2019 at 08:33:33PM +0300, Eli Zaretskii wrote:
>> From: Stephen Berman <stephen.berman@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.

bug#37806: 27.0.50; Need to "extend" face-remap.el

[Eli Zaretskii]

> Date: Sun, 20 Oct 2019 16:39:30 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 37806@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.

bug#37806: 27.0.50; Need to "extend" face-remap.el

[Eli Zaretskii]

> Date: Sun, 20 Oct 2019 17:00:32 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Stephen Berman <stephen.berman@gmx.net>, 37806@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.

bug#37806: 27.0.50; Need to "extend" face-remap.el

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

Fixed in commit: 270ca8c2735cfc7935b9e4d0b770a55138493e

bug#37806: 27.0.50; Need to "extend" face-remap.el

[Eli Zaretskii]

> Date: Sun, 20 Oct 2019 18:05:31 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: stephen.berman@gmx.net, 37806@debbugs.gnu.org
> 
> > Date: Sun, 20 Oct 2019 16:39:30 +0200
> > From: Ergus <spacibba@aol.com>
> > Cc: Eli Zaretskii <eliz@gnu.org>, 37806@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.