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

[David Ponce <da_vid@orange.fr>]

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!