GNU bug report logs - #61521
"default" is now the first item returned from (font-faces), breaking various code.

Previous Next

Package: emacs;

Reported by: Brennan Vincent <brennan <at> umanwizard.com>

Date: Wed, 15 Feb 2023 00:32:01 UTC

Severity: normal

Done: Eli Zaretskii <eliz <at> gnu.org>

Bug is archived. No further changes may be made.

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

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

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 00:32:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Brennan Vincent <brennan <at> umanwizard.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Wed, 15 Feb 2023 00:32:02 GMT) Full text and rfc822 format available.

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

From: Brennan Vincent <brennan <at> umanwizard.com>
To: bug-gnu-emacs <at> gnu.org
Subject: "default" is now the first item returned from (font-faces), breaking
 various code.
Date: Tue, 14 Feb 2023 19:31:30 -0500
Various code seems to expect "default" to be the /last/ item in the list
returned by that function, not the first. For example, this comment in faces.el:

  ;; The `reverse' is so that `default' goes first.
  (dolist (face (nreverse (face-list)))


Also, org-html-htmlize-generate-css does not work when default comes first in
the list (as it skips processing all fonts after default).

I am not sure why this was changed and if the change was intentional, but it can
be fixed by changing the "<" to a ">" in the last line of face-list, so I
suspect it might have been a mistake.

diff --git lisp/faces.el lisp/faces.el
index 4933b495a6c..e998dc504e5 100644
--- lisp/faces.el
+++ lisp/faces.el
@@ -199,7 +199,7 @@ face-list
     (maphash (lambda (face spec)
                (push `(,(car spec) . ,face) faces))
              face--new-frame-defaults)
-    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
+    (mapcar #'cdr (sort faces (lambda (f1 f2) (> (car f1) (car f2)))))))

 (defun make-face (face)
   "Define a new face with name FACE, a symbol.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 00:53:02 GMT) Full text and rfc822 format available.

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

From: Brennan Vincent <brennan <at> umanwizard.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: "default" is now the first item returned from (font-faces),
 breaking various code.
Date: Tue, 14 Feb 2023 19:52:23 -0500
I suspect frame-face-alist needs to be changed in a similar way.

On 2023-02-14 19:31, Brennan Vincent wrote:
> Various code seems to expect "default" to be the /last/ item in the list
> returned by that function, not the first. For example, this comment in faces.el:
> 
>   ;; The `reverse' is so that `default' goes first.
>   (dolist (face (nreverse (face-list)))
> 
> 
> Also, org-html-htmlize-generate-css does not work when default comes first in
> the list (as it skips processing all fonts after default).
> 
> I am not sure why this was changed and if the change was intentional, but it can
> be fixed by changing the "<" to a ">" in the last line of face-list, so I
> suspect it might have been a mistake.
> 
> diff --git lisp/faces.el lisp/faces.el
> index 4933b495a6c..e998dc504e5 100644
> --- lisp/faces.el
> +++ lisp/faces.el
> @@ -199,7 +199,7 @@ face-list
>      (maphash (lambda (face spec)
>                 (push `(,(car spec) . ,face) faces))
>               face--new-frame-defaults)
> -    (mapcar #'cdr (sort faces (lambda (f1 f2) (< (car f1) (car f2)))))))
> +    (mapcar #'cdr (sort faces (lambda (f1 f2) (> (car f1) (car f2)))))))
> 
>  (defun make-face (face)
>    "Define a new face with name FACE, a symbol.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 01:01:02 GMT) Full text and rfc822 format available.

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

From: Gregory Heytings <gregory <at> heytings.org>
To: Brennan Vincent <brennan <at> umanwizard.com>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 01:00:50 +0000
>
> Various code seems to expect "default" to be the /last/ item in the list 
> returned by that function, not the first. For example, this comment in 
> faces.el:
>

Can you perhaps clarify what you mean by "that function"?  The subject 
line of your bug report mentions 'font-faces', but no such function exists 
in Emacs.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 01:08:02 GMT) Full text and rfc822 format available.

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

From: Brennan Vincent <brennan <at> umanwizard.com>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Tue, 14 Feb 2023 20:06:51 -0500

On 2023-02-14 20:00, Gregory Heytings wrote:
> 
>>
>> Various code seems to expect "default" to be the /last/ item in the list
>> returned by that function, not the first. For example, this comment in faces.el:
>>
> 
> Can you perhaps clarify what you mean by "that function"?  The subject line of
> your bug report mentions 'font-faces', but no such function exists in Emacs.
> 

Apologies: I meant to write "face-list".





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 09:01:02 GMT) Full text and rfc822 format available.

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

From: Gregory Heytings <gregory <at> heytings.org>
To: Brennan Vincent <brennan <at> umanwizard.com>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 09:00:44 +0000
[Message part 1 (text/plain, inline)]
>>> Various code seems to expect "default" to be the /last/ item in the 
>>> list returned by that function, not the first. For example, this 
>>> comment in faces.el:
>>
>> Can you perhaps clarify what you mean by "that function"?  The subject 
>> line of your bug report mentions 'font-faces', but no such function 
>> exists in Emacs.
>
> Apologies: I meant to write "face-list".
>

Thanks.  It seems the change you describe is not a recent one: the first 
element of the list returned by 'face-list' is 'default' in Emacs 27, 28, 
29 and 30.  (This is caused by e3b8ddd500, since which frame faces are 
stored in a hash table instead of an alist.)

Given this, and the fact that the docstring of 'face-list' does not 
specify the order in which the faces are returned, it's not clear to me 
that there is a bug here.  Code that assumes a given order should probably 
be fixed.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 12:59:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Brennan Vincent <brennan <at> umanwizard.com>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 14:58:00 +0200
> Date: Tue, 14 Feb 2023 19:31:30 -0500
> From: Brennan Vincent <brennan <at> umanwizard.com>
> 
> Various code seems to expect "default" to be the /last/ item in the list
> returned by that function, not the first. For example, this comment in faces.el:
> 
>   ;; The `reverse' is so that `default' goes first.
>   (dolist (face (nreverse (face-list)))

That comment is obsolete and needs to be changed (and the call to
nreverse should perhaps be removed).

> Also, org-html-htmlize-generate-css does not work when default comes first in
> the list (as it skips processing all fonts after default).

Isn't that a bug in that Org function?  It shouldn't rely on any
particular order.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 13:44:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: brennan <at> umanwizard.com, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 15:43:34 +0200
> Cc: 61521 <at> debbugs.gnu.org
> Date: Wed, 15 Feb 2023 09:00:44 +0000
> From: Gregory Heytings <gregory <at> heytings.org>
> 
> Thanks.  It seems the change you describe is not a recent one: the first 
> element of the list returned by 'face-list' is 'default' in Emacs 27, 28, 
> 29 and 30.  (This is caused by e3b8ddd500, since which frame faces are 
> stored in a hash table instead of an alist.)

Right.  So I wonder whether we should remove the nreverse call in
face-set-after-frame-default.  WDYT?

> Given this, and the fact that the docstring of 'face-list' does not 
> specify the order in which the faces are returned, it's not clear to me 
> that there is a bug here.  Code that assumes a given order should probably 
> be fixed.

I agree.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 14:02:02 GMT) Full text and rfc822 format available.

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

From: Brennan Vincent <brennan <at> umanwizard.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 09:01:31 -0500
> On Feb 15, 2023, at 07:58, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> 
>> 
>> Date: Tue, 14 Feb 2023 19:31:30 -0500
>> From: Brennan Vincent <brennan <at> umanwizard.com>
>> 
>> Various code seems to expect "default" to be the /last/ item in the list
>> returned by that function, not the first. For example, this comment in faces.el:
>> 
>>  ;; The `reverse' is so that `default' goes first.
>>  (dolist (face (nreverse (face-list)))
> 
> That comment is obsolete and needs to be changed (and the call to
> nreverse should perhaps be removed).

If the order returned by face-list is not guaranteed, then why does it do sorting at all?

>> Also, org-html-htmlize-generate-css does not work when default comes first in
>> the list (as it skips processing all fonts after default).
> 
> Isn't that a bug in that Org function?  It shouldn't rely on any
> particular order.

In that case, we can consider this bug report to relate to the broken Org function, which is how I initially discovered the face-list issue.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 14:12:01 GMT) Full text and rfc822 format available.

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

From: Gregory Heytings <gregory <at> heytings.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: brennan <at> umanwizard.com, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 14:11:55 +0000
>> Thanks.  It seems the change you describe is not a recent one: the 
>> first element of the list returned by 'face-list' is 'default' in Emacs 
>> 27, 28, 29 and 30.  (This is caused by e3b8ddd500, since which frame 
>> faces are stored in a hash table instead of an alist.)
>
> Right.  So I wonder whether we should remove the nreverse call in 
> face-set-after-frame-default.  WDYT?
>

There are three occurrences of '(nreverse (face-list))': one in 
facemenu-complete-face-list, which seems to date from the 1990s, one in 
x-create-frame-with-faces, which was added by e3b8ddd500 "to handle subtle 
semantic change to how frame faces propagate, which otherwise breaks frame 
creation with reverse video enabled (bug#41200)", and the third one in 
'face-set-after-frame-default'.

The comment there is definitely outdated and should be removed, but given 
that '(nreverse (face-list))' is placed in a dolist whose body starts with 
'(face-spec-recalc face frame)', like in x-create-frame-with-faces, I'm 
not sure the nreverse can be removed without introducing a subtle bug. 
It is probably safer to leave the code unchanged.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 14:20:02 GMT) Full text and rfc822 format available.

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

From: Gregory Heytings <gregory <at> heytings.org>
To: Brennan Vincent <brennan <at> umanwizard.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 14:19:55 +0000
>> That comment is obsolete and needs to be changed (and the call to 
>> nreverse should perhaps be removed).
>
> If the order returned by face-list is not guaranteed, then why does it 
> do sorting at all?
>

Note that the sorting was added recently, in e3b8ddd500.  Before that 
there was no sorting, it just happened that 'default' was the last element 
of that alist.  It's not clear to me why the sorting is there, apparently 
it was added to fix bug#41200.





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 14:26:01 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Brennan Vincent <brennan <at> umanwizard.com>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 16:24:46 +0200
> From: Brennan Vincent <brennan <at> umanwizard.com>
> Date: Wed, 15 Feb 2023 09:01:31 -0500
> Cc: 61521 <at> debbugs.gnu.org
> 
> > On Feb 15, 2023, at 07:58, Eli Zaretskii <eliz <at> gnu.org> wrote:
> > 
> > 
> >> 
> >> Date: Tue, 14 Feb 2023 19:31:30 -0500
> >> From: Brennan Vincent <brennan <at> umanwizard.com>
> >> 
> >> Various code seems to expect "default" to be the /last/ item in the list
> >> returned by that function, not the first. For example, this comment in faces.el:
> >> 
> >>  ;; The `reverse' is so that `default' goes first.
> >>  (dolist (face (nreverse (face-list)))
> > 
> > That comment is obsolete and needs to be changed (and the call to
> > nreverse should perhaps be removed).
> 
> If the order returned by face-list is not guaranteed, then why does it do sorting at all?

Good question.  AFAICT, the sorting was added when we switched from
storing faces in alists to storing them in hash tables.  It probably
sorted faces to be more compatible with what face-list returned before
the switch to hash table.  So I suspect the order we have now is
simply a bug, and we do need to change the order of sorting to get
back the original order.

Gregory, any counter-arguments?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Wed, 15 Feb 2023 16:21:02 GMT) Full text and rfc822 format available.

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

From: Brennan Vincent <brennan <at> umanwizard.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Wed, 15 Feb 2023 11:19:54 -0500

On 2023-02-15 09:24, Eli Zaretskii wrote:
>> From: Brennan Vincent <brennan <at> umanwizard.com>
>> Date: Wed, 15 Feb 2023 09:01:31 -0500
>> Cc: 61521 <at> debbugs.gnu.org
>>
>>> On Feb 15, 2023, at 07:58, Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>
>>> 
>>>>
>>>> Date: Tue, 14 Feb 2023 19:31:30 -0500
>>>> From: Brennan Vincent <brennan <at> umanwizard.com>
>>>>
>>>> Various code seems to expect "default" to be the /last/ item in the list
>>>> returned by that function, not the first. For example, this comment in faces.el:
>>>>
>>>>  ;; The `reverse' is so that `default' goes first.
>>>>  (dolist (face (nreverse (face-list)))
>>>
>>> That comment is obsolete and needs to be changed (and the call to
>>> nreverse should perhaps be removed).
>>
>> If the order returned by face-list is not guaranteed, then why does it do sorting at all?
> 
> Good question.  AFAICT, the sorting was added when we switched from
> storing faces in alists to storing them in hash tables.  It probably
> sorted faces to be more compatible with what face-list returned before
> the switch to hash table.  So I suspect the order we have now is
> simply a bug, and we do need to change the order of sorting to get
> back the original order.

I tend to agree. Sorry for not explaining this reasoning more fully in my
original message.

Here's what I suspect happened (not 100% sure, it's just a theory):

(1) Initially set of faces was stored as a list, so it was naturally maintained
in the inverse order that things were added to it (thus default would be at the
end).

(2) Now faces are stored in a hash table whose key is the face and whose value
contains various pieces of data, including the face ID.

(3) This face ID is allocated in increasing order (see e.g. this code in xfaces.c:
      Lisp_Object face_id = make_fixnum (next_lface_id);
      lface_id_to_name[next_lface_id] = face;
      Fput (face, Qface, face_id);
      ++next_lface_id;

(4) Thus, `face-list` and `frame-face-alist` sorted the faces by face ID in
order to maintain the old ordering behavior. However, the author accidentally
inverted the comparison when doing so.


> Gregory, any counter-arguments?





Reply sent to Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility. (Fri, 17 Feb 2023 08:30:02 GMT) Full text and rfc822 format available.

Notification sent to Brennan Vincent <brennan <at> umanwizard.com>:
bug acknowledged by developer. (Fri, 17 Feb 2023 08:30:03 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Brennan Vincent <brennan <at> umanwizard.com>
Cc: 61521-done <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Fri, 17 Feb 2023 10:29:39 +0200
> Date: Wed, 15 Feb 2023 11:19:54 -0500
> Cc: 61521 <at> debbugs.gnu.org
> From: Brennan Vincent <brennan <at> umanwizard.com>
> 
> > Good question.  AFAICT, the sorting was added when we switched from
> > storing faces in alists to storing them in hash tables.  It probably
> > sorted faces to be more compatible with what face-list returned before
> > the switch to hash table.  So I suspect the order we have now is
> > simply a bug, and we do need to change the order of sorting to get
> > back the original order.
> 
> I tend to agree. Sorry for not explaining this reasoning more fully in my
> original message.
> 
> Here's what I suspect happened (not 100% sure, it's just a theory):
> 
> (1) Initially set of faces was stored as a list, so it was naturally maintained
> in the inverse order that things were added to it (thus default would be at the
> end).
> 
> (2) Now faces are stored in a hash table whose key is the face and whose value
> contains various pieces of data, including the face ID.
> 
> (3) This face ID is allocated in increasing order (see e.g. this code in xfaces.c:
>       Lisp_Object face_id = make_fixnum (next_lface_id);
>       lface_id_to_name[next_lface_id] = face;
>       Fput (face, Qface, face_id);
>       ++next_lface_id;
> 
> (4) Thus, `face-list` and `frame-face-alist` sorted the faces by face ID in
> order to maintain the old ordering behavior. However, the author accidentally
> inverted the comparison when doing so.
> 
> 
> > Gregory, any counter-arguments?

No further comments, so I've now installed the proposed changes on the
emacs-29 branch, and I'm boldly closing this bug as done.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Fri, 17 Feb 2023 22:18:02 GMT) Full text and rfc822 format available.

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

From: Kai Ma <justksqsf <at> gmail.com>
To: Gregory Heytings <gregory <at> heytings.org>
Cc: brennan <at> umanwizard.com, Eli Zaretskii <eliz <at> gnu.org>, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Sat, 18 Feb 2023 06:17:25 +0800
Gregory Heytings <gregory <at> heytings.org> writes:

>>> Thanks.  It seems the change you describe is not a recent one: the
>>> first element of the list returned by 'face-list' is 'default' in
>>> Emacs 27, 28, 29 and 30.  (This is caused by e3b8ddd500, since
>>> which frame faces are stored in a hash table instead of an alist.)
>>
>> Right.  So I wonder whether we should remove the nreverse call in
>> face-set-after-frame-default.  WDYT?
>>
>
> There are three occurrences of '(nreverse (face-list))': one in
> facemenu-complete-face-list, which seems to date from the 1990s, one
> in x-create-frame-with-faces, which was added by e3b8ddd500 "to handle
> subtle semantic change to how frame faces propagate, which otherwise
> breaks frame creation with reverse video enabled (bug#41200)", and the
> third one in 'face-set-after-frame-default'.
>
> The comment there is definitely outdated and should be removed, but
> given that '(nreverse (face-list))' is placed in a dolist whose body
> starts with '(face-spec-recalc face frame)', like in
> x-create-frame-with-faces, I'm not sure the nreverse can be removed
> without introducing a subtle bug. It is probably safer to leave the
> code unchanged.

My config becomes broken after pulling this change: the child frame of
vertico-posframe does not appear under certain themes, and signals
errors like:

  Error in post-command-hook (vertico--exhibit): (error "Invalid face" consult-separator)
  Error in post-command-hook (vertico--exhibit): (error "Invalid face" hl-todo)

I'm not sure if this is a downstream issue, but this problem can be
solved by either reverting this commit or removing the nreverse in
x-create-frame-with-faces:

diff --git a/lisp/faces.el b/lisp/faces.el
index 4933b495a6c..e91107e98cc 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2226,7 +2226,7 @@ x-create-frame-with-faces
     (unwind-protect
 	(progn
 	  (x-setup-function-keys frame)
-	  (dolist (face (nreverse (face-list)))
+	  (dolist (face (face-list)))
 	    (face-spec-recalc face frame))
 	  (x-handle-reverse-video frame parameters)
 	  (frame-set-background-mode frame t)

This piece of code (w/ nreverse) was written as part of the hash table
rewrite, and at that time (face-list) did not sort its results.  I don't
know why nreverse is significant here though.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Sat, 18 Feb 2023 06:50:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Kai Ma <justksqsf <at> gmail.com>
Cc: brennan <at> umanwizard.com, gregory <at> heytings.org, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Sat, 18 Feb 2023 08:49:29 +0200
> From: Kai Ma <justksqsf <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  brennan <at> umanwizard.com,
>   61521 <at> debbugs.gnu.org
> Date: Sat, 18 Feb 2023 06:17:25 +0800
> 
> My config becomes broken after pulling this change: the child frame of
> vertico-posframe does not appear under certain themes, and signals
> errors like:
> 
>   Error in post-command-hook (vertico--exhibit): (error "Invalid face" consult-separator)
>   Error in post-command-hook (vertico--exhibit): (error "Invalid face" hl-todo)
> 
> I'm not sure if this is a downstream issue, but this problem can be
> solved by either reverting this commit or removing the nreverse in
> x-create-frame-with-faces:
> 
> diff --git a/lisp/faces.el b/lisp/faces.el
> index 4933b495a6c..e91107e98cc 100644
> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -2226,7 +2226,7 @@ x-create-frame-with-faces
>      (unwind-protect
>  	(progn
>  	  (x-setup-function-keys frame)
> -	  (dolist (face (nreverse (face-list)))
> +	  (dolist (face (face-list)))
>  	    (face-spec-recalc face frame))
>  	  (x-handle-reverse-video frame parameters)
>  	  (frame-set-background-mode frame t)
> 
> This piece of code (w/ nreverse) was written as part of the hash table
> rewrite, and at that time (face-list) did not sort its results.  I don't
> know why nreverse is significant here though.

Which change are you talking about?  Please clarify.

The original problem was solved recently on the emacs-29 branch by
reversing the order in which face-list sorts the faces, to make it
similar to what we had before the hash table rewrite.  So there should
be no reason to change any other code anywhere else.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Sat, 18 Feb 2023 06:55:01 GMT) Full text and rfc822 format available.

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

From: Kai Ma <justksqsf <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: brennan <at> umanwizard.com, gregory <at> heytings.org, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Sat, 18 Feb 2023 14:54:12 +0800
[Message part 1 (text/plain, inline)]

> On Feb 18, 2023, at 14:49, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>> From: Kai Ma <justksqsf <at> gmail.com <mailto:justksqsf <at> gmail.com>>
>> Cc: Eli Zaretskii <eliz <at> gnu.org <mailto:eliz <at> gnu.org>>,  brennan <at> umanwizard.com <mailto:brennan <at> umanwizard.com>,
>>  61521 <at> debbugs.gnu.org <mailto:61521 <at> debbugs.gnu.org>
>> Date: Sat, 18 Feb 2023 06:17:25 +0800
>> 
>> My config becomes broken after pulling this change: the child frame of
>> vertico-posframe does not appear under certain themes, and signals
>> errors like:
>> 
>>  Error in post-command-hook (vertico--exhibit): (error "Invalid face" consult-separator)
>>  Error in post-command-hook (vertico--exhibit): (error "Invalid face" hl-todo)
>> 
>> I'm not sure if this is a downstream issue, but this problem can be
>> solved by either reverting this commit or removing the nreverse in
>> x-create-frame-with-faces:
>> 
>> diff --git a/lisp/faces.el b/lisp/faces.el
>> index 4933b495a6c..e91107e98cc 100644
>> --- a/lisp/faces.el
>> +++ b/lisp/faces.el
>> @@ -2226,7 +2226,7 @@ x-create-frame-with-faces
>>     (unwind-protect
>> 	(progn
>> 	  (x-setup-function-keys frame)
>> -	  (dolist (face (nreverse (face-list)))
>> +	  (dolist (face (face-list)))
>> 	    (face-spec-recalc face frame))
>> 	  (x-handle-reverse-video frame parameters)
>> 	  (frame-set-background-mode frame t)
>> 
>> This piece of code (w/ nreverse) was written as part of the hash table
>> rewrite, and at that time (face-list) did not sort its results.  I don't
>> know why nreverse is significant here though.
> 
> Which change are you talking about?  Please clarify.

The fix of this bug, commit a555abc56d5 on branch emacs-29.

> The original problem was solved recently on the emacs-29 branch by
> reversing the order in which face-list sorts the faces, to make it
> similar to what we had before the hash table rewrite.  So there should
> be no reason to change any other code anywhere else.
> 
> Thanks.

[Message part 2 (text/html, inline)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#61521; Package emacs. (Sat, 18 Feb 2023 07:41:02 GMT) Full text and rfc822 format available.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Kai Ma <justksqsf <at> gmail.com>
Cc: brennan <at> umanwizard.com, gregory <at> heytings.org, 61521 <at> debbugs.gnu.org
Subject: Re: bug#61521: "default" is now the first item returned from
 (font-faces), breaking various code.
Date: Sat, 18 Feb 2023 09:40:35 +0200
> From: Kai Ma <justksqsf <at> gmail.com>
> Date: Sat, 18 Feb 2023 14:54:12 +0800
> Cc: gregory <at> heytings.org,
>  brennan <at> umanwizard.com,
>  61521 <at> debbugs.gnu.org
> 
>  Which change are you talking about?  Please clarify.
> 
> The fix of this bug, commit a555abc56d5 on branch emacs-29.

Ah, thanks.  So I've now removed the call to nreverse from
x-create-frame-with-faces, as you suggested.  I agree that the call to
nreverse was probably added as part of the hash table rewrite due to
the wrong order returned by face-list back then.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 18 Mar 2023 11:24:07 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 177 days ago.

Previous Next


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