Re: XSetting for font

[Owen Taylor <otaylor redhat com>]

Richard Hestilow <hestilow ximian com> writes:

> The attached patch implements an xsetting/GtkSetting for the default
> font. This seemed a more elegant way of setting the default font in the
> control center than editing ~/.gtkrc-2.0. May I commit? 

Sorry for the slow respones on this, got lost in my gtk-devel-list
backlog. (if it is in bugzilla, it won't get lost...)

The patch looks excellent in detail; at a larger-scale level, I think
we might need to do a few things a little bit differently to get
everything working reliably.

> @@ -532,22 +533,27 @@
>  {
>    gchar *new_theme_name;
>    gchar *new_key_theme_name;
> +  gchar *new_font_name;
>  
>    g_object_get (settings,
>  		"gtk-theme-name", &new_theme_name,
>  		"gtk-key-theme-name", &new_key_theme_name,
> +		"gtk-font-name", &new_font_name,
>  		NULL);
>  
>    if ((new_theme_name != context->theme_name && 
>         !(new_theme_name && context->theme_name && strcmp (new_theme_name, context->theme_name) == 0)) ||
>        (new_key_theme_name != context->key_theme_name &&
> -       !(new_key_theme_name && context->key_theme_name && strcmp (new_key_theme_name, context->key_theme_name) == 0)))
> +       !(new_key_theme_name && context->key_theme_name && strcmp (new_key_theme_name, context->key_theme_name) == 0)) ||
> +      (new_font_name != context->font_name &&
> +       !(new_font_name && context->font_name && strcmp (new_font_name, context->font_name) == 0)))
>      {
>        gtk_rc_reparse_all_for_settings (settings, TRUE);

reparse_all_for_settings() is actually a bit more than you need for
a change in the default font. What should be sufficient is:

 a) Do the first item in gtk_rc_clear_styles ();

  if (context->rc_style_ht)
    {
      g_hash_table_foreach (context->rc_style_ht, gtk_rc_clear_hash_node, NULL);
      g_hash_table_destroy (context->rc_style_ht);
      context->rc_style_ht = NULL;
    }

 b)  Call gtk_rc_reset_widgets ()

We don't necessarily need to do this optimzation now, but we should
at least file a bug indicating that it is possible. (It strikes me
as a little silly to reread all the RC files when nothing about
them has changed, just the default font name; once we add colors
into the mix, avoiding rereading the RC files unecessarily is
probably be even more of an issue.)

> @@ -1362,9 +1374,12 @@
>  
>        g_free (context->theme_name);
>        g_free (context->key_theme_name);
> +      g_free (context->font_name);
> +
>        g_object_get (context->settings,
>  		    "gtk-theme-name", &context->theme_name,
>  		    "gtk-key-theme-name", &context->key_theme_name,
> +		    "gtk-font-name", &context->font_name,
>  		    NULL);
>  
>        if (context->theme_name && context->theme_name[0])

I don't think this quite works. The reason that we store the theme_name
and key_theme_name values here is that this is this is the
place, and the place we use them. But that's not true of the
font name. I think if you do something like:

 gtk_init (&argc, &argv);
 
 gtk_rc_parse_string ("gtk-font-name = Sans 20");
 widget = gtk_widget_new ();
 gtk_rc_parse_string ("gtk-font-name = Sans 10");

Then widget's font won't get properly reset to "Sans 10"
because context->font_name == Sans 10, so when the 
::notify signal is received, the font name matches the
original font name.

A simple correct algorithm should be:

 - Add a function _gtk_rc_context_get_default_font_name ()
 - Every time it is called, compare the new value with
   the value from context->font_name, if it is different
   (and context->font_name is non-NULL), do a) and b) from
   above.
 - When you get a notify::gtk-font-name, also call
   _gtk_rc_context_get_default_font_name().

A future optimization would be to add some way of telling
if gtk_rc_reset_widgets () had to do anything. [ If no
styles have been looked up since we last reset widgets,
we don't need to reset the widgets again ]

> Index: gtk/gtkstyle.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkstyle.c,v
> retrieving revision 1.105
> diff -u -r1.105 gtkstyle.c
> --- gtk/gtkstyle.c	2002/02/04 15:43:09	1.105
> +++ gtk/gtkstyle.c	2002/02/10 05:48:01
> @@ -448,8 +448,12 @@
>  gtk_style_init (GtkStyle *style)
>  {
>    gint i;
> -  
> -  style->font_desc = pango_font_description_from_string ("Sans 10");
> +  GtkSettings *settings = gtk_settings_get_default ();
> +  gchar *font_name;
> +
> +  g_object_get (settings, "gtk-font-name", &font_name, NULL);
> +  style->font_desc = pango_font_description_from_string (font_name);
> +  g_free (font_name);
>  
>    style->attach_count = 0;
>    style->colormap = NULL;

Using gtk_rc_context_get (gtk_settings_get_default ()) to call
_gtk_rc_context_get_default_font_name() should be OK for now.

For 2.2 (multihead) we'll have to do something different - I think the
correct approach though is to add a ::font_desc GObject property,
and then set a default style->font_desc in a constructor() method
if it hasn't already been set. (A bug about this would be useful
as well.)

Regards,
                                        Owen