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:

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