GNU bug report logs - #54564
29.0.50; [PATCH] Use gsettings font rendering entries for pgtk builds

Previous Next

Package: emacs;

Reported by: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>

Date: Fri, 25 Mar 2022 14:10:03 UTC

Severity: normal

Tags: patch

Merged with 54565

Found in version 29.0.50

Done: Po Lu <luangruo <at> yahoo.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54564 in the body.
You can then email your comments to 54564 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 25 Mar 2022 14:10:03 GMT) Full text and rfc822 format available.

Acknowledgement sent to Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Fri, 25 Mar 2022 14:10:03 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Use gsettings font rendering entries for pgtk builds
Date: Fri, 25 Mar 2022 12:23:10 +0100
[Message part 1 (text/plain, inline)]
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
    
[0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch (text/x-patch, attachment)]

Forcibly Merged 54564 54565. Request was from Lars Ingebrigtsen <larsi <at> gnus.org> to control <at> debbugs.gnu.org. (Fri, 25 Mar 2022 14:19:01 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 01:17:02 GMT) Full text and rfc822 format available.

Message #10 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 09:16:03 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 06:02:01 GMT) Full text and rfc822 format available.

Message #13 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50;
 [PATCH] Use gsettings font rendering entries for pgtk builds
Date: Sat, 26 Mar 2022 09:01:00 +0300
> Cc: 54564 <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 06:08:02 GMT) Full text and rfc822 format available.

Message #16 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 14:07:21 +0800
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 06:21:01 GMT) Full text and rfc822 format available.

Message #19 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 09:20:14 +0300
> From: Po Lu <luangruo <at> yahoo.com>
> Cc: pieter.van.prooijen <at> teloden.nl,  54564 <at> debbugs.gnu.org
> Date: Sat, 26 Mar 2022 14:07:21 +0800
> 
> Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 06:46:01 GMT) Full text and rfc822 format available.

Message #22 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 14:44:53 +0800
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 07:46:01 GMT) Full text and rfc822 format available.

Message #25 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 10:45:21 +0300
> From: Po Lu <luangruo <at> yahoo.com>
> Cc: pieter.van.prooijen <at> teloden.nl,  54564 <at> debbugs.gnu.org
> Date: Sat, 26 Mar 2022 14:44:53 +0800
> 
> Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 08:13:02 GMT) Full text and rfc822 format available.

Message #28 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 16:11:54 +0800
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 09:37:02 GMT) Full text and rfc822 format available.

Message #31 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 12:36:24 +0300
> From: Po Lu <luangruo <at> yahoo.com>
> Cc: pieter.van.prooijen <at> teloden.nl,  54564 <at> debbugs.gnu.org
> Date: Sat, 26 Mar 2022 14:44:53 +0800
> 
> Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 26 Mar 2022 15:49:02 GMT) Full text and rfc822 format available.

Message #34 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: Po Lu <luangruo <at> yahoo.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 26 Mar 2022 16:48:30 +0100
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 <at> 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.






Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sun, 27 Mar 2022 01:00:03 GMT) Full text and rfc822 format available.

Message #37 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sun, 27 Mar 2022 08:59:32 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Wed, 30 Mar 2022 08:03:02 GMT) Full text and rfc822 format available.

Message #40 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Wed, 30 Mar 2022 10:01:49 +0200
[Message part 1 (text/plain, inline)]
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 <at> 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.

[0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Wed, 30 Mar 2022 09:00:02 GMT) Full text and rfc822 format available.

Message #43 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Wed, 30 Mar 2022 16:59:19 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Thu, 31 Mar 2022 17:32:01 GMT) Full text and rfc822 format available.

Message #46 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Thu, 31 Mar 2022 19:30:58 +0200
[Message part 1 (text/plain, inline)]
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 <at> 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 (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 01 Apr 2022 02:01:01 GMT) Full text and rfc822 format available.

Message #49 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 01 Apr 2022 10:00:10 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Thu, 07 Apr 2022 19:25:01 GMT) Full text and rfc822 format available.

Message #52 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Thu, 07 Apr 2022 21:24:28 +0200
[Message part 1 (text/plain, inline)]
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 <at> 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 <at> 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.

[0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Thu, 07 Apr 2022 23:39:01 GMT) Full text and rfc822 format available.

Message #55 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 08 Apr 2022 07:38:05 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 08 Apr 2022 18:46:01 GMT) Full text and rfc822 format available.

Message #58 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 08 Apr 2022 20:44:47 +0200
[Message part 1 (text/plain, inline)]
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 <at> 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 <at> 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.

[0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Sat, 09 Apr 2022 00:36:02 GMT) Full text and rfc822 format available.

Message #61 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Sat, 09 Apr 2022 08:35:14 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 13 May 2022 11:39:02 GMT) Full text and rfc822 format available.

Message #64 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 13 May 2022 13:38:16 +0200
[Message part 1 (text/plain, inline)]
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 <at> 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 <at> 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.
> 

[0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 13 May 2022 11:56:01 GMT) Full text and rfc822 format available.

Message #67 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, Eli Zaretskii <eliz <at> gnu.org>,
 54564 <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 13 May 2022 19:55:05 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 13 May 2022 11:59:02 GMT) Full text and rfc822 format available.

Message #70 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 54564 <at> debbugs.gnu.org,
 Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 13 May 2022 13:57:35 +0200
Po Lu <luangruo <at> 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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#54564; Package emacs. (Fri, 13 May 2022 12:13:02 GMT) Full text and rfc822 format available.

Message #73 received at 54564 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: larsi <at> gnus.org, 54564 <at> debbugs.gnu.org, pieter.van.prooijen <at> teloden.nl
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 13 May 2022 15:12:06 +0300
> From: Po Lu <luangruo <at> yahoo.com>
> Cc: 54564 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, Lars Ingebrigtsen
>  <larsi <at> gnus.org>
> Date: Fri, 13 May 2022 19:55:05 +0800
> 
> Pieter van Prooijen <pieter.van.prooijen <at> 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.




Reply sent to Po Lu <luangruo <at> yahoo.com>:
You have taken responsibility. (Fri, 13 May 2022 12:52:01 GMT) Full text and rfc822 format available.

Notification sent to Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>:
bug acknowledged by developer. (Fri, 13 May 2022 12:52:02 GMT) Full text and rfc822 format available.

Message #78 received at 54564-done <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>
Cc: 54564-done <at> debbugs.gnu.org
Subject: Re: bug#54564: 29.0.50; [PATCH] Use gsettings font rendering
 entries for pgtk builds
Date: Fri, 13 May 2022 20:51:36 +0800
Pieter van Prooijen <pieter.van.prooijen <at> 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.




Reply sent to Po Lu <luangruo <at> yahoo.com>:
You have taken responsibility. (Fri, 13 May 2022 12:52:02 GMT) Full text and rfc822 format available.

Notification sent to Pieter van Prooijen <pieter.van.prooijen <at> teloden.nl>:
bug acknowledged by developer. (Fri, 13 May 2022 12:52:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 11 Jun 2022 11:24:06 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 292 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.