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

[Po Lu]

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?