bug#54564: 29.0.50; [PATCH] Use gsettings font rendering entries for pgtk builds

[Pieter van Prooijen <pieter.van.prooijen@teloden.nl>]

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

Hi All,

Thanks for the review, I've updated the commit message and other
changes you mentioned in the attached patch (against
c5af19cba5924de89a38e7a177c07f42fd3cd543)

I've requested the form for the copyright assignment, but have not
received it yet, will send it in as soon as it arrives.

No progress yet on the problem with subpixel antialiasing, but it
doesn't look like a fontconfig issue, as a non-pgtk build from the same
source works correctly, will have to dig deeper to find out what is
happening. 

Kind Regards,

Pieter

On Wed, 2022-03-30 at 16:59 +0800, Po Lu wrote:
> Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> 
> > Here's the second version of my patch to use the gsettings for font
> > rendering:
> 
> Thanks.
> 
> > When playing around with the font settings, I noticed that
> > cairo/pgtk
> > doesn't seem to use the subpixel antialias (e.g. cleartype style)
> > rendering setting, even though that option is set. I'll investigate
> > further, it doesn't look like a regression due to this patch, but
> > existing behavior.
> 
> It works here for me, so I don't know why you're seeing that
> behavior.
> There could be something wrong with fontconfig on your system.
> 
> Some minor comments below:
> 
> > * src/ftcrfont.c (ftcrfont_open): Use the font_options derived from
> > gsettings
> >   when opening a font.
> > * src/ftcrfont.c: (ftcrfont_cached_font_ok): Report a cached font
> > as invalid
> >   if its font options differ from the current gsettings ones.
> > * src/xsettings.c, src/xsettings.h (xsettings_get_font_options):
> > Retrieve the
> >   current gsettings derived cairo font_options.
> > * src/xsettings.c (apply_gsettings_font_hinting): Convert the
> > gsettings hint
> >   setting to the font_options struct.
> > * src/xsettings.c (apply_gsettings_font_antialias): Convert the
> > gsettings
> >   antialias setting to the font_options struct.
> > * src/xsettings.c (apply_gsettings_font_rgba_order): Convert the
> > gsettings
> >   rgba order setting to the font_options struct.
> > * src/xsettings.c (init_gsettings): Invoke the apply functions when
> >   initializing from gsettings.
> > * src/xsettings.c (something_changed_gsettingsCB): Invoke the apply
> > functions
> >   if the relevant gsettings changed.
> > * src/xsettings.c (store_font_options_changed): Store an event to
> > re-render
> >   the fonts.
> 
> Please don't indent the first asterisk of the filename inside the
> commit
> message, or the other lines of the text, and don't repeat the
> filename
> for each change within the same file.  (You can type C-c C-w inside
> the
> *vc-log* buffer to generate an appropriate commit message.)
> 
> In many cases there is also no need to repeat the same description of
> a change with changes to a single word.
> 
> Here is how I would write the commit message (make sure the entire
> message is no wider than 64 characters):
> 
> 
> * src/ftcrfont.c (ftcrfont_open): Use the font_options derived
> from gsettings when opening a font.
> (ftcrfont_cached_font_ok): Report a cached font as invalid if
> its font options differ from the current options inside
> gsettings.
> 
> * src/xsettings.c (apply_gsettings_font_hinting)
> (apply_gsettings_font_alias, apply_gsettings_font_rgba_order):
> Convert the settings from GSettings to the cairo_font_options_t
> object.
> (init_gsettings, something_changed_gsettingsCB): Invoke the
> apply functions if the relevant settings changed.
> (store_font_options_changed): Store an event to re-render the
> fonts.
> (xsetting_get_font_options)
> * src/xsettings.h (xsettings_get_font_options): New function.
> 
> > +/* Determine if the cached font should be re-opened, by comparing
> > +   its font_options with the ones derived from gsettings.  */
> 
> I would say "determine if FONT_OBJECT is a valid cached font for
> ENTITY by comparing the options used to open it with the user's
> current preferences specified via GSettings".
> 
> > +static void
> > +store_font_options_changed (void)
> > +{
> > +  if (dpyinfo_valid (first_dpyinfo))
> > +    {
> > +      store_config_changed_event (Qfont_render,
> > +                                  XCAR (first_dpyinfo-
> > >name_list_element));
> > +    }
> > +}
> 
> Please remove the extra braces in this if statement.
> 
> > +#ifdef HAVE_PGTK
> > +  else if (strcmp (key, GSETTINGS_FONT_ANTIALIASING) == 0)
> > +    {
> > +      apply_gsettings_font_antialias (settings);
> > +      store_font_options_changed ();
> > +    }
> > +  else if (strcmp (key, GSETTINGS_FONT_HINTING) == 0)
> > +    {
> > +      apply_gsettings_font_hinting (settings);
> > +      store_font_options_changed ();
> > +    }
> > +  else if (strcmp (key, GSETTINGS_FONT_RGBA_ORDER) == 0)
> > +    {
> > +      apply_gsettings_font_rgba_order (settings);
> > +      store_font_options_changed ();
> > +    }
> > +#endif /* HAVE_PGTK */
> >  }
> 
> Once again, please reword these tests as !strcmp (key, ...
> 
> > +  /* Only use the gsettings font entries for non-X11 cairo
> > backends
> > +     (which is the recommended way of running pgtk builds).
> > +     For the X11 backend cairo will apply these entries itself,
> > +     reading them from xrdb.  */
> > +#ifdef HAVE_PGTK
> > +  font_options = cairo_font_options_create ();
> > +  apply_gsettings_font_antialias (gsettings_client);
> > +  apply_gsettings_font_hinting (gsettings_client);
> > +  apply_gsettings_font_rgba_order (gsettings_client);
> > +#endif /* HAVE_PGTK */
> 
> This should say "the Cairo backend running on PGTK".
> 
> > +/* Return the cairo font options, updated from the gsettings font
> > +   config entries. The caller should call
> > cairo_font_options_destroy
> > +   on the result.  */
> 
> This needs two spaces between "entries." and "The".
> 
> > +#ifdef HAVE_PGTK
> > +#include <cairo.h>
> 
> Please move this include to the top of the file.
> 
> Otherwise, LGTM.  Thanks for working on Emacs.  Have you completed
> the
> necessary copyright paperwork?


[-- Attachment #2: 0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch --]
[-- Type: text/x-patch, Size: 9804 bytes --]

From bbf5abb4b2aef399fe2463e3a3943132963310b3 Mon Sep 17 00:00:00 2001
From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
Date: Tue, 29 Mar 2022 21:21:52 +0200
Subject: [PATCH] Use gsettings font rendering entries for pgtk builds

If present, apply the gsettings font hinting and antialiasing
entries when creating a font in cairo.  Do this at
initialization and when the entries change, re-rendering the
frames.

* src/ftcrfont.c (ftcrfont_open): Use the font_options derived
from gsettings when opening a font.
(ftcrfont_cached_font_ok): Report a cached font as invalid if
its font options differ from the current options inside
gsettings.
* src/xsettings.c (apply_gsettings_font_hinting)
(apply_gsettings_font_alias, apply_gsettings_font_rgba_order):
Convert the settings from GSettings to the cairo_font_options_t
object.
(init_gsettings, something_changed_gsettingsCB): Invoke the
apply functions if the relevant settings changed.
(store_font_options_changed): Store an event to re-render the
fonts.
(xsetting_get_font_options)
* src/xsettings.h (xsettings_get_font_options): New function.
---
 src/ftcrfont.c  |  33 +++++++++++
 src/xsettings.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xsettings.h |   5 ++
 3 files changed, 185 insertions(+)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index 98a28af5f2..6bb41110d5 100644
--- a/src/ftcrfont.c
+++ b/src/ftcrfont.c
@@ -37,6 +37,9 @@
 #include "font.h"
 #include "ftfont.h"
 #include "pdumper.h"
+#ifdef HAVE_PGTK
+#include "xsettings.h"
+#endif
 
 #ifdef USE_BE_CAIRO
 #define RED_FROM_ULONG(color)	(((color) >> 16) & 0xff)
@@ -168,7 +171,12 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   cairo_matrix_t font_matrix, ctm;
   cairo_matrix_init_scale (&font_matrix, pixel_size, pixel_size);
   cairo_matrix_init_identity (&ctm);
+
+#ifdef HAVE_PGTK
+  cairo_font_options_t *options = xsettings_get_font_options ();
+#else
   cairo_font_options_t *options = cairo_font_options_create ();
+#endif
 #ifdef USE_BE_CAIRO
   if (be_use_subpixel_antialiasing ())
     cairo_font_options_set_antialias (options, CAIRO_ANTIALIAS_SUBPIXEL);
@@ -624,6 +632,28 @@ ftcrfont_draw (struct glyph_string *s,
   return len;
 }
 
+#ifdef HAVE_PGTK
+/* Determine if FONT_OBJECT is a valid cached font for ENTITY by
+   comparing the options used to open it with the user's current
+   preferences specified via GSettings.  */
+static bool
+ftcrfont_cached_font_ok (struct frame *f, Lisp_Object font_object,
+			 Lisp_Object entity)
+{
+  struct font_info *info = (struct font_info *) XFONT_OBJECT (font_object);
+
+  cairo_font_options_t *options = cairo_font_options_create ();
+  cairo_scaled_font_get_font_options (info->cr_scaled_font, options);
+  cairo_font_options_t *gsettings_options = xsettings_get_font_options ();
+
+  bool equal = cairo_font_options_equal (options, gsettings_options);
+  cairo_font_options_destroy (options);
+  cairo_font_options_destroy (gsettings_options);
+
+  return equal;
+}
+#endif
+
 #ifdef HAVE_HARFBUZZ
 
 static Lisp_Object
@@ -694,6 +724,9 @@ ftcrhbfont_end_hb_font (struct font *font, hb_font_t *hb_font)
 #endif
   .filter_properties = ftfont_filter_properties,
   .combining_capability = ftfont_combining_capability,
+#ifdef HAVE_PGTK
+  .cached_font_ok = ftcrfont_cached_font_ok
+#endif
   };
 #ifdef HAVE_HARFBUZZ
 struct font_driver ftcrhbfont_driver;
diff --git a/src/xsettings.c b/src/xsettings.c
index 71d02e6152..7469eb5e40 100644
--- a/src/xsettings.c
+++ b/src/xsettings.c
@@ -215,11 +215,116 @@ #define GSETTINGS_MONO_FONT  "monospace-font-name"
 #define GSETTINGS_FONT_NAME  "font-name"
 #endif
 
+#ifdef HAVE_PGTK
+#define GSETTINGS_FONT_ANTIALIASING  "font-antialiasing"
+#define GSETTINGS_FONT_RGBA_ORDER    "font-rgba-order"
+#define GSETTINGS_FONT_HINTING       "font-hinting"
+#endif
 
 /* The single GSettings instance, or NULL if not connected to GSettings.  */
 
 static GSettings *gsettings_client;
 
+#ifdef HAVE_PGTK
+
+/* The cairo font_options as obtained using gsettings.  */
+static cairo_font_options_t *font_options;
+
+/* Store an event for re-rendering of the fonts.  */
+static void
+store_font_options_changed (void)
+{
+  if (dpyinfo_valid (first_dpyinfo))
+      store_config_changed_event (Qfont_render,
+                                  XCAR (first_dpyinfo->name_list_element));
+}
+
+/* Apply changes in the hinting system setting.  */
+static void
+apply_gsettings_font_hinting (GSettings *settings)
+{
+  GVariant *val = g_settings_get_value (settings, GSETTINGS_FONT_HINTING);
+  if (val)
+    {
+      g_variant_ref_sink (val);
+      if (g_variant_is_of_type (val, G_VARIANT_TYPE_STRING))
+	{
+	  const char *hinting = g_variant_get_string (val, NULL);
+
+	  if (!strcmp (hinting, "full"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_FULL);
+	  else if (!strcmp (hinting, "medium"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_MEDIUM);
+	  else if (!strcmp (hinting, "slight"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_SLIGHT);
+	  else if (!strcmp (hinting, "none"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_NONE);
+	}
+      g_variant_unref (val);
+    }
+}
+
+/* Apply changes in the antialiasing system setting.  */
+static void
+apply_gsettings_font_antialias (GSettings *settings)
+{
+  GVariant *val = g_settings_get_value (settings, GSETTINGS_FONT_ANTIALIASING);
+  if (val)
+    {
+      g_variant_ref_sink (val);
+      if (g_variant_is_of_type (val, G_VARIANT_TYPE_STRING))
+	{
+	  const char *antialias = g_variant_get_string (val, NULL);
+
+	  if (!strcmp (antialias, "none"))
+	    cairo_font_options_set_antialias (font_options,
+					      CAIRO_ANTIALIAS_NONE);
+	  else if (!strcmp (antialias, "grayscale"))
+	    cairo_font_options_set_antialias (font_options,
+					      CAIRO_ANTIALIAS_GRAY);
+	  else if (!strcmp (antialias, "rgba"))
+	    cairo_font_options_set_antialias (font_options,
+					      CAIRO_ANTIALIAS_SUBPIXEL);
+	}
+      g_variant_unref (val);
+    }
+}
+
+/* Apply the settings for the rgb element ordering.  */
+static void
+apply_gsettings_font_rgba_order (GSettings *settings)
+{
+  GVariant *val = g_settings_get_value (settings,
+					GSETTINGS_FONT_RGBA_ORDER);
+  if (val)
+    {
+      g_variant_ref_sink (val);
+      if (g_variant_is_of_type (val, G_VARIANT_TYPE_STRING))
+	{
+	  const char *rgba_order = g_variant_get_string (val, NULL);
+
+	  if (!strcmp (rgba_order, "rgb"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_RGB);
+	  else if (!strcmp (rgba_order, "bgr"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_BGR);
+	  else if (!strcmp (rgba_order, "vrgb"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_VRGB);
+	  else if (!strcmp (rgba_order, "vbgr"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_VBGR);
+	}
+      g_variant_unref (val);
+    }
+}
+#endif /* HAVE_PGTK */
+
 /* Callback called when something changed in GSettings.  */
 
 static void
@@ -273,6 +378,23 @@ something_changed_gsettingsCB (GSettings *settings,
         }
     }
 #endif /* USE_CAIRO || HAVE_XFT */
+#ifdef HAVE_PGTK
+  else if (!strcmp (key, GSETTINGS_FONT_ANTIALIASING))
+    {
+      apply_gsettings_font_antialias (settings);
+      store_font_options_changed ();
+    }
+  else if (!strcmp (key, GSETTINGS_FONT_HINTING))
+    {
+      apply_gsettings_font_hinting (settings);
+      store_font_options_changed ();
+    }
+  else if (!strcmp (key, GSETTINGS_FONT_RGBA_ORDER))
+    {
+      apply_gsettings_font_rgba_order (settings);
+      store_font_options_changed ();
+    }
+#endif /* HAVE_PGTK */
 }
 
 #endif /* HAVE_GSETTINGS */
@@ -900,6 +1022,16 @@ init_gsettings (void)
         dupstring (&current_font, g_variant_get_string (val, NULL));
       g_variant_unref (val);
     }
+
+  /* Only use the gsettings font entries for the Cairo backend
+     running on PGTK.  */
+#ifdef HAVE_PGTK
+  font_options = cairo_font_options_create ();
+  apply_gsettings_font_antialias (gsettings_client);
+  apply_gsettings_font_hinting (gsettings_client);
+  apply_gsettings_font_rgba_order (gsettings_client);
+#endif /* HAVE_PGTK */
+
 #endif /* USE_CAIRO || HAVE_XFT */
 
 #endif /* HAVE_GSETTINGS */
@@ -1021,6 +1153,17 @@ xsettings_get_system_normal_font (void)
 }
 #endif
 
+#ifdef HAVE_PGTK
+/* Return the cairo font options, updated from the gsettings font
+   config entries.  The caller should call cairo_font_options_destroy
+   on the result.  */
+cairo_font_options_t *
+xsettings_get_font_options (void)
+{
+  return cairo_font_options_copy (font_options);
+}
+#endif
+
 DEFUN ("font-get-system-normal-font", Ffont_get_system_normal_font,
        Sfont_get_system_normal_font,
        0, 0, 0,
@@ -1073,6 +1216,10 @@ syms_of_xsettings (void)
   gconf_client = NULL;
   PDUMPER_IGNORE (gconf_client);
 #endif
+#ifdef HAVE_PGTK
+  font_options = NULL;
+  PDUMPER_IGNORE (font_options);
+#endif
 
   DEFSYM (Qmonospace_font_name, "monospace-font-name");
   DEFSYM (Qfont_name, "font-name");
diff --git a/src/xsettings.h b/src/xsettings.h
index ccaa36489d..5e5df37062 100644
--- a/src/xsettings.h
+++ b/src/xsettings.h
@@ -23,6 +23,8 @@ #define XSETTINGS_H
 #ifndef HAVE_PGTK
 #include "dispextern.h"
 #include <X11/Xlib.h>
+#else
+#include <cairo.h>
 #endif
 
 struct x_display_info;
@@ -41,5 +43,8 @@ #define XSETTINGS_H
 extern const char *xsettings_get_system_normal_font (void);
 #endif
 
+#ifdef HAVE_PGTK
+extern cairo_font_options_t *xsettings_get_font_options (void);
+#endif
 
 #endif /* XSETTINGS_H */
-- 
2.32.0