Re: [PATCH] Re: About the :distant-foreground face attribute

[Daniel Colascione]

On 01/13/2014 08:33 AM, Jan Djärv wrote:
> 13 jan 2014 kl. 14:13 skrev Daniel Colascione <address@hidden>:
>
> > The attached patch might be another solution to the problem. It replaces 
> > :distant-foreground with :contrast-function, which punts the actual contrast 
> > logic to lisp by calling the named function during face realization. 
> > (Performance isn't a problem in practice because we cache face realizations.) 
> > In lisp, we implement four low-contrast-mitigation policies: do not adjust for 
> > contrast, adjust automatically (by adjusting CIE L*A*B color space L values), 
> > adjust automatically (by adjusting the V values in HSV color space), or just 
> > set the foreground to a specific color if the contrast dips below a certain 
> > point (the current :distant-foreground behavior). Both the policy and the 
> > parameters (well, the override color) are customizable on a per-face basis; 
> > when merging faces, the one with the highest priority sets the whole behavior.
>
> The main complaint (as I see it) was that a new attribute was added.
So does this.

No. The main complaint is that we're adding a brittle face attribute 
with a very confusing name, not that just that we're adding an 
attribute. This patch allows for flexible policy and treats face 
foreground and background colors identically.

> > The patch uses the CIE L*A*B colorspace algorithm by default.
>
> Do not change the defaults please. Reinstate the
> *_selection_fg_color.
They are system defined and should be honored.

There are two sane defaults: the 24.3 behavior, where we always use the 
system selection foreground and background, and my proposed behavior, 
where we use the fontified foreground and automatically adjust it so 
that it's legible. The current behavior is worse because it uses the 
system selection foreground only sometimes and doesn't preserve theme 
hues when possible.

> > It produces surprisingly good results, at least in my tests, adapting 
> > automatically to light and dark backgrounds while preserving the hues of theme 
> > foreground colors.
> >
> > (Changing themes nukes the face property right now, so you'll have to reset it 
> > each time.)
> > <adaptive-face.patch>
>
>
> @@ -1070,7 +1070,8 @@
>       (:foreground . "foreground color")
>       (:background . "background color")
>       (:stipple . "background stipple")
> -    (:inherit . "inheritance"))
> +    (:inherit . "inheritance")
> +    (:contrast-function "contrast function"))
>     "An alist of descriptive names for face attributes.
>   Each element has the form (ATTRIBUTE-NAME . DESCRIPTION) where
>   ATTRIBUTE-NAME is a face attribute name (a keyword symbol), and
> @@ -1351,7 +1352,6 @@
>
>
> There is a . missing after :contrast-function.

Thanks.

> Have you checked that no event handling code accesses a face
> fore/background?  You can not call a lisp function in that context.

We only run this code when we're realizing faces, not when we're just 
accessing their attributes. We call lisp from merge_face_heights in the 
same context, so this use should be safe as well.