Created attachment 68415 [details]
testcase

you MUST have clearlooks engine available in your gtk engine directory to get the failed test.

Created attachment 68416 [details] [review]
Patch fixing the bug

Note that above patch applies the same fix to the list of icon_factories,
even tho i have no idea if they need it :-)

Created attachment 68418 [details]
updated testcase showing that the same problem exists with stock["foo"]

Can you explain how this relates to the code in gtk_rc_parse_style()
that handles copying icon factories and color hashes from the parent
style?

I'm pretty sure that your patch will have the effect that changes
to the colors/icons in the derived class will leak back to the
parent class: see the comment in gtk_rc_parse_style() starting
/* Add a factory for ourselves if we have none ... */

Just remembered why the engine case is special ... when it sees
the engine declaration it needs to change the type of the object
from the base GtkRcStyle to an engine-specific subclass.

Since in this case, we are throwing the old GtkRcStyle out, we don't
have to do the full dance from parse_style(), we should just
move the icon/color hashes over and remove them from the old
GtkRcStyle. But in any case, we can't do it in merge(), because
we need to do different things in different places where we call
merge().

Ok, I was just writing a comment stating that I don't fully understand
gtkrc.c, but now you lost me :-) While your words totally make sense
to me, I fail to find the "old GtkRcStyle" you mention.

I'm guessing we're talking about gtk_rc_init_style(), but I don't
see which factories/hashes have to be moved from where to where,
and I also don't see where we throw out said old GtkRcStyle.

Back to reading gtkrc.c, hoping for some more enlightenment...

One more question: Which place that calls merge() doesn't need the
factories/hashes to be copied? To me it looks like all places need it...

Mitch, take a look at gtk_rc_parse_engine(), that's where a new rc-style gets created and is exchanged for the passed in rc-style.

Ok thanks I see the code now. Still I fail to see why any user
of merge() wouldn't need the copying of the merged style's
factories and hashes:

More detailled, I see 3 places that call merge():

- gtk_rc_style_copy() lacks the copying of factories and hashes.
  I'm pretty sure it needs it.

- gtk_rc_init_style() had the copying code that I moved to real_merge()

- gtk_rc_parse_engine() calls real_merge() and doesn't copy the
  factories and hashes.

But actually I'm still trying to understand all of this completely...

Created attachment 68462 [details] [review]
Next try with a bit more insight...

Ok I understand now why the merge() in gtk_rc_parse_engine() is different,
it must take over the old style's lists without prepending a newly created
own factory/hash.

Attached patch does this, plus factors factory/hash copying from both
parent styles and GtkRcContext out to a new utility function (including
prepending the style's own factory/hash in order to avoid leaking icons
and colors to the parent).

* I would avoid the name parent for the parameter to copy_factories, since
  it's only the parent style in 1 of 3 usages.

* The behavior to create a new hash/icon factory for our "own" usage is
  not right for gtk_rc_init_style(), since the merged prototype style 
  we are creating is read-only. It's not legitimate to start modifying
  style->rc_style. Ithink we want to void creating the extra hashes / 
  factories. (We could be talking dozens or hundreds here)

  I think it adds significant confusion to share copy_factories between
  init_style() and the other two uses, in fact. In init_style(), we 
  just literally want to concatenate the factories from the source
  styles, in the other cases, we need special handling.
 
  I could see adding some helper functions to factor out the 3 lines
  of repeated code to concat the icon factories and the 3 lines to 
  concat the color hashes.

  I haven't convinced myself that there are other problems than that
  noted above from the reuse, but I haven't convinced myself that there
  aren't problems either. The fact that copy_factories can either
  be called once or called repeatedly is mind-blowing.

* Rather than calling copy_factories once with parent_style and context
  NULL,  and then  again once with parent_style = NULL and the context,
  from gtk_rc_parse_style(), I'd rework the code to call it only
  once. I think that's easy by just moving the if (parent_style)
  block outside the if token == G_TOKEN_EQUAL_SIGN block.

* I don't think the name copy_factories is right. I might call it something
  like copy_icons_and_colors()

* The repeated sequence in copy_factories().

+          GHashTable *hash = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                                    g_free,
+                                                    (GDestroyNotify) gdk_color_free);
+          priv->color_hashes = g_slist_prepend (priv->color_hashes, hash);

  needs to be factored out

Created attachment 68476 [details] [review]
Next patch iteration

Should address all issued raised.

Created attachment 68477 [details] [review]
Stylistic cleanups

Fixed in CVS:

2006-07-06  Michael Natterer  <mitch@imendio.com>

	* gtk/gtkrc.c: added a bunch of utility functions to copy
	icon_factories and color_hashes between GtkRcStyles and make sure
	that newly created, duplicated and merged styles have access to
	all icon_factories and color_hashes they need. Fixes bug #346668.