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

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

[Pieter van Prooijen]

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

Hello, 

When using the pgtk build of emacs under Wayland/Ubuntu, I noticed that
the font hinting was different from the regular X (Ubuntu supplied)
version of emacs.

I have the gsettings config for font hinting set to "full" on my
system, using the fonts tab in the gnome-tweaks tool.

It looks like Cairo defaults to the "slight" hinting setting when using
the Wayland backend for display and will only use the gsettings config
when rendering to an X backend, when it reads the current font settings
from xrdb.

I've attached a patch (against
380f0443b288c68df3762ee20d78719f08dd92ff) which applies the font
entries from gsettings (if present) when creating a font in Cairo.

Note that this patch won't dynamically re-render when emacs is already
running (like it does when changing the system font).
I made an attempt to hook into the gsettings change callback, but could
not force a re-creation of the font with the changed parameters,for
instance using a 'font-render' config changed event. Any pointers on
how to achieve this?
 
Do you think if this patch is a good approach to get the pgtk build to
use the system preferences for font hinting and anti-aliasing?

Kind Regards,

Pieter van Prooijen
    

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

From 6fe888e7c14ef5aad124c0d68fcd071cd5b65e81 Mon Sep 17 00:00:00 2001
From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
Date: Fri, 25 Mar 2022 11:47:39 +0100
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.
* src/xsettings.c, src/xsettings.h: read and apply gsetting entries.
* src/ftcrfont.c: use the font_options from xsettings.c when creating a font.
---
 src/ftcrfont.c  |   6 ++-
 src/xsettings.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xsettings.h |   5 +-
 3 files changed, 127 insertions(+), 2 deletions(-)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index 98a28af5f2..b659024524 100644
--- a/src/ftcrfont.c
+++ b/src/ftcrfont.c
@@ -37,6 +37,7 @@
 #include "font.h"
 #include "ftfont.h"
 #include "pdumper.h"
+#include "xsettings.h"
 
 #ifdef USE_BE_CAIRO
 #define RED_FROM_ULONG(color)	(((color) >> 16) & 0xff)
@@ -168,10 +169,13 @@ 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);
-  cairo_font_options_t *options = cairo_font_options_create ();
+
 #ifdef USE_BE_CAIRO
+  cairo_font_options_t *options = cairo_font_options_create();
   if (be_use_subpixel_antialiasing ())
     cairo_font_options_set_antialias (options, CAIRO_ANTIALIAS_SUBPIXEL);
+#else
+  cairo_font_options_t *options = xsettings_get_font_options();
 #endif
   cairo_scaled_font_t *scaled_font
     = cairo_scaled_font_create (font_face, &font_matrix, &ctm, options);
diff --git a/src/xsettings.c b/src/xsettings.c
index 71d02e6152..c72d59e99b 100644
--- a/src/xsettings.c
+++ b/src/xsettings.c
@@ -57,6 +57,7 @@ Copyright (C) 2009-2022 Free Software Foundation, Inc.
 #if defined USE_CAIRO || defined HAVE_XFT
 #ifdef USE_CAIRO
 #include <fontconfig/fontconfig.h>
+#include <cairo.h>
 #else  /* HAVE_XFT */
 #include <X11/Xft/Xft.h>
 #endif
@@ -67,6 +68,10 @@ Copyright (C) 2009-2022 Free Software Foundation, Inc.
 static Display_Info *first_dpyinfo;
 static Lisp_Object current_tool_bar_style;
 
+#ifdef USE_CAIRO
+static cairo_font_options_t *font_options;
+#endif
+
 /* Store a config changed event in to the event queue.  */
 
 static void
@@ -215,11 +220,96 @@ #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;
 
+/* For hinting and antialias no dynamic re-display is (yet) possible,
+   because cairo applies these options when creating the font, not when
+   rendering it.
+*/
+
+#ifdef HAVE_PGTK
+/* 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") == 0)
+	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_FULL);
+	  else if (strcmp (hinting, "medium") == 0)
+	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_MEDIUM);
+	  else if (strcmp (hinting, "slight") == 0)
+	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_SLIGHT);
+	  else if (strcmp (hinting, "none") == 0)
+	    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") == 0)
+	    cairo_font_options_set_antialias(font_options, CAIRO_ANTIALIAS_NONE);
+	  else if (strcmp(antialias, "grayscale") == 0)
+	    cairo_font_options_set_antialias(font_options, CAIRO_ANTIALIAS_GRAY);
+	  else if (strcmp(antialias, "rgba") == 0)
+	    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") == 0)
+	    cairo_font_options_set_subpixel_order(font_options, CAIRO_SUBPIXEL_ORDER_RGB);
+	  else if (strcmp (rgba_order, "bgr") == 0)
+	    cairo_font_options_set_subpixel_order(font_options, CAIRO_SUBPIXEL_ORDER_BGR);
+	  else if (strcmp (rgba_order, "vrgb") == 0)
+	    cairo_font_options_set_subpixel_order(font_options, CAIRO_SUBPIXEL_ORDER_VRGB);
+	  else if (strcmp (rgba_order, "vbgr") == 0)
+	    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
@@ -900,6 +990,18 @@ init_gsettings (void)
         dupstring (&current_font, g_variant_get_string (val, NULL));
       g_variant_unref (val);
     }
+
+  /* Only use the gsettings font entries for non-X cairo backends
+     (which is the recommended way of running pgtk builds).
+     For the X 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 */
+
 #endif /* USE_CAIRO || HAVE_XFT */
 
 #endif /* HAVE_GSETTINGS */
@@ -1021,6 +1123,18 @@ xsettings_get_system_normal_font (void)
 }
 #endif
 
+#ifdef USE_CAIRO
+/* Return the cairo font options, possibly initialized in
+   init_gsettings() */
+cairo_font_options_t *
+xsettings_get_font_options (void) {
+  if (font_options == NULL)
+    return cairo_font_options_create();
+  else
+    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 +1187,10 @@ syms_of_xsettings (void)
   gconf_client = NULL;
   PDUMPER_IGNORE (gconf_client);
 #endif
+#ifdef USE_CAIRO
+  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..15984e8865 100644
--- a/src/xsettings.h
+++ b/src/xsettings.h
@@ -40,6 +40,9 @@ #define XSETTINGS_H
 #ifdef USE_LUCID
 extern const char *xsettings_get_system_normal_font (void);
 #endif
-
+#ifdef USE_CAIRO
+struct cairo_font_options_t;
+extern cairo_font_options_t *xsettings_get_font_options (void);
+#endif
 
 #endif /* XSETTINGS_H */
-- 
2.32.0

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

> When using the pgtk build of emacs under Wayland/Ubuntu, I noticed that
> the font hinting was different from the regular X (Ubuntu supplied)
> version of emacs.
>
> I have the gsettings config for font hinting set to "full" on my
> system, using the fonts tab in the gnome-tweaks tool.
>
> It looks like Cairo defaults to the "slight" hinting setting when using
> the Wayland backend for display and will only use the gsettings config
> when rendering to an X backend, when it reads the current font settings
> from xrdb.
>
> I've attached a patch (against
> 380f0443b288c68df3762ee20d78719f08dd92ff) which applies the font
> entries from gsettings (if present) when creating a font in Cairo.
>
> Note that this patch won't dynamically re-render when emacs is already
> running (like it does when changing the system font).  I made an
> attempt to hook into the gsettings change callback, but could not
> force a re-creation of the font with the changed parameters,for
> instance using a 'font-render' config changed event. Any pointers on
> how to achieve this?

I'm not sure, but perhaps someone else around here knows.

> Do you think if this patch is a good approach to get the pgtk build to
> use the system preferences for font hinting and anti-aliasing?

The basic approach is good, thanks for working on this.  However, I have
some comments on your code below.

> Kind Regards,
>
> Pieter van Prooijen
>
> From 6fe888e7c14ef5aad124c0d68fcd071cd5b65e81 Mon Sep 17 00:00:00 2001
> From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
> Date: Fri, 25 Mar 2022 11:47:39 +0100
> 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.
> * src/xsettings.c, src/xsettings.h: read and apply gsetting entries.
> * src/ftcrfont.c: use the font_options from xsettings.c when creating a font.

Please capitalize the first character of each line in the changelog
message after the files.  Also, try to write an entry for each function
that was changed, instead of lumping all the changes together in one
file.

> +#include "xsettings.h"

This must be conditional on HAVE_X_WINDOWS || HAVE_PGTK.

> -  cairo_font_options_t *options = cairo_font_options_create ();
> +
>  #ifdef USE_BE_CAIRO
> +  cairo_font_options_t *options = cairo_font_options_create();
>    if (be_use_subpixel_antialiasing ())
>      cairo_font_options_set_antialias (options, CAIRO_ANTIALIAS_SUBPIXEL);
> +#else
> +  cairo_font_options_t *options = xsettings_get_font_options();
>  #endif

This else statement should be conditional on HAVE_PGTK, correct?  The
Cairo font backend currently supports 3 platforms: X Windows, where
Cairo will detect the font options by itself, Haiku (where it is
configured manually), and PGTK.

> +#ifdef USE_CAIRO
> +static cairo_font_options_t *font_options;
> +#endif

This is conditional on USE_CAIRO, but most of the code below is
conditional on HAVE_PGTK.  Is that really correct?

> +/* For hinting and antialias no dynamic re-display is (yet) possible,
> +   because cairo applies these options when creating the font, not when
> +   rendering it.
> +*/

This is not how we write comments.  Please keep the closing part on the
same line, with only two spaces separating it from the last line, like
this:

/* Changes in hinting and antialiasing preferences cannot currently be
   applied to existing fonts, since the Cairo font backend does not
   support changing font options after a font object is created.  */

> +#ifdef HAVE_PGTK
> +/* 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") == 0)
> +	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_FULL);
> +	  else if (strcmp (hinting, "medium") == 0)
> +	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_MEDIUM);
> +	  else if (strcmp (hinting, "slight") == 0)
> +	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_SLIGHT);
> +	  else if (strcmp (hinting, "none") == 0)
> +	    cairo_font_options_set_hint_style(font_options, CAIRO_HINT_STYLE_NONE);
> +	}
> +      g_variant_unref (val);
> +    }
> +}

This will need some stylistic changes as well: we put a space between
the function name and its parameter list, like such:

static void
apply_gsettings_font_hinting (GSettings *settings)

And inside function calls as well:

cairo_font_options_set_hint_style (font_options, CAIRO_HINT_STYLE_FULL);

Also, the idomatic style is to write !strcmp (hinting, "full") instead
of saying strcmp (hinting, "full") == 0.

> +/* Apply changes in the antialiasing system setting */

Please also put a period after the sentences in each comment, followed
by two spaces.

> +#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 */

Similar stylistic fixes are also needed to the function calls here.

> +#ifdef USE_CAIRO
> +/* Return the cairo font options, possibly initialized in
> +   init_gsettings() */
> +cairo_font_options_t *
> +xsettings_get_font_options (void) {
> +  if (font_options == NULL)
> +    return cairo_font_options_create();
> +  else
> +    return cairo_font_options_copy(font_options);
> +}
> +#endif

Please don't say "init_gsettings()" inside a comment to mean that
`init_gsettings' is a function.  That makes it look like a function call
with no arguments.  I would write the comment like this:

/* Return the cairo font options based on the user's font preferences,
   possibly initialized in `init_gsettings'.  */

Our style is also to put the opening brace of a function definition on a
new line, so that tools which look for defuns by looking for opening
parens in column 0 can operate correctly.

This should probably be conditional on HAVE_PGTK instead of USE_CAIRO as
well.

> +#ifdef USE_CAIRO
> +  font_options = NULL;
> +  PDUMPER_IGNORE (font_options);
> +#endif

Likewise.

> +#ifdef USE_CAIRO
> +struct cairo_font_options_t;
> +extern cairo_font_options_t *xsettings_get_font_options (void);
> +#endif

This too.  Also, just include `cairo.h' instead of declaring `struct
cairo_font_options_t'.

Thanks again for working on Emacs.

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

[Eli Zaretskii]

> Cc: 54564@debbugs.gnu.org
> Date: Sat, 26 Mar 2022 09:16:03 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > Note that this patch won't dynamically re-render when emacs is already
> > running (like it does when changing the system font).  I made an
> > attempt to hook into the gsettings change callback, but could not
> > force a re-creation of the font with the changed parameters,for
> > instance using a 'font-render' config changed event. Any pointers on
> > how to achieve this?
> 
> I'm not sure, but perhaps someone else around here knows.

Maybe I could help if I understood the difficulty well enough.  What
exactly is the problem here?  In particular, what is meant by "force a
re-creation of the font with the changed parameters"?  How can Emacs
"re-create" a font?

> The basic approach is good, thanks for working on this.

AFAIU, this uses gsettings to determine some Emacs font-related
functionality.  One aspect that bothers me is whether users will have
the means to tell Emacs to ignore those gsettings and use the usual
Emacs defaults instead?  I don't think it's a good idea to apply those
gsettings unconditionally without letting users override that.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe I could help if I understood the difficulty well enough.  What
> exactly is the problem here?  In particular, what is meant by "force a
> re-creation of the font with the changed parameters"?  How can Emacs
> "re-create" a font?

Basically, Emacs needs to close every open font object created by the
ftcr (or ftcrhb) font driver, and open it again, if that makes any
sense.

> AFAIU, this uses gsettings to determine some Emacs font-related
> functionality.  One aspect that bothers me is whether users will have
> the means to tell Emacs to ignore those gsettings and use the usual
> Emacs defaults instead?  I don't think it's a good idea to apply those
> gsettings unconditionally without letting users override that.

I think all the gsettings-related behavior is controlled by
`font-use-system-font', but if it's not, we could always make it work
that way, or add a new variable.

Thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: pieter.van.prooijen@teloden.nl,  54564@debbugs.gnu.org
> Date: Sat, 26 Mar 2022 14:07:21 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Maybe I could help if I understood the difficulty well enough.  What
> > exactly is the problem here?  In particular, what is meant by "force a
> > re-creation of the font with the changed parameters"?  How can Emacs
> > "re-create" a font?
> 
> Basically, Emacs needs to close every open font object created by the
> ftcr (or ftcrhb) font driver, and open it again, if that makes any
> sense.

Does the below fit the bill?

   clear_face_cache (true);

Or did you mean to do this only on a single frame (or on specific
selected frames)?  Then looking inside clear_face_cache will tell you
how to do that.

And one more thing: care should be taken if this is done in response
to some async notification, because clearing all the faces will need a
thorough redisplay.  Most probably all the affected frames need to be
marked as "garbaged".

> > AFAIU, this uses gsettings to determine some Emacs font-related
> > functionality.  One aspect that bothers me is whether users will have
> > the means to tell Emacs to ignore those gsettings and use the usual
> > Emacs defaults instead?  I don't think it's a good idea to apply those
> > gsettings unconditionally without letting users override that.
> 
> I think all the gsettings-related behavior is controlled by
> `font-use-system-font'

I don't see any references to that variable in the patch you are
discussing.  And its name and doc string don't seem to give any clue
that it's relevant to this issue.  Is this only for fixed-pitch fonts?

> but if it's not, we could always make it work that way, or add a new
> variable.

Yes, I think so.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Does the below fit the bill?
>
>    clear_face_cache (true);
>
> Or did you mean to do this only on a single frame (or on specific
> selected frames)?  Then looking inside clear_face_cache will tell you
> how to do that.

It should be done for each frame on every display, so clear_face_cache
probably does fit the bill.  (Though perhaps clear_font_cache on each
window system frame by itself will be enough?)

> And one more thing: care should be taken if this is done in response
> to some async notification, because clearing all the faces will need a
> thorough redisplay.  Most probably all the affected frames need to be
> marked as "garbaged".

Thanks.  Pieter, do you need any help in implementing the feature along
those lines?

> I don't see any references to that variable in the patch you are
> discussing.  And its name and doc string don't seem to give any clue
> that it's relevant to this issue.  Is this only for fixed-pitch fonts?

No, this is supposed to work for all fonts.

> Yes, I think so.

Sure, thanks.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: pieter.van.prooijen@teloden.nl,  54564@debbugs.gnu.org
> Date: Sat, 26 Mar 2022 14:44:53 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does the below fit the bill?
> >
> >    clear_face_cache (true);
> >
> > Or did you mean to do this only on a single frame (or on specific
> > selected frames)?  Then looking inside clear_face_cache will tell you
> > how to do that.
> 
> It should be done for each frame on every display, so clear_face_cache
> probably does fit the bill.  (Though perhaps clear_font_cache on each
> window system frame by itself will be enough?)

You cannot clear the font cache of a frame without also clearing all
the faces on that frame.  Fonts are face attributes in Emacs, so faces
have references to fonts.

> > I don't see any references to that variable in the patch you are
> > discussing.  And its name and doc string don't seem to give any clue
> > that it's relevant to this issue.  Is this only for fixed-pitch fonts?
> 
> No, this is supposed to work for all fonts.

Then I Think the existing variable is not the solution, as it
explicitly talks about fixed-pitch fonts.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> You cannot clear the font cache of a frame without also clearing all
> the faces on that frame.  Fonts are face attributes in Emacs, so faces
> have references to fonts.

Thanks, I see.

> Then I Think the existing variable is not the solution, as it
> explicitly talks about fixed-pitch fonts.

That's reasonable.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: pieter.van.prooijen@teloden.nl,  54564@debbugs.gnu.org
> Date: Sat, 26 Mar 2022 14:44:53 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does the below fit the bill?
> >
> >    clear_face_cache (true);
> >
> > Or did you mean to do this only on a single frame (or on specific
> > selected frames)?  Then looking inside clear_face_cache will tell you
> > how to do that.
> 
> It should be done for each frame on every display, so clear_face_cache
> probably does fit the bill.  (Though perhaps clear_font_cache on each
> window system frame by itself will be enough?)

Actually, I see that clear_face_cache doesn't force clearing of the
font cache on every frame, only those frames which have "enough" fonts
defined.  So I guess an explicit call to clear_font_cache for every
frame, followed by free_all_realized_faces, will be needed.

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

[Pieter van Prooijen]

Hello All, 

Thanks for looking into my patch!, I'll fix the formatting issues you
mentioned and narrow the changes to the HAVE_PGTK define in the various
source files, so it's is only present for that build.

Not sure about having this functionality controlled by a user defined
variable (as is possible with using the system font), as far as I know,
the current cairo X backend doesn't have this as well, it will always
use the gsettings/xrdb defined parameters for rendering.

Regarding the re-rendering upon a config change, I'll have a look at
the options mentioned (clearing the font cache and marking each frame
for redisplay), but that will probably be a separate patch.

Kind Regards,

Pieter

On Sat, 2022-03-26 at 14:44 +0800, Po Lu wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does the below fit the bill?
> > 
> >    clear_face_cache (true);
> > 
> > Or did you mean to do this only on a single frame (or on specific
> > selected frames)?  Then looking inside clear_face_cache will tell
> > you
> > how to do that.
> 
> It should be done for each frame on every display, so
> clear_face_cache
> probably does fit the bill.  (Though perhaps clear_font_cache on each
> window system frame by itself will be enough?)
> 
> > And one more thing: care should be taken if this is done in
> > response
> > to some async notification, because clearing all the faces will
> > need a
> > thorough redisplay.  Most probably all the affected frames need to
> > be
> > marked as "garbaged".
> 
> Thanks.  Pieter, do you need any help in implementing the feature
> along
> those lines?
> 
> > I don't see any references to that variable in the patch you are
> > discussing.  And its name and doc string don't seem to give any
> > clue
> > that it's relevant to this issue.  Is this only for fixed-pitch
> > fonts?
> 
> No, this is supposed to work for all fonts.
> 
> > Yes, I think so.
> 
> Sure, thanks.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

> Hello All, 
>
> Thanks for looking into my patch!, I'll fix the formatting issues you
> mentioned and narrow the changes to the HAVE_PGTK define in the various
> source files, so it's is only present for that build.

Indeed, and thanks for working on Emacs as well.

> Not sure about having this functionality controlled by a user defined
> variable (as is possible with using the system font), as far as I know,
> the current cairo X backend doesn't have this as well, it will always
> use the gsettings/xrdb defined parameters for rendering.

We don't support using the PGTK build on X Windows anyway, so that's not
a real problem.  Besides, manually specified font options will override
whatever Cairo thinks is best.

> Regarding the re-rendering upon a config change, I'll have a look at
> the options mentioned (clearing the font cache and marking each frame
> for redisplay), but that will probably be a separate patch.

I think it would be best to have that feature as part of this change.

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

[Pieter van Prooijen]

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

Hello All,

Here's the second version of my patch to use the gsettings for font
rendering:

- Every change is scoped to the pgtk build. 
- Changing settings redisplay dynamically by hooking into the
cached_font_ok field of the cairo font driver and storing a font-render
event upon a gsettings change.
- Formatting is now in line with the gnu c style.
- Commit message was split into multiple parts per changed file.

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.

Kind Regards,

Pieter

On Sun, 2022-03-27 at 08:59 +0800, Po Lu wrote:
> Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> 
> > Hello All, 
> > 
> > Thanks for looking into my patch!, I'll fix the formatting issues
> > you
> > mentioned and narrow the changes to the HAVE_PGTK define in the
> > various
> > source files, so it's is only present for that build.
> 
> Indeed, and thanks for working on Emacs as well.
> 
> > Not sure about having this functionality controlled by a user
> > defined
> > variable (as is possible with using the system font), as far as I
> > know,
> > the current cairo X backend doesn't have this as well, it will
> > always
> > use the gsettings/xrdb defined parameters for rendering.
> 
> We don't support using the PGTK build on X Windows anyway, so that's
> not
> a real problem.  Besides, manually specified font options will
> override
> whatever Cairo thinks is best.
> 
> > Regarding the re-rendering upon a config change, I'll have a look
> > at
> > the options mentioned (clearing the font cache and marking each
> > frame
> > for redisplay), but that will probably be a separate patch.
> 
> I think it would be best to have that feature as part of this change.


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

From bc0aa23777f2aae42b29597a04c0d14007063149 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.
* 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.
---
 src/ftcrfont.c  |  32 ++++++++++
 src/xsettings.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xsettings.h |   4 ++
 3 files changed, 187 insertions(+)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index 98a28af5f2..103da93d51 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,27 @@ ftcrfont_draw (struct glyph_string *s,
   return len;
 }
 
+#ifdef HAVE_PGTK
+/* Determine if the cached font should be re-opened, by comparing
+   its font_options with the ones derived from 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 +723,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..2538ab51b3 100644
--- a/src/xsettings.c
+++ b/src/xsettings.c
@@ -215,11 +215,118 @@ #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 +380,23 @@ something_changed_gsettingsCB (GSettings *settings,
         }
     }
 #endif /* USE_CAIRO || HAVE_XFT */
+#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 */
 }
 
 #endif /* HAVE_GSETTINGS */
@@ -900,6 +1024,18 @@ init_gsettings (void)
         dupstring (&current_font, g_variant_get_string (val, NULL));
       g_variant_unref (val);
     }
+
+  /* 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 */
+
 #endif /* USE_CAIRO || HAVE_XFT */
 
 #endif /* HAVE_GSETTINGS */
@@ -1021,6 +1157,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 +1220,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..c7fe861c40 100644
--- a/src/xsettings.h
+++ b/src/xsettings.h
@@ -41,5 +41,9 @@ #define XSETTINGS_H
 extern const char *xsettings_get_system_normal_font (void);
 #endif
 
+#ifdef HAVE_PGTK
+#include <cairo.h>
+extern cairo_font_options_t *xsettings_get_font_options (void);
+#endif
 
 #endif /* XSETTINGS_H */
-- 
2.32.0

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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?

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

[Pieter van Prooijen]

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

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

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

Thanks.

>> > +   on the result.  */

Hmm... Is there an actual non-breaking space character here, or is your
MUA munging the messages?

> +  if (dpyinfo_valid (first_dpyinfo))
> +      store_config_changed_event (Qfont_render,
> +                                  XCAR (first_dpyinfo->name_list_element));

You got the indentation wrong here.  The "s" in
"store_config_changed_event" should be two spaces behind where it is
now.

Otherwise, LGTM.  Hopefully the copyright paperwork can be finished soon.

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

[Pieter van Prooijen]

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

Hi All, 

It took a fair bit of digging, but sub-pixel anti-aliasing on pgtk now
seems to work correctly.

The problem lies with the "source" operator set by pgtk when drawing a
font, this will take a particular execution path in the cairo glyph
compositor (using a mask image) which converts the colors created by
freetype for the sub pixel effect to greys-cale. Using the default
"over" operator takes a different path which preserves the colors. Note
that on small screens the difference between sub-pixel and grey-scale
rendering is hard to see, it's more pronounced on larger displays)

I've amended my gsettings patch with this change, only replacing the
operator for anti-aliased fonts in ftcrfont_draw, because I'm not sure
about its impact for non-anti-aliased fonts.    

I've also fixed the indent error, but couldn't find the non-breaking
space in the patch, it's perhaps caused by the mail program.

Regarding the copyright assignment, I've had no response after sending
in the questionnaire at
https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future
to assign@gnu.org

Do you get sent a paper form of some kind which has to be filled in?
(I'm not at my home address atm so I will have missed that)

Kind Regards,

Pieter


On Fri, 2022-04-01 at 10:00 +0800, Po Lu wrote:
> Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> 
> > 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. 
> 
> Thanks.
> 
> > > > +   on the result.  */
> 
> Hmm... Is there an actual non-breaking space character here, or is your
> MUA munging the messages?
> 
> > +  if (dpyinfo_valid (first_dpyinfo))
> > +      store_config_changed_event (Qfont_render,
> > +                                  XCAR (first_dpyinfo-
> > >name_list_element));
> 
> You got the indentation wrong here.  The "s" in
> "store_config_changed_event" should be two spaces behind where it is
> now.
> 
> Otherwise, LGTM.  Hopefully the copyright paperwork can be finished
> soon.


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

From b9d96a47c4b301e0445a6d7b80cfb8876c921c05 Mon Sep 17 00:00:00 2001
From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
Date: Thu, 7 Apr 2022 18:52:14 +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.
(ftcrfont_draw): Use a different cairo operator when
drawing subpixel antialiased fonts.
* 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  |  43 ++++++++++++++
 src/xsettings.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xsettings.h |   5 ++
 3 files changed, 195 insertions(+)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index 98a28af5f2..e6067d25c7 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);
@@ -599,6 +607,16 @@ ftcrfont_draw (struct glyph_string *s,
   x_set_cr_source_with_gc_foreground (f, s->gc, false);
 #else
   pgtk_set_cr_source_with_color (f, s->xgcv.foreground, false);
+
+  /* The CAIRO_OPERATOR_SOURCE operator set by pgtk_set_cr_source_with_color
+     above renders sub-pixel anti-aliased fonts in grey-scale, use the
+     default CAIRO_OPERATOR_OVER instead.  */
+  cairo_font_options_t *options = cairo_font_options_create ();
+  cairo_scaled_font_get_font_options (ftcrfont_info->cr_scaled_font,
+				      options);
+  if (cairo_font_options_get_antialias (options) == CAIRO_ANTIALIAS_SUBPIXEL)
+    cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
+  cairo_font_options_destroy (options);
 #endif
 #else
   uint32_t col = be_foreground;
@@ -624,6 +642,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 +734,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..e71887e03d 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

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

> Hi All, 
>
> It took a fair bit of digging, but sub-pixel anti-aliasing on pgtk now
> seems to work correctly.
>
> The problem lies with the "source" operator set by pgtk when drawing a
> font, this will take a particular execution path in the cairo glyph
> compositor (using a mask image) which converts the colors created by
> freetype for the sub pixel effect to greys-cale. Using the default
> "over" operator takes a different path which preserves the colors. Note
> that on small screens the difference between sub-pixel and grey-scale
> rendering is hard to see, it's more pronounced on larger displays)

Ah thanks.  Does this fix the problem as well?

diff --git a/src/pgtkterm.c b/src/pgtkterm.c
index b2816aa04a..5fbc56ae81 100644
--- a/src/pgtkterm.c
+++ b/src/pgtkterm.c
@@ -7037,8 +7037,11 @@ pgtk_set_cr_source_with_color (struct frame *f, unsigned long color,
   pgtk_query_color (f, &col);
 
   if (!respects_alpha_background)
-    cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
-			  col.green / 65535.0, col.blue / 65535.0);
+    {
+      cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
+			    col.green / 65535.0, col.blue / 65535.0);
+      cairo_set_operator (FRAME_CR_CONTEXT (f), CAIRO_OPERATOR_OVER);
+    }
   else
     {
       cairo_set_source_rgba (FRAME_CR_CONTEXT (f), col.red / 65535.0,

> I've also fixed the indent error, but couldn't find the non-breaking
> space in the patch, it's perhaps caused by the mail program.

It was previously filled with non-breaking spaces, but the patch you
attached looks fine now.

> Regarding the copyright assignment, I've had no response after sending
> in the questionnaire at
> https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future
> to assign@gnu.org

It takes a while; if you don't get a response in 2 weeks, you should
contact Eli or Lars, and they will ask the folks at the FSF to expedite
things.

> Do you get sent a paper form of some kind which has to be filled in?
> (I'm not at my home address atm so I will have missed that)

You get sent a document you have to print and fill in, I think, but that
depends on the country you're in.

Thanks.

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

[Pieter van Prooijen]

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

Hi,

Yes, your patch fixes the problem as well, and I don't see any
other side effects at a first glance. I've changed my patch to
incorporate it and revert the changes to ftcrfont_draw. 

Thanks,

Pieter


On Fri, 2022-04-08 at 07:38 +0800, Po Lu wrote:
> Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> 
> > Hi All, 
> > 
> > It took a fair bit of digging, but sub-pixel anti-aliasing on pgtk
> > now
> > seems to work correctly.
> > 
> > The problem lies with the "source" operator set by pgtk when
> > drawing a
> > font, this will take a particular execution path in the cairo glyph
> > compositor (using a mask image) which converts the colors created
> > by
> > freetype for the sub pixel effect to greys-cale. Using the default
> > "over" operator takes a different path which preserves the colors.
> > Note
> > that on small screens the difference between sub-pixel and grey-
> > scale
> > rendering is hard to see, it's more pronounced on larger displays)
> 
> Ah thanks.  Does this fix the problem as well?
> 
> diff --git a/src/pgtkterm.c b/src/pgtkterm.c
> index b2816aa04a..5fbc56ae81 100644
> --- a/src/pgtkterm.c
> +++ b/src/pgtkterm.c
> @@ -7037,8 +7037,11 @@ pgtk_set_cr_source_with_color (struct frame
> *f, unsigned long color,
>    pgtk_query_color (f, &col);
>  
>    if (!respects_alpha_background)
> -    cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
> -                         col.green / 65535.0, col.blue / 65535.0);
> +    {
> +      cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
> +                           col.green / 65535.0, col.blue / 65535.0);
> +      cairo_set_operator (FRAME_CR_CONTEXT (f),
> CAIRO_OPERATOR_OVER);
> +    }
>    else
>      {
>        cairo_set_source_rgba (FRAME_CR_CONTEXT (f), col.red /
> 65535.0,
> 
> > I've also fixed the indent error, but couldn't find the non-
> > breaking
> > space in the patch, it's perhaps caused by the mail program.
> 
> It was previously filled with non-breaking spaces, but the patch you
> attached looks fine now.
> 
> > Regarding the copyright assignment, I've had no response after
> > sending
> > in the questionnaire at
> > https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/re
> > quest-assign.future
> > to assign@gnu.org
> 
> It takes a while; if you don't get a response in 2 weeks, you should
> contact Eli or Lars, and they will ask the folks at the FSF to
> expedite
> things.
> 
> > Do you get sent a paper form of some kind which has to be filled
> > in?
> > (I'm not at my home address atm so I will have missed that)
> 
> You get sent a document you have to print and fill in, I think, but
> that
> depends on the country you're in.
> 
> Thanks.


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

From 7cb458944e825281df59a6dd360e0e40ff312d26 Mon Sep 17 00:00:00 2001
From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
Date: Thu, 7 Apr 2022 18:52:14 +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/pgtkterm.c (pgtk_set_cr_source_with_color): Use the
default CAIRO_OPERATOR_OVER for non-respects_alpha_background
calls to preserve the color information added by sub-pixel
anti-aliased rendered fonts.
---
 src/ftcrfont.c  |  33 +++++++++++
 src/pgtkterm.c  |   5 +-
 src/xsettings.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xsettings.h |   5 ++
 4 files changed, 189 insertions(+), 1 deletion(-)

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/pgtkterm.c b/src/pgtkterm.c
index b2816aa04a..9bc2ddfd79 100644
--- a/src/pgtkterm.c
+++ b/src/pgtkterm.c
@@ -7037,8 +7037,11 @@ pgtk_set_cr_source_with_color (struct frame *f, unsigned long color,
   pgtk_query_color (f, &col);
 
   if (!respects_alpha_background)
-    cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
+    {
+      cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
 			  col.green / 65535.0, col.blue / 65535.0);
+      cairo_set_operator (FRAME_CR_CONTEXT (f), CAIRO_OPERATOR_OVER);
+    }
   else
     {
       cairo_set_source_rgba (FRAME_CR_CONTEXT (f), col.red / 65535.0,
diff --git a/src/xsettings.c b/src/xsettings.c
index 71d02e6152..e71887e03d 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

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

> Hi,
>
> Yes, your patch fixes the problem as well, and I don't see any
> other side effects at a first glance. I've changed my patch to
> incorporate it and revert the changes to ftcrfont_draw. 

You don't have to incorporate that, I will install it now since it fixes
another unrelated problem.  Thanks.

BTW, the non-breaking spaces are back again.

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

[Pieter van Prooijen]

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

Hello All,

The copyright assignment paperwork came through, is it possible to
incorporate the gsetting font rendering changes? I've attached a patch
against the current master, let me know if I need to change anything.

Kind Regards,

Pieter



On Fri, 2022-04-08 at 20:44 +0200, Pieter van Prooijen wrote:
> Hi,
> 
> Yes, your patch fixes the problem as well, and I don't see any
> other side effects at a first glance. I've changed my patch to
> incorporate it and revert the changes to ftcrfont_draw. 
> 
> Thanks,
> 
> Pieter
> 
> 
> On Fri, 2022-04-08 at 07:38 +0800, Po Lu wrote:
> > Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> > 
> > > Hi All, 
> > > 
> > > It took a fair bit of digging, but sub-pixel anti-aliasing on
> > > pgtk
> > > now
> > > seems to work correctly.
> > > 
> > > The problem lies with the "source" operator set by pgtk when
> > > drawing a
> > > font, this will take a particular execution path in the cairo
> > > glyph
> > > compositor (using a mask image) which converts the colors created
> > > by
> > > freetype for the sub pixel effect to greys-cale. Using the
> > > default
> > > "over" operator takes a different path which preserves the
> > > colors.
> > > Note
> > > that on small screens the difference between sub-pixel and grey-
> > > scale
> > > rendering is hard to see, it's more pronounced on larger
> > > displays)
> > 
> > Ah thanks.  Does this fix the problem as well?
> > 
> > diff --git a/src/pgtkterm.c b/src/pgtkterm.c
> > index b2816aa04a..5fbc56ae81 100644
> > --- a/src/pgtkterm.c
> > +++ b/src/pgtkterm.c
> > @@ -7037,8 +7037,11 @@ pgtk_set_cr_source_with_color (struct frame
> > *f, unsigned long color,
> >    pgtk_query_color (f, &col);
> >  
> >    if (!respects_alpha_background)
> > -    cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red / 65535.0,
> > -                         col.green / 65535.0, col.blue / 65535.0);
> > +    {
> > +      cairo_set_source_rgb (FRAME_CR_CONTEXT (f), col.red /
> > 65535.0,
> > +                           col.green / 65535.0, col.blue /
> > 65535.0);
> > +      cairo_set_operator (FRAME_CR_CONTEXT (f),
> > CAIRO_OPERATOR_OVER);
> > +    }
> >    else
> >      {
> >        cairo_set_source_rgba (FRAME_CR_CONTEXT (f), col.red /
> > 65535.0,
> > 
> > > I've also fixed the indent error, but couldn't find the non-
> > > breaking
> > > space in the patch, it's perhaps caused by the mail program.
> > 
> > It was previously filled with non-breaking spaces, but the patch
> > you
> > attached looks fine now.
> > 
> > > Regarding the copyright assignment, I've had no response after
> > > sending
> > > in the questionnaire at
> > > https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/
> > > re
> > > quest-assign.future
> > > to assign@gnu.org
> > 
> > It takes a while; if you don't get a response in 2 weeks, you
> > should
> > contact Eli or Lars, and they will ask the folks at the FSF to
> > expedite
> > things.
> > 
> > > Do you get sent a paper form of some kind which has to be filled
> > > in?
> > > (I'm not at my home address atm so I will have missed that)
> > 
> > You get sent a document you have to print and fill in, I think, but
> > that
> > depends on the country you're in.
> > 
> > Thanks.
> 


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

From 84d4401a9a9e628815fa30147ee08d4597af53d6 Mon Sep 17 00:00:00 2001
From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
Date: Sun, 8 May 2022 16:27:38 +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..e71887e03d 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.34.1

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

> Hello All,
>
> The copyright assignment paperwork came through, is it possible to
> incorporate the gsetting font rendering changes? I've attached a patch
> against the current master, let me know if I need to change anything.

Great, thanks!  Once Eli or Lars say your copyright paperwork is in
order, I will install your change.

Again, thanks for working on Emacs.

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

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Great, thanks!  Once Eli or Lars say your copyright paperwork is in
> order, I will install your change.

The paperwork is in order.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: 54564@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, Lars Ingebrigtsen
>  <larsi@gnus.org>
> Date: Fri, 13 May 2022 19:55:05 +0800
> 
> Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> 
> > Hello All,
> >
> > The copyright assignment paperwork came through, is it possible to
> > incorporate the gsetting font rendering changes? I've attached a patch
> > against the current master, let me know if I need to change anything.
> 
> Great, thanks!  Once Eli or Lars say your copyright paperwork is in
> order, I will install your change.

Pieter's assignment is on file, so please go ahead.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:

> Hello All,
>
> The copyright assignment paperwork came through, is it possible to
> incorporate the gsetting font rendering changes? I've attached a patch
> against the current master, let me know if I need to change anything.

Thank you.  I will install this shortly and am closing this bug.