Re: Theme engine revision

[Owen Taylor <otaylor redhat com>]

Thanks for the comments. I've fixed most of them now.

Tim Janik <timj@gtk.org> writes:

> On 16 Jul 2000, Owen Taylor wrote:
> 
> > 
> > Owen Taylor <otaylor@redhat.com> writes:
> 
> 
> ok, i haven't yet had a chance to read up and comment on
>  http://www.gtk.org/~otaylor/gtk/2.0/theme-engines.html
> i'll do that as soon as time permits (there're at least some
> issues with ThemeMatchData as far as i saw from glancing).

ThemeMatchData is just part of the pixbuf engine implementation -
not part of the GTK+ code base at all.
 
> > > I've attached the patch implementing this. [...]
> 
> 
> don't use g_type_create_instance(), it is an function only
> intended for implementators of fundamental types, use
> g_object_new() instead (yes, that's part of the as of now
> unrelease GObject/GType docs).

OK. I changed most of these but forgot this one. Any particular
reason for this, though? The trailing NULL you need for
g_object_new() is rather annoying.

> +  for (i=0; i<5; i++)
> 
> did we recently change coding style?

That's old code which has been moved around. I've fixed it
though.
 
> +         if (G_TYPE_FROM_INSTANCE (rc_style) != rc_style_type)
> 
> here, use the G_OBJECT macro G_OBJECT_TYPE() instead (for others
> reading this, there's also GTK_OBJECT_TYPE() to retrive a Gtk
> object's type id, in 1.2.x even).

[ It's not a GtkObject..., and GTK_OBJECT_TYPE I hope is
  deprecated. In my list of comments on the type system
  that you have, you'll find some whining about random
  duplicate macros for generic operations ]

I've changed it to G_OBJECT_TYPE(), though I don't quite
understand why.
 
>  static guint
>  gtk_rc_parse_engine (GScanner   *scanner,
> -                    GtkRcStyle  *rc_style)
> +                    GtkRcStyle **rc_style)
>   
> hm, will have to look at gtk_rc_parse_engine() seperatedly when
> the patch is applied, the diff looks a bit dubious, as if you
> could go on parsing more tokens even if you return something
> != G_TOKEN_NONE.

It should be fine ... rc_style needs to be inout here because
parse_engine will occasionally need to convert rc_style() from say,
GtkRcStyle to PixbufRcStyle. It does this by creating the
PixbufRcStyle, copying over the data, and then freeing the GtkRcStyle.

>  gtk_style_copy (GtkStyle *style)
> 
> urg, this function doesn't copy x/y thickness (not due to your
> patch though), that needs to be fixed.
> and in gtkstyle.h, the position of x/y thickness within
> struct _GtkStyle should prolly be move *before* all the
> GdkGC* fields, to maintain grouping of valid fields for
> unrealized styles (not due to your patch either).

I've fixed these problems.

> +  if (GTK_STYLE_GET_CLASS (style)->realize)
> +    GTK_STYLE_GET_CLASS (style)->realize (style);
> 
> since you special cased this and ->unrealize all over the place...
> i can't see a vlid case where a style would have NULL realizers or
> unrealizers, i think we should simply make those functions mandatory.

Hmmm, most theme engines _don't_ need their own realize functions,

But since the base GtkStyle class does have these functions,
since classes deriving from should preserve that, and since
a general rule in GTK+ is that you can't replace a non-NULL 
vfunc with a NULL vfunc, you are most likely right. Changed.
 
> +static void
> +gtk_style_real_unrealize (GtkStyle *style)
> +{
> +  int i;
> +
> +  gtk_gc_release (style->black_gc);
> +  gtk_gc_release (style->white_gc);
> +
> +  for (i = 0; i < 5; i++)
> 
> please fix those ints to use g* types to avoid further s/type/gtype/ orgies,
> also as a minor stylistic comment, such loop vartiables should really
> be unsigned (you should hear linus rant about such things).

That's a bad idea unless there is a real possibility of overflowing
the range, because then someday someone will write:

 guint i;

 for (i = 4; i >= 0 ; i--)
   {
     [..]
   }

If Linus said that, well then, Linus is sometimes wrong. To avoid
problems with mixing signed and unsigned quantities, one should simply
avoid using unsigned quantities whenever possible.
 
> +static void
> +gtk_theme_engine_plugin_ref (GTypePlugin *plugin)
> +{
> +  GtkThemeEnginePlugin *theme_plugin = (GtkThemeEnginePlugin *)plugin;
> +
> +  if (theme_plugin->engine == NULL)
> +    {
> +      gtk_theme_engine_get (theme_plugin->engine_name);
> +      if (!theme_plugin->engine)
> +       {
> +         g_warning ("An attempt was made to create an instance of a type from\n"
> +                    "a previously loaded theme engine was made after the engine\n"
> +                    "was unloaded, but the engine could not be reloaded or no longer\n"
> +                    "implements the type. Bad things will happen!\n");
> +       }
> +    }
> +  else
> +    gtk_theme_engine_ref (theme_plugin->engine);
> +}
> 
> you may want to fix up the warning text here ;)
> also, since you don't have an engine that you could add the reference count
> for, you're already vialoting GTypePlugin constraints, so simply g_error()
> out right away, using a g_warning() isn't in any way beneficial.

g_error() seems wrong to me. g_error() should ONLY ever be
used if the problem has to be bug in the program, since it will most
likely result in either:

 - A core file being left in the user's home directory, or
 - A bug being reported to the GNOME bug tracker when gnome_segv
   calls bug-buddy.

[ For those on gtk-devel-list - the problem here is something that Tim
  and I have already discussed a fair bit. The basic problem is that 
  the GObject system can't unregister types that were previously
  registered. Tim claims that any time that you register a type,
  you have to allow for somebody to query the type system any
  time in the future, and create an instance of that type. 

  Unfortunately, once you unload a theme engine, there is no real
  guarantee you can reload it, or that the theme engine you
  reload registers the same types. The above warning never occurs in
  normal operation, since the theme engine code can handle an
  engine failing to load just fine. But there are some corner cases
  where someone could trigger it by doing unusual things. ]

The "typical" sequence that causes the the above problem would be:

 - The program calls G_OBJECT_TYPE (widget->style) and saves the
   result

 - Later, the user switches themes, causing the first
   theme engine to be unloaded. And then deletes the first
   theme engine from their hard drive

 - Still later, the program tries to create an object from the
   stored type using g_object_new().

Now, we have various possibilities for what we consider the
failure here to be:

 a) The program wasn't allowed to store the type like that.

 b) The program was allowed to store the type like this,
    but if it does, it has to expect that creating objects
    might fail.

 c) The user wasn't supposed to delete the theme engine from
    the hard drive.

If a) is the case, then the g_error() is fine, since it is
a program bug. But if you've resisted this position strongly.

If b) is the case, then we need a mechanism; either 
a failure return code from plugin_ref() or g_type_set_instantiatable().

If c) is the case, then a g_error() is most inappropriate, and if we
can't recover, we we should probably print a warning and exit(1).

> +         g_warning ("Two different theme engines tried to register '%s'!", type_name);
> +         return 0;
> +       }
> +
> +      if (plugin->parent_type != parent_type)
> +       {
> +         g_warning ("Type '%s' recreated with different parent type!\n"
> +                    "(was '%s', now '%s')", type_name,
> +                    g_type_name (plugin->parent_type),
> +                    g_type_name (parent_type));
> 
> minor nitpick, this should say `%s' according to the GNU coding style (and to
> be consistent with the rest of our warning). also i don't think the
> exclamaition marks would help bring your point across any better, we
> don't usually shout in warnings.

The X fonts for XFree86-4.0 intentionally do not have  symmetic 
` and '. The shapes they have for these characters, a grave 
accent and a vertical quote are not suitable to act as paired
single quotes.  

Basically, ASCII does not have paired single quotes, so it is
better to use paired vertical quotes. The GNU coding standards
are sometimes wrong ;-)

Regards,
                                        Owen