GNOME Bugzilla – Bug 698614
GObject: prevent installing properties after init
Last modified: 2014-06-10 21:00:45 UTC
This will allow us to nuke GParamSpecPool, which will be awesome.
Created attachment 242171 [details] [review] GObject: prevent installing properties after init GObject has previously allowed installing properties after class_init has finished running. This means that you could install some of your own properties on G_TYPE_OBJECT, for example, although they wouldn't have worked properly. Prevent this from happening. Require that all properties are installed by the time class_init has finished. Complaints go to this bug:
Review of attachment 242171 [details] [review]: otherwise it looks good ::: gobject/gtype.c @@ +4649,3 @@ + +gboolean +g_type_is_in_init (GType type) This only works for classed types (due to looking at node->data->class), so it would be nice to reflect that in the name. @@ +4655,3 @@ + node = lookup_type_node_I (type); + + return node->data->class.init_state != INITIALIZED; Will crash if node->data == NULL
This isn't a public API and it only gets called from places where the problems that you point out won't matter... What do you prefer I call it?
Review of attachment 242171 [details] [review]: argued offlines that these are non-issues
Attachment 242171 [details] pushed as ddb0ce1 - GObject: prevent installing properties after init
We had to revert this already -- problems in gjs and gnome-settings-daemon.
I've uploaded a new version of this patch with a critical instead.
With this patch, metacity tells me: (metacity:8953): GLib-GObject-CRITICAL **: Attempt to add property GtkSettings::gtk-button-images after class was initialised and Log level 8: Attempt to add property GtkSettings::gtk-label-select-on-focus after class was initialised metacity is 2.34.13 from fedora packages, gtk+-2 is from jhbuild Other gtk2 apps don't seem to have this problem.
You want more? Here you are: (gnome-contacts:2271): GLib-GObject-CRITICAL **: Attempt to add property ESourceCamelEws::filter-inbox after class was initialised (one for each ESourceCamel* property).
The gtk2 warning is gtk2 brain damage: it installs setting properties from various class_init instead of using GtkSettings.class_init(); it should actually be fixed regardless of this warning, as it makes settings depend on certain GTypes being initialised. I remember fixing that for gtk3, so it should be a case of back porting the fix and releasing a new 2.24 tarball.
(In reply to comment #11) > The gtk2 warning is gtk2 brain damage: it installs setting properties from > various class_init instead of using GtkSettings.class_init(); it should > actually be fixed regardless of this warning, as it makes settings depend on > certain GTypes being initialised. I remember fixing that for gtk3, so it should > be a case of back porting the fix and releasing a new 2.24 tarball. I don't disagree with that, but it shows there are apps and libraries out there that hard depend on this misfeature, which IMHO means we cannot remove it until glib 4.
(In reply to comment #12) > (In reply to comment #11) > > The gtk2 warning is gtk2 brain damage: it installs setting properties from > > various class_init instead of using GtkSettings.class_init(); it should > > actually be fixed regardless of this warning, as it makes settings depend on > > certain GTypes being initialised. I remember fixing that for gtk3, so it should > > be a case of back porting the fix and releasing a new 2.24 tarball. > > I don't disagree with that, but it shows there are apps and libraries out there > that hard depend on this misfeature, which IMHO means we cannot remove it until > glib 4. we may very well *never* have a GLib API major bump (be it 3.0 or 4.0), given the grief implied in such a thing. I also very much doubt people are actually relying on this dubious "feature" (which as it is lies in the grey area of undocumented behaviour). the code in gtk2 literally introduced bugs and weird behaviour (requiring classes to be created in order to query settings). I fully appreciate the fact that there may be code out there doing this kind of crazy crack with the type system, but I also think that long term maintainability and performance are much more critical than preserving undocumented/undefined behaviour.
random thought, and I don't know how well this plays with your "nuke GParamSpecPool" plan, but... we can sort of tell if a program is old and crufty and not keeping up to date with modern APIs, based on whether or not it manually calls g_type_init(). So could we make it so that old code gets to keep the old behavior, and only new code is forced into the new behavior?
(In reply to comment #14) > random thought, and I don't know how well this plays with your "nuke > GParamSpecPool" plan, but... we can sort of tell if a program is old and crufty > and not keeping up to date with modern APIs, based on whether or not it > manually calls g_type_init(). So could we make it so that old code gets to keep > the old behavior, and only new code is forced into the new behavior? In that plan then, we keep the old code paths forever? If that's not too much pain, it seems like a kind of reasonable hack. We definitely need to try hard to support people using a brand new glib with old versions of gtk and applications, as much as reasonable. Anyways, let's expand the reports here into proper bugs.
The problem with this approach is that we already told people to stop calling g_type_init(). We can't introduce new restrictions on what that means now... It also doesn't solve the problem of old libraries being used from new programs. I'm not sure about keeping the old codepaths around for a long time.... with interfaces, people sort of fixed their stuff and the world moved on. C++ and C# bindings are the last case there, and they're expected to be done during this cycle. Meanwhile, distros tend to update these things more or less in sync with each other, so although we don't explicitly say "you can't install a new glib unless you also take the new pygobject", everyone is already doing this anyway... I think this is not too dissimilar: there are a few cases of problems that this is causing. We need to fix them and move on... That said, I'm not about to declare a hard cut-off until I'm sure that it won't be a problem. This is exactly why I want the critical here -- we know that there are some problems in the wild already... this has convinced me that there are probably a few more (3rd party or closed source apps?) that we don't know about... We need to make sure we keep getting this information.
(In reply to comment #16) > Meanwhile, distros tend to update these things more or less in > sync with each other, In Red Hat Enterprise Linux, we definitely have cases where we want to run incredibly ancient code against brand new glib. I don't always expect that to work perfectly, but even small incompatibilities can magnify into a lot of pain fixing up apps that aren't worth much engineering time. > so although we don't explicitly say "you can't install a > new glib unless you also take the new pygobject", everyone is already doing > this anyway... I'd say bindings are expected to be a bit more special, yes. gobject-introspection is certainly hard-tied to a specific GLib version. > That said, I'm not about to declare a hard cut-off until I'm sure that it won't > be a problem. This is exactly why I want the critical here -- we know that > there are some problems in the wild already... this has convinced me that there > are probably a few more (3rd party or closed source apps?) that we don't know > about... We need to make sure we keep getting this information. Yeah, I understand that. I guess though I'd like to be in a world where we make a coordinated change to "gnome" (glib + bindings + core apps), and then once that works, we work on the rest of the ecosystem.
the ability to define custom Gtkmm widgets with Glib::Properties depends upon being able to register properties after class init. C++ has no introspection, so it is not possible for the code that calls g_type_register_static to know what properties need to be registered. By the time Glib::PropertyBase::install_property() is called (as the class member property is created), class init has already been done. This does not affect Gtkmm itself, because all of its Glib::Properties are actually just wrappers around existing Gtk/Glib-level properties. But removing this will completely prevent custom widgets with new properties without a significant API and ABI change in gtkmm, I think.
I'm registering another "please revert this" on behalf of my coworkers who've already filed two different bugs against our entirely gtkmm product asking how to shut up this spew. Can we at least change it so that release builds don't have this spew?
(In reply to comment #19) > Can we at least change it so that release builds don't have this spew? This is really absolutely missing the point. That output is there specifically because we want to hear about people who are having problems. I'm starting to get the impression that this might be one that we won't be able to get away with (and may just not be worth it in the long run). I'm thinking that a complete revert may be in order.
A few of the developers for Inkscape, which uses gtkmm, are quite annoyed, since there is no way to fix this with out "C-ifying" everything to be able to control the timing of class_init(). As it is currently, classes derived from Gtk classes cannot control the timing of the constructor initialization lists we give: ImageToggler::ImageToggler(char const* on, char const* off) : Glib::ObjectBase(typeid(ImageToggler)), Gtk::CellRendererPixbuf(), _pixOnName(on), _pixOffName(off), _property_active(*this, "active", false), _property_activatable(*this, "activatable", true), _property_pixbuf_on(*this, "pixbuf_on", Glib::RefPtr<Gdk::Pixbuf>(0)), _property_pixbuf_off(*this, "pixbuf_off", Glib::RefPtr<Gdk::Pixbuf>(0)) and class_init() will /always/ finish by the time the properties are registered: (inkscape:19755): GLib-GObject-WARNING **: Attempt to add property gtkmm__CustomObject_N8Inkscape2UI6Widget12ImageTogglerE::active after class was initialised ... etc. Also note that according to Google(tm), gtkmm was not the only thing affected by this change: http://stackoverflow.com/questions/20708657/python-gtk-how-to-suppress-warnings#comment31525797_20920122 $./helloworld.py ./helloworld.py:52: Warning: Attempt to add property GtkSettings::gtk-button-images after class was initialised self.button = gtk.Button("Hello World") ./helloworld.py:52: Warning: Attempt to add property GtkSettings::gtk-label-select-on-focus after class was initialised self.button = gtk.Button("Hello World") Seriously, please revert this. Or change everything else to make it work correctly.
Another vote for completely reverting this change. The requirement of installing all properties before class_init makes the implementation of custom properties on C++ side essentially impossible. In our proprietary applications we have several important use-cases which depend on the ability to define custom C++ properties and can't be emulated in a reasonable way. After the revelation of the possibility that the properties may be gone in the future, we're seriously considering moving away from Gtkmm (and thus GTK+) for new code. It's a pity, since I like Gtkmm quite a lot. Some clarity about the fate of the creation of GObject properties after init would certainly help much. Thanks!
OK.
Created attachment 278055 [details] [review] Revert "GObject: prevent installing properties after init" This reverts commit ddb0ce14215cd62c7a2497d6cf9f2ea63c40ebb5. Conflicts: gobject/gobject.c
Created attachment 278056 [details] [review] docs: warn about installing properties after init Leave ourselves a little wiggle room: if people install properties after initialisation then we reserve the right to handle that in a way that may not be threadsafe.
Attachment 278055 [details] pushed as 85e9455 - Revert "GObject: prevent installing properties after init" Attachment 278056 [details] pushed as f2f66bf - docs: warn about installing properties after init
Many thanks, Ryan.