bug#51469: 27.2; Mismatch: `set-face-attribute' and `face-all-attributes'

bug#51469: 27.2; Mismatch: `set-face-attribute' and `face-all-attributes'

[Drew Adams]

Why is the &rest argument ARGUMENTS of `set-face-attribute' a plist of
attributes, but `face-all-attributes' returns an alist of attributes?

Just doing something like the following isn't possible directly:

(apply #'set-face-attribute 'some-face
                            nil
                            (face-all-attributes 'other-face))

To accomplish that hand-off, you need to convert the alist returned by
`face-all-attributes' to a plist, and pass that to `set-face-attribute'.
And the conversion function needs to check that the attributes are
acceptable for `set-face-attribute' (else an error is raised).

Why should we need to do that each time, instead of just being able to
pass the result of one to the other?

E.g., something like `foo', applied to (face-all-attributes FACE1
FRAME1), should give a PLIST1 as expected by `set-frame-attribute',
using (apply #'set-face-attribute FACE2 FRAME2 PLIST1):

(defun foo (alist)
  "Return a plist of valid face attributes from ALIST.
ALIST is a list of conses with car a face attribute and with cdr
its value.  The returned PLIST is acceptable as an argument to
`set-face-attribute'."
  (let ((plist  ()))
    (dolist (pair  alist)
      (when (member (car pair) 
		    '(:family :foundry :width :height :weight
			      :slant :foreground :background
			      :underline :color :overline
			      :strike-through :box
			      :inverse-video :stipple :font
			      :inherit))
	(push (car pair) plist)
	(push (cdr pair) plist)))
    (setq plist  (nreverse plist))))

What other uses of these two functions would suggest that they should,
as they do, use different ways to express the list of face attributes?
Is there a good reason for this status quo?  If not, can we change it to
get a better fit and not require conversion?

In GNU Emacs 27.2 (build 1, x86_64-w64-mingw32)
 of 2021-03-26 built on CIRROCUMULUS
Repository revision: deef5efafb70f4b171265b896505b92b6eef24e6
Repository branch: HEAD
Windowing system distributor 'Microsoft Corp.', version 10.0.19042
System Description: Microsoft Windows 10 Pro (v10.0.2009.19042.1288)

bug#51469: 27.2; Mismatch: `set-face-attribute' and `face-all-attributes'

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> Why is the &rest argument ARGUMENTS of `set-face-attribute' a plist of
> attributes, but `face-all-attributes' returns an alist of attributes?

It's not essentially a plist, but a list of :keyword value pairs, but
that's splitting hairs.

face-all-attributes could return such a plist, but we usually prefer to
return alists from functions instead of plists.

> Is there a good reason for this status quo?  If not, can we change it to
> get a better fit and not require conversion?

We can't change the output of face-all-attributes (because that would
break code), so I don't think there's anything to do here, and I'm
closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#51469: [External] : Re: bug#51469: 27.2; Mismatch: `set-face-attribute' and `face-all-attributes'

[Drew Adams]

> face-all-attributes could return such a plist, but we usually prefer to
> return alists from functions instead of plists.
> 
> > Is there a good reason for this status quo?  If not, can we change it
> > to get a better fit and not require conversion?
> 
> We can't change the output of face-all-attributes (because that would
> break code), so I don't think there's anything to do here, and I'm
> closing this bug report.

Do as you like (you already did), but FTR, I disagree.
___

We could certainly allow another optional arg that
specifies that the result be a plist.  All that's
lacking is the will (so far).

If you want a patch for that, and if you'll actually
use it, let me know.

The definition below is what the patch would do.  But I
have a comment and a question first.

A comment is that if you prefer to move the test for
PLIST that's inside the loop outside of it, let me know.

A question is why `face-all-attributes' excludes `:font'
from the list of attributes it returns.

More generally, shouldn't attribute `:font' be included
in the value of defconst `face-attribute-name-alist'?

If it should, then the code would be simpler, as would
use by users (no special value needed for arg PLIST, to
include `:font' for use by `set-face-attribute').

However, that's a "constant", and existing (3rd-party)
code might now depend on `:font' not being included.
So a 2nd part of the question is why we use `defconst'.

If Emacs later adds an attribute (as it did with
`:extend') then the "constant" is effectively broken.
`defconst' is supposed to mean that neither program nor
programmer is expected to change the value.
___

(defun face-all-attributes (face &optional frame plist)
  "Return an alist or plist of the attributes of FACE.
By default, return an alist, with elements of the form
\(ATTR-NAME . ATTR-VALUE).

By default, ATTR-VALUE is the value provided by default for
ATTR-NAME of FACE before it is defined with `defface', which
means it is typically the symbol `unspecified'.  But non-nil
FRAME means ATTR-VALUE is the value of ATTR-NAME for FACE as
it is defined for FRAME.

Non-nil PLIST means return a plist, not an alist:
\( ATTR-NAME-1 ATTR-VALUE-1 ATTR-NAME-2 ATTR-VALUE-2...).

If PLIST is `font-also' then include attribute `:font'.
In that case, the result includes all attributes valid as
arguments to function `set-face-attribute'."
  (let ((names   (if (eq plist 'font-also)
                     (cons (cons :font "font")
                           face-attribute-name-alist)
                   face-attribute-name-alist))
        (result  ())
        att val)
    (while names
      (setq att    (caar names)
            val    (face-attribute face att (or frame  t))
            names  (cdr names))
      (if (not plist)
          (push (cons att val) result)
        (push att result)
        (push val result)))
    (when plist (setq result  (nreverse result)))
    result))

Eli recently made some kind of improvement to the doc
of this function for bug #51465, to clarify what
"default" attribute values mean for this when FRAME
is nil.  Dunno just what change he made, but the doc
I show here can be changed to say whatever he said or
whatever you like.  Let me know the text you prefer,
before I send any patch.