Re: [RFC PATCH] Per-window face support

[Eli Zaretskii]

> Date: Thu, 7 Jun 2018 16:42:30 -0700
> From: address@hidden
> 
> This patch lets us actually vary faces on a window-by-window basis. It
> works by allowing face-remapping-alist entries to apply only to windows
> with specific parameters set. A package that wants to apply a face
> remapping to all windows with certain characteristics should use
> face-remapping to add a remapping in all buffers, then set window
> parameters to make that remapping take effect some of the time.

Thanks.  I think you should put this on a branch, so that people could
play with the feature and report any issues they find, before we
decide to merge (which I think will be very soon, given the relative
simplicity of the implementation).

A few minor comments below.

> -  /* The purpose of the following code is to signal an error if FACE
> -     is not a face.  This is for the caller's convenience only; the
> -     redisplay code should be able to fail gracefully.  Skip the check
> -     if FRINGE_FACE_ID is unrealized (as in batch mode and during
> -     daemon startup).  */
> -  if (!NILP (face))
> -    {
> -      struct frame *f = SELECTED_FRAME ();
> -
> -      if (FACE_FROM_ID_OR_NULL (f, FRINGE_FACE_ID)
> -       && lookup_derived_face (f, face, FRINGE_FACE_ID, 1) < 0)
> -     error ("No such face");
> -    }
> -
> +  /* We used to check, as a convenience to callers, for basic face
> +     validity here, but since validity can depend on the specific
> +     _window_ in which this buffer is being displayed, defer the check
> +     to redisplay, which can cope with bad face specifications.  */

How about leaving this check in the code, but only if the face spec
doesn't include a filter?  Is that feasible?

>    /* The default face, possibly remapped. */
> -  default_face = FACE_FROM_ID_OR_NULL (f,
> -                                    lookup_basic_face (f, DEFAULT_FACE_ID));
> +  default_face = FACE_FROM_ID_OR_NULL (
> +    f,
> +    lookup_basic_face (it->w, f, DEFAULT_FACE_ID));

This mostly changes whitespace, and the result is not according to our
style, I think.  Maybe move FACE_FROM_ID_OR_NULL to a new line?

> +/* Determine whether the face filter FILTER evaluated in window W
> +   matches.  W can be NULL if the window context is unknown.
> +
> +   A face filter is either nil, which always matches, or a list
> +   (:window PARAMETER VALUE), which matches if the current window has
> +   a PARAMETER EQ to VALUE.
> +
> +   If the filter is invalid, set *OK to false and, if ERR_MSGS is
> +   true, log an error message.  */
> +static bool
> +evaluate_face_filter (Lisp_Object filter, struct window *w,
> +                      bool *ok, bool err_msgs)

I think the commentary should explicitly document the return value,
even though this is a predicate function with a boolean return type.

More importantly, *OK is not touched if the filter is valid.  If this
is intentional, it should be documented, because it requires the
caller to assign a value before calling this function.

> +static bool
> +evaluate_face_filter (Lisp_Object filter, struct window *w,
> +                      bool *ok, bool err_msgs)
> +{
> +  Lisp_Object orig_filter = filter;
> +
> +  {
> +    if (NILP (filter))
> +      return true;

The inner braces seem to be redundant here?

> +    if (w) {
> +      Lisp_Object found = assq_no_quit (parameter, w->window_parameters);
> +      if (!NILP (found) && EQ (XCDR (found), value))
> +        match = true;
> +    }

The style of braces here is not according to our conventions.

Also, I wonder whether EQ should actually be equal_no_quit, because
the latter would allow parameters with values that are not just
integers or symbols.

> +/* Determine whether FACE_REF is a "filter" face specification (case
> +   #4 in merge_face_ref). If it is, evaluate the filter, and if the
> +   filter matches, return the filtered expression. Otherwise, return
> +   the original expression.

The function can also return nil, but that is not documented.

> +   On error, set *OK to false, having logged an error message if

Same issue as above here with the *OK thing.

> +   ERR_MSGS is true, with return value unspecified.
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If this last part is about returning nil, I'm not sure I see the
reason for being opaque here.  This is an internal function used only
in this one source file; why do we need to hide from potential
programmers its return values in various circumstances?

> +   W is either NULL or a window used to evaluate filters. If W is
> +   null, no window-based face specification filter matches.

Please use 2 spaces between sentences in comments and strings, per our
conventions (here and elsewhere in the patch).

> +static Lisp_Object
> +filter_face_ref (Lisp_Object face_ref,
> +                 struct window *w,
> +                 bool *ok,
> +                 bool err_msgs)
> +{
> +  Lisp_Object orig_face_ref = face_ref;
> +  if (!CONSP (face_ref))
> +    return face_ref;
> +
> +  {

The inner braces are again redundant, I think.

> +   5. nil, which means to merge nothing.

Is this special case needed?  I'm not sure such no-op arguments are a
good idea, as they require to look at the called functions to
understand what's going on on higher levels.  IME it is better to
expect the callers to test for nil explicitly before calling.

>        if (EQ (first, Qforeground_color)
> -       || EQ (first, Qbackground_color))
> +               || EQ (first, Qbackground_color))

Unsolicited whitespace change?

> +   face isn't realized and cannot be realized.  If window W is given,
> +   consider face remappings specified for W or for W's buffer. If W is
> +   NULL, consider only frame-level face configuration.  */

Two spaces between sentences, please.

> +  DEFVAR_BOOL ("face-filters-always-match", face_filters_always_match,
> +               doc: /* Non-nil means that face filters are always deemed to
> +match. Use only when evaluating face attributes.  */);

The first line of the doc string should be 79 characters at most, so I
suggest to move the second sentence to a new line.

Also, by "use only..." do you mean this should be let-bound or
something?  IOW, I think the doc string should be somewhat more
explicit regarding the restrictions of this variable's use.

Finally, this will need documentation updates in lispref and in NEWS.

Thanks again for working on this.