bug#59347: 29.0.50; `:family` face setting ignored

[Eli Zaretskii <eliz@gnu.org>]

> Date: Wed, 07 Dec 2022 23:19:57 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: monnier@iro.umontreal.ca, 59347@debbugs.gnu.org
> 

Thanks for the detailed response.

I don't want to restart this long discussion from the beginning, so I
will limit myself only to the a few comments, and try to focus on the
patch you suggest.  We don't need to agree about the rest.

> I don't understand what you mean by "try deducing that from the font data 
> Emacs records about each font".  We could double-check that when we want a 
> fixed-pitch font max-width == average-width == space-width, and when we 
> want a variable-pitch font max-width != average-width != space-width, but 
> what would be the benefit of doing that?

(The check you are suggesting above doesn't work reliably, IME.)  My
point was that _if_ we wanted to implement some logic in Emacs that a
family implying variable-pitch fonts must yield only variable-pitch
fonts (and similarly for fixed-pitch fonts), we'd need to have code
for doing that, something that we don't have and AFAIR cannot be
implemented for all the font backends we want to support.

You don't suggest adding such a test, so we don't need to discuss this
tangent.

> > My question was not about this basic relative importance, it was about 
> > something else: when none of the fonts of the given FAMILY fits the font 
> > spec, why do you consider keeping the family to be more important than 
> > keeping the weight?
> 
> I don't understand your question.  If we agree that there is an order of 
> importance in the attributes of a font spec, and that the family is the 
> most important one, it seems clear to me that keeping the family is more 
> important than keeping the weight.  What am I missing?

The order on which we agreed is only about the numerical attributes:
width, height, weight, and slant.  I'm asking about the other
attributes, and about their importance relative to the numerical
ones.  You seem to say that this order is self-evident, and I'm
questioning that.

But we don't need to keep arguing about this tangent, either.

> I checked in particular it with the recipes of bug#37473, bug#57555, 
> bug#59347 and bug#59371, and with some variants.  All seem to work 
> correctly.

What about bug#51768, bug#52493, bug#52888, and the problem reported
in https://lists.gnu.org/archive/html/emacs-devel/2021-12/msg01643.html?

Also, did you test Emacs invocation as in "emacs -fn FONT" where FONT
is specified as Fontconfig and XLFD patterns documented in the "Fonts"
node of the Emacs user manual?  E.g., what happens if you use the
Fontconfig pattern such as
"Sans Serif-12:weight=black:slant=oblique:width=condensed"? -- does
Emacs start with a font with the expected attributes, both when
such a font which matches exactly exists and when an exact match
doesn't exist?

I'm asking this because AFAIR realize_gui_face is called at startup
for the default face of the initial frame, and we need to make sure
your proposed patch works in that case as well, even though it
basically throws away the weight, slant, and width attributes that
were requested.

> diff --git a/src/xfaces.c b/src/xfaces.c
> index df078227c8..71042a3126 100644
> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -6071,8 +6071,24 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
>  	    emacs_abort ();
>  	}
>        if (! FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
> -	attrs[LFACE_FONT_INDEX]
> -	  = font_load_for_lface (f, attrs, attrs[LFACE_FONT_INDEX]);
> +	{
> +	  Lisp_Object spec = copy_font_spec (attrs[LFACE_FONT_INDEX]);
> +	  /* Unset the weight, slant and width in spec.  The best
> +	     possible values for these attributes is determined in
> +	     font_find_for_lface, called by font_load_for_lface, when
> +	     the candidate list returned by font_list_entities is
> +	     sorted by font_select_entity (which calls
> +	     font_sort_entities, which calls font_score).  If these
> +	     attributes are not unset here, the candidate font list
> +	     returned by font_list_entities only contains fonts that
> +	     are exact matches for these weight, slant and width
> +	     attributes, which leads to suboptimal or wrong font
> +	     choices.  See bug#59347.  */
> +	  ASET (spec, FONT_WEIGHT_INDEX, Qnil);
> +	  ASET (spec, FONT_SLANT_INDEX, Qnil);
> +	  ASET (spec, FONT_WIDTH_INDEX, Qnil);
> +	  attrs[LFACE_FONT_INDEX] = font_load_for_lface (f, attrs, spec);
> +	}
>        if (FONT_OBJECT_P (attrs[LFACE_FONT_INDEX]))
>  	{
>  	  face->font = XFONT_OBJECT (attrs[LFACE_FONT_INDEX]);

As I mentioned earlier, any such change must be controlled by a
variable exposed to Lisp, which could then be used to investigate and
perhaps countermand any regressions it could cause.  If you agree to
adding such a variable, I'm okay with installing this on the emacs-29
branch.