* bug#37806: 27.0.50; Need to "extend" face-remap.el @ 2019-10-18 9:14 Stephen Berman 2019-10-19 17:33 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: Stephen Berman @ 2019-10-18 9:14 UTC (permalink / raw) To: 37806 [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-18 9:14 bug#37806: 27.0.50; Need to "extend" face-remap.el Stephen Berman @ 2019-10-19 17:33 ` Eli Zaretskii 2019-10-20 12:21 ` Stephen Berman 2019-10-20 15:00 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 2 replies; 9+ messages in thread From: Eli Zaretskii @ 2019-10-19 17:33 UTC (permalink / raw) To: Stephen Berman, Ergus; +Cc: 37806 > 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-19 17:33 ` Eli Zaretskii @ 2019-10-20 12:21 ` Stephen Berman 2019-10-20 14:39 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 19:58 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 15:00 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 2 replies; 9+ messages in thread From: Stephen Berman @ 2019-10-20 12:21 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Ergus, 37806 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-20 12:21 ` Stephen Berman @ 2019-10-20 14:39 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 15:05 ` Eli Zaretskii 2019-10-20 19:58 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 1 reply; 9+ messages in thread From: Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2019-10-20 14:39 UTC (permalink / raw) To: Stephen Berman; +Cc: Eli Zaretskii, 37806 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-20 14:39 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2019-10-20 15:05 ` Eli Zaretskii 2019-10-21 7:20 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: Eli Zaretskii @ 2019-10-20 15:05 UTC (permalink / raw) To: Ergus; +Cc: stephen.berman, 37806 > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-20 15:05 ` Eli Zaretskii @ 2019-10-21 7:20 ` Eli Zaretskii 0 siblings, 0 replies; 9+ messages in thread From: Eli Zaretskii @ 2019-10-21 7:20 UTC (permalink / raw) To: spacibba; +Cc: stephen.berman, 37806 > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-20 12:21 ` Stephen Berman 2019-10-20 14:39 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2019-10-20 19:58 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 0 replies; 9+ messages in thread From: Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2019-10-20 19:58 UTC (permalink / raw) To: 37806-done Fixed in commit: 270ca8c2735cfc7935b9e4d0b770a55138493e ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-19 17:33 ` Eli Zaretskii 2019-10-20 12:21 ` Stephen Berman @ 2019-10-20 15:00 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 15:12 ` Eli Zaretskii 1 sibling, 1 reply; 9+ messages in thread From: Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2019-10-20 15:00 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Stephen Berman, 37806 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#37806: 27.0.50; Need to "extend" face-remap.el 2019-10-20 15:00 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2019-10-20 15:12 ` Eli Zaretskii 0 siblings, 0 replies; 9+ messages in thread From: Eli Zaretskii @ 2019-10-20 15:12 UTC (permalink / raw) To: Ergus; +Cc: stephen.berman, 37806 > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-21 7:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-18 9:14 bug#37806: 27.0.50; Need to "extend" face-remap.el Stephen Berman 2019-10-19 17:33 ` Eli Zaretskii 2019-10-20 12:21 ` Stephen Berman 2019-10-20 14:39 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 15:05 ` Eli Zaretskii 2019-10-21 7:20 ` Eli Zaretskii 2019-10-20 19:58 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 15:00 ` Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors 2019-10-20 15:12 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).