bug#57499: Documentation bug in the docstring of set-face-attribute?

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

The docstring of set-face-attribute says:

"As an exception, to reset the value of some attribute to `unspecified' in 
a way that overrides the non-`unspecified' value defined by the face's 
spec in `defface', for new frames, you must explicitly call this function 
with FRAME set to t and the attribute's value set to `unspecified'; just 
using FRAME of nil will not affect new frames in this case."

Not only is that sentence hard to parse, it also seems wrong.

Can someone come up with a scenario in which a call

(set-face-attribute 'some-face nil :some-attribute 'unspecified)

only affects existing frames?  In my testing it affects all frames 
(existing and future ones), and that's also what the code seems to do: 
set-face-attribute sets where to 0 when frame is nil, and calls 
internal-set-face-attribute

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

[Sorry, my previous post was incomplete.]

>
> The docstring of set-face-attribute says:
>
> "As an exception, to reset the value of some attribute to `unspecified' 
> in a way that overrides the non-`unspecified' value defined by the 
> face's spec in `defface', for new frames, you must explicitly call this 
> function with FRAME set to t and the attribute's value set to 
> `unspecified'; just using FRAME of nil will not affect new frames in 
> this case."
>
> Not only is that sentence hard to parse, it also seems wrong.
>
> Can someone come up with a scenario in which a call
>
> (set-face-attribute 'some-face nil :some-attribute 'unspecified)
>
> only affects existing frames?  In my testing it affects all frames 
> (existing and future ones), and that's also what the code seems to do: 
> set-face-attribute sets where to 0 when frame is nil, and calls 
> internal-set-face-attribute
>

with frame = 0, which according to the docstring of 
internal-set-lisp-face-attribute "means change the face on all frames, and 
change the default for new frames".

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 08:14:00 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> 
> The docstring of set-face-attribute says:
> 
> "As an exception, to reset the value of some attribute to `unspecified' in 
> a way that overrides the non-`unspecified' value defined by the face's 
> spec in `defface', for new frames, you must explicitly call this function 
> with FRAME set to t and the attribute's value set to `unspecified'; just 
> using FRAME of nil will not affect new frames in this case."
> 
> Not only is that sentence hard to parse, it also seems wrong.
> 
> Can someone come up with a scenario in which a call
> 
> (set-face-attribute 'some-face nil :some-attribute 'unspecified)
> 
> only affects existing frames?  In my testing it affects all frames 
> (existing and future ones), and that's also what the code seems to do: 
> set-face-attribute sets where to 0 when frame is nil, and calls 
> internal-set-face-attribute

This was discussed in bug#54156.  Are there any new findings or
considerations that would require to reopen that discussion?

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> The docstring of set-face-attribute says:
>>
>> "As an exception, to reset the value of some attribute to `unspecified' 
>> in a way that overrides the non-`unspecified' value defined by the 
>> face's spec in `defface', for new frames, you must explicitly call this 
>> function with FRAME set to t and the attribute's value set to 
>> `unspecified'; just using FRAME of nil will not affect new frames in 
>> this case."
>>
>> Not only is that sentence hard to parse, it also seems wrong.
>>
>> Can someone come up with a scenario in which a call
>>
>> (set-face-attribute 'some-face nil :some-attribute 'unspecified)
>>
>> only affects existing frames?  In my testing it affects all frames 
>> (existing and future ones), and that's also what the code seems to do: 
>> set-face-attribute sets where to 0 when frame is nil, and calls 
>> internal-set-lisp-face-attribute with frame = 0, which according to the 
>> docstring of internal-set-lisp-face-attribute "means change the face on 
>> all frames, and change the default for new frames".
>
> This was discussed in bug#54156.  Are there any new findings or 
> considerations that would require to reopen that discussion?
>

As far as I can tell, there are, but if you disagree, feel free to close 
the bug.  Bug#54156 starts with someone telling that

(set-face-attribute 'some-face nil :background nil)

did not have an effect in new frames.  To which you replied:

>
> The correct way to do [that] is this:
> 
> (set-face-attribute 'some-face nil :background 'unspecified)
> (set-face-attribute 'some-face t :background 'unspecified)
>
> That is, one must explicitly call set-face-attribute with FRAME = t (as 
> well as nil), and pass 'unspecified' (NOT nil!) as the value.
>

and you later added that the call with frame = t is "a special trick to 
override defface with 'unspecified'".

It seems however that the call with frame = t is unnecessary, or at least, 
I could not come up with a scenario in which the first call does not also 
affect new frames.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 12:04:03 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> > This was discussed in bug#54156.  Are there any new findings or 
> > considerations that would require to reopen that discussion?
> >
> 
> As far as I can tell, there are, but if you disagree, feel free to close 
> the bug.  Bug#54156 starts with someone telling that
> 
> (set-face-attribute 'some-face nil :background nil)
> 
> did not have an effect in new frames.  To which you replied:
> 
> >
> > The correct way to do [that] is this:
> > 
> > (set-face-attribute 'some-face nil :background 'unspecified)
> > (set-face-attribute 'some-face t :background 'unspecified)
> >
> > That is, one must explicitly call set-face-attribute with FRAME = t (as 
> > well as nil), and pass 'unspecified' (NOT nil!) as the value.
> >
> 
> and you later added that the call with frame = t is "a special trick to 
> override defface with 'unspecified'".
> 
> It seems however that the call with frame = t is unnecessary, or at least, 
> I could not come up with a scenario in which the first call does not also 
> affect new frames.

I still don't understand what is new here.  All of that was said in
that old discussion. no?

Or let me turn the table and ask: what do you want to change in the
current doc string?  You want to tell that nil requires 2 calls, but
unspecified doesn't?

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>
> Or let me turn the table and ask: what do you want to change in the 
> current doc string?  You want to tell that nil requires 2 calls, but 
> unspecified doesn't?
>

I see that this issue is repeatedly confusing users, so I would suggest 
something like:

If FRAME is nil, set the attributes for all existing frames, as well as 
the default for new frames.  If FRAME is t, change the default for new 
frames only.

To reset the value of some attribute to `unspecified', you must use 
'unspecified, not nil.

This seems both much clearer than what we have now, and more correct, 
given that as far as I can tell

(set-face-attribute 'some-face nil :some-attribute 'unspecified)

changes the face attribute to "unspecified" on existing and future frame, 
and

(set-face-attribute 'some-face t :some-attribute 'unspecified)

changes the face attribute to "unspecified" on future frames only.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 12:53:26 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> I see that this issue is repeatedly confusing users, so I would suggest 
> something like:
> 
> If FRAME is nil, set the attributes for all existing frames, as well as 
> the default for new frames.  If FRAME is t, change the default for new 
> frames only.
> 
> To reset the value of some attribute to `unspecified', you must use 
> 'unspecified, not nil.

You consider this an improvement and clarification?  How many Lisp
programmers even know about unspecified, let alone understand how it
differs from nil?

> This seems both much clearer than what we have now, and more correct, 
> given that as far as I can tell
> 
> (set-face-attribute 'some-face nil :some-attribute 'unspecified)
> 
> changes the face attribute to "unspecified" on existing and future frame, 
> and
> 
> (set-face-attribute 'some-face t :some-attribute 'unspecified)
> 
> changes the face attribute to "unspecified" on future frames only.

It is clear to you because you've read bug#54156 and the recent
discussion on help-gnu-emacs.  The challenge is to make it clear to
others, who haven't.

This issue goes to the very intimate levels of the implementation
details of face handling, and of how we merge their attributes so as
to keep them independent on each frame.  At the time, I thought that
simplifying the issue, albeit at the price of telling half-lies, is
the best we could do, so that users have a cookbook-type recipe that
always works.  It's quite possible that better ways of documenting
this tricky aspect exist, but rest assured that just saying
"unspecified, not nil" is not such a better way, because it leaves too
many questions that beg answers.  Not for you and me, perhaps, but for
many others.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> If FRAME is nil, set the attributes for all existing frames, as well as 
>> the default for new frames.  If FRAME is t, change the default for new 
>> frames only.
>>
>> To reset the value of some attribute to `unspecified', you must use 
>> 'unspecified, not nil.
>
> You consider this an improvement and clarification?
>

I'm not sure, but I think it is, yes.

>
> How many Lisp programmers even know about unspecified, let alone 
> understand how it differs from nil?
>

Well, the next paragraph in the docstring says:

ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
attribute name.  All attributes can be set to `unspecified'; this fact is 
not further mentioned below.

So we could even move the sentence there:  To set an attribute to 
`unspecified', the symbol 'unspecified must be used.  Using nil may 
produce the same effect in some cases, but is not guaranteed to work.

>
> This issue goes to the very intimate levels of the implementation 
> details of face handling, and of how we merge their attributes so as to 
> keep them independent on each frame.  At the time, I thought that 
> simplifying the issue, albeit at the price of telling half-lies, is the 
> best we could do, so that users have a cookbook-type recipe that always 
> works.  It's quite possible that better ways of documenting this tricky 
> aspect exist, but rest assured that just saying "unspecified, not nil" 
> is not such a better way, because it leaves too many questions that beg 
> answers.
>

Is that not a quarter-lie, which would be better than a half-lie?

In which cases is the above sentence still wrong?  It seems to cover all 
cases I can think of, frame = nil (in which case all frames are affected), 
frame = t (in which case only future frames are affected) and frame = some 
frame (in which case only that frame is affected).

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 13:43:25 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> > How many Lisp programmers even know about unspecified, let alone 
> > understand how it differs from nil?
> 
> Well, the next paragraph in the docstring says:
> 
> ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
> attribute name.  All attributes can be set to `unspecified'; this fact is 
> not further mentioned below.
> 
> So we could even move the sentence there:  To set an attribute to 
> `unspecified', the symbol 'unspecified must be used.  Using nil may 
> produce the same effect in some cases, but is not guaranteed to work.

I don't think this answers the questions that did and will pop up.

> In which cases is the above sentence still wrong?

It isn't wrong, it just doesn't explain itself.  What do we want to
say with that passage that isn't said elsewhere in the doc string?
Would you be happy if that paragraph would have been removed?  If not,
why not?

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> So we could even move the sentence there:  To set an attribute to 
>> `unspecified', the symbol 'unspecified must be used.  Using nil may 
>> produce the same effect in some cases, but is not guaranteed to work.
>
> I don't think this answers the questions that did and will pop up.
>

Hmm... at least it would have answered the question of bug#54156, and the 
recent question on help-gnu-emacs.

>> In which cases is the above sentence still wrong?
>
> It isn't wrong, it just doesn't explain itself.  What do we want to say 
> with that passage that isn't said elsewhere in the doc string?
>

That nil shouldn't be used for attributes which don't give an explicit 
meaning to nil.  For example, :underline nil means "explicitly don't 
underline".  But, for example again, the docstring does not give an 
explicit meaning to :height nil or to :background nil.

>
> Would you be happy if that paragraph would have been removed?  If not, 
> why not?
>

I'm not sure I understand your question.  You mean, the sentence "All 
attributes can be set to `unspecified'; this fact is not further mentioned 
below"?

Just to be clear, I would be happier with the following docstring:

Set attributes of FACE on FRAME from ARGS.

This function overrides the face attributes specified by FACE's face spec. 
It is mostly intended for internal use only.

If FRAME is a frame, set the attributes only for that frame.  If FRAME is 
nil, set the attributes for all existing frames, as well as the default 
for new frames.  If FRAME is t, change the default for new frames only.

ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
attribute name.  All attributes can be set to `unspecified'; this fact is 
not further mentioned below.  To set an attribute to `unspecified', the 
symbol 'unspecified must be used.  Using nil may produce the same effect 
in some cases, but is not guaranteed to work.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 18:33:15 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> Just to be clear, I would be happier with the following docstring:
> 
> Set attributes of FACE on FRAME from ARGS.
> 
> This function overrides the face attributes specified by FACE's face spec. 
> It is mostly intended for internal use only.
> 
> If FRAME is a frame, set the attributes only for that frame.  If FRAME is 
> nil, set the attributes for all existing frames, as well as the default 
> for new frames.  If FRAME is t, change the default for new frames only.
> 
> ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
> attribute name.  All attributes can be set to `unspecified'; this fact is 
> not further mentioned below.  To set an attribute to `unspecified', the 
> symbol 'unspecified must be used.  Using nil may produce the same effect 
> in some cases, but is not guaranteed to work.

This is a step back, IMO.  It also repeats the same information more
than once, and confuses the nil vs unspecified issue for no good
reason.  How about the following instead:

  Set attributes of FACE on FRAME from ARGS.

  This function overrides the face attributes specified by FACE's face spec. 
  It is mostly intended for internal use only.

  If FRAME is a frame, set the attributes only for that frame.  If FRAME is 
  nil, set the attributes for all existing frames, as well as the default 
  for new frames.  If FRAME is t, change the default for new frames only.

  ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
  attribute name and VALUE must be a value that is valid for ATTRIBUTE,
  as described below for each attribute.

  All attributes can also be set to the special value `unspecified';
  this can be used to reset the value of ATTRIBUTE in a way that
  overrides any value defined by the face's spec in `defface'.

(Of course, after making such a change, we will again need to answer
questions how come using value of nil and FRAME = nil doesn't reset
the attribute, something that the current doc string avoids.  Oh
well.)

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>
> Set attributes of FACE on FRAME from ARGS.
>
> This function overrides the face attributes specified by FACE's face 
> spec. It is mostly intended for internal use only.
>
> If FRAME is a frame, set the attributes only for that frame.  If FRAME 
> is nil, set the attributes for all existing frames, as well as the 
> default for new frames.  If FRAME is t, change the default for new 
> frames only.
>
> ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
> attribute name and VALUE must be a value that is valid for ATTRIBUTE, as 
> described below for each attribute.
>
> All attributes can also be set to the special value `unspecified'; this 
> can be used to reset the value of ATTRIBUTE in a way that overrides any 
> value defined by the face's spec in `defface'.
>

This is better indeed, but I'd add "by using the symbol 'unspecified for 
VALUE" after "the special value `unspecified'".  Or perhaps use "the 
special VALUE `unspecified' with the explicit symbol 'unspecified".

>
> (Of course, after making such a change, we will again need to answer 
> questions how come using value of nil and FRAME = nil doesn't reset the 
> attribute, something that the current doc string avoids.  Oh well.)
>

I'm not sure I understand what you mean.  If the docstring says one should 
use the symbol 'unspecified, it should be clear to everyone that nil 
shouldn't be used, no?  What am I missing?

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 19:33:53 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> This is better indeed, but I'd add "by using the symbol 'unspecified for 
> VALUE" after "the special value `unspecified'".  Or perhaps use "the 
> special VALUE `unspecified' with the explicit symbol 'unspecified".

I don't see the difference between my text and these two variants.
Why repeat that the value `unspecified' is a symbol `unspecified'?  We
never say such tautological things in doc strings.

> > (Of course, after making such a change, we will again need to answer 
> > questions how come using value of nil and FRAME = nil doesn't reset the 
> > attribute, something that the current doc string avoids.  Oh well.)
> >
> 
> I'm not sure I understand what you mean.  If the docstring says one should 
> use the symbol 'unspecified, it should be clear to everyone that nil 
> shouldn't be used, no?  What am I missing?

Read the discussion in bug#54156 again!  That's what it was about.

Or read the code in xfaces which deals with value of unspecified when
FRAME = t -- it doesn't just store the symbol 'unspecified' in the
face's attribute, it does something more sneaky.  And it interprets
nil as unspecified in some cases, but not in others.

People stumble on these subtleties all the time, and the advice to use
an explicit separate call with FRAME = t in the current doc string was
intended to prevent that.  And note that it did work in Joost's case:
he maybe didn't fully understand _why_ he needs to do it, but he did
understand _how_ to do what he wanted.  Now we want to take that out,
because it hurts our excessive sense of rigor, and we will get the
same confusion back...

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>
> I don't see the difference between my text and these two variants. Why 
> repeat that the value `unspecified' is a symbol `unspecified'?  We never 
> say such tautological things in doc strings.
>

I just want to make it as clear as possible that to get that special value 
`unspecified' one should use the symbol 'unspecified.  That might be 
unclear, because after using nil describe-face also displays 
`unspecified'.

I bet that what happened with both Damien and Joost is that they wanted to 
unset the background/foreground color of a face, they tried to use nil 
(because that's the typical value one would use everywhere else in Elisp), 
and it seemed to work: describe-face said the attribute was "unspecified", 
and the visual effect was also that of an "unspecified" attribute.  Later 
they discovered that for new frames it didn't work.

>
> Read the discussion in bug#54156 again!  That's what it was about.
>
> Or read the code in xfaces which deals with value of unspecified when 
> FRAME = t -- it doesn't just store the symbol 'unspecified' in the 
> face's attribute, it does something more sneaky.  And it interprets nil 
> as unspecified in some cases, but not in others.
>
> People stumble on these subtleties all the time, and the advice to use 
> an explicit separate call with FRAME = t in the current doc string was 
> intended to prevent that.  And note that it did work in Joost's case: he 
> maybe didn't fully understand _why_ he needs to do it, but he did 
> understand _how_ to do what he wanted.  Now we want to take that out, 
> because it hurts our excessive sense of rigor, and we will get the same 
> confusion back...
>

Hmmm...  Joost's conclusion was that using frame = nil and 'unspecified 
solved his problem, and that he would do that.

Just to be clear: I certainly do not want to take anything out.  I simply 
do not understand (neither by testing nor by reading the code) what

(set-face-attribute 'some-face t :some-attribute 'unspecified)

does when

(set-face-attribute 'some-face nil :some-attribute 'unspecified)

has already been executed.  And in fact that's how this bug report 
started: I asked whether anyone could come up with a scenario that would 
make the effect of that call with frame = t apparent.

If there are cases where frame = t does something more, then clearly that 
information should stay in docstring.  My reading of the code is that it 
doesn't do anything more, because calling set-face-attribute with nil 
implies that Finternal_set_lisp_face_attribute is called with frame = 0, 
which in turn implies that Finternal_set_lisp_face_attribute is 
(recursively) called with frame = t.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Wed, 31 Aug 2022 21:13:58 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> > I don't see the difference between my text and these two variants. Why 
> > repeat that the value `unspecified' is a symbol `unspecified'?  We never 
> > say such tautological things in doc strings.
> 
> I just want to make it as clear as possible that to get that special value 
> `unspecified' one should use the symbol 'unspecified.

We have gazillions of such situations everywhere in Emacs where symbol
values are documented, and we never say anything beyond the name of
the symbol with proper quoting.

> > Read the discussion in bug#54156 again!  That's what it was about.
> >
> > Or read the code in xfaces which deals with value of unspecified when 
> > FRAME = t -- it doesn't just store the symbol 'unspecified' in the 
> > face's attribute, it does something more sneaky.  And it interprets nil 
> > as unspecified in some cases, but not in others.
> >
> > People stumble on these subtleties all the time, and the advice to use 
> > an explicit separate call with FRAME = t in the current doc string was 
> > intended to prevent that.  And note that it did work in Joost's case: he 
> > maybe didn't fully understand _why_ he needs to do it, but he did 
> > understand _how_ to do what he wanted.  Now we want to take that out, 
> > because it hurts our excessive sense of rigor, and we will get the same 
> > confusion back...
> 
> Hmmm...  Joost's conclusion was that using frame = nil and 'unspecified 
> solved his problem, and that he would do that.

Let me try explaining the issue which that part of the doc string
tries to address, one last time.

The text mentions 'defface'.  Why does it do that?  Because a face's
'defface' is relevant when realizing the faces of newly-created
frames; it has no effect whatsoever on existing frames.

So the issue here is: how do you reset a face's attribute to
'unspecified' in a way that would override what its 'defface' spec
says, and set the attribute to 'unspecified' on future frames?  The
doc string says that if you want to do that (and it is not clear that
you do, because faces are frame-local), _then_ you should call
set-face-attribute with FRAME = t and the attribute's value set to
'unspecified', and that call does special wizardry to ensure 'defface'
is overridden when new frames are created.

That is why the doc string mentions 'defface', and that's why it talks
specifically about new frames.  I thought mentioning both makes the
extra set-face-attribute call with FRAME = t natural and easier to
remember and apply.

So here's one more attempt to clarify the doc string without losing
that information:

  Set attributes of FACE on FRAME from ARGS.

  This function overrides the face attributes specified by FACE's face spec. 
  It is mostly intended for internal use.

  If FRAME is a frame, set the FACE's attributes only for that frame.  If
  FRAME is nil, set attribute values for all existing frames, as well as
  the default for new frames.  If FRAME is t, change the default values
  of attributes for new frames.

  ARGS must come in pairs ATTRIBUTE VALUE.  ATTRIBUTE must be a valid face 
  attribute name and VALUE must be a value that is valid for ATTRIBUTE,
  as described below for each attribute.

  In addition to the attribute values listed below, all attributes can
  also be set to the special value `unspecified', which means the face
  doesn't by itself specify a value for the attribute.

  When a new frame is created, attribute values in the FACE's
  `defspec' normally override the `unspecified' values in the FACE's
  default attributes.  To avoid that, i.e. to cause ATTRIBUTE's value
  be reset to `unspecified' when creating new frames, disregarding
  what the FACE's face spec says, call this function with FRAME set to
  t and the ATTRIBUTE's value set to `unspecified'.

> Just to be clear: I certainly do not want to take anything out.  I simply 
> do not understand (neither by testing nor by reading the code) what
> 
> (set-face-attribute 'some-face t :some-attribute 'unspecified)
> 
> does when
> 
> (set-face-attribute 'some-face nil :some-attribute 'unspecified)
> 
> has already been executed.

Do you understand what

  (set-face-attribute 'some-face t :some-attribute 'unspecified)

does differently from what

  (set-face-attribute 'some-face FRAME :some-attribute 'unspecified)

does?  I mean, besides the difference in FRAME argument value?

> And in fact that's how this bug report started: I asked whether
> anyone could come up with a scenario that would make the effect of
> that call with frame = t apparent.

See above: that's not what that part of the doc string attempts to
address.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> I just want to make it as clear as possible that to get that special 
>> value `unspecified' one should use the symbol 'unspecified.
>
> We have gazillions of such situations everywhere in Emacs where symbol 
> values are documented, and we never say anything beyond the name of the 
> symbol with proper quoting.
>

For some reason this situation seems different (from a user point of 
view), give that the same question pops again and again.  Why is adding 
such a note a problem?

>
> That is why the doc string mentions 'defface', and that's why it talks 
> specifically about new frames.  I thought mentioning both makes the 
> extra set-face-attribute call with FRAME = t natural and easier to 
> remember and apply.
>

I'm really puzzled.  Why should the value 'unspecified be mentioned there 
as if it were somehow different from the other possible values, when in 
fact it isn't?  Once again when one calls

(set-face-attribute 'isearch nil :background 'unspecified)

this is what is happening:

(internal-set-lisp-face-attributes 'isearch :background 'unspecified 0)

which calls, recursively:

(internal-set-lisp-face-attributes 'isearch :background 'unspecified t)
(internal-set-lisp-face-attributes 'isearch :background 'unspecified #<frame-1>)
...
(internal-set-lisp-face-attributes 'isearch :background 'unspecified #<frame-n>)

And when one calls

(set-face-attribute 'isearch t :background 'unspecified)

this is what is happening:

(internal-set-lisp-face-attributes 'isearch :background 'unspecified t)

So this call is already included in the previous one.  Why should we tell 
users to add such a redundant call in their code?

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.

>
> In addition to the attribute values listed below, all attributes can 
> also be set to the special value `unspecified', which means the face 
> doesn't by itself specify a value for the attribute.
>

With "... the special value `unspecified' (for which the explicit symbol 
'unspecified must be used), which means ...", that'd be okay.

>
> When a new frame is created, attribute values in the FACE's `defspec' 
> normally override the `unspecified' values in the FACE's default 
> attributes.  To avoid that, i.e. to cause ATTRIBUTE's value be reset to 
> `unspecified' when creating new frames, disregarding what the FACE's 
> face spec says, call this function with FRAME set to t and the 
> ATTRIBUTE's value set to `unspecified'.
>

See above: I really don't understand why the 'unspecified value should be 
detailed as if it were different from the other values, when in fact it 
isn't.  The real and actual problem here is that users are confused by the 
fact that a nil value for an attribute is equivalent to an 'unspecified 
value for existing frames, but is not equivalent to 'unspecified for new 
frames.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Thu, 01 Sep 2022 08:25:35 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499@debbugs.gnu.org
> 
> >> I just want to make it as clear as possible that to get that special 
> >> value `unspecified' one should use the symbol 'unspecified.
> >
> > We have gazillions of such situations everywhere in Emacs where symbol 
> > values are documented, and we never say anything beyond the name of the 
> > symbol with proper quoting.
> 
> For some reason this situation seems different (from a user point of 
> view), give that the same question pops again and again.  Why is adding 
> such a note a problem?

Because we don't say anything like that anywhere else.

> And when one calls
> 
> (set-face-attribute 'isearch t :background 'unspecified)
> 
> this is what is happening:
> 
> (internal-set-lisp-face-attributes 'isearch :background 'unspecified t)
> 
> 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 FRAM = t 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.

> > When a new frame is created, attribute values in the FACE's `defspec' 
> > normally override the `unspecified' values in the FACE's default 
> > attributes.  To avoid that, i.e. to cause ATTRIBUTE's value be reset to 
> > `unspecified' when creating new frames, disregarding what the FACE's 
> > face spec says, call this function with FRAME set to t and the 
> > ATTRIBUTE's value set to `unspecified'.
> 
> See above: I really don't understand why the 'unspecified value should be 
> detailed as if it were different from the other values, when in fact it 
> isn't.  The real and actual problem here is that users are confused by the 
> fact that a nil value for an attribute is equivalent to an 'unspecified 
> value for existing frames, but is not equivalent to 'unspecified for new 
> frames.

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

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> 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;

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Thu, 01 Sep 2022 09:02:50 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499-done@debbugs.gnu.org
> 
> >> 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.

I disagree with your certainty that everyone will understand that.
The text deliberately doesn't say that.

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

No.  I don't want to lose this compatibility feature because lots of
people still use nil.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> Would the patch below be okay?  This would be another way to clarify 
>> the matter.
>
> No.  I don't want to lose this compatibility feature because lots of 
> people still use nil.
>

Would a warning in *Messages* be okay then?  People who use nil with frame 
= t or nil will not get what they expect, right?

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Thu, 01 Sep 2022 11:56:06 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499-done@debbugs.gnu.org
> 
> >> Would the patch below be okay?  This would be another way to clarify 
> >> the matter.
> >
> > No.  I don't want to lose this compatibility feature because lots of 
> > people still use nil.
> >
> 
> Would a warning in *Messages* be okay then?

We could try that, yes.  But it has to be only in *Messages*, not an
actual warning, since faces get merged and realized as part of
redisplay, when signaling an error is a bad idea.

> People who use nil with frame = t or nil will not get what they
> expect, right?

If they expect what you think they expect, yes.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

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


>> Would a warning in *Messages* be okay then?
>
> We could try that, yes.  But it has to be only in *Messages*, not an 
> actual warning, since faces get merged and realized as part of 
> redisplay, when signaling an error is a bad idea.
>

Like this?  I just checked, it passes make bootstrap and make check.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Display-a-warning-when-using-nil-as-a-face-attribute.patch --]
[-- Type: text/x-diff; name=Display-a-warning-when-using-nil-as-a-face-attribute.patch, Size: 2488 bytes --]

From c37b2cfdf9c5cbcba7cc174fcf8d6d17152c88a2 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 1 Sep 2022 13:02:27 +0000
Subject: [PATCH] Display a warning when using nil as a face attribute value.

* src/xfaces.c (Finternal_set_lisp_face_attribute): Using the
attribute value nil for the face attributes foreground,
distant-foreground and background is a backward compatibility feature,
that works only with existing frames.  Using it with frame = t (and
therefore also with frame = nil) will not produce the expected effect.
Display a warning in these cases.
---
 src/xfaces.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/xfaces.c b/src/xfaces.c
index 70d5cbeb4c..db7bc028c5 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3051,6 +3051,22 @@ DEFUN ("internal-copy-lisp-face", Finternal_copy_lisp_face,
   return to;
 }
 
+char warning_attribute_nil[] =
+  "Warning: using :%s nil with frame t has no effect.";
+
+#define WARNING_ATTRIBUTE_NIL(S)					\
+  if (NILP (value) && EQ (frame, Qt))					\
+    {									\
+      if (redisplaying_p)						\
+	CALLN (Fmessage,						\
+	       build_string (warning_attribute_nil),			\
+	       build_string (S));					\
+      else								\
+	Fsignal (Quser_error,						\
+		 list1 (CALLN (Fformat,					\
+			       build_string (warning_attribute_nil),	\
+			       build_string (S))));			\
+    }
 
 DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
        Sinternal_set_lisp_face_attribute, 3, 4, 0,
@@ -3390,6 +3406,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCforeground))
     {
+      WARNING_ATTRIBUTE_NIL ("foreground");
       /* Compatibility with 20.x.  */
       if (NILP (value))
 	value = Qunspecified;
@@ -3409,6 +3426,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCdistant_foreground))
     {
+      WARNING_ATTRIBUTE_NIL ("distant-foreground");
       /* Compatibility with 20.x.  */
       if (NILP (value))
 	value = Qunspecified;
@@ -3428,6 +3446,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCbackground))
     {
+      WARNING_ATTRIBUTE_NIL ("background");
       /* Compatibility with 20.x.  */
       if (NILP (value))
 	value = Qunspecified;
-- 
2.35.1

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Thu, 01 Sep 2022 13:15:42 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499-done@debbugs.gnu.org
> 
> >> Would a warning in *Messages* be okay then?
> >
> > We could try that, yes.  But it has to be only in *Messages*, not an 
> > actual warning, since faces get merged and realized as part of 
> > redisplay, when signaling an error is a bad idea.
> >
> 
> Like this?  I just checked, it passes make bootstrap and make check.

Not exactly what I had in mind.

> +char warning_attribute_nil[] =
> +  "Warning: using :%s nil with frame t has no effect.";

I thought you wanted to warn about using nil where nil is not a valid
value.  The calls to the macro are all in the case where the actual
problem is that nil is not a valid value, so the warning text is not
what we want to convey (or so I thought).

> +      if (redisplaying_p)						\
> +	CALLN (Fmessage,						\
> +	       build_string (warning_attribute_nil),			\
> +	       build_string (S));					\

There's no need to use Fmessage, just add_to_log should be enough.

> +      else								\
> +	Fsignal (Quser_error,						\
> +		 list1 (CALLN (Fformat,					\
> +			       build_string (warning_attribute_nil),	\
> +			       build_string (S))));			\

And I don't agree with signaling a user-error for these cases, when
they are outside redisplay.  For starters, the face code could be
called during C-n or C-v or some other command that uses the display
code internally.  Signaling an error is only TRT if we stop supporting
this back-compatibility shim.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

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


>
> Not exactly what I had in mind.
>

Alas, I don't have a crystal ball (yet) 😉

What about the attached, which deprecates that backward compability shim 
(instead of stopping to support it)?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Display-a-warning-for-some-uses-of-nil-for-face-attr.patch --]
[-- Type: text/x-diff; name=Display-a-warning-for-some-uses-of-nil-for-face-attr.patch, Size: 3462 bytes --]

From 1132daf0b11bc11d34e5806cdbf500f68849ce09 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 1 Sep 2022 16:59:45 +0000
Subject: [PATCH] Display a warning for some uses of nil for face attributes.

* src/xfaces.c (HANDLE_INVALID_OR_DEPRECATED_NIL_VALUE): New macro,
which displays a warning for invalid or deprecated uses of nil as a
face attribute value.
(Finternal_set_lisp_face_attribute): Use the macro for the attributes
:foreground, :distant-foreground and :background.

* etc/NEWS: Announce the deprecation.
---
 etc/NEWS     |  6 ++++++
 src/xfaces.c | 26 +++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 89f4cd0ac7..e3abfe76ce 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2754,6 +2754,12 @@ request the name of the ".eln" file which defined a given symbol.
 +++
 ** New macro 'with-memoization' provides a very primitive form of memoization.
 
+---
+** The value nil is now deprecated for some face attributes.
+The face attributes :background, :foreground and :distant-foreground
+could be given a value nil for backward-compatibility with Emacs 20.
+This is now deprecated, the 'unspecified value should be used instead.
+
 ** Themes
 
 ---
diff --git a/src/xfaces.c b/src/xfaces.c
index 70d5cbeb4c..486814fb38 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3052,6 +3052,20 @@ DEFUN ("internal-copy-lisp-face", Finternal_copy_lisp_face,
 }
 
 
+#define HANDLE_INVALID_OR_DEPRECATED_NIL_VALUE(A)			\
+  if (NILP (value))							\
+    {									\
+      if (EQ (frame, Qt))						\
+	add_to_log ("Warning: invalid value nil for attribute `%s' "	\
+		    "with frame t or nil: use 'unspecified instead of " \
+		    "nil", A);						\
+      else								\
+	add_to_log ("Warning: using value nil for attribute `%s' is "	\
+		    "deprecated: use 'unspecified instead of nil", A);	\
+      /* Compatibility with 20.x.  */					\
+      value = Qunspecified;						\
+    }
+
 DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
        Sinternal_set_lisp_face_attribute, 3, 4, 0,
        doc: /* Set attribute ATTR of FACE to VALUE.
@@ -3390,9 +3404,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCforeground))
     {
-      /* Compatibility with 20.x.  */
-      if (NILP (value))
-	value = Qunspecified;
+      HANDLE_INVALID_OR_DEPRECATED_NIL_VALUE (QCforeground);
       if (!UNSPECIFIEDP (value)
 	  && !IGNORE_DEFFACE_P (value)
 	  && !RESET_P (value))
@@ -3409,9 +3421,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCdistant_foreground))
     {
-      /* Compatibility with 20.x.  */
-      if (NILP (value))
-	value = Qunspecified;
+      HANDLE_INVALID_OR_DEPRECATED_NIL_VALUE (QCdistant_foreground);
       if (!UNSPECIFIEDP (value)
 	  && !IGNORE_DEFFACE_P (value)
 	  && !RESET_P (value))
@@ -3428,9 +3438,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCbackground))
     {
-      /* Compatibility with 20.x.  */
-      if (NILP (value))
-	value = Qunspecified;
+      HANDLE_INVALID_OR_DEPRECATED_NIL_VALUE (QCbackground);
       if (!UNSPECIFIEDP (value)
 	  && !IGNORE_DEFFACE_P (value)
 	  && !RESET_P (value))
-- 
2.35.1

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Thu, 01 Sep 2022 17:07:13 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499-done@debbugs.gnu.org
> 
> +---
> +** The value nil is now deprecated for some face attributes.
> +The face attributes :background, :foreground and :distant-foreground
> +could be given a value nil for backward-compatibility with Emacs 20.
> +This is now deprecated, the 'unspecified value should be used instead.

It isn't deprecated, it's invalid.  There's no need to have anything
like this in NEWS, because all our documentation says the color
attributes can have only string values (in addition to 'unspecified').

> +#define HANDLE_INVALID_OR_DEPRECATED_NIL_VALUE(A)			\
> +  if (NILP (value))							\
> +    {									\
> +      if (EQ (frame, Qt))						\
> +	add_to_log ("Warning: invalid value nil for attribute `%s' "	\
> +		    "with frame t or nil: use 'unspecified instead of " \
> +		    "nil", A);						\
> +      else								\
> +	add_to_log ("Warning: using value nil for attribute `%s' is "	\
> +		    "deprecated: use 'unspecified instead of nil", A);	\
> +      /* Compatibility with 20.x.  */					\
> +      value = Qunspecified;						\

No need to differentiate between FRAME = t and the other cases.

And the message text should just say

  Warning: nil value for `%s' is invalid, use 'unspecified instead.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

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


>
> It isn't deprecated, it's invalid.  There's no need to have anything 
> like this in NEWS, because all our documentation says the color 
> attributes can have only string values (in addition to 'unspecified').
>

Okay (I thought you wanted to retain the "undocumented but not really 
invalid" use of nil).

>
> Warning: nil value for `%s' is invalid, use 'unspecified instead.
>

I added the name of the face in the message, which is I think useful to 
locate the cause of the warning.

Should this go to master or to emacs-28?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Display-a-warning-for-some-uses-of-nil-in-face-attri.patch --]
[-- Type: text/x-diff; name=Display-a-warning-for-some-uses-of-nil-in-face-attri.patch, Size: 2531 bytes --]

From cbe76f8959bc1c7713ae84ab657adb3bbf0c4fde Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 1 Sep 2022 19:31:34 +0000
Subject: [PATCH] Display a warning for some uses of nil in face attributes.

* src/xfaces.c (HANDLE_INVALID_NIL_VALUE): New macro, which displays
a warning for invalid uses of nil as a face attribute value.
(Finternal_set_lisp_face_attribute): Use the macro for the attributes
:foreground, :distant-foreground and :background.
---
 src/xfaces.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/xfaces.c b/src/xfaces.c
index 70d5cbeb4c..539f50bce9 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3052,6 +3052,15 @@ DEFUN ("internal-copy-lisp-face", Finternal_copy_lisp_face,
 }
 
 
+#define HANDLE_INVALID_NIL_VALUE(A,F)					\
+  if (NILP (value))							\
+    {									\
+      add_to_log ("Warning: setting attribute `%s' of face `%s': nil "	\
+		  "value is invalid, use 'unspecified instead.", A, F); \
+      /* Compatibility with 20.x.  */					\
+      value = Qunspecified;						\
+    }
+
 DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
        Sinternal_set_lisp_face_attribute, 3, 4, 0,
        doc: /* Set attribute ATTR of FACE to VALUE.
@@ -3390,9 +3399,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCforeground))
     {
-      /* Compatibility with 20.x.  */
-      if (NILP (value))
-	value = Qunspecified;
+      HANDLE_INVALID_NIL_VALUE (QCforeground, face);
       if (!UNSPECIFIEDP (value)
 	  && !IGNORE_DEFFACE_P (value)
 	  && !RESET_P (value))
@@ -3409,9 +3416,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCdistant_foreground))
     {
-      /* Compatibility with 20.x.  */
-      if (NILP (value))
-	value = Qunspecified;
+      HANDLE_INVALID_NIL_VALUE (QCdistant_foreground, face);
       if (!UNSPECIFIEDP (value)
 	  && !IGNORE_DEFFACE_P (value)
 	  && !RESET_P (value))
@@ -3428,9 +3433,7 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
     }
   else if (EQ (attr, QCbackground))
     {
-      /* Compatibility with 20.x.  */
-      if (NILP (value))
-	value = Qunspecified;
+      HANDLE_INVALID_NIL_VALUE (QCbackground, face);
       if (!UNSPECIFIEDP (value)
 	  && !IGNORE_DEFFACE_P (value)
 	  && !RESET_P (value))
-- 
2.35.1

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Thu, 01 Sep 2022 19:35:35 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499-done@debbugs.gnu.org
> 
> > It isn't deprecated, it's invalid.  There's no need to have anything 
> > like this in NEWS, because all our documentation says the color 
> > attributes can have only string values (in addition to 'unspecified').
> >
> 
> Okay (I thought you wanted to retain the "undocumented but not really 
> invalid" use of nil).
> 
> >
> > Warning: nil value for `%s' is invalid, use 'unspecified instead.
> >
> 
> I added the name of the face in the message, which is I think useful to 
> locate the cause of the warning.

Thanks, SGTM.

> Should this go to master or to emacs-28?

To master, please.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> Should this go to master or to emacs-28?
>
> To master, please.
>

Thanks, now done.

A last suggestion: I think the docstring of set-face-attribute would be 
even clearer with

call this function with FRAME set to t or nil, and the ATTRIBUTE's value 
set to `unspecified'.

or with

call this function with FRAME set to t (or nil), and the ATTRIBUTE's value 
set to `unspecified'.

Just as a reminder that nil "implies" t.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> >
>> > Warning: nil value for `%s' is invalid, use 'unspecified instead.
>> >
>> 
>> I added the name of the face in the message, which is I think useful to 
>> locate the cause of the warning.
>
> Thanks, SGTM.

Shouldn't the message say `unspecified' instead of 'unspecified?

'unspecified reads to (quote unspecified), and a list of `quote' and
`unspecified' is definitely not the right thing.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Eli Zaretskii]

> Date: Fri, 02 Sep 2022 20:48:06 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 57499-done@debbugs.gnu.org
> 
> A last suggestion: I think the docstring of set-face-attribute would be 
> even clearer with
> 
> call this function with FRAME set to t or nil, and the ATTRIBUTE's value 
> set to `unspecified'.
> 
> or with
> 
> call this function with FRAME set to t (or nil), and the ATTRIBUTE's value 
> set to `unspecified'.

That would miss the point which that part of the doc string is trying
to make: that using 'unspecified' with FRAME = t does special things.
Explaining that is the whole point of that part of the doc string.

> Just as a reminder that nil "implies" t.

Such a reminder isn't needed because just a few sentences above the
doc string said that nil implies t as well.

bug#57499: Documentation bug in the docstring of set-face-attribute?

[Gregory Heytings]

>> Just as a reminder that nil "implies" t.
>
> Such a reminder isn't needed because just a few sentences above the doc 
> string said that nil implies t as well.
>

Okay, then I guess we found a good compromise, and this bug can rest in 
peace.