bug#67404: 30.0.50; Issue with :box face attribute

[David Ponce]

On 30/11/2023 16:20, Eli Zaretskii wrote:
> > Date: Thu, 30 Nov 2023 07:46:02 -0300
> > Cc: 67404@debbugs.gnu.org
> > From: Mauro Aranda <maurooaranda@gmail.com>
> >
> > On 26/11/23 07:27, Eli Zaretskii wrote:
> >
> >   >> Date: Thu, 23 Nov 2023 16:23:15 +0100
> >   >> From: David Ponce <da_vid@orange.fr>
> >   >>
> >   >> Hello,
> >   >>
> >   >> The below face with specified :box attribute can be defined and further
> >   >> used without problem:
> >   >>
> >   >> (defface test-face
> >   >>    '((t
> >   >>       (:box (:style released-button :line-width (3 . 0)))
> >   >>       ))
> >   >>    "Test face.")
> >   >>
> >   >> (insert (propertize "XXXXX" 'face 'test-face))
> >   >>
> >   >> However, it is not possible to apply any change to this face with
> >   >> `customize-face', for example to change :line-width (3 . 0) to
> >   >> :line-width (2 . 0), or to add a :foreground color, etc. This fails
> >   >> with the message "set-face-attribute: Invalid face box: :line-width,
> >   >> <value>, :style, <value>", and the :box attribute is discarded.
> >   >>
> >   >> AFAICS, :box attribute with any zero :line-width value works as
> >   >> expected: the box line with zero width is not displayed. But zero is
> >   >> not considered as a valid value by `set-face-attribute' (in fact, the
> >   >> internal function `internal-set-lisp-face-attribute').
> >   >>
> >   >> For consistency, a zero :line-width value either should be accepted by
> >   >> `internal-set-lisp-face-attribute', or never accepted at all.
> >   >
> >   > Mauro, any comments about these issues?
> >
> > Hi Eli,
> >
> > Not really, no.  The widget used for line-width just assumes 0 is a
> > valid value.  I don't know if that's the case or not, it is not
> > explicitly documented in the ELisp manual and I don't really know the
> > xfaces.c code.
> >
> > For some reason, the code that rejects a value of 0 in
> > Finternal_set_lisp_face_attribute doesn't complain (maybe it does not
> > get to run?) when using defface, but it does complain when Customize
> > tries to set it, by calling face-spec-set.
>
> Thanks for the hints.  This is a (very old) bug in
> Finternal_set_lisp_face_attribute.  The value zero for line-width is
> not valid, but because of the bug it (and invalid values of other box
> attributes) could be accepted, but only if the invalid attribute is
> the last attribute of :box.  Try this equivalent definition:
>
> (defface test-face
>     '((t
>        (:box (:line-width (3 . 0) :style released-button))
>        ))
>     "Test face.")
>
> which just swaps the :box attributes, and you will see that defface
> signals an error.  Customizing the :line-width attribute just happens
> to move that attribute to the front of the list, and then you get the
> error.
>
> I've now fixed this on the master branch, and now zero width values
> will always be rejected.

Hi Eli,

I confirm that with your fix on master branch, the behavior is now
consistent between defface and customize-face: both signal an error
on value zero for line-width :-)

However, please note that it remains possible to use a value zero for
line-width on an anonymous face.  For example, in the *scratch* buffer
you still can eval the below expression without error:
 
(insert
  (propertize "xxxx" 'font-lock-face
              '(:box (:style released-button :line-width (3 . 0)))))

Thanks!