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

[Pieter van Prooijen]

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?

0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch
Description: Text Data