Created attachment 27645 [details]
testcase

While I'm not entirely happy with the system in genreal, things
are working as documented and expected. The text in the menu item is
not in the GtkMenuItem, but in a GtkLabel contained within
the GtkMenuItem.

It doesnt appear to work as documented.

The documentation found here:
http://www.gtk.org/~otaylor/gtk/2.0/theme-engines.html

clearly states that either the above quoted line, or the following line should
allow subclasses to inherit the rc setting:

widget_class "*.GtkMenuItem.*" style "bluecurve-menu-item"

Regardless of if the bug is in the general case, or the specific case, when a
developer subclasses a GtkMenuItem and then puts a label in it, the expectation
that it works the same as sticking a label inside GtkImageMenuItem is a given.

What steps would I be able to take to apply the style set for something inside a
GtkMenuItem or its gtk+ subclasses to my own subclass?

widget_class does not inherit, is not supposed to inherit, and is
not documented to inherit.

I'm not disputing that there is an issue here, but there is no code bug.

alright, can you give me some pointers as to the best way to workaround.

I guess i want to simulate having

widget_class "*.GtkMenuItem.*" style "bluecurve-menu-item"

But instead of GtkMenuItem, i want MyClassName or whatever.

Owen, 
http://www.gtk.org/~otaylor/gtk/2.0/theme-engines.html
says, about "class", that
" This declares that the my-button RC style should apply to all widgets in the
GtkButton class and in classes deriving from GtkButton"

It doesn't seem to say anything different about "widget_class", though it
doesn't seem to say much of anything about "widget_class", and this is the first
time that  I've tried to read that document.


Is there a problem with making it apply to subclasses, until another it is
overriden by a more specific command?

I'm reopening this because it's clearly a bug in something somewhere.

*** Bug 128560 has been marked as a duplicate of this bug. ***

That document is just informative; the API docs are normative.

We can't just arbitrarily change the semantics of what a RC style
option does because we think it would be better some other way!
widget_class is defined as a string match against a string path.

If wanted to do something here, we'd have to add yet one more matching 
mechanism.

I'm generally not very much in favor of incrementally evolving RC
system to be more and more complex, but sure we can leave this bug open 
...

I dont really see how this change is in any way negative or bad. Already
Bluecurve and Industrial (and many others) work around this problem by defining
a "*.GtkMenuItem.*" type string for all the subclasses of GtkMenuItem inside gtk+.

Any other way doesnt make sense. When someone subclasses a widget, they should
be able to expect that the rc style is inherited to the new subclass.

If we can't/won't fix this bug in gtk+, can you explain how (or point to where
to look) about fixing this in the language bindings where it is a semi-large
visual issue.

Replying to the test case found on Bug #128560 :

I believe this should be fixed on GTK+. The themes should not be allowed to 
fall into this trap. I tried Drivel 1.0 today for example (a Livejournal 
client whose gui was created with glade) and all of its combo boxes's fonts 
had the wrong color:

1. Change your widget theme to "Default".
2. Go to the preferences of Drivel and use the two combo boxes you got there.
3. You will see that the open combo box's text remains black when selected, 
while it should have being white.

Other applications' combo boxes (e.g. gedit's) don't exhibit the problem and 
if Glade falls into this trap, many apps would exhibit the problem (I have 
personally seen it on a lot of apps).

In case someone wants to cook a patch for this, the functions that need tweaking
are gtk_rc_get_style() and gtk_rc_get_style_by_paths().

A patch that changes existing behavior is in my opinion wrong.

i agree with comment #9 - this lack of inheritance just doesn't make logical
sense.  furthermore, i don't see how changing this behavior will have negative
impact.  for developers that are manually setting styles on their widget
subclasses, the manual settings will still take precedence, and for devs who are
frustrated and don't know why it doesn't work, they'll have a nice surprise in
the next release.

using gtk_rc_get_style_by_paths() works fine to start with (if you apply the
style to the GtkLabel contained in the menu item), but if the gtk theme later
changes *the widget will not pick up the theme change*.  so here's what i had to
do...  i left the menu item's style alone, and connected to its "style-set"
signal, and did this in the callback:

style = gtk_rc_get_style_by_paths(gtk_settings_get_default(), "GtkMenuItem",
        "GtkMenuItem", GTK_TYPE_MENU_ITEM);
children = gtk_container_get_children(GTK_CONTAINER(menu_item));
for(l = children; l; l = l->next) {
    if(GTK_IS_WIDGET(l->data))
        gtk_widget_set_style(GTK_WIDGET(l->data), style);
}

after trying many many many different things, this is the only method i could
get to work reliably.  i think this is an *awful* hack, and gtk's behavior
really needs to be fixed here.  i can't think of _any_ reason why someone making
a widget subclass wouldn't want the style to be inherited.

and re: comment #12 - that apparently didn't stop something going into gtk 2.4.2
which changed some behavior, causing some custom widgets we're using (that hold
images) to start sizing the widgets differently such that the images were
getting clipped.  if something negative like that can go in, i don't see why
something positive like this can't.

granted, i'm not at all familiar with the gtk style code, so i have no idea of
the complexity we're talking about.

Brian, there is a difference between the behaviour change wrt to button sizing
in 2.4.2 and the one discussed here: the button sizing was clearly buggy before,
while the rc widget_class matching works exactly as documented... 

matthias - yes, i should have left that bit out.  i was just frustrated and
lashing out, sorry.

personally i find it irrelevant if the matching works as documented.  it's
unintuitive and undesirable behavior, and in my book, that's a bug.  to be
blunt, you're not going to convince me otherwise.  all that remains is whether
or not you agree that it _should_ behave differently, and if it's ok to change
the behavior in the middle of a stable release cycle.  i suppose it isn't too
bad, since i have a workaround (though it only works for widget subclasses that
contain child widgets), but it would be really nice to have some behavior that
makes logical sense.

OK, summary from a discussion I had with Owen:

1. Yes, this is a genuine bug.  But we can't change the behavior because we are
in a stable branch.

2. It would be nice if someone cooked a patch to support class inheritance when
something like this appears:

  widget_class "*.<GtkMenuItem>.*" style "style-for-menu-items"

That is, angle brackets indicate that derived classes should match as well as
the specified class.

Owen says that this is a bit hard to do efficiently.

I'm going to update the documentation about this; I retitled the bug for clarity.

1. I am personally happy for a slight behaviour change to happen in GTK+ 2.6
rather than the stable branch. It is not suddenly more urgent than before.

1. is a misunderstanding of my position. 2.4 and 2.6 are *compatible*
versions of GTK+. We can't change RC file interpretation in an incompatible way
between them.

The main things we can do in the unstable branch that we can't do in the
stable branch:
 
 - Add API
 - Make changes that require significant new code or code changes
 - Make changes that we think are sufficiently compatible
   but we aren't entirely sure without testing

For further reference:
http://mail.gnome.org/archives/desktop-devel-list/2004-August/msg00282.html

Guys, this really does suck, NetworkManager's applet runs into it too because
the menu items are actually subclasses of GtkCheckMenuItem.

Is there an "official" workaround at all, and is anything being done on this bug
for further versions of GTK?

Requiring changes to the gtkrc file isn't going to work well, because you have
to explicitly specify in all theme files that you want it to apply to derived
widgets as well, and who really expects all themes to pick that up quickly? 
These widgets are subclasses, and you expect them to have the same properties
and behavior as their parent, unless you have changed that behavior explicitly
in your subclass, which neither Brian nor I have done.

The workaround is:

 A) All GtkMenuItem subclasses should end in MenuItem
 
 B) Themes that want to match labesl in menu items should 
    do it as

 wiget_class "*MenuItem.*GtkLabel"

    rather than matching on GtkMenuItem specifically

While we should fix this by extending the RC file syntax,
any RC file additions won't land in GNOME releases for
another 6 months... the workaround above is an immediate 
fix.

Owen, you mean your (a) or Brian's workaround above is the "immediate workaround"?

My workaround is A) + B). It's a something that subclassers need to do
and something theme writers need to do.

Something on the order of Brian's approach could work in individual
apps ...that's what gtk_rc_gets_style_by_paths() is there for.
(Though *as* described above it will work only for a tiny subset
of RC files. The paths passed in need to be more realistic.)
But I wouldn't advise it... it's just too complex and fragile.

*** Bug 168897 has been marked as a duplicate of this bug. ***

Created attachment 52331 [details] [review]
initial patch

This is an initial try to implement this.

Now, this implementation is only for styles and it needs to be extended to work
for key bindings.

I think that this would help to solve part the problem that was mentioned in the
thread about themeing the header of lists which was started by Richard.

http://mail.gnome.org/archives/gtk-devel-list/2005-September/msg00069.html

Todd mentioned there that it is a pain to fake the look of lists. I think that
by implementing this the check for the class of the parent widget could be
removed from the engine, and instead be done with a special style in the rc file.

If this is done, it should be possible to use gtk_rc_get_style_by_paths to fake
the look of eg. the TreeView instead of really creating all these widgets.

Created attachment 52635 [details] [review]
2nd patch

Ok, this is an improved patch. It should now also work for bindings.

With this patch the following two matches are the same. ie. The dot between the
two classes is ignored.
  widget_class "*.<GtkTreeView>.<GtkButton>" style "foo"
  widget_class "*.<GtkTreeView><GtkButton>" style "foo"

One thing I have noticed is, that as the code walks the class path and uses
g_type_from_name, the class needs to be registered already. That means that
people might require to run gtk_XXX_get_type for some widgets in the class path
if they use gtk_rc_get_style_by_paths. This needs to be documented.

*** Bug 319411 has been marked as a duplicate of this bug. ***

Hmm, quite a lot of code.

Do you think the CLASS_PSPEC stuff is really useful, Benjamin ?
I would be happier with a simpler patch that only allows straight 
class names between < >, but can be done in less than 500 lines.

I don't remember any reason for doing it this way. I'll update the patch without the pspec stuff for classes.

Created attachment 61776 [details] [review]
new patch without CLASS_PSPEC

The bigger differences are:
 1. no pspec matching for classes
 2. adjusted the gtk default theme
 3. save a lot of g_strdup calls. (The previous one was broken in this regard)

Hmm, the patch itself actually got larger, but diffstat shows less additions (and more deletions).

Created attachment 61780 [details] [review]
patch with fixed freeing

Oops, this one fixes freeing so that gtk does not crash and/or print critical warnings when the theme is changed.

Thanks, I committed a slightly modified version

2006-03-23  Matthias Clasen  <mclasen@redhat.com>

	Support subclasses in RC files.  (#142417, Todd Berman, patch
	based on a patch by Benjamin Berg)
	
	* gtk/gtkrc.h: 
	* gtk/gtkrc.c: Support <classname> elements in widget_class paths 
	in rc files which match any classes derived from named class.

	(_gtk_rc_init): Use the new syntax in the default rc string.

	* gtk/gtkbindings.c: Support the new syntax for bindings too.
	
	* tests/testrc.c: Tests for widget_class path matching