GNOME Bugzilla – Bug 687659
drop support for adding interfaces after class_init
Last modified: 2018-05-24 14:48:01 UTC
We want to drop the (unused?) feature for adding interfaces to classes after the class_init has run. Step 1: make sure nobody is using it.
Created attachment 228138 [details] [review] gtype: disallow adding interfaces after the fact Add a check to prevent adding an interface to a class that has already had its class_init done. This is an incompatible change but it is suspected that there are not many users of this functionality. Once we confirm that nobody is using this functionality we can remove a rather large amount of code for dealing with this case.
fyi, http://git.gnome.org/browse/libsoup/commit/?id=d10215f
Attachment 228138 [details] pushed as d6a075b - gtype: disallow adding interfaces after the fact
gjs uses this to implement subclassing GObjects: http://git.gnome.org/browse/gjs/tree/modules/overrides/GObject.js?id=06c1aa9eb7f65c8e3cb57f579980a1b84ba961f3#n66 I'm not sure how much work it will be to statically analyze the given class and add the interfaces statically.
Dia is using this as well, see bug 694025 for the analysis. Given that the DiaGdkRenderer would have to be deprecated for gtk-3-0 anyway, maybe I should start thinking about deprecating GObject for Dia as well ;)
This has broken any custom interface implementation using gtkmm, such as custom GtkTreeModels. Can this please at least be just a warning until we figure out how, if at all, we can fix this? This seems to have got into the stable glib 2.36.0 release, which is a disaster for gtkmm apps that use custom TreeModels, such as Glom.
Here is a typical backtrace at that warning to help gtkmm people understand what is happening:
+ Trace 231709
.NET bindings are also affected by this regression: https://bugzilla.xamarin.com/show_bug.cgi?id=11510
If this is not going to be reverted, there's a docs bug, as [1] mentions this limitation, but [2] doesn't. [1] https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-interface-install-property [2] https://developer.gnome.org/gobject/stable/gobject-Type-Information.html#g-type-add-interface-static
This looks pretty much impossible to do in gtkmm without breaking API and ABI.
Created attachment 240609 [details] [review] Proposed patch (In reply to comment #0) > Step 1: make sure nobody is using it. So, Ryan: what happened with Step 1? (In reply to comment #6) > Can this please at least be just a warning until we figure out > how, if at all, we can fix this? I just implemented Murray's suggestion in this patch. Please review. If you want to further discuss this, there is "prior art", for example: https://developer.gnome.org/glib/2.30/glib-compiling.html, quote: "To help with the transition, the enforcement is not turned on by default..." So, helping with transitions is a GoodThing(TM). Just saying...
Ryan has posted a proposed fix, but in the wrong bug report. I think he's aware that we are already looking for a permanent solution. This is his post: https://bugzilla.gnome.org/show_bug.cgi?id=697229#c5
(In reply to comment #12) > Ryan has posted a proposed fix, but in the wrong bug report. I think he's > aware that we are already looking for a permanent solution. This is his post: > > https://bugzilla.gnome.org/show_bug.cgi?id=697229#c5 Just checked out that patch and is GlibMM specific, when this is affecting more consumers.
Created attachment 240621 [details] [review] gtype: interface-after-init exception for gtk# C# also has a problem with the new interface-after-init restriction that nobody noticed until now. Add an exception for them as well so that they have a cycle or so to sort things out.
Comment on attachment 240621 [details] [review] gtype: interface-after-init exception for gtk# Looks good, just nitpick about the commit msg: it would be ".NET" (many languages), not just "C#". Thanks
Comment on attachment 240621 [details] [review] gtype: interface-after-init exception for gtk# Attachment 240621 [details] pushed as 96f7e6d - gtype: interface-after-init exception for gtk#
Ryan, I've found a problem implementing a fix for this in gtk# bindings, and I think it is potentially a chicken&egg problem with GObject interfaces, please correct me if I'm wrong. It boils down to the following question: when the GInterface includes properties, g_type_add_interface_static expects that, when calling the interface_init function of the GInterfaceInfo, this sets them up via g_object_class_override_property, however this function receives a GObjectClass (not a GType), which AFAIU can only be obtained via g_type_class_ref (gtype), which in turn calls class_init! If I'm right that this is certainly a chicken&egg problem, isn't this bug requesting a restriction that is impossible to meet if the GInterface to implement needs properties? More details in https://bugzilla.xamarin.com/show_bug.cgi?id=11510#c3 Thanks
There is not currently a restriction on not adding properties after g_type_class_ref()... only interfaces, and all of those should be added from the _get_type() function, which is called before class_init. The class_init and interface init functions are not called until later, when the class is actually being initialised. This doesn't happen when you call _get_type() -- you must actually call g_type_class_ref(). It's currently possible that iface_init functions get called (long) after class_init. This happens now when adding an interface to a class that has already been initialised -- you have to setup the iface immediately since some instances could already exist. Once we have completely removed the possibility of addition of interfaces after g_type_class_ref() returns then we will also lose this possibility that g_type_add_interface_static() will have to do the 'work' immediately. This means that the class_init() and all iface init functions will be called at the same time and all properties will be setup at this time. After that's locked down (with no exceptions) I intend to remove the ability to add properties later (ie: after g_type_class_ref() has returned). This removal of the possibility that g_type_add_interface_static() might have to init the interface itself is really the gist of why this work is important. When this could be called at any stage during class initialisation, from any thread, the state tracking for "who is responsible for calling base_init on this iface vtable?" and "who is responsible for calling init on this iface vtable?" is rather complex and requires a lot of locking. In short, setting up a class should look like this: Step 1: _get_type() function: - register the type with GType - add all interfaces from here ... at this point we have GType, but no GTypeClass ... ... someone calls g_type_class_ref() ... Step 2: class/iface_init functions: - fill in vtables - add properties ... now we have GTypeClass.
TL;DR: by the time you're in an iface_init function, someone has _already_ called g_type_class_ref(), so no harm doing it again yourself.
Thanks for your prompt response Ryan. (In reply to comment #18) > There is not currently a restriction on not adding properties after > g_type_class_ref()... Just did a proof of concept patch [1] based on this feedback, and this is not the behaviour that I'm seeing. In particular, after I added the interfaces, but before adding the properties of those interfaces, I call g_type_class_ref() and I'm getting these CRITICALs when doing it: (custom-scrollable:12345): GLib-GObject-CRITICAL **: Object class __gtksharp_1_CustomScrollableWidget+601+5b+5bSystem_String+2c+20mscorlib+2c+20Version+3d2_0_0_0+2c+20Culture+3dneutral+2c+20PublicKeyToken+3db77a5c561934e089+5d+5d doesn't implement property 'vscroll-policy' from interface 'GtkScrollable' (custom-scrollable:12345): GLib-GObject-CRITICAL **: Object class __gtksharp_1_CustomScrollableWidget+601+5b+5bSystem_String+2c+20mscorlib+2c+20Version+3d2_0_0_0+2c+20Culture+3dneutral+2c+20PublicKeyToken+3db77a5c561934e089+5d+5d doesn't implement property 'vadjustment' from interface 'GtkScrollable' So I'm wondering: do you mean that this restriction has been removed in newer glib, or that it has never been there? If it's the former, it would make sense, because I'm testing now with v.2.34, but then we would need different code to register interfaces in gtk# depending on the glib version used, which will get really hairy :( If you want that we stop spamming this bug, you may want to CC yourself in the bugzilla.xamarin bug? Thanks [1] https://bugzilla.xamarin.com/attachment.cgi?id=3761
Replying to myself: I think I know now what the problem is: - Calling g_object_class_override_property before class_init is impossible. - Calling g_object_class_override_property after class_init is impossible. - Therefore I need to call g_object_class_override_property inside class_init. I got to this conclusion after seeing this example: https://git.gnome.org/browse/gtk+/tree/gtk/gtkviewport.c I'll try this approach and report back.
(In reply to comment #21) > Replying to myself: I think I know now what the problem is: > > - Calling g_object_class_override_property before class_init is impossible. > - Calling g_object_class_override_property after class_init is impossible. > - Therefore I need to call g_object_class_override_property inside class_init. > This may be the example since every class I know does it this way, but I'd be surprised to find out that it _must_ be class_init and could not be one of the iface_init functions... I guess iface_init is only for setting up the vtable, then.
Could we have a glib 2.36.1 release, please, so that glibmm can do a release that depends on it?
Ryan, do you still plan to disable this again in git master for glib 2.37/38? If you are going to do it, sooner would be better than later, please.
It's already disabled except for the exceptions I added for C# and C++. Are you asking me to remove the C++ exception?
Ryan, I understood that you would disable it even for C++ (glibmm/gtkmm) during this release cycle. I just want to know if you still plan to do that, please.
I do plan to do that and I told you that I'd wait until you were ready. Does this mean that you're ready?
No, we are still not found a way to adapt to the change without breaking our own ABI, and we still don't expect to. I'm just asking because, if you had decided not to do it in this cycle, we could start making glibmm releases for this cycle with confidence that they would ever be able to reach stable .0 releases.
So you would like for the exception to remain for one more cycle? That's fine with me. I'd want to remove it at the start of next cycle, though (ie: GLib 2.39.1).
OK. Thanks for extending our reprieve.
(In reply to comment #18) > In short, setting up a class should look like this: > > Step 1: _get_type() function: > > - register the type with GType > - add all interfaces from here > > ... at this point we have GType, but no GTypeClass ... > > ... someone calls g_type_class_ref() ... > > Step 2: class/iface_init functions: > > - fill in vtables > - add properties > > ... now we have GTypeClass. I run into this bug while invastigating how to implement optimization for Vala (bug #706712). One of the ways I've discovered is to add interface during base_init run (see attach proof-of-concept to bug). Short look on the code indicated that the code could be removed also when the interface adding is during base_init as, IIUC, it deals in case when the interfaces are not yet added. Would it be problematic if the interface could be added before base_init instead of class_init (I've filled bug #706998 separately)?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/624.