bug#64347: 30.0.50; Some customize faces shown as edited with -Q

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Stephen Berman]

0. emacs -Q
1. M-x customize-face RET RET
2. Toggle all face entries in the buffer *Customize Faces* (e.g. by
   creating this keyboard macro: C-s C-q C-j S RET C-f and then
   executing it 164 times) and search for the string "EDITED" in the
   buffer.
   => The following faces show the State "EDITED, shown value does not
   take effect until you set or save it.":
   confusingly-reordered
   custom-button
   custom-button-mouse
   custom-button-pressed
   mode-line
   mode-line-highlight
   mode-line-inactive
   tab-bar-tab
   tool-bar
   All other faces show the State "STANDARD".
3. Clicking the State button of these faces and selecting either "Undo
   Edits" or "Revert This Session's Customization" does not change the
   State shown.
4. Clicking the State button of, e.g., mode-line and selecting "Set for
   Current Session" changes the State shown to "SET for current session
   only."  I see no difference in the appearance of the mode line before
   and after this State change.
5. Clicking the State button of mode-line again and selecting "Revert
   This Session's Customization" changes the State shown back to
   "EDITED, shown value does not take effect until you set or save it.",
   and again the appearance of the mode-line is unchanged.


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.6) of 2023-06-27 built on strobelfssd
Repository revision: cf4ccc58284de50959ea66b1cd2655ab2fa4d15b
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: Linux From Scratch r11.3-100-systemd

Configured using:
 'configure -C --with-xwidgets 'CFLAGS=-Og -g3'
 PKG_CONFIG_PATH=/opt/qt5/lib/pkgconfig'

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

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Thu, 29 Jun 2023 12:15:00 +0200
> 
> 0. emacs -Q
> 1. M-x customize-face RET RET
> 2. Toggle all face entries in the buffer *Customize Faces* (e.g. by
>    creating this keyboard macro: C-s C-q C-j S RET C-f and then
>    executing it 164 times) and search for the string "EDITED" in the
>    buffer.
>    => The following faces show the State "EDITED, shown value does not
>    take effect until you set or save it.":
>    confusingly-reordered
>    custom-button
>    custom-button-mouse
>    custom-button-pressed
>    mode-line
>    mode-line-highlight
>    mode-line-inactive
>    tab-bar-tab
>    tool-bar
>    All other faces show the State "STANDARD".
> 3. Clicking the State button of these faces and selecting either "Undo
>    Edits" or "Revert This Session's Customization" does not change the
>    State shown.
> 4. Clicking the State button of, e.g., mode-line and selecting "Set for
>    Current Session" changes the State shown to "SET for current session
>    only."  I see no difference in the appearance of the mode line before
>    and after this State change.
> 5. Clicking the State button of mode-line again and selecting "Revert
>    This Session's Customization" changes the State shown back to
>    "EDITED, shown value does not take effect until you set or save it.",
>    and again the appearance of the mode-line is unchanged.

This is a regression between Emacs 27.2 and Emacs 28.1.  Bisecting
will be welcome.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> From: Stephen Berman <stephen.berman@gmx.net>
 >> Date: Thu, 29 Jun 2023 12:15:00 +0200
 >>
 >> 0. emacs -Q
 >> 1. M-x customize-face RET RET
 >> 2. Toggle all face entries in the buffer *Customize Faces* (e.g. by
 >>    creating this keyboard macro: C-s C-q C-j S RET C-f and then
 >>    executing it 164 times) and search for the string "EDITED" in the
 >>    buffer.
 >>    => The following faces show the State "EDITED, shown value does not
 >>    take effect until you set or save it.":
 >>    confusingly-reordered
 >>    custom-button
 >>    custom-button-mouse
 >>    custom-button-pressed
 >>    mode-line
 >>    mode-line-highlight
 >>    mode-line-inactive
 >>    tab-bar-tab
 >>    tool-bar
 >>    All other faces show the State "STANDARD".
 >> 3. Clicking the State button of these faces and selecting either "Undo
 >>    Edits" or "Revert This Session's Customization" does not change the
 >>    State shown.
 >> 4. Clicking the State button of, e.g., mode-line and selecting "Set for
 >>    Current Session" changes the State shown to "SET for current session
 >>    only."  I see no difference in the appearance of the mode line before
 >>    and after this State change.
 >> 5. Clicking the State button of mode-line again and selecting "Revert
 >>    This Session's Customization" changes the State shown back to
 >>    "EDITED, shown value does not take effect until you set or save it.",
 >>    and again the appearance of the mode-line is unchanged.
 >
 > This is a regression between Emacs 27.2 and Emacs 28.1. Bisecting
 > will be welcome.

I tried to bisect but I'm finding build errors on older commits:
CC       sysdep.o
sysdep.c:1784:22: error: variably modified ‘sigsegv_stack’ at file scope
  1784 | static unsigned char sigsegv_stack[SIGSTKSZ];


So I did some debugging.  I noted that all the faces posted by Stephen
(except confusingly-reordered) have a Horizontal Width widget.  So
something like this is enough to get Custom confused:
(defface test
   '((t :box (:line-width 2 :style released-button)))
   "...")

M-x customize-face RET test
Shows the EDITED State.

So that points to Custom fiddling with the real value, i.e., what
face-attribute would return, but not with the "customized value", the
value that holds the Widget.

Looking at the changes in custom-face-attributes, I see this commit:
commit 34ae2d0c220c945443e94a43d043a4a63c444bf4
Author: Alexandre Adolphe <alexandre.adolphe@gmail.com>
Date:   Sat Aug 10 22:57:24 2019 +0200

     Allow negative line width for :box face attribute

And I noticed that it modified the real-value filter, but not the
customized-value filter.  So I suspect that might be the problem.

Maybe someone that is able to build Emacs for that and previous commits
can confirm.

In the meantime, I'll read the documentation on :line-width, since I'm
pretty sure a changed in the customized value filter is required.

(And I don't know what's wrong with the confusingly-reordered face yet)

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Drew Adams]

Thanks for saying what you did to try to
track this down (steps, logic).  Even for
people such as me, who don't help on this,
this helps us understand better.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Mauro Aranda <maurooaranda@gmail.com> writes:

 > (And I don't know what's wrong with the confusingly-reordered face yet)

OK, some information about the confusingly-reordered face.  Take the
definition of the face, only change the name.
(defface test
   '((((supports :underline (:style wave)))
      :underline (:style wave :color "Red1"))
     (t
      :inherit warning))
   "...")

And evaluate that in Emacs 27, with -Q.  Then:
M-x customize-face RET test
Notice that it says Edited, and that the Widget shows the option to
modify Underline, Color, Style, as expected.

Now take the same definition to Emacs 29, with -Q and do the same.
The State is still EDITED (which is wrong, and the cause of the bug
report), but now you don't see the option to modify the Underline.
Somehow it didn't match.  So there's something different (and still
wrong) here going on.  I took a look at why the matching has changed and
it looks to me like this code in custom-face-attributes is responsible:
      ,(lambda (real-value)
     (and real-value
          (let ((color
             (or (and (consp real-value) (plist-get real-value :color))
                 (and (stringp real-value) real-value)
                 'foreground-color))
            (style
             (or (and (consp real-value) (plist-get real-value :style))
                 'line))
                    (position (and (consp real-value)
                                   (plist-get real-value :style))))
                (nconc (and color  `(:color ,color))
                       `(:style ,style)
                       (and position `(:position ,position))))))

(plist-get real-value :style) for position looks like a typo introduced
in:
commit 4f50d964e51bbe5219f40df4353f4314c7ade985
Author: Po Lu <luangruo@yahoo.com>
Date:   Mon Jan 10 19:26:46 2022 +0800

     Allow controlling the underline position of faces

Changing that line to (plist-get real-value :position) brings back the
correct match, so that's one less bug.

Still, the important one is left.  Now, go back in time to Emacs 27
again, but slightly change the definition for test:
(defface test
   '((((supports :underline (:style wave)))
      :underline (:color "Red1" :style wave))
     (t
      :inherit warning))
   "...")

Notice the properties in :underline were swapped.
M-x customize-face RET test
Shows the Standard State, as expected.  So it seems to me that something
somewhere is checking for list equality where it should check for plist
equality.


With the previous typo fix, go back to Emacs 29 and repeat.  It still
shows EDITED as the State, so again, there's something else wrong here.
I think what was left out in terms of the :underline property is that
:position doesn't have to be always specified.  Changing the filter for
the customized-value to return something like this:
(nconc `(:color ,color) `(:style ,style) (and position `(:position 
,position)))
fixes it, now customizing the unedited face shows STANDARD as the state.


To sum it up, I think there are bugs in custom-face-attributes. One is
most surely a typo, and the other ones are oversights in the filters for
the :underline and :box properties.  Fixing those, we are left with one
bug, I think, that will be reproducible with Emacs -Q and evaluating:

(defface test
   '((((supports :underline (:style wave)))
      :underline (:color "Red1" :style wave))
     (t
      :inherit warning))
   "...")

(defface test-2
   '((((supports :underline (:style wave)))
      :underline (:style wave :color "Red1"))
     (t
      :inherit warning))
   "...")

M-x customize-face RET test
will show STANDARD state

while
M-x customize-face RET test-2
will show EDITED state

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Fri, 30 Jun 2023 11:05:33 -0300
> From: Mauro Aranda <maurooaranda@gmail.com>
> Cc: Stephen Berman <stephen.berman@gmx.net>, Eli Zaretskii <eliz@gnu.org>
> 
> To sum it up, I think there are bugs in custom-face-attributes. One is
> most surely a typo, and the other ones are oversights in the filters for
> the :underline and :box properties.  Fixing those, we are left with one
> bug, I think, that will be reproducible with Emacs -Q and evaluating:
> 
> (defface test
>    '((((supports :underline (:style wave)))
>       :underline (:color "Red1" :style wave))
>      (t
>       :inherit warning))
>    "...")
> 
> (defface test-2
>    '((((supports :underline (:style wave)))
>       :underline (:style wave :color "Red1"))
>      (t
>       :inherit warning))
>    "...")
> 
> M-x customize-face RET test
> will show STANDARD state
> 
> while
> M-x customize-face RET test-2
> will show EDITED state

Thanks.

Can you show a patch for the two bugs you've succeeded to identify?

Did you make any progress with the one bug that's left after the other
two are fixed?

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 > Thanks.
 >
 > Can you show a patch for the two bugs you've succeeded to identify?

Not yet.  I wrote a working post-filter for the :box property, but
there are still some faces that show with the EDITED state.

Since there are both ways to specify the same information for the
:line-width value, there's always a chance of not sending the expected
format to face-spec-match-p.

Let's say we have (:line-width 1).  Then the pre-filter converts it into
(:line-width (1 . 1)), and then the post-filter should convert it back
to (:line-width 1), and that works.

But if we have (:line-width (1 . 1)), the pre-filter does nothing, and
the post-filter will transform it into (:line-width 1), and that way
specs won't match.

So sometimes we want to post-filter to (:line-width WIDTH) and sometimes
to (VWIDTH . HWIDTH) and there's currently not enough information to do
that, or at least I missed it.

 > Did you make any progress with the one bug that's left after the other
 > two are fixed?

I did find that face-attr-match-p uses equal even with properties like
:underline and :box that can have plists as values.  But I haven't tried
to fix it, because I got frustrated with the post-filter thing.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Sat, 8 Jul 2023 18:32:15 -0300
> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Since there are both ways to specify the same information for the
> :line-width value, there's always a chance of not sending the expected
> format to face-spec-match-p.
> 
> Let's say we have (:line-width 1).  Then the pre-filter converts it into
> (:line-width (1 . 1)), and then the post-filter should convert it back
> to (:line-width 1), and that works.
> 
> But if we have (:line-width (1 . 1)), the pre-filter does nothing, and
> the post-filter will transform it into (:line-width 1), and that way
> specs won't match.
> 
> So sometimes we want to post-filter to (:line-width WIDTH) and sometimes
> to (VWIDTH . HWIDTH) and there's currently not enough information to do
> that, or at least I missed it.
> 
>  > Did you make any progress with the one bug that's left after the other
>  > two are fixed?
> 
> I did find that face-attr-match-p uses equal even with properties like
> :underline and :box that can have plists as values.  But I haven't tried
> to fix it, because I got frustrated with the post-filter thing.

I guess I'm missing something here: why do we need those pre-filter
and post-filter conversions?  The C code understands both forms of
:line-width, so there should be no need for Lisp to do any
conversions, right?  So why do we do that? why not simply leave the
spec as it was originally?

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sat, 8 Jul 2023 18:32:15 -0300
 >> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> Since there are both ways to specify the same information for the
 >> :line-width value, there's always a chance of not sending the expected
 >> format to face-spec-match-p.
 >>
 >> Let's say we have (:line-width 1).  Then the pre-filter converts it into
 >> (:line-width (1 . 1)), and then the post-filter should convert it back
 >> to (:line-width 1), and that works.
 >>
 >> But if we have (:line-width (1 . 1)), the pre-filter does nothing, and
 >> the post-filter will transform it into (:line-width 1), and that way
 >> specs won't match.
 >>
 >> So sometimes we want to post-filter to (:line-width WIDTH) and sometimes
 >> to (VWIDTH . HWIDTH) and there's currently not enough information to do
 >> that, or at least I missed it.
 >>
 >>  > Did you make any progress with the one bug that's left after the 
other
 >>  > two are fixed?
 >>
 >> I did find that face-attr-match-p uses equal even with properties like
 >> :underline and :box that can have plists as values.  But I haven't tried
 >> to fix it, because I got frustrated with the post-filter thing.
 >
 > I guess I'm missing something here: why do we need those pre-filter
 > and post-filter conversions?  The C code understands both forms of
 > :line-width, so there should be no need for Lisp to do any
 > conversions, right?  So why do we do that? why not simply leave the
 > spec as it was originally?

Custom needs the pre-filter in order to present a Custom buffer to edit
the face.
Let's say there's a face:
(defface foo
   '((t (:box (:line-width 1 :color "black"))))
   "...")

And let's say a user wants to customize it via Custom.
M-x customize-face RET foo
should show the user a buffer with all the capabilities to edit it.
Because we have an integer for the :line-width property, the user will
be presented with an integer Widget to edit the value, without giving
the user the opportunity to edit the HWIDTH and VWIDTH separately.

So the pre-filter takes the (:line-width 1), and converts it into
(:line-width (1 . 1)), and now the Custom buffer will have
a cons Widget.  If we didn't do that conversion, that would be a Bug
report, I'm sure.

Of course, if the face is defined like so:
(defface foo
   '((t (:box (:line-width (1 . 1) :color "black"))))
   "...")
then the pre-filter doesn't have to do anything.  So, sometimes it does
something, sometimes not, but I hope I've convinced you about why is
needed.

Now, your question about the post-filter made me question its need a
little bit.  At first sight, I thought it was clear than a post-filter
was needed too.  As I said in my first message, Custom is fiddling with
the value for :line-width, but should present it to the face-spec-match-p
"unfiltered".  And currently, that's a need because when it comes to the
face-attr-match-p function, it will use equal to try to see if
attributes in two specs match.  So the post-filter should take
(:line-width (1 . 1)) and turn it back into (:line-width 1)

That's where I was originally, before your mail.  Now I think that maybe
face-attr-match-p could do something slightly smarter when comparing
these values, since, as you say, the C code understands both forms.
IOW, maybe the fix is that face-attr-match-p shouldn't produce a
mismatch between specs that have these both forms being compared.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Sun, 9 Jul 2023 08:44:23 -0300
> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>  > I guess I'm missing something here: why do we need those pre-filter
>  > and post-filter conversions?  The C code understands both forms of
>  > :line-width, so there should be no need for Lisp to do any
>  > conversions, right?  So why do we do that? why not simply leave the
>  > spec as it was originally?
> 
> Custom needs the pre-filter in order to present a Custom buffer to edit
> the face.
> Let's say there's a face:
> (defface foo
>    '((t (:box (:line-width 1 :color "black"))))
>    "...")
> 
> And let's say a user wants to customize it via Custom.
> M-x customize-face RET foo
> should show the user a buffer with all the capabilities to edit it.
> Because we have an integer for the :line-width property, the user will
> be presented with an integer Widget to edit the value, without giving
> the user the opportunity to edit the HWIDTH and VWIDTH separately.
> 
> So the pre-filter takes the (:line-width 1), and converts it into
> (:line-width (1 . 1)), and now the Custom buffer will have
> a cons Widget.  If we didn't do that conversion, that would be a Bug
> report, I'm sure.

OK, but why does it have to do that on the original value?  It could
do that on a copy that serves for the display and editing, in which
case the original value could be left intact if the user didn't change
it or did change, but didn't click Apply.  (If the user does modify
the original value, then any conversions are okay, since the variable
is really "edited".)

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sun, 9 Jul 2023 08:44:23 -0300
 >> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> Eli Zaretskii <eliz@gnu.org> writes:
 >>
 >>  > I guess I'm missing something here: why do we need those pre-filter
 >>  > and post-filter conversions?  The C code understands both forms of
 >>  > :line-width, so there should be no need for Lisp to do any
 >>  > conversions, right?  So why do we do that? why not simply leave the
 >>  > spec as it was originally?
 >>
 >> Custom needs the pre-filter in order to present a Custom buffer to edit
 >> the face.
 >> Let's say there's a face:
 >> (defface foo
 >>    '((t (:box (:line-width 1 :color "black"))))
 >>    "...")
 >>
 >> And let's say a user wants to customize it via Custom.
 >> M-x customize-face RET foo
 >> should show the user a buffer with all the capabilities to edit it.
 >> Because we have an integer for the :line-width property, the user will
 >> be presented with an integer Widget to edit the value, without giving
 >> the user the opportunity to edit the HWIDTH and VWIDTH separately.
 >>
 >> So the pre-filter takes the (:line-width 1), and converts it into
 >> (:line-width (1 . 1)), and now the Custom buffer will have
 >> a cons Widget.  If we didn't do that conversion, that would be a Bug
 >> report, I'm sure.
 >
 > OK, but why does it have to do that on the original value? It could
 > do that on a copy that serves for the display and editing, in which
 > case the original value could be left intact if the user didn't change
 > it or did change, but didn't click Apply.  (If the user does modify
 > the original value, then any conversions are okay, since the variable
 > is really "edited".)

I think my description was inaccurate, because it seemed to imply that
it is a destructive operation.  It is not, it leaves the original value
intact.

But when deciding to set a state, Custom always consults the spec built
from the data the face Widget has.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Sun, 9 Jul 2023 20:12:38 -0300
> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>  > OK, but why does it have to do that on the original value? It could
>  > do that on a copy that serves for the display and editing, in which
>  > case the original value could be left intact if the user didn't change
>  > it or did change, but didn't click Apply.  (If the user does modify
>  > the original value, then any conversions are okay, since the variable
>  > is really "edited".)
> 
> I think my description was inaccurate, because it seemed to imply that
> it is a destructive operation.  It is not, it leaves the original value
> intact.
> 
> But when deciding to set a state, Custom always consults the spec built
> from the data the face Widget has.

Can we change this last aspect, so that the state is set using the
original spec if the setting was not changed by the user?

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sun, 9 Jul 2023 20:12:38 -0300
 >> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> Eli Zaretskii <eliz@gnu.org> writes:
 >>
 >>  > OK, but why does it have to do that on the original value? It could
 >>  > do that on a copy that serves for the display and editing, in which
 >>  > case the original value could be left intact if the user didn't 
change
 >>  > it or did change, but didn't click Apply.  (If the user does modify
 >>  > the original value, then any conversions are okay, since the variable
 >>  > is really "edited".)
 >>
 >> I think my description was inaccurate, because it seemed to imply that
 >> it is a destructive operation.  It is not, it leaves the original value
 >> intact.
 >>
 >> But when deciding to set a state, Custom always consults the spec built
 >> from the data the face Widget has.
 >
 > Can we change this last aspect, so that the state is set using the
 > original spec if the setting was not changed by the user?

That might be possible.  I will check if it doesn't lead to any troubles
and if it actually fixes these inconsistencies.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

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

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sun, 9 Jul 2023 20:12:38 -0300
 >> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> Eli Zaretskii <eliz@gnu.org> writes:
 >>
 >>  > OK, but why does it have to do that on the original value? It could
 >>  > do that on a copy that serves for the display and editing, in which
 >>  > case the original value could be left intact if the user didn't 
change
 >>  > it or did change, but didn't click Apply.  (If the user does modify
 >>  > the original value, then any conversions are okay, since the variable
 >>  > is really "edited".)
 >>
 >> I think my description was inaccurate, because it seemed to imply that
 >> it is a destructive operation.  It is not, it leaves the original value
 >> intact.
 >>
 >> But when deciding to set a state, Custom always consults the spec built
 >> from the data the face Widget has.
 >
 > Can we change this last aspect, so that the state is set using the
 > original spec if the setting was not changed by the user?

OK, here's a patch for doing that.  It seems to me that after creating
the widget, the only reason to use the value that's represented in the
widget is if we loaded the spec from the :shown-value property (meaning
we are redrawing the widget and we want to keep a spec that was already
in the widget)

[-- Attachment #2: 0001-Pass-original-spec-just-after-creating-the-face-widg.patch --]
[-- Type: text/x-patch, Size: 3666 bytes --]

From 7ffb3f466cb3c0804f8cb0ec25171e6013f41dff Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Mon, 10 Jul 2023 10:47:23 -0300
Subject: [PATCH] Pass original spec just after creating the face-widget

* lisp/cus-edit.el (custom-face-get-current-spec-unfiltered): New
function, extracted from custom-face-get-current-spec.
(custom-face-get-current-spec): Use it.
(custom-face-state-set): Take an optional argument, to decide if we
should check against a filtered or unfiltered spec.
(custom-face-value-create): Use the new optional argument.  (Bug#64347)
---
 lisp/cus-edit.el | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index dbef5f47cd6..c5ddca9bc29 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3717,7 +3717,8 @@ custom-face-widget-to-spec
 	 `((t ,(widget-value child)))
        (widget-value child)))))
 
-(defun custom-face-get-current-spec (face)
+(defun custom-face-get-current-spec-unfiltered (face)
+  "Return the current spec for face FACE, without filtering it."
   (let ((spec (or (get face 'customized-face)
 		  (get face 'saved-face)
 		  (get face 'face-defface-spec)
@@ -3728,7 +3729,11 @@ custom-face-get-current-spec
     ;; edit it as the user has specified it.
     (if (not (face-spec-match-p face spec (selected-frame)))
 	(setq spec `((t ,(face-attr-construct face (selected-frame))))))
-    (custom-pre-filter-face-spec spec)))
+    spec))
+
+(defun custom-face-get-current-spec (face)
+  "Return the current spec for face FACE, filtering it."
+  (custom-pre-filter-face-spec (custom-face-get-current-spec-unfiltered face)))
 
 (defun custom-toggle-hide-face (visibility-widget &rest _ignore)
   "Toggle the visibility of a `custom-face' parent widget.
@@ -3848,8 +3853,8 @@ custom-face-value-create
 	(unless (widget-get widget :custom-form)
 	  (widget-put widget :custom-form custom-face-default-form))
 
-	(let* ((spec (or (widget-get widget :shown-value)
-			 (custom-face-get-current-spec symbol)))
+	(let* ((shown-value (widget-get widget :shown-value))
+               (spec (or shown-value (custom-face-get-current-spec symbol)))
 	       (form (widget-get widget :custom-form))
 	       (indent (widget-get widget :indent))
 	       face-alist face-entry spec-default spec-match editor)
@@ -3890,7 +3895,7 @@ custom-face-value-create
 		   widget 'sexp :value spec))))
           (push editor children)
           (widget-put widget :children children)
-	  (custom-face-state-set widget))))))
+	  (custom-face-state-set widget (not shown-value)))))))
 
 (defun cus--face-link (widget _format)
   (widget-create-child-and-convert
@@ -4010,13 +4015,18 @@ custom-face-state
 	'changed
       state)))
 
-(defun custom-face-state-set (widget)
+(defun custom-face-state-set (widget &optional no-filter)
   "Set the state of WIDGET, a custom-face widget.
 If the user edited the widget, set the state to modified.  If not, the new
-state is one of the return values of `custom-face-state'."
+state is one of the return values of `custom-face-state'.
+Optional argument NO-FILTER means to check against an unfiltered spec."
   (let ((face (widget-value widget)))
     (widget-put widget :custom-state
-                (if (face-spec-match-p face (custom-face-widget-to-spec widget))
+                (if (face-spec-match-p
+                     face
+                     (if no-filter
+                         (custom-face-get-current-spec-unfiltered face)
+                       (custom-face-widget-to-spec widget)))
                     (custom-face-state face)
                   'modified))))
 
-- 
2.34.1

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

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

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Fri, 30 Jun 2023 11:05:33 -0300
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >> Cc: Stephen Berman <stephen.berman@gmx.net>, Eli Zaretskii 
<eliz@gnu.org>
 >>
 >> To sum it up, I think there are bugs in custom-face-attributes. One is
 >> most surely a typo, and the other ones are oversights in the filters for
 >> the :underline and :box properties.  Fixing those, we are left with one
 >> bug, I think, that will be reproducible with Emacs -Q and evaluating:
 >>
 >> (defface test
 >>    '((((supports :underline (:style wave)))
 >>       :underline (:color "Red1" :style wave))
 >>      (t
 >>       :inherit warning))
 >>    "...")
 >>
 >> (defface test-2
 >>    '((((supports :underline (:style wave)))
 >>       :underline (:style wave :color "Red1"))
 >>      (t
 >>       :inherit warning))
 >>    "...")
 >>
 >> M-x customize-face RET test
 >> will show STANDARD state
 >>
 >> while
 >> M-x customize-face RET test-2
 >> will show EDITED state
 >
 > Thanks.
 >
 > Can you show a patch for the two bugs you've succeeded to identify?
 >
 > Did you make any progress with the one bug that's left after the other
 > two are fixed?

Here's a patch for the typo.

Concerning the other bugs I discovered, I think that while the filters
could be tweaked, a better fix would be to teach face-spec-match-p
about matching plists correctly and not just by equality.

[-- Attachment #2: 0001-Fix-typo-in-pre-filter-for-underline-property.patch --]
[-- Type: text/x-patch, Size: 979 bytes --]

From 409b59f21fec542e16740bfa62daf09df4cdcecb Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sat, 15 Jul 2023 17:05:23 -0300
Subject: [PATCH] Fix typo in pre-filter for underline property

* lisp/cus-face.el (custom-face-attributes): Fix typo (Bug#64347)
---
 lisp/cus-face.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index ec89b4f7ff6..a3a27263a7c 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -158,7 +158,7 @@ custom-face-attributes
 		    (or (and (consp real-value) (plist-get real-value :style))
 		        'line))
                    (position (and (consp real-value)
-                                  (plist-get real-value :style))))
+                                  (plist-get real-value :position))))
 	       (list :color color :style style :position position))))
      ;; filter to make customized-value suitable for storing
      ,(lambda (cus-value)
-- 
2.34.1

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Sat, 15 Jul 2023 17:01:44 -0300
> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
>  > Can we change this last aspect, so that the state is set using the
>  > original spec if the setting was not changed by the user?
> 
> OK, here's a patch for doing that.  It seems to me that after creating
> the widget, the only reason to use the value that's represented in the
> widget is if we loaded the spec from the :shown-value property (meaning
> we are redrawing the widget and we want to keep a spec that was already
> in the widget)

Thanks, installed on master.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Sat, 15 Jul 2023 17:11:21 -0300
> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Here's a patch for the typo.

Thanks, installed on the emacs-29 branch.

> Concerning the other bugs I discovered, I think that while the filters
> could be tweaked, a better fix would be to teach face-spec-match-p
> about matching plists correctly and not just by equality.

Probably.

Do we have anything left to do in this bug, or could it be closed?

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> Concerning the other bugs I discovered, I think that while the filters
 >> could be tweaked, a better fix would be to teach face-spec-match-p
 >> about matching plists correctly and not just by equality.
 >
 > Probably.
 >
 > Do we have anything left to do in this bug, or could it be closed?

I think this can be closed.

bug#64347: 30.0.50; Some customize faces shown as edited with -Q

[Eli Zaretskii]

> Date: Thu, 20 Jul 2023 15:56:02 -0300
> Cc: 64347@debbugs.gnu.org, stephen.berman@gmx.net
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>  > Do we have anything left to do in this bug, or could it be closed?
> 
> I think this can be closed.

Thanks, done.