Re: Theme engine revision

[Tim Janik <timj gtk org>]

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

for now, i'll just comment on your patch.

> 
> > I've attached the patch implementing this. [...]

+  return GTK_RC_STYLE (g_type_create_instance (G_TYPE_FROM_INSTANCE (style)));

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

+  for (i=0; i<5; i++)

did we recently change coding style?

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

 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.

+  /* Create an empty RC style of the same type as this RC style.
+   * The default implementation, which does
+   * g_type_create_instance (G_TYPE_FROM_INSTANCE (style));
+   * should work in most cases.
+   */

"which does g_object_new (G_OBJECT_TYPE (style))".

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

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

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

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


> 
> Or maybe not.
> 
> 

---
ciaoTJ