Created an attachment (id=1233) [details]
Screenshot showing the different style in the beta SDK

Created an attachment (id=1234) [details]
Testcase: C sourcecode

Created an attachment (id=1235) [details]
Testcase: GtkBuilder .ui file

Steps to reproduce:

 * Download the two Testcase files attached to this bug
   in the same folder inside the SDK
 * Compile hildonbutton.c in Scratchbox with:

  gcc -o hildonbutton hildonbutton.c \
     `pkg-config --libs --cflags hildon-1 gtk+-2.0 gobject-2.0`

 * Run the compiled binary in scratchbox:

  run-standalone.sh ./hildonbutton

This produces the result shown in the screenshot. The widget appearance should
be the same, independent of whether the widget is created using GtkBuilder or
via code.

The problem here is that the proper theming of different-size buttons is set by
using the GtkWidget::name property. GtkBuilder, otoh, uses this property to
store the "id" attribute.

One possible fix is to make HildonButton implement GtkBuildable, store the "id"
in a different form, by implementing (* set_name). This might probably be
necessary for other widgets that use named widgets to get the proper theming.

I'll look into this.

Created an attachment (id=1237) [details]
fix for HildonButton

This is a preliminary fix for this problem in HildonButton. It stores the
GtkBuildable name to a private buildable_name string, leaving the
GtkWidget::name id untouched. This way, HildonButton gets the proper theming.

A similar patch could be cooked for all the hildon widgets that rely in
GtkWidget::name to get proper theming, but I think this deserves a bit of
discussion before proceeding.

Berto, what do you think?

(In reply to comment #5)
> The problem here is that the proper theming of different-size buttons is set by
> using the GtkWidget::name property. GtkBuilder, otoh, uses this property to
> store the "id" attribute.

That's just stupid! Changing the widget name (that should "belong" to the
developer) in order to get themeing right is The Wrong Way (themeing a "group"
of widget, like "thumb button", "finger button", "normal button", that is...).

There are several cases (i.e. a generic signal handler that chooses its action
based on the widget name) and even gtk_builder_get_object() that depend on the
name of the widget.

Please tell me that I'm wrong, but this smells fishy. I don't know of any other
stock GTK widgets that change the name in order to get themeing right.

(In reply to comment #9)
> (In reply to comment #5)
> > The problem here is that the proper theming of different-size buttons is set by
> > using the GtkWidget::name property. GtkBuilder, otoh, uses this property to
> > store the "id" attribute.
> 
> That's just stupid!

This is unnecessary.

> Changing the widget name (that should "belong" to the
> developer) in order to get themeing right is The Wrong Way (themeing a "group"
> of widget, like "thumb button", "finger button", "normal button", that is...).

The difference, of course, being that we use the sapwood theme engine, which is
pixmap-based. For performance reasons (basically to avoid pixmap scaling), the
theme includes specific theming for all the standard button sizes.

In GTK+, there are three ways in which you can tell the theme engine (through
GtkRC) how to select a specific theming for a particular widget instance:

1. By its class.
2. By its widget hierarchy in the UI.
3. By its widget name.

Using (1) and (2) is definitely not enough for perfect-size matching, unless
you want to have a different class for each different button size (which would
be horrible, I'd say). The only resource left is to use the widget name.

> There are several cases (i.e. a generic signal handler that chooses its action
> based on the widget name) and even gtk_builder_get_object() that depend on the
> name of the widget.

I think you are confusing the GtkWidget::name with GtkBuildable's
set/get_name(). In the GtkWidget implementation of the interface, the
GtkBuilder object's name is stored in the GtkWidget::name property, but there's
nothing, AFAIK, that stops a widget from keeping the object name stored in a
different way, as long as the interface is kept consistent and the name stored
in a different place. That's what I did in my patch.

There might be another limitations when using GtkWidget::name for theming, of
course, but that's certainly a different topic.

> Please tell me that I'm wrong, but this smells fishy. I don't know of any
> other stock GTK widgets that change the name in order to get themeing right.

Most likely, there aren't. In the desktop. Desktop theme engines are not using
perfect-size theming, because scaling is hardly an issue (and, anyway, most
modern theme engines are rendering on the fly, using cairo, for instance). This
is a well known difference between the maemo UI and the desktop environments.

Btw, I just noticed that it is possible to set the set/get_name interface
methods to NULL, and GtkBuildable will store then the buildable object's name
as object data. This is even better than adding a private field to
HildonButton.

(In reply to comment #10)
> pixmap-based. For performance reasons (basically to avoid pixmap scaling), the
> theme includes specific theming for all the standard button sizes.
> 
> In GTK+, there are three ways in which you can tell the theme engine (through
> GtkRC) how to select a specific theming for a particular widget instance:
> 
> 1. By its class.
> 2. By its widget hierarchy in the UI.
> 3. By its widget name.
> 
> Using (1) and (2) is definitely not enough for perfect-size matching, unless
> you want to have a different class for each different button size (which would
> be horrible, I'd say). The only resource left is to use the widget name.

How many button sizes are there? From what I know after playing a bit with the
widgets, there is "auto", "finger" and "thumb":

http://maemo.org/api_refs/5.0/pre-alpha/apis/libhildon-2.1.24/hildon-hildon-gtk.html#HildonSizeType

That means in addition to HildonButton, you would have to introduce two more
widgets (HildonFingerButton and HildonThumbButton) in order to be able to
differentiate between these three styles (and the FingerButton and ThumbButton
probably just need to be subclasses of HildonButton without any added
functionality).

> > There are several cases (i.e. a generic signal handler that chooses its action
> > based on the widget name) and even gtk_builder_get_object() that depend on the
> > name of the widget.
> 
> I think you are confusing the GtkWidget::name with GtkBuildable's
> set/get_name(). In the GtkWidget implementation of the interface, the
> GtkBuilder object's name is stored in the GtkWidget::name property, but there's
> nothing, AFAIK, that stops a widget from keeping the object name stored in a
> different way, as long as the interface is kept consistent and the name stored
> in a different place. That's what I did in my patch.

Still, when I'm loading my interface from the .ui file, I use the
"gtk_builder_get_objects()" function to get all objects and store them
somewhere, so I can access them later on. After getting these objects, I use
the get_name method on them to find out which widget it is. If the name is
mangled, things stop working from here. Special-casing the name-getting for
Hildon widgets and using some other method to get the name is tedious and not
really obvious.

> There might be another limitations when using GtkWidget::name for theming, of
> course, but that's certainly a different topic.
> 
> > Please tell me that I'm wrong, but this smells fishy. I don't know of any
> > other stock GTK widgets that change the name in order to get themeing right.
> 
> Most likely, there aren't. In the desktop. Desktop theme engines are not using
> perfect-size theming, because scaling is hardly an issue (and, anyway, most
> modern theme engines are rendering on the fly, using cairo, for instance). This
> is a well known difference between the maemo UI and the desktop environments.

Agreed. Still, I think using subclasses instead of changing the content of
existing properties would be the right way to go. I am sure several developers
that come from a "Desktop" background will be confused by this.

It would be nice to have an overview of how many different button types there
are to differentiate for themes. Currently, setting the widget name involves
some "magic" for the developer that just uses the widgets to get work done.
Having different widget classes makes it obvious what to use and also does not
cause side-effects (i.e. the get_name method returning a different value than
expected).

(In reply to comment #12)
> Still, when I'm loading my interface from the .ui file, I use the
> "gtk_builder_get_objects()" function to get all objects and store them
> somewhere, so I can access them later on. After getting these objects, I use
> the get_name method on them to find out which widget it is.

You should use gtk_buildable_get_name() for this. That works perfectly fine
with/without the patch and is the correct way of accessing the object name.

(In reply to comment #7)
> A similar patch could be cooked for all the hildon widgets that rely
> in GtkWidget::name to get proper theming, but I think this deserves
> a bit of discussion before proceeding.

Yes, because that includes standard Gtk buttons (set with
_set_theme_size()).

I discussed with Daniel, Berto, and Kris about using a different approach for
specifying the size of the buttons to the theme. One possibility would be to
use the detail in gtk_paint_box() and change the theming to filter the size of
the button in this way. However, this would imply modifications in GTK+, the
theme, and hildon, and even then, it's hard to be sure that we wouldn't be
breaking applications relying on the widget name to get the proper theming,
unless we propagate the fix for all of them. This is, at this point, unlikely
to be accepted.

(In reply to comment #14)
> (In reply to comment #7)
> > A similar patch could be cooked for all the hildon widgets that rely
> > in GtkWidget::name to get proper theming, but I think this deserves
> > a bit of discussion before proceeding.
> 
> Yes, because that includes standard Gtk buttons (set with
> _set_theme_size()).
> 

In that case, we will probably need to change the GtkBuildable implementation
in the GtkButton class itself or even in GtKWidget. I think that the only risk
here is that applications ported to maemo using GtkBuilder that rely on
gtk_widget_get_name() might not work, but switching to gtk_buildable_set_name()
is an easy fix and also the right thing to do.

Moving to GTK+. The GTK+ people can decide how to deal with this.

Any progress on this? Having the correct themeing when using GtkBuilder would
be very important for many app developers that don't create their UI in code.

Comment 17 Andre Klapper 2009-08-28 14:50:12 UTC

This currently does not have a high priority and most likely will not get fixed
for the final Fremantle release.

Just a quick follow-up for developers who ran into the same problem and want to
work around this bug in their apps for Fremantle until it gets fixed:

You can get the desired style by calling gtk_widget_set_name() on your
HildonButton with the correct name (after it has been created by GtkBuilder,
but before it is displayed preferably). For my purposes, this is
"HildonButton-thumb" (have not found a list of possible names, though..)

Example in Python:

  # blubb_button is the button you want to style
  blubb_button.set_name('HildonButton-thumb')

Example in C:

  GtkWidget* blubb_button = /* the button you want to style */;
  gtk_widget_set_name(blubb_button, "HildonButton-thumb");

This works at least in the beta2 SDK. The file in which this style is defined
seems to be /usr/share/themes/default/gtk-2.0/gtkrc. It might be hacky and not
work for all use cases, but it works for me :)

Comment 19 Andre Klapper 2009-10-15 13:58:13 UTC

Upstream ticket at https://bugzilla.gnome.org/show_bug.cgi?id=591085 .

FYI, the upstream bug has now been closed as RESOLVED/FIXED:

https://bugzilla.gnome.org/show_bug.cgi?id=591085

Maybe this fix can be backported to the GTK+/Hildon version in Fremantle?

Comment 21 Andre Klapper 2009-12-03 15:02:35 UTC

This is done currently - that's why I've set the Target Milestone. :)

Still an issue in 2.2009.51-1. Compiled the example code from comment 2 in
Scratchbox and copied the resulting binary and the .ui file from comment 3 to
the device, and the result is still the same as shown in the screenshot in
comment 1 (although of course the themeing is different).

This was fixed in the GTK+ repository.

Comment 24 Andre Klapper 2010-01-13 18:22:15 UTC

This has been fixed in package
gtk+2.0 2:2.14.7-1maemo23+0m5
which is part of the internal build version
2010.02-5
(Note: 2010 is the year, and the number after is the week.)

A future public update released with the year/week later than this internal
build version will include the fix. (This is not always already the next public
update.)
Please verify that this new version fixes the bug by marking this bug report as
VERIFIED after the public update has been released and if you have some time.


To answer popular followup questions:
 * Nokia does not announce release dates of public updates in advance.
 * There is currently no access to these internal, non-public build versions.
   A Brainstorm proposal to change this exists at
http://maemo.org/community/brainstorm/view/undelayed_bugfix_releases_for_nokia_open_source_packages-002/

Comment 25 Andre Klapper 2010-03-15 20:53:30 UTC

Setting explicit PR1.2 milestone (so it's clearer in which public release the
fix will be available to users).

Sorry for the bugmail noise (you can filter on this message).