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

[Gregory Heytings <gregory@heytings.org>]

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


>
> I will note right here that Emacs has no way of knowing whether the 
> fonts returned by the font driver are or aren't variable-pitch.  In 
> fact, AFAIR it is a tricky and not very reliable to try deducing that 
> from the font data Emacs records about each font (see 'font-info').  We 
> just blindly trust the font driver to give us the appropriate list of 
> fonts.  IOW, for Emacs the family is just a meaningless string.
>

In a sense the family name is just a meaningless string indeed.  The names 
"Serif", "Sans Serif" and "Monospace" are just a convention.  And indeed, 
Emacs trusts the font driver, why wouldn't it?  And what else could it do? 
The font driver itself calculates the "monospace" or "proportional" 
property based on the actual glyph widths in each font (at least that's 
what Fontconfig does).  When all glyphs have the same width, the font is 
considered monospace and its 'spacing' property is set to 100.  Otherwise 
the font is considered proportional and its 'spacing' property is set to 
0.  And if for some reason Fontconfig does not set that property correctly 
for a font, it can be forced by the user.

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?

>>> why do you consider the family attribute of a face be more important 
>>> than other attributes?  if not all the attributes of a spec are 
>>> "equal" in their importance, which attributes are more important, and 
>>> why?
>>
>> Indeed, the attributes are not equal, in fact none of the attributes 
>> are ever equal in their importance.  The family is the most important 
>> one, followed by the foundry, the registry, the additional style (in 
>> that order, see the loop at the end of font_find_for_lface in which 
>> Emacs tries to make each of these attributes less specific in turn, 
>> starting with the least important one, namely the additional style), 
>> followed by the width, height (or size), weight, slant (in the order 
>> specified by the variable face-font-selection-order).
>
> That is not the relative importance of interest in the context of this 
> discussion, because Emacs already does look for a suitable font in the 
> order of the importance you describe.
>

The problem is precisely that currently it doesn't do that, when faces 
such as variable-pitch are realized.  The font list returned by 
font_list_entities called in font_find_for_lface (called by 
font_load_for_lface, called by realize_gui_face) is erroneously limited to 
fonts which have the exact same width/slant/weight attributes as the 
default face (which is _not_ the font that is being realized).  If the 
default face has for example weight = medium, and Emacs realizes the 
variable-pitch face whose only non-nil attribute (in emacs -Q) is "family 
= Sans Serif", and if there are no fonts in the Sans Serif family with a 
weight equal to medium, font_list_entities returns an empty list.  Which 
is wrong.

>
> 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?  By the way, that's 
what Emacs already does, in most cases (the exception being the current 
bug).  If (in emacs -Q) you

(set-face-attribute 'variable-pitch nil :weight medium)

and if (1) there are no fonts in the Sans Serif family with a medium 
weight on your system but (2) there are fonts in the Monospace family with 
a medium weight, Emacs will not suddenly select a fixed pitch font for the 
variable-pitch face.

>
> And another question: if we are to follow face-font-selection-order, to 
> observe the relative importance of the attributes as set by the user, 
> then why did your patch only consider relaxing the weight (which is in 
> the penultimate place in the order of importance), and not the slant 
> (which is the least important attribute, in the default order we use)?
>

That's not what the last patch I had sent did (see 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59347#164).  In that patch 
all these attributes (weight/slant/width) were set to nil.

However, after spending a few more hours on this, I concluded that the fix 
I proposed in that patch was placed a bit too low in the abstraction 
layers, and that it is safer to place it where it belongs, namely in 
realize_gui_face.  font_find_for_lface has other callers, which may depend 
in subtle ways on its current behavior.  I attach that new, and hopefully 
final, patch.

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.

>> It is also in that loop (at the end of font_find_for_lface) that 
>> face-font-family-alternatives are used.  If the generic "Sans Serif", 
>> "Monospace" and "Monospace Serif" families that Emacs uses are not a 
>> recognized by the font driver (IOW, if font_list_entities returns an 
>> empty result for these families), Emacs falls back to some hard-coded, 
>> less generic, family names.
>
> I'm not sure I agree with this part of your description.  The code looks 
> up face-font-family-alternatives _before_ the loop in 
> font_find_for_lface, i.e., _before_ font_list_entities is called. Where 
> exactly do you see what you describe above?
>

The code in font_find_for_lface just above the final loop that looks up 
face-font-family-alternatives only populates the 'family' array. Nothing 
"concrete" happens in font_find_for_lface before the final loop: the code 
only populates the 'family', 'foundry', 'registry' and 'adstyle' arrays, 
as well as the 'work' font spec.

>>> what are the criteria here and with other similar attributes?
>>
>> The family, foundry, registry and additional style attributes are 
>> passed "as is" to the font driver, which returns a list of fonts 
>> matching these attributes.  The width, weight and/or slant are 
>> converted to numerical values (with font-{width,weight,slant}-table), 
>> and font_score, called by font_sort_entities, called by 
>> font_select_entity, which is applied on the list of fonts returned by 
>> font_list_entities, selects the best match in that list (according the 
>> the preferences in face-font-selection-order). If the width, weight 
>> and/or slant were already passed to font_list_entities, the list of 
>> fonts passed to font_select_entity contains only fonts that match these 
>> width, weight and/or slant, and that mechanism is bypassed.
>
> IOW, you want to disable the filtering of candidate fonts in 
> font_list_entities, and instead consider _all_ the candidates, selecting 
> the best match for the numerical attributes: width, height, weight, and 
> slant.
>

Yes, that's still what I want to do, but in realize_gui_face and not in 
font_find_for_lface anymore.

>
> And you don't want to relax the non-numerical attributes (family, 
> foundry, registry, adstyle) unless there's really no font, of any 
> width/height/weight/slant, installed for the specified 
> family/foundry/registry/adstyle.
>

It is not necessary to change anything there, because the loop at the end 
of font_find_for_lface already relaxes these non-numerical attributes when 
necessary: if there is no font of any width/height/weight/slant for the 
specified family/foundry/registry/adstyle, Emacs tries to set adstyle to 
nil (if it isn't already), then to set registry to nil (if it isn't 
already), then to set foundry to nil (if it isn't already).  And if doing 
that had no effect, Emacs tries the alternative family names listed in 
face-font-family-alternatives.

>
> If that is what you want us to do, then I must ask at least about the 
> height: is it really reasonable to prefer _any_ height from the given 
> family, even if it's radically different from what was requested?
>

The height (or size) attribute already receives a special treatment in 
font_find_for_lface, where it is already set to nil before being passed to 
font_list_entities.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Unset-the-weight-slant-width-in-the-spec-when-realiz.patch --]
[-- Type: text/x-diff; name=Unset-the-weight-slant-width-in-the-spec-when-realiz.patch, Size: 2825 bytes --]

From 47b4f09786f1edff0a65a9c52eb4f62612f3f5e3 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Wed, 7 Dec 2022 23:15:06 +0000
Subject: [PATCH] Unset the weight/slant/width in the spec when realizing a
 font

Between commits bf0d3f76dc (2014) and 6b1ed2f2c9 (2022),
realize_gui_face called font_load_for_lface with an empty or
partly emptied font spec, i.e. it ignored a part of its attrs
argument.  The rationale given in bug#17973, which led to
bf0d3f76dc, is not clear.  However, 6b1ed2f2c9, which passes
the full font spec to font_load_for_lface and
font_find_for_lface, leads to suboptimal font choices, for
example when the font chosen for the default face has a
weight, slant or width that is not supported by other
available fonts on the system, such as 'medium' or 'heavy'.

If these attributes are not unset here, the call to
font_list_entities in font_find_for_lface arbitrarily limits
the candidate font list to those that are perfect matches for
these attributes, which means that the scoring mechanism is
bypassed.  Note that the size attribute in spec is also unset,
in font_find_for_lface.

* src/xfaces.c (realize_gui_face): Unset the weight, slant and
width of the font spec.  Fixes bug#57555 and bug#59347.
---
 src/xfaces.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

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]);
-- 
2.35.1