After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 685733 - Call ->constructed() after all properties are set
Call ->constructed() after all properties are set
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-10-08 15:39 UTC by Allison Karlitskaya (desrt)
Modified: 2012-11-18 20:44 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
[gobject] set all properties before constructed() (1.46 KB, patch)
2012-10-08 15:55 UTC, Allison Karlitskaya (desrt) 		committed Details | Review

Description Allison Karlitskaya (desrt) 2012-10-08 15:39:16 UTC 
Bug 425324 introduced the ->constructed() vcall on GObject.

The original version of the patch called constructed() after all properties were set.  Unfortunately, the version that made it into the tree moved the call to a point where only the construct properties were set (with the others being set after).

The general consensus seems to be that this was a bad idea and has substantially reduced the usefulness of this function.  We're going to move the call to after the other properties being set and see if it breaks anything...
Comment 1 Allison Karlitskaya (desrt) 2012-10-08 15:55:23 UTC 
Created attachment 226053 [details] [review]
[gobject] set all properties before constructed()

Move the constructed() call to happen after all of the properties are
set (not just the construct properties).

This is an incompatible change but we are making it under the belief
that it should be safe.  If this change impacts you in a negative way
please comment on the bug.
Comment 2 Allison Karlitskaya (desrt) 2012-10-08 15:55:49 UTC 
Comment on attachment 226053 [details] [review]
[gobject] set all properties before constructed()

Attachment 226053 [details] pushed as 028d4a0 - [gobject] set all properties before constructed()


Going to leave the bug open for now and wait for comments...
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-10-17 21:18:50 UTC 
First hit in Rhythmbox, which is now seeing a (harmless) critical.

http://git.gnome.org/browse/rhythmbox/tree/sources/rb-static-playlist-source.c#n439

	case PROP_SHOW_BROWSER:
		if (g_value_get_boolean (value))
			gtk_widget_show (GTK_WIDGET (priv->browser));
		else
			gtk_widget_hide (GTK_WIDGET (priv->browser));
		break;

The widget is constructed in the constructed handler, so is causing the GTK_WIDGET cast to fail. The incorrect fix is to bail out if priv->browser is NULL. The ugly but correct fix is to move the construction of the widget to the constructor vfunc.
Comment 4 Giovanni Campagna 2012-11-18 18:25:47 UTC 
I filed bug 688596 before finding that this is still open.

I'm sorry but this change needs to be reverted now. I don't need to cite our favorite kernel hacker to make it clear that API changes must be avoided for a platform critical library to be taken seriously.

That aside, this change does not make sense even without considering the breakages, as now property setters cannot assume that the object is fully constructed.
Take rhythmbox as an example. The only possible "fix" is to introduce a separate g_object_set() after the object is fully constructed.
Moreover, it becomes worse once you deal with introspected languages, since constructors for those are just wrappers around g_object_newv, and it is a reasonable expectation that all properties can be set at once in those.
Comment 5 Allison Karlitskaya (desrt) 2012-11-18 20:44:33 UTC 
Since we now have bug 688596, we may as well close this one...