GNU bug report logs - #57499
Documentation bug in the docstring of set-face-attribute?

Previous Next

Full log

View this message in rfc822 format

From: Gregory Heytings <gregory <at> heytings.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57499-done <at> debbugs.gnu.org
Subject: bug#57499: Documentation bug in the docstring of set-face-attribute?
Date: Thu, 01 Sep 2022 09:02:50 +0000

>> So this call is already included in the previous one.  Why should we 
>> tell users to add such a redundant call in their code?
>
> The new text doesn't say the call with FRAME = t should be an additional 
> call.
>

It doesn't say so explicitly indeed, but without reading the code of 
set-face-attribute everyone understands that it should be an additional 
call.

>> As far as I understand, the actual and real problem here is some users 
>> use nil when they should use 'unspecified, because they are not aware 
>> that nil and 'unspecified are subtly different.  The subtle difference 
>> is that using nil works for frame = #<frame-1> ... #<frame-n>, but does 
>> not work with frame = t.
>
> That is a backward-compatibility feature that I don't want to mention in 
> the doc string.  Lisp programs should only use valid values that are 
> documented in the doc string.
>

I wasn't suggesting to mention this.  I was suggesting to add a mention 
that the symbol 'unspecified should be used for the value `unspecified', 
which might be clear to you and me but is evidently not clear for users.

>
> I've installed the last text I proposed, and I'm closing this bug with 
> that.
>

Would the patch below be okay?  This would be another way to clarify the 
matter.

diff --git a/src/xfaces.c b/src/xfaces.c
index 70d5cbeb4c..2dfba51f74 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3390,6 +3390,8 @@ DEFUN ("internal-set-lisp-face-attribute", 
Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCforeground))
     {
+      if (NILP (value) && EQ (frame, Qt))
+       signal_error ("Invalid foreground face attribute value", value);
       /* Compatibility with 20.x.  */
       if (NILP (value))
        value = Qunspecified;
@@ -3410,6 +3412,8 @@ DEFUN ("internal-set-lisp-face-attribute", 
Finternal_set_lisp_face_attribute,
   else if (EQ (attr, QCdistant_foreground))
     {
       /* Compatibility with 20.x.  */
+      if (NILP (value) && EQ (frame, Qt))
+       signal_error ("Invalid distant-foreground face attribute value", value);
       if (NILP (value))
        value = Qunspecified;
       if (!UNSPECIFIEDP (value)
@@ -3428,6 +3432,8 @@ DEFUN ("internal-set-lisp-face-attribute", 
Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCbackground))
     {
+      if (NILP (value) && EQ (frame, Qt))
+       signal_error ("Invalid background face attribute value", value);
       /* Compatibility with 20.x.  */
       if (NILP (value))
        value = Qunspecified;

Previous Next

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