bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Dave Goel]

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

   (Thanks for an excellent discussion with Eli (-devel) and Davis, which
ended in Eli suggesting a bug-report.)

You have constructed an awesome new face, f1, for your mode which inherits
from 20 other ancestors, and sets attributes just right.

You now want to make up a new face f2, which would inherit-from f1, except
you would like to clear the value of :height, so that emacs would render it
the same as the user's default at run-time. That  seems like a plausible,
practical, use-case to me.  (And, I was trying to do just that in some code
recently.)

Imagine a face f3 that's just like f1, except that the final (chased) value
of :height is 'unspecified. Can we make f2 behave like f3?


How do you clear :height in f2?

Setting nil doesn't do it. For :height, nil is an invalid value.
Practically, if you set it nil, it behaves like 'unspecified for this
attribute. Nil is often a valid value for attributes like :slant. Nil is
not same as 'unspecified. And, even if it was, see below.

Setting 'unspecified doesn't do it. Emacs will now look up the value from
f2's parent, f1.

You could set f2's height to be the same as default's height at
define-time, but that is not the same as leaving it 'unspecified. Indeed,
the user could have changed the defaults by run-time, and f2 fails to
adhere to that. In summary, f2 != f3.

Finally, you could chase every attribute from f1, and then define f2 based
on that, and leave :height  'unspecified. In other words, set f2=f3 (after
a lot of work). But, even that is unsatsfactory and loses the purpose of
inheritance in multiple ways. (a) That's a lot of work. The whole point of
inheritance was to save all that work. (b) More importantly, we  now we
lose the run-time values of f1. If f1 has been customized, the
customization no longer carries over to f2. The purpose of inheriting f2
from f1 was to carry over most attributes automatically and continuously
even after the user costumizes f1.

It seems that there's no way to clear an attribute, and it would be nice to
allow that.

Could we allow a 'reset? Where the effect of 'reset is to stop all chasing,
and immediately render the final value as 'unspecified (so that emacs looks
up the final value from 'default during rendering)

----
If we are feeling generous, we could also allow an additional 'clear,
slightly different from 'reset, as follows:

If point has five faces (+default) active (f1 f2 f3 f4 f5, default), then
'reset found at, say, f2 behaves as above, sending us straight to 'default.

But, 'clear in f2 merely stops chasing f2's own ancestors. We next go to f3
in this case.

[-- Attachment #2: Type: text/html, Size: 3375 bytes --]

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Lars Ingebrigtsen]

Dave Goel <deego3@gmail.com> writes:

> Could we allow a 'reset? Where the effect of 'reset is to stop all
> chasing, and immediately render the final value as 'unspecified (so
> that emacs looks up the final value from 'default during rendering)

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

Makes sense to me.  I had a brief peek at xfaces.c, and decided not to
take a stab at implementing this at this time, but if somebody else is
interested, here's the test case:

---
(defface foo '((t (:height 200 :bold t))) "")
(defface child '((t :inherit foo :height unspecified)) "")

(progn
  (pop-to-buffer "*foo*")
  (erase-buffer)
  (insert "none" (propertize "foo" 'face 'foo)
	  (propertize "child" 'face 'child)))
---

So we want to be able to say :height reset there and get a bold "child"
string in the default height.

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

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Eli Zaretskii]

> Cc: 38771@debbugs.gnu.org
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 23 May 2022 12:46:15 +0200
> 
> (defface foo '((t (:height 200 :bold t))) "")
> (defface child '((t :inherit foo :height unspecified)) "")
> 
> (progn
>   (pop-to-buffer "*foo*")
>   (erase-buffer)
>   (insert "none" (propertize "foo" 'face 'foo)
> 	  (propertize "child" 'face 'child)))
> ---
> 
> So we want to be able to say :height reset there and get a bold "child"
> string in the default height.

"reset" just means "use the value of the default face at face
realization time".  Nothing else will work, because you cannot "undo"
inherited attributes.  Thus, even if nil is valid, it won't do.

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> "reset" just means "use the value of the default face at face
> realization time".  Nothing else will work, because you cannot "undo"
> inherited attributes.  Thus, even if nil is valid, it won't do.

Yes.

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

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: deego3@gmail.com,  38771@debbugs.gnu.org
> Date: Mon, 23 May 2022 13:58:58 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > "reset" just means "use the value of the default face at face
> > realization time".  Nothing else will work, because you cannot "undo"
> > inherited attributes.  Thus, even if nil is valid, it won't do.
> 
> Yes.

I've now implemented this on master.  The recipe below now works as
expected:

  (defface foo '((t (:height 200 :bold t))) "")
  (defface child '((t :inherit foo :height reset)) "")

  (progn
    (pop-to-buffer "*foo*")
    (erase-buffer)
    (insert "none" (propertize "foo" 'face 'foo)
	    (propertize "child" 'face 'child)))

I hope other uses of this pseudo-value will also work, but I couldn't
test all the possible ways of merging faces: there are too many of
them.

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I've now implemented this on master. 

Great!

I'm getting the following compilation warning (gcc (Ubuntu
11.2.0-19ubuntu1)) now:

In file included from xfaces.c:226:
xfaces.c: In function ‘Finternal_set_lisp_face_attribute’:
lisp.h:337:26: warning: ‘tmp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  337 | #  define lisp_h_XLI(o) ((EMACS_INT) (o))
      |                          ^
xfaces.c:3539:23: note: ‘tmp’ was declared here
 3539 |           Lisp_Object tmp;
      |                       ^~~

(I haven't checked whether it's a valid warning or not.)

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

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: deego3@gmail.com,  38771@debbugs.gnu.org
> Date: Sun, 03 Jul 2022 11:57:25 +0200
> 
> In file included from xfaces.c:226:
> xfaces.c: In function ‘Finternal_set_lisp_face_attribute’:
> lisp.h:337:26: warning: ‘tmp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   337 | #  define lisp_h_XLI(o) ((EMACS_INT) (o))
>       |                          ^
> xfaces.c:3539:23: note: ‘tmp’ was declared here
>  3539 |           Lisp_Object tmp;
>       |                       ^~~

Oops.  Should be fixed now.

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> xfaces.c:3539:23: note: ‘tmp’ was declared here
>>  3539 |           Lisp_Object tmp;
>>       |                       ^~~
>
> Oops.  Should be fixed now.

Thanks; the warning is gone now.

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

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Dave Goel]

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

Lars and Eli,

Nice! Thanks a lot!
Dave


>

[-- Attachment #2: Type: text/html, Size: 689 bytes --]

bug#38771: Allow face-attribute to be 'reset, (in addition the usual 'unspecified and valid-values)

[Lars Ingebrigtsen]

It seems like this was fixed a month ago, and there was no followup, so
I'm assuming Eli's fixes worked, and I'm closing this bug report.  (If
this is wrong, please respond to the debbugs address and we'll reopen.)