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