bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

This is a useful, general function, not just a helper.
Please consider giving it a better name and doc string.

Wrt the doc, the SPEC arg is apparently NOT a face spec, i.e., a spec
such as is returned by (get FACE 'face-defface-spec).  Instead, it seems
to be an attributes list such as what is returned by `face-spec-choose'
or `face-attr-construct'.

In GNU Emacs 24.5.1 (i686-pc-mingw32)
 of 2015-04-11 on LEG570
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/usr --host=i686-pc-mingw32'

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Sun, 4 Jun 2017 10:42:37 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> 
> This is a useful, general function, not just a helper.

FWIW, I don't think it's general enough, because it is tightly couples
with face-x-resources.

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

> > This is a useful, general function, not just a helper.
> 
> FWIW, I don't think it's general enough, because it is tightly couples
> with face-x-resources.

That's only the _implementation_.

(That reasoning would apply also to `face-spec-recalc', `face-spec-set',
`face-set-after-frame-default', `custom-face-set', `custom-face-save',
`custom-face-mark-to-save', `custom-face-reset-saved',
`custom-face-mark-to-reset-standard', `custom-declare-face',
`custom-theme-set-faces', `custom-reset-faces', `woman-default-faces',
...)

Unless you essentially repeat its body, it is the only way
to do what it does.  Here's one use case:

(face-spec-set-2 TARGET-FACE
                 FRAME
                 (face-spec-choose
                   (get SOURCE-FACE 'face-defface-spec)
                   FRAME))

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Sun, 4 Jun 2017 12:38:10 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 27238@debbugs.gnu.org
> 
> (That reasoning would apply also to `face-spec-recalc', `face-spec-set',
> `face-set-after-frame-default', `custom-face-set', `custom-face-save',
> `custom-face-mark-to-save', `custom-face-reset-saved',
> `custom-face-mark-to-reset-standard', `custom-declare-face',
> `custom-theme-set-faces', `custom-reset-faces', `woman-default-faces',
> ...)

They all call face-spec-recalc.

> Unless you essentially repeat its body, it is the only way
> to do what it does.  Here's one use case:
> 
> (face-spec-set-2 TARGET-FACE
>                  FRAME
>                  (face-spec-choose
>                    (get SOURCE-FACE 'face-defface-spec)
>                    FRAME))

Why can't you do this by calling a higher-level function?

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

> > > > This is a useful, general function, not just a helper.
> > > 
> > > FWIW, I don't think it's general enough, because it is
> > > tightly couples with face-x-resources.
> > 
> > That's only the _implementation_.
> > 
> > (That reasoning would apply also to `face-spec-recalc', `face-spec-set',
> > `face-set-after-frame-default', `custom-face-set', `custom-face-save',
> > `custom-face-mark-to-save', `custom-face-reset-saved',
> > `custom-face-mark-to-reset-standard', `custom-declare-face',
> > `custom-theme-set-faces', `custom-reset-faces', `woman-default-faces',
> > ...)
> 
> They all call face-spec-recalc.

`face-spec-recalc' calls `face-spec-2', which calls
`face-x-resources'.

Your complaint is that `face-spec-2' should not be treated as
first-class (i.e., given a reasonable name and accurate doc)
because it depends on using `face-x-resources'.  As I said,
that is only its current _implementation_.

As far as _using_ `face-spec-2' is concerned (which is what
this is about), all of the above make use of it.  Given its
current implementation, they depend on that implementation
just as much as it does.  Change the implementation and there
is no coupling with it.  Leave the implementation as is and
they all continue to depend on it.

Your reasoning applies also to all of those functions.  If
your argument is that we should not encourage users to use
this function (by improving its name and doc) because its
implementation is fragile, then that same argument applies
to all current uses of it.

And there are plenty, including uses defining functions,
such as `face-spec-set', that are very widely used, including
by users.

> > Unless you essentially repeat its body, it is the only way
> > to do what it does.  Here's one use case:
> >
> > (face-spec-set-2 TARGET-FACE
> >                  FRAME
> >                  (face-spec-choose
> >                    (get SOURCE-FACE 'face-defface-spec)
> >                    FRAME))
> 
> Why can't you do this by calling a higher-level function?

What higher-level function would you suggest?  That function
does that job - and so it is used in `faces.el' (ultimately
by all of the functions listed above).

How would _you_ set one face (`fringe' or whatever), in
only a given frame, to the spec of another face (or to
a spec that isn't yet used for any face)?

I don't see `face-spec-set' offering that possibility, at
least not directly.  Am I missing something?  I tried it
with each of the allowed SPEC-TYPEs, to see what they'd
do, but none of them did that.  A nil SPEC-TYPE sort of
does the job, but without the ability to limit the setting
to a given frame.

But perhaps you have some other higher-level function in
mind to do this job?  If so, I wonder why `face-set-recalc'
doesn't use it, instead of `face-spec-set-2.

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Sun, 4 Jun 2017 22:44:40 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 27238@debbugs.gnu.org
> 
> > > (face-spec-set-2 TARGET-FACE
> > >                  FRAME
> > >                  (face-spec-choose
> > >                    (get SOURCE-FACE 'face-defface-spec)
> > >                    FRAME))
> > 
> > Why can't you do this by calling a higher-level function?
> 
> What higher-level function would you suggest?

face-spec-recalc and face-spec-set come to mind, for example.

> How would _you_ set one face (`fringe' or whatever), in
> only a given frame, to the spec of another face (or to
> a spec that isn't yet used for any face)?

I asked whether the higher-level functions can do the job.  If they
cannot, please explain why, and please provide specific details about
the difficulties.  Answering my question by another question doesn't
help, since I'm sure I don't know enough about the job you wanted to
do.

> But perhaps you have some other higher-level function in
> mind to do this job?  If so, I wonder why `face-set-recalc'
> doesn't use it, instead of `face-spec-set-2.

There are many documented functions that set face attributes in
various forms and for various sets of frames.  My question is
precisely whether any of them can do this job.  If not, perhaps we
could extend one of them to support whatever you need to do.  That'd
be an alternative to renaming face-spec-set-2 and making it public.

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

> > > > (face-spec-set-2 TARGET-FACE
> > > >                  FRAME
> > > >                  (face-spec-choose
> > > >                    (get SOURCE-FACE 'face-defface-spec)
> > > >                    FRAME))
> > >
> > > Why can't you do this by calling a higher-level function?
> >
> > What higher-level function would you suggest?
> 
> face-spec-recalc and face-spec-set come to mind, for example.

I don't see how either of those can be used to do the same
thing.  If you think otherwise, please show how.

> > How would _you_ set one face (`fringe' or whatever), in
> > only a given frame, to the spec of another face (or to
> > a spec that isn't yet used for any face)?
> 
> I asked whether the higher-level functions can do the job.  If they
> cannot, please explain why, and please provide specific details about
> the difficulties.  Answering my question by another question doesn't
> help, since I'm sure I don't know enough about the job you wanted to
> do.

I don't know of any that could do the job, as I've said.
I described the situation with `face-spec-set', and said
that I tried to use it.  You can, I think, see that it
is not frame-specific.

The job to do is shown in the code I gave above.  And it is
described in the description I gave:

  "set one face (`fringe' or whatever), in only a given frame,
                                        ^^^^^^^^^^^^^^^^^^^^^
  to the spec of another face (or to a spec that isn't yet used
  for any face)"

I don't see a "higher-level function" that does that.  I don't
see any function that does, apart from `face-spec-2'.

> > But perhaps you have some other higher-level function in
> > mind to do this job?  If so, I wonder why `face-set-recalc'
> > doesn't use it, instead of `face-spec-set-2.
> 
> There are many documented functions that set face attributes in
> various forms and for various sets of frames.  My question is
> precisely whether any of them can do this job.

If I thought that any of them could then I would not have used
`face-spec-set-2'.  If you know of one that can do the job (see
above), please enlighten me.

> If not, perhaps we could extend one of them to support
> whatever you need to do.  That'd be an alternative to renaming
> face-spec-set-2 and making it public.

Perhaps.  Please consider letting, for example, `face-spec-set'
take an optional FRAME argument.

That would work for what I wanted to do.

But that would not directly help someone who wants to pass,
not a full face SPEC but the kind of non-spec "SPEC" arg that
`face-spec-set-2' accepts, which is the kind of thing that
`face-spec-choose' returns.  IOW, in a use case where what
you have to start with is not a face spec but a face-attribute
plist.  Not a big deal, but worth mentioning; `face-spec-set-2'
accepts such a plist directly.

In any case: (1) the name of `face-spec-2' is not very
meaningful, and more importantly, the doc is wrong and the
argument name "SPEC" is wrong.  The "SPEC" arg is the kind
of thing that `face-spec-choose' returns: a plist of face
attributes - it is not a face spec.

It's easy to see the difference when you check the definition
of something like `face-spec-match-p' (or other uses of
`face-spec-choose').  Its whole job is to just use
`face-spec-choose' to convert a full face SPEC to a list of
attributes for the given frame, so that it can then call
`face-attr-match-p'.

The doc for a function such as `face-spec-set-2' should, just
like that for `face-attr-match-p', refer to the "SPEC" argument
as ATTRS, not "SPEC", and it should say explicitly that it is
a plist of face attributes and their values.

Similar cleanup is called for in the code and comments of
`face-spec-set-recalc', where `face-spec-set-2' is used: The
variable "SPEC" there should be given another name, and the
comments should talk about it appropriately - it is not a
face spec.

A face-attribute plist is not a face spec.

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Mon, 5 Jun 2017 08:55:06 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 27238@debbugs.gnu.org
> 
> The job to do is shown in the code I gave above.  And it is
> described in the description I gave:
> 
>   "set one face (`fringe' or whatever), in only a given frame,
>                                         ^^^^^^^^^^^^^^^^^^^^^
>   to the spec of another face (or to a spec that isn't yet used
>   for any face)"
> 
> I don't see a "higher-level function" that does that.  I don't
> see any function that does, apart from `face-spec-2'.

Doesn't this boil down to setting the attributes of the target face to
the values of the same attributes of the source face?

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

> > The job to do is shown in the code I gave above.  And it is
> > described in the description I gave:
> >
> >   "set one face (`fringe' or whatever), in only a given frame,
> >                                         ^^^^^^^^^^^^^^^^^^^^^
> >   to the spec of another face (or to a spec that isn't yet used
> >   for any face)"
> >
> > I don't see a "higher-level function" that does that.  I don't
> > see any function that does, apart from `face-spec-2'.
> 
> Doesn't this boil down to setting the attributes of the target face to
> the values of the same attributes of the source face?

Yes, of course, which is why `face-spec-set-2' does that:

(defun face-spec-set-2 (face frame spec)
  "Set the face attributes of FACE on FRAME according to SPEC."
  (let (attrs)
    (while spec
      (when (assq (car spec) face-x-resources)
	(push (car spec) attrs)
	(push (cadr spec) attrs))
      (setq spec (cddr spec)))
    (apply 'set-face-attribute face frame (nreverse attrs))))

Which is also why I said, in my first reply to you:

  Unless you essentially repeat its body, it is the only way
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  to do what it does.

Are suggesting that everyone who needs to "set the attributes
of the target face to the values of the same attributes of the
source face" should just write such an explicit loop (repeat
the body of `face-spec-set-2') instead of just calling
`face-spec-set-2'?

Or are you hinting that there is some other "higher-level
function" that already does exactly that?  And if there is,
why wouldn't `face-spec-set-recalc' use that instead of
`face-spec-set-2'?

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Mon, 5 Jun 2017 09:40:18 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 27238@debbugs.gnu.org
> 
> > >   "set one face (`fringe' or whatever), in only a given frame,
> > >                                         ^^^^^^^^^^^^^^^^^^^^^
> > >   to the spec of another face (or to a spec that isn't yet used
> > >   for any face)"
> > >
> > > I don't see a "higher-level function" that does that.  I don't
> > > see any function that does, apart from `face-spec-2'.
> > 
> > Doesn't this boil down to setting the attributes of the target face to
> > the values of the same attributes of the source face?
> 
> Yes, of course, which is why `face-spec-set-2' does that:

No, it also consults face-x-resources, which is not necessarily what
the caller wants.

> Are suggesting that everyone who needs to "set the attributes
> of the target face to the values of the same attributes of the
> source face" should just write such an explicit loop (repeat
> the body of `face-spec-set-2') instead of just calling
> `face-spec-set-2'?

Are you suggesting that for every loop someone might possibly need at
some point we should have a documented and public function in Emacs?

> Or are you hinting that there is some other "higher-level
> function" that already does exactly that?

Well, there's defface with :inherit, and there's copy-face, to name
just two.  You didn't describe enough context of what you want to do,
so I cannot be sure they are not relevant.

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

> > > >   "set one face (`fringe' or whatever), in only a given frame,
> > > >                                         ^^^^^^^^^^^^^^^^^^^^^
> > > >   to the spec of another face (or to a spec that isn't yet used
> > > >   for any face)"
> > > >
> > > > I don't see a "higher-level function" that does that.  I don't
> > > > see any function that does, apart from `face-spec-2'.
> > >
> > > Doesn't this boil down to setting the attributes of the target face to
> > > the values of the same attributes of the source face?
> >
> > Yes, of course, which is why `face-spec-set-2' does that:
> 
> No, it also consults face-x-resources, which is not necessarily what
> the caller wants.

That's not the point. The point is code to "set the attributes of the
target face to the values of the same attributes of the source face".

Do you see another function that does that?

> > Are suggesting that everyone who needs to "set the attributes
> > of the target face to the values of the same attributes of the
> > source face" should just write such an explicit loop (repeat
> > the body of `face-spec-set-2') instead of just calling
> > `face-spec-set-2'?
> 
> Are you suggesting that for every loop someone might possibly need at
> some point we should have a documented and public function in Emacs?

Do you think I am?  Why?

> > Or are you hinting that there is some other "higher-level
> > function" that already does exactly that?
> 
> Well, there's defface with :inherit, and there's copy-face, to name
> just two.  You didn't describe enough context of what you want to do,
> so I cannot be sure they are not relevant.

Should I explicitly have mentioned that I don't want to inherit another
face?

But I did forget that `copy-face', where the NEW-FACE arg is an
existing face, does what I was looking for.  Thanks for the reminder
about it.  That works for my use case.

(Why doesn't `face-spec-recalc' use `copy-face' instead of
`face-spec-set-2'?  I guess it's to check the return value of
`face-spec-choose', which it goes to the trouble of calling
for each face that it iterates over.)

BTW, while looking to see if Emacs 22 would support my code, since it
will not use `face-spec-set-2', I (re)discovered that prior to Emacs
24.3 `frame-set-spec' DID let you specify the FRAME.  The signature of
`frame-spec-set' was changed incompatibly after Emacs 24.3.

(I still suggest fixing the doc string of `face-spec-set-2', as it is
misleading about its "SPEC" arg.)

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Mon, 5 Jun 2017 10:56:13 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 27238@debbugs.gnu.org
> 
> (I still suggest fixing the doc string of `face-spec-set-2', as it is
> misleading about its "SPEC" arg.)

I'm not sure I understand why it's misleading.  You said:

> Wrt the doc, the SPEC arg is apparently NOT a face spec, i.e., a spec
> such as is returned by (get FACE 'face-defface-spec).  Instead, it seems
> to be an attributes list such as what is returned by `face-spec-choose'
> or `face-attr-construct'.

However, "face spec" is described in '(elisp)Defining Faces' as and
alist whose elements each have the form

          (DISPLAY . PLIST)

and I believe the SPEC argument accepted by face-spec-choose has this
form.  The argument received by face-spec-set-2 is just the PLIST
which matches the frame's display, i.e. a simple list.  So what would
you want the doc string to say about this?

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Drew Adams]

> > (I still suggest fixing the doc string of `face-spec-set-2', as it is
> > misleading about its "SPEC" arg.)
> 
> I'm not sure I understand why it's misleading.  You said:
> 
> > Wrt the doc, the SPEC arg is apparently NOT a face spec, i.e., a spec
> > such as is returned by (get FACE 'face-defface-spec).  Instead, it seems
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > to be an attributes list such as what is returned by `face-spec-choose'
                                             ^^^^^^^^^^^
> > or `face-attr-construct'.
> 
> However, "face spec" is described in '(elisp)Defining Faces' as and
> alist whose elements each have the form
> 
>           (DISPLAY . PLIST)
> 
> and I believe the SPEC argument accepted by face-spec-choose has this
                                  ^^^^^^^^^^^
> form.

Yes.

> The argument received by face-spec-set-2 is just the PLIST
> which matches the frame's display, i.e. a simple list.

Yes.  It is not a face spec.  It is a face-attributes plist.

> So what would you want the doc string to say about this?

The SPEC arg accepted by `face-spec-choose', and by (all of?) the other
face functions whose signatures mention a SPEC arg, is of the same form.

But the "SPEC" accepted by `face-spec-set-2' is not of that form.

Instead, it is of the form that is _returned_ by `face-spec-choose' - it
is a plist of face attributes.  Such a plist is also what is accepted as
the ATTRS argument of `face-attr-match-p'.  It is more properly referred
to as ATTRS or ATTRIBUTES or some such.  It is not a face spec.

This is a face SPEC:

 ((((background dark)) (:background "DarkMagenta"))
  (t (:background "LightGreen" :foreground "Red")))

This is a face-attributes list:

 (:background "LightGreen" :foreground "Red")

I've said the same thing several times now.  HTH.

bug#27238: 24.5; Rename `face-spec-set-2' and give it a better doc string

[Eli Zaretskii]

> Date: Mon, 5 Jun 2017 12:36:54 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 27238@debbugs.gnu.org
> 
> > The argument received by face-spec-set-2 is just the PLIST
> > which matches the frame's display, i.e. a simple list.
> 
> Yes.  It is not a face spec.  It is a face-attributes plist.

OK, I renamed SPEC to FACE-ATTRS where appropriate, and improved the
related doc strings.

Thanks.