GNOME Bugzilla – Bug 675504
Fix up GObject interface documentation
Last modified: 2012-07-09 19:00:51 UTC
Document how to override interfaces already implemented in a base class, and also call those base class implementations from a derived reimplementation. Don't recomend people use base_init() style functions to initialize interface signals and properties, use default_init() aka class_init() instead (as G_DEFINE_INTERFACE() uses). The above solves the interface init called multiple times problem, so remove some needless naysaying about that. Document default_init() in the interface initialization discussion And more cleanup to the interface tutorial.
Created attachment 213489 [details] [review] Fix up GObject interface documentation * Document how to override interfaces already implemented in a base class, and also call those base class implementations from a derived reimplementation. * Don't recomend people use base_init() style functions to initialize interface signals and properties, use default_init() aka class_init() instead (as G_DEFINE_INTERFACE() uses). * The above solves the interface init called multiple times problem, so remove some needless naysaying about that. * Document default_init() in the interface initialization discussion * Linkify more stuff. * Remove some crud and typos
... as requested a while ago by Matthias on IRC, with a bit more cleanup thrown in :)
Review of attachment 213489 [details] [review]: thanks for the patch. the gobject tutorial is in dire need to being completely rewritten. ;-) a couple of comments follow. ::: docs/reference/gobject/tut_gtype.xml @@ +814,3 @@ + It is thus recommended to use a <function>default_init</function> function to + add signals and properties to an interface. This function is called only once + for the interface. The <function>default_init</function> function iss declared "is declared" @@ +815,3 @@ + add signals and properties to an interface. This function is called only once + for the interface. The <function>default_init</function> function iss declared + by <function>G_DEFINE_INTERFACE</function> or you can do that yourself in the you can use %G_DEFINE_INTERFACE to link to the macro. ::: docs/reference/gobject/tut_howto.xml @@ +1268,3 @@ + MamanIbazInterface *base_iface = g_type_interface_peek_parent (iface); + iface->do_action = maman_derived_ibaz_do_action; + <para>In this example MamanDerivedBaz is derived from MamanBaz. Both strictly speaking, doing: iface->do_something = base_iface->do_something is not needed - given that the interface vtable will be initialized by the parent type first, exactly like as it happens with classes. the peek_parent() is only needed for chaining up. I do wonder if G_IMPLEMENT_INTERFACE ought to define a parent_iface global, like G_DEFINE_TYPE does for parent_class.
Created attachment 213508 [details] [review] Updated: Fix up GObject interface documentation Thanks. Fixed up for comments. As far as a parent_iface global, I guess it would be NULL most of the time? But since it wouldn't be visible perhaps that's not a big deal.
Comment on attachment 213508 [details] [review] Updated: Fix up GObject interface documentation >+ When having no special requirements you also can use the >+ <link linkend="G-DEFINE-INTERFACE:CAPS">G_DEFINE_INTERFACE</link> macro: "When having" isn't great English. (Yeah, I know, it's what was there before, but while you're here...) Also, the section before this one should probably mention G_IMPLEMENT_INTERFACE? >+ It is thus recommended to use a <function>default_init</function> function to >+ add signals and properties to an interface. I don't think we need to call out signals and properties here any more; we just recommend you do *everything* in default_init. Also, you don't explain at what point default_init is called. >+ This function is called only once >+ for the interface. Which is correct, but the paragraph before this one gets it wrong. (In the sentence which starts "It is important to understand that...". Apparently not *that* important.) Also, "most-derived" doesn't make sense in the context of interfaces, since you can't "subclass" an interface type, so it should be removed wherever it appears in discussions of interfaces. And... the section needs even more rewriting, because it only talks about the case of registering a type "A" that implements interface "Z", and doesn't explain what happens in subclasses of A that don't reimplement "Z" (in which case Z's base_init overrides A's interface_init, which is *the* key point to understand about the difference between the two functions). So, the first paragraph should say something like "which implements an interface (either directly or by inheriting an implementation from a superclass)" rather than "which registered an interface implementation". And the base_init-and-interface_init paragraph needs to be split up to make it clear that base_init is always called, but interface_init is only called if the type has registered its own implementation of the interface. >+ maman_ibaz_default_init, >+ NULL, /* class_finalize */ >+ NULL, /* class_data */ s/class/default/ >+maman_ibaz_default_init (gpointer g_iface) >+{ >+ /* add properties and signals here, will only called once */ /* Add instance properties and signals, and set default implementations * of the interface's virtual methods. */ Also, the parameter should be "MamanIBazInterface *ibaz_iface", for clarity. >+ <entry>interface's <function>base_init</function> function</entry> >+ <entry>On interface's vtable</entry> >+ <entry>Rarely necessary to use this. May be called multiple times.</entry> "May be called multiple times" isn't correct in the context of registering a single type. >+ <entry>interface's <function>default_init</function> function</entry> >+ <entry>On interface's vtable</entry> >+ <entry>Register interface's signals and properties here. Will be called once.</entry> And set up default vmethod implementations. And also, this is backwards; default_init() is called before base_init(). Table should probably be something like: First call to g_type_create_instance for <em>any</em> type implementing interface: default_init First call to g_type_create_instance for <em>each</em> type implementing interface: base_init interface_init >-GType maman_bar_get_type (void); >+GType maman_bar_get_type (void) G_GNUC_CONST; If we're going to start officially recommending this, we should explain how it's wrong too (it makes it impossible to call the get_type function *just* for its side effects). (And we should land bug 605976...) >+ <listitem><para>The interface methods <function>maman_ibaz_do_action</function> >+ and <function>maman_ibaz_do_something</function> dereference the class >+ structure to access its associated class function and call it. s/class/interface/g >+ /* add properties and signals to the interface here */ and set default virtual method implementations >+ <!-- Ha ha! "nothing wierd or scary". I actually laughed out loud. Oh boy. "weird" :) >+ The fact that we're so intimate with GObject that all this doesn't look >+ wierd, that's the scary thing. :) --> > There is clearly nothing specifically weird or scary about this header: > it does not define any weird API or derives from a weird type. "or derive" >+ is the GType of either an interface or a class. In this case the >+ the MamanIbaz interface is a prerequisite of the MamanIbar. The code s/the the MamanIbaz/the MamanIbaz/ >- small change: it must declare the properties of the interface it >+ small change: it can declare the properties of the interface it > implements using <function><link linkend="g-object-class-override-property">g_object_class_override_property</link></function> > instead of <function><link linkend="g-object-class-install-property">g_object_class_install_property</link></function>. Isn't "must" correct? >+ <title>Overriding interface methods</title> >+ >+ <para>If a base class already implements an interface, and in a derived newline after <para> (likewise elsewhere) >+ class you wish to implement the same interface and override certain >+ methods of that interface, you can reimplement the interface and use >+ the methods from the base class implementation in the interface >+ initialization function. needs to be rewritten since you're not actually explicitly using the methods from the base class any more >+ <para>To access the base class interface implementation use >+ <function><link linkend="g-type-interface-peek-parent">g_type_interface_peek_parent</link></function> >+ from within an interface's <function>default_init</function> function. >+ </para> this doesn't belong here any more >+ <para>In this example MamanDerivedBaz is derived from MamanBaz. Both >+ implement the MamanIBaz interface. MamanDerivedBaz only implements one "MamanIBaz"? Ha ha. You must have somehow failed to notice that tut_gobject.xml and tut_gtype.xml gratuitously use different capitalization. It's "MamanIbaz" here. >+static void >+maman_derived_ibaz_interface_init (MamanIbazInterface *iface) >+{ >+ iface->do_action = maman_derived_ibaz_do_action; >+} you need to set maman_ibaz_parent_interface here too
(In reply to comment #3) > I do wonder if G_IMPLEMENT_INTERFACE ought to define a parent_iface global, > like G_DEFINE_TYPE does for parent_class. It can't, because it's expanded inside the get_type() function, not at global scope.
(In reply to comment #5) > (From update of attachment 213508 [details] [review]) > >+ When having no special requirements you also can use the > >+ <link linkend="G-DEFINE-INTERFACE:CAPS">G_DEFINE_INTERFACE</link> macro: > > "When having" isn't great English. (Yeah, I know, it's what was there before, > but while you're here...) Fixed > Also, the section before this one should probably > mention G_IMPLEMENT_INTERFACE? Done. I really wasn't intending to do too much to tut_gtype.xml. I really just wanted to get rid of the bad advice to use base_init() and the static boolean variable. But anyway, slipped this in. > >+ It is thus recommended to use a <function>default_init</function> function to > >+ add signals and properties to an interface. > > I don't think we need to call out signals and properties here any more; we just > recommend you do *everything* in default_init. Also, you don't explain at what > point default_init is called. Done. > > >+ This function is called only once > >+ for the interface. > > Which is correct, but the paragraph before this one gets it wrong. (In the > sentence which starts "It is important to understand that...". Apparently not > *that* important.) Also, "most-derived" doesn't make sense in the context of > interfaces, since you can't "subclass" an interface type, so it should be > removed wherever it appears in discussions of interfaces. Done. > And... the section needs even more rewriting, because it only talks about the > case of registering a type "A" that implements interface "Z", and doesn't > explain what happens in subclasses of A that don't reimplement "Z" (in which > case Z's base_init overrides A's interface_init, which is *the* key point to > understand about the difference between the two functions). So, the first > paragraph should say something like "which implements an interface (either > directly or by inheriting an implementation from a superclass)" rather than > "which registered an interface implementation". And the > base_init-and-interface_init paragraph needs to be split up to make it clear > that base_init is always called, but interface_init is only called if the type > has registered its own implementation of the interface. Done and done. > >+ maman_ibaz_default_init, > >+ NULL, /* class_finalize */ > >+ NULL, /* class_data */ > > s/class/default/ Those reflect the field names of GTypeInfo. So 'class_xxx' is correct. > >+maman_ibaz_default_init (gpointer g_iface) > >+{ > >+ /* add properties and signals here, will only called once */ > > /* Add instance properties and signals, and set default implementations > * of the interface's virtual methods. > */ > > Also, the parameter should be "MamanIBazInterface *ibaz_iface", for clarity. Fixed. > >+ <entry>interface's <function>base_init</function> function</entry> > >+ <entry>On interface's vtable</entry> > >+ <entry>Rarely necessary to use this. May be called multiple times.</entry> > > "May be called multiple times" isn't correct in the context of registering a > single type. > > >+ <entry>interface's <function>default_init</function> function</entry> > >+ <entry>On interface's vtable</entry> > >+ <entry>Register interface's signals and properties here. Will be called once.</entry> > > And set up default vmethod implementations. Added 'etc.'. We don't talk about default virtual method implementations here. > And also, this is backwards; default_init() is called before base_init(). That's not what I see. > Table should probably be something like: > > First call to g_type_create_instance for <em>any</em> type implementing > interface: > default_init > > First call to g_type_create_instance for <em>each</em> type implementing > interface: > base_init > interface_init Done. But in correct order. > >-GType maman_bar_get_type (void); > >+GType maman_bar_get_type (void) G_GNUC_CONST; > > If we're going to start officially recommending this, we should explain how > it's wrong too (it makes it impossible to call the get_type function *just* for > its side effects). (And we should land bug 605976...) Removed it. > >+ <listitem><para>The interface methods <function>maman_ibaz_do_action</function> > >+ and <function>maman_ibaz_do_something</function> dereference the class > >+ structure to access its associated class function and call it. > > s/class/interface/g Fixed. > >+ /* add properties and signals to the interface here */ > > and set default virtual method implementations > > >+ <!-- Ha ha! "nothing wierd or scary". I actually laughed out loud. Oh boy. > > "weird" :) > > >+ The fact that we're so intimate with GObject that all this doesn't look > >+ wierd, that's the scary thing. :) --> > > There is clearly nothing specifically weird or scary about this header: > > it does not define any weird API or derives from a weird type. > > "or derive" > > >+ is the GType of either an interface or a class. In this case the > >+ the MamanIbaz interface is a prerequisite of the MamanIbar. The code > > s/the the MamanIbaz/the MamanIbaz/ All fixed. > > >- small change: it must declare the properties of the interface it > >+ small change: it can declare the properties of the interface it > > implements using <function><link linkend="g-object-class-override-property">g_object_class_override_property</link></function> > > instead of <function><link linkend="g-object-class-install-property">g_object_class_install_property</link></function>. > > Isn't "must" correct? No you can use g_object_class_install_property as long as the property you define is similar enough. For example the param flags can't be more restrictive than the interface property and so on. > >+ <title>Overriding interface methods</title> > >+ > >+ <para>If a base class already implements an interface, and in a derived > > newline after <para> (likewise elsewhere) > > >+ class you wish to implement the same interface and override certain > >+ methods of that interface, you can reimplement the interface and use > >+ the methods from the base class implementation in the interface > >+ initialization function. > > needs to be rewritten since you're not actually explicitly using the methods > from the base class any more Fixed. > >+ <para>To access the base class interface implementation use > >+ <function><link linkend="g-type-interface-peek-parent">g_type_interface_peek_parent</link></function> > >+ from within an interface's <function>default_init</function> function. > >+ </para> > > this doesn't belong here any more > > >+ <para>In this example MamanDerivedBaz is derived from MamanBaz. Both > >+ implement the MamanIBaz interface. MamanDerivedBaz only implements one > > "MamanIBaz"? Ha ha. You must have somehow failed to notice that tut_gobject.xml > and tut_gtype.xml gratuitously use different capitalization. It's "MamanIbaz" > here. :) > >+static void > >+maman_derived_ibaz_interface_init (MamanIbazInterface *iface) > >+{ > >+ iface->do_action = maman_derived_ibaz_do_action; > >+} > > you need to set maman_ibaz_parent_interface here too Wooooooo. The end.
Created attachment 213701 [details] [review] Fixed for dan's comments: Fix up GObject interface documentation * Document how to override interfaces already implemented in a base class, and also call those base class implementations from a derived reimplementation. * Don't recomend people use base_init() style functions to initialize interface signals and properties, use default_init() aka class_init() instead (as G_DEFINE_INTERFACE() uses). * The above solves the interface init called multiple times problem, so remove some needless naysaying about that. * Document default_init() in the interface initialization discussion * Linkify more stuff. * Remove some crud and typos
(In reply to comment #7) > > >+ maman_ibaz_default_init, > > >+ NULL, /* class_finalize */ > > >+ NULL, /* class_data */ > > > > s/class/default/ > > Those reflect the field names of GTypeInfo. So 'class_xxx' is correct. Well, and the first field is called "class_init", but we're calling the function we put there "default_init" because it's going to be copied into IFaceData.dflt_init later. So calling the other two default_finalize and default_data is consistent, even if it's wrong. :) Either way is fine I guess. > > And also, this is backwards; default_init() is called before base_init(). > > That's not what I see. Ah, indeed, I must have temporarily confused default_init() and interface_init().
(In reply to comment #9) > (In reply to comment #7) > > > >+ maman_ibaz_default_init, > > > >+ NULL, /* class_finalize */ > > > >+ NULL, /* class_data */ > > > > > > s/class/default/ > > > > Those reflect the field names of GTypeInfo. So 'class_xxx' is correct. > > Well, and the first field is called "class_init", but we're calling the > function we put there "default_init" because it's going to be copied into > IFaceData.dflt_init later. So calling the other two default_finalize and > default_data is consistent, even if it's wrong. :) > > Either way is fine I guess. To help with consistency I added /* class_init */ on the line with the maman_ibaz_default_init argument. So ... good to commit?