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

[Gregory Heytings <gregory@heytings.org>]

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


Thanks for your detailed feedback.

>> 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.
>

Okay, now I see what you mean.  It is not self-evident indeed, it is the 
intended behavior that is visible (or at least that I see) in the existing 
code, and it seems to me that it is the most natural behavior, because 
changing the font itself, e.g. from DejaVu to Courier, has more effect / 
creates more visual diversity than only changing the 
width/height/weight/slant attributes, and in graphical user interfaces 
more uniformity is better than more diversity.

>> 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?
>

bug#51768: Works as expected here, with '(default ((t (:family "Source 
Code Pro" :foundry "outline" :slant normal :weight medium :height 90 
:width normal)))).

bug#52493: Works as expected here, the Inconsolata_dz font is used and 
rendered correctly.  But the original recipe doesn't work anymore (it does 
not work in Emacs 28 either), it should be :font "Inconsolata_dz" instead 
of :family "Inconsolata_dz".

bug#52888: That bug seems unrelated to the current bug, but I see no 
difference in behavior between Emacs 28 and Emacs 29 with the patch.

problem reported on emacs-devel: The medium weight is selected both for 
frames that are created by calling emacs and for frames that are created 
by calling emacsclient.

>
> 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?
>

emacs -Q -fn 'Sans Serif-12:slant=oblique:width=condensed'

works as expected and behaves like Emacs 28: the font for the default face 
is a condensed and oblique variable-pitch sans serif font.

emacs-28 -Q -fn 'Sans Serif-12:weight=black:slant=oblique:width=condensed'

displays the same error in Emacs 28, 29, and 29 with the patch:

Font 'Sans Serif-12:weight=black:slant=oblique:width=condensed' is not 
defined

>
> 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.
>

Okay, so here is an updated patch.  To make it easier to investigate bugs 
in this area, I think it makes sense to control each field separately, and 
also to allow unsetting other attributes, which is what the new variable 
does.

[-- 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: 4715 bytes --]

From 60cae79ac38a2860579532bf8450d8d81688f380 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 8 Dec 2022 12:45: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.

Also allow unsetting the other attributes, for debugging purposes.

* src/xfaces.c (realize_gui_face): Unset the weight, slant and
width of the font spec.  Fixes bug#57555 and bug#59347.
(syms_of_xfaces): New variable
'realize-gui-face-ignored-spec-attributes'.
---
 src/xfaces.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/src/xfaces.c b/src/xfaces.c
index df078227c8..c335193999 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -6071,8 +6071,42 @@ 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]);
+#define   MAYBE_UNSET_ATTRIBUTE(ATTR)			\
+	  if (realize_gui_face_ignored_spec_attributes	\
+	      & (1 << FONT_##ATTR##_INDEX))		\
+	    ASET (spec, FONT_##ATTR##_INDEX, Qnil);
+	  /* The default value of
+	     realize_gui_face_ignored_spec_attributes unsets 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.  */
+	  MAYBE_UNSET_ATTRIBUTE (WEIGHT);
+	  MAYBE_UNSET_ATTRIBUTE (SLANT);
+	  MAYBE_UNSET_ATTRIBUTE (WIDTH);
+	  /* Also allow unsetting other attributes for debugging
+	     purposes.  */
+	  MAYBE_UNSET_ATTRIBUTE (FAMILY);
+	  MAYBE_UNSET_ATTRIBUTE (FOUNDRY);
+	  MAYBE_UNSET_ATTRIBUTE (REGISTRY);
+	  MAYBE_UNSET_ATTRIBUTE (ADSTYLE);
+	  MAYBE_UNSET_ATTRIBUTE (SIZE);
+	  MAYBE_UNSET_ATTRIBUTE (DPI);
+	  MAYBE_UNSET_ATTRIBUTE (SPACING);
+	  MAYBE_UNSET_ATTRIBUTE (AVGWIDTH);
+	  MAYBE_UNSET_ATTRIBUTE (EXTRA);
+#undef    MAYBE_UNSET_ATTRIBUTE
+	  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]);
@@ -7360,6 +7394,23 @@ syms_of_xfaces (void)
 clear the face cache, see `clear-face-cache'.  */);
   face_near_same_color_threshold = 30000;
 
+  DEFVAR_INT ("realize-gui-face-ignored-spec-attributes",
+	      realize_gui_face_ignored_spec_attributes,
+	      doc: /* Ignored font-spec attributes in realize_gui_face.
+
+The value is an integer number and represents a bit mask.
+The attribute corresponding to each bit that is set is cleared in
+realize_gui_face.  The bits are: 1 = :foundry, 2 = :family,
+3 = :adstyle, 4 = :registry, 5 = :weight, 6 = :slant, 7 = :width,
+8 = :size, 9 = :dpi, 10 = :spacing, 11 = :avgwidth, 12 = extra
+attributes (:name, :script, :lang and :otf).
+
+There is no reason to change that value except for debugging purposes.  */);
+  realize_gui_face_ignored_spec_attributes =
+    (1 << FONT_WEIGHT_INDEX) |
+    (1 << FONT_SLANT_INDEX) |
+    (1 << FONT_WIDTH_INDEX);
+
 #ifdef HAVE_WINDOW_SYSTEM
   defsubr (&Sbitmap_spec_p);
   defsubr (&Sx_list_fonts);
-- 
2.35.1