bug#19117: 25.0.50; emacs on x11 chooses different fonts for the same face sometimes

[Dima Kogan <dima@secretsauce.net>]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Dima Kogan <dima@secretsauce.net>
>> Date: Tue, 30 Dec 2014 10:33:17 -0800
>> Cc: 19117@debbugs.gnu.org
>> 
>> I will patch xfont_list_pattern() to accept a 3rd type of
>> scalable-fonts-allowed: 'fallback where we add these fonts only if no
>> others are available, and in realize_basic_faces() I'll use THAT value.
>> Does that sound reasonable?
>
> Please note that on MS-Windows the scalable fonts were always used in
> this scenario, back with that code, and there was a comment explaining
> that otherwise too few fonts will be available on Windows.

Hi. I'm attaching patches. The first one does this:

  scalable font logic now matches the docs and is more consistent

  A nil value of scalable-fonts-allowed allows scalable fonts if no others
  were found.  This is the previously-documented behavior.

  realize_basic_faces() no longer forces scalable-fonts-allowed to t.  If
  nil, some fonts will always be found even if only scalable fonts are
  available, as set by the other part of this patch.

This is simpler than what I proposed earlier, since it does not
introduce a third setting to scalable-fonts-allowed: the default nil
value is sufficient. The patch mimics the older behavior to loop through
the whole font list a second time if no non-scalable fonts were found
and some scalable fonts WERE found, but were skipped the first time
around due to scalable-fonts-allowed. This fixes my bug. The patch
touches many lines, but almost all of it is indentation.


The second patch is mostly unrelated. It removes some redundant code.

I don't believe the behavior on Windows should be affected at all, but I
have not verified this.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-scalable-font-logic-now-matches-the-docs-and-is-more.patch --]
[-- Type: text/x-diff, Size: 8924 bytes --]

From af0ec4452c5157e526af3c1190c3e8ba6f72c982 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 30 Dec 2014 20:05:11 -0800
Subject: [PATCH 1/2] scalable font logic now matches the docs and is more
 consistent

A nil value of scalable-fonts-allowed allows scalable fonts if no others
were found.  This is the previously-documented behavior.

realize_basic_faces() no longer forces scalable-fonts-allowed to t.  If
nil, some fonts will always be found even if only scalable fonts are
available, as set by the other part of this patch.
---
 src/xfaces.c |   3 --
 src/xfont.c  | 172 +++++++++++++++++++++++++++++++++--------------------------
 2 files changed, 95 insertions(+), 80 deletions(-)

diff --git a/src/xfaces.c b/src/xfaces.c
index 446107e..0aa9968 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5228,12 +5228,10 @@ static bool
 realize_basic_faces (struct frame *f)
 {
   bool success_p = 0;
-  ptrdiff_t count = SPECPDL_INDEX ();
 
   /* Block input here so that we won't be surprised by an X expose
      event, for instance, without having the faces set up.  */
   block_input ();
-  specbind (Qscalable_fonts_allowed, Qt);
 
   if (realize_default_face (f))
     {
@@ -5267,7 +5265,6 @@ realize_basic_faces (struct frame *f)
       success_p = 1;
     }
 
-  unbind_to (count, Qnil);
   unblock_input ();
   return success_p;
 }
diff --git a/src/xfont.c b/src/xfont.c
index 06a4479..c6a3f2b 100644
--- a/src/xfont.c
+++ b/src/xfont.c
@@ -388,94 +388,112 @@ xfont_list_pattern (Display *display, const char *pattern,
       Lisp_Object *props = XVECTOR (xfont_scratch_props)->contents;
       Lisp_Object scripts = Qnil, entity = Qnil;
 
+      /* We take two passes over the font list.  The second pass is
+         taken only if scalable-fonts-allowed is nil, and only
+         scalable fonts were found.
+       */
+      int i_pass;
+      bool skipped_some_scalable_fonts = false;
+
       for (i = 0; i < ASIZE (xfont_scratch_props); i++)
 	ASET (xfont_scratch_props, i, Qnil);
       for (i = 0; i < num_fonts; i++)
 	indices[i] = names[i];
       qsort (indices, num_fonts, sizeof (char *), compare_font_names);
 
-      for (i = 0; i < num_fonts; i++)
-	{
-	  ptrdiff_t len;
+      for (i_pass = 0; i_pass < 2; i_pass++)
+        {
+          for (i = 0; i < num_fonts; i++)
+            {
+              ptrdiff_t len;
 
-	  if (i > 0 && xstrcasecmp (indices[i - 1], indices[i]) == 0)
-	    continue;
-	  if (NILP (entity))
-	    entity = font_make_entity ();
-	  len = xfont_decode_coding_xlfd (indices[i], -1, buf);
-	  if (font_parse_xlfd (buf, len, entity) < 0)
-	    continue;
-	  ASET (entity, FONT_TYPE_INDEX, Qx);
-	  /* Avoid auto-scaled fonts.  */
-	  if (INTEGERP (AREF (entity, FONT_DPI_INDEX))
-	      && INTEGERP (AREF (entity, FONT_AVGWIDTH_INDEX))
-	      && XINT (AREF (entity, FONT_DPI_INDEX)) != 0
-	      && XINT (AREF (entity, FONT_AVGWIDTH_INDEX)) == 0)
-	    continue;
-	  /* Avoid not-allowed scalable fonts.  */
-	  if (NILP (Vscalable_fonts_allowed))
-	    {
-	      int size = 0;
+              if (i > 0 && xstrcasecmp (indices[i - 1], indices[i]) == 0)
+                continue;
+              if (NILP (entity))
+                entity = font_make_entity ();
+              len = xfont_decode_coding_xlfd (indices[i], -1, buf);
+              if (font_parse_xlfd (buf, len, entity) < 0)
+                continue;
+              ASET (entity, FONT_TYPE_INDEX, Qx);
+              /* Avoid auto-scaled fonts.  */
+              if (INTEGERP (AREF (entity, FONT_DPI_INDEX))
+                  && INTEGERP (AREF (entity, FONT_AVGWIDTH_INDEX))
+                  && XINT (AREF (entity, FONT_DPI_INDEX)) != 0
+                  && XINT (AREF (entity, FONT_AVGWIDTH_INDEX)) == 0)
+                continue;
+              /* Avoid not-allowed scalable fonts.  */
+              if (NILP (Vscalable_fonts_allowed))
+                {
+                  int size = 0;
 
-	      if (INTEGERP (AREF (entity, FONT_SIZE_INDEX)))
-		size = XINT (AREF (entity, FONT_SIZE_INDEX));
-	      else if (FLOATP (AREF (entity, FONT_SIZE_INDEX)))
-		size = XFLOAT_DATA (AREF (entity, FONT_SIZE_INDEX));
-	      if (size == 0)
-		continue;
-	    }
-	  else if (CONSP (Vscalable_fonts_allowed))
-	    {
-	      Lisp_Object tail, elt;
+                  if (INTEGERP (AREF (entity, FONT_SIZE_INDEX)))
+                    size = XINT (AREF (entity, FONT_SIZE_INDEX));
+                  else if (FLOATP (AREF (entity, FONT_SIZE_INDEX)))
+                    size = XFLOAT_DATA (AREF (entity, FONT_SIZE_INDEX));
+                  if (size == 0 && i_pass == 0)
+                    {
+                      skipped_some_scalable_fonts = true;
+                      continue;
+                    }
+                }
+              else if (CONSP (Vscalable_fonts_allowed))
+                {
+                  Lisp_Object tail, elt;
 
-	      for (tail = Vscalable_fonts_allowed; CONSP (tail);
-		   tail = XCDR (tail))
-		{
-		  elt = XCAR (tail);
-		  if (STRINGP (elt)
-		      && fast_c_string_match_ignore_case (elt, indices[i],
-							  len) >= 0)
-		    break;
-		}
-	      if (! CONSP (tail))
-		continue;
-	    }
+                  for (tail = Vscalable_fonts_allowed; CONSP (tail);
+                       tail = XCDR (tail))
+                    {
+                      elt = XCAR (tail);
+                      if (STRINGP (elt)
+                          && fast_c_string_match_ignore_case (elt, indices[i],
+                                                              len) >= 0)
+                        break;
+                    }
+                  if (! CONSP (tail))
+                    continue;
+                }
 
-	  /* Avoid fonts of invalid registry.  */
-	  if (NILP (AREF (entity, FONT_REGISTRY_INDEX)))
-	    continue;
+              /* Avoid fonts of invalid registry.  */
+              if (NILP (AREF (entity, FONT_REGISTRY_INDEX)))
+                continue;
 
-	  /* Update encoding and repertory if necessary.  */
-	  if (! EQ (registry, AREF (entity, FONT_REGISTRY_INDEX)))
-	    {
-	      registry = AREF (entity, FONT_REGISTRY_INDEX);
-	      if (font_registry_charsets (registry, &encoding, &repertory) < 0)
-		encoding = NULL;
-	    }
-	  if (! encoding)
-	    /* Unknown REGISTRY, not supported.  */
-	    continue;
-	  if (repertory)
-	    {
-	      if (NILP (script)
-		  || xfont_chars_supported (chars, NULL, encoding, repertory))
-		list = Fcons (entity, list), entity = Qnil;
-	      continue;
-	    }
-	  if (memcmp (props, aref_addr (entity, FONT_FOUNDRY_INDEX),
-		      word_size * 7)
-	      || ! EQ (AREF (entity, FONT_SPACING_INDEX), props[7]))
-	    {
-	      vcopy (xfont_scratch_props, 0,
-		     aref_addr (entity, FONT_FOUNDRY_INDEX), 7);
-	      ASET (xfont_scratch_props, 7, AREF (entity, FONT_SPACING_INDEX));
-	      scripts = xfont_supported_scripts (display, indices[i],
-						 xfont_scratch_props, encoding);
-	    }
-	  if (NILP (script)
-	      || ! NILP (Fmemq (script, scripts)))
-	    list = Fcons (entity, list), entity = Qnil;
-	}
+              /* Update encoding and repertory if necessary.  */
+              if (! EQ (registry, AREF (entity, FONT_REGISTRY_INDEX)))
+                {
+                  registry = AREF (entity, FONT_REGISTRY_INDEX);
+                  if (font_registry_charsets (registry, &encoding, &repertory) < 0)
+                    encoding = NULL;
+                }
+              if (! encoding)
+                /* Unknown REGISTRY, not supported.  */
+                continue;
+              if (repertory)
+                {
+                  if (NILP (script)
+                      || xfont_chars_supported (chars, NULL, encoding, repertory))
+                    list = Fcons (entity, list), entity = Qnil;
+                  continue;
+                }
+              if (memcmp (props, aref_addr (entity, FONT_FOUNDRY_INDEX),
+                          word_size * 7)
+                  || ! EQ (AREF (entity, FONT_SPACING_INDEX), props[7]))
+                {
+                  vcopy (xfont_scratch_props, 0,
+                         aref_addr (entity, FONT_FOUNDRY_INDEX), 7);
+                  ASET (xfont_scratch_props, 7, AREF (entity, FONT_SPACING_INDEX));
+                  scripts = xfont_supported_scripts (display, indices[i],
+                                                     xfont_scratch_props, encoding);
+                }
+              if (NILP (script)
+                  || ! NILP (Fmemq (script, scripts)))
+                list = Fcons (entity, list), entity = Qnil;
+            }
+
+          /* We skip the second pass unless we really need it.  */
+          if (!(NILP (list)                      /* No fonts found on the first pass */
+                && skipped_some_scalable_fonts)) /* and we skipped some scalable ones.  */
+            break;
+        }
       XFreeFontNames (names);
     }
 
-- 
2.1.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-removed-unused-variable-assignment.patch --]
[-- Type: text/x-diff, Size: 642 bytes --]

From 282bee4cfe6775063358c616e966593501620b54 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 30 Dec 2014 20:05:58 -0800
Subject: [PATCH 2/2] removed unused variable assignment

---
 src/font.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/font.c b/src/font.c
index 70e6316..dc821dd 100644
--- a/src/font.c
+++ b/src/font.c
@@ -2160,7 +2160,6 @@ font_score (Lisp_Object entity, Lisp_Object *spec_prop)
       }
 
   /* Score the size.  Maximum difference is 127.  */
-  i = FONT_SIZE_INDEX;
   if (! NILP (spec_prop[FONT_SIZE_INDEX])
       && XINT (AREF (entity, FONT_SIZE_INDEX)) > 0)
     {
-- 
2.1.3