You will have to convince rambokid that this is a good idea.

You will have to convince rambokid that this is a good idea.

Sven, can you supply a patch implementing the suggested macros?

flagging NEEDINFO until there's any further progress.

OK, comparing criawips's G_DEFINE_IFACE
(http://svn.gnome.org/viewcvs/criawips/trunk/copy-src/helpers/gobject-helpers.h?revision=712&view=markup)
and libsoup's SOUP_MAKE_INTERFACE
(http://svn.gnome.org/viewcvs/libsoup/trunk/libsoup/soup-types.h?revision=795&view=markup)
there are a few minor differences and one major one.

The major difference is that criawips sets the GTypeInfo's
"class_init", but libsoup sets its "base_init". Gtk is inconsistent
internally: GtkCellEditable, GtkEditable, GtkFileFolder,
GtkFileSystem, GtkPrintOperationPreview, GtkTreeModel, and
GtkTreeSortable use base_init. GtkFileChooser, GtkFileChooserEmbed,
and GtkRecentChooser use class_init.

The GObject docs say that interfaces should use base_init... AFAICT,
the only time there's a big difference is when you have a class that
implements an interface, and then you subclass the class and
reimplement the interface in the subclass. If the interface
initializes its vtable in base_init, then the subclass will default to
the method implementations defined by the interface. But if the
interface initializes its vtable in class_init, then the subclass will
default to the method implementations defined by its superclass.

If you don't have a subclass reimplementing one of its parent class's
interfaces, I can't see any difference between the two, besides the
fact that base_init needs an "if (!initialized) ..." check around
signal and property definitions to make sure they only run once. And
we couldn't put that check into the G_DEFINE_INTERFACE wrapper,
because if we did it would just make base_init end up working exactly
the same as class_init anyway.

So the choices seem to be:

	1. Have G_DEFINE_INTERFACE set class_init
	2. Have G_DEFINE_INTERFACE set base_init (and force the caller
	   to do an "if (!initialized)" check if necessary)
	3. Have G_DEFINE_INTERFACE set both of them, and force the
	   caller to define both init methods (one for vtable, one for
	   signals/properties).

I think #1 makes the most sense (for a general-use G_DEFINE_INTERFACE
macro). If the caller needs to do something more obscure, they can
write out the get_type method by hand.


The minor differences:

criawips expects a class structure called "FooIface", libsoup expects
one called "FooClass". Gtk uses "Iface" everywhere except
"GtkEditableClass", so I guess "Iface" is the preferred convention.
(Should we rename the foo_class_init method to foo_iface_init to
match?)

criawips requires the caller to pass in the interface's parent type.
G_TYPE_INTERFACE isn't DEEP_DERIVABLE, so we can pretty safely assume
the parent type is G_TYPE_INTERFACE.

criawips calls "g_type_interface_add_prerequisite(type, G_TYPE_OBJECT);".
(libsoup doesn't provide any way to set a prereq.) Gtk uses various different
prerequisite types on different interfaces, so that should probably be made a
parameter of the macro.

Created attachment 84726 [details] [review]
patch that defines G_DEFINE_INTERFACE as described above

*** Bug 444060 has been marked as a duplicate of this bug. ***

Patch looks good to me. Tim ?

Created attachment 102479 [details] [review]
updated patch

Updates the previous patch to use g_once_init_enter/g_once_init_leave like G_DEFINE_TYPE does now, and includes documentation. (Also updates the G_DEFINE_TYPE_WITH_CODE docs to reflect the current expansion of that macro.)

(In reply to comment #9)
> Created an attachment (id=102479) [edit]
> updated patch
> 
> Updates the previous patch to use g_once_init_enter/g_once_init_leave like
> G_DEFINE_TYPE does now, and includes documentation. (Also updates the
> G_DEFINE_TYPE_WITH_CODE docs to reflect the current expansion of that macro.)

thanks Dan, i have a few smaller issues with the patch:
1) the initializer name should be _default_init if that's the function pointer it implements.
2) "Iface" is a very wierd abbreviation, it might be ok for non-API words like variables, but for programming interfaces we should use proper nameing like "Interface¨.
3) i think we should check for TYPE_PREREQ!=0 in case the user doesn't want to use it and passes in 0.

an adapted version of your macro is in glib/gobject/tests/threadtests.c, i'd apprechiate you reviewing it. i think we can add this to gtype.h as is, unless more issues are being brought up (and when doing that, you'll be properly credited, which i forget for threadtests.c, sorry ;)

(In reply to comment #10)
> 1) the initializer name should be _default_init if that's the function pointer
> it implements.

Ok... but it's not called _default_init. It's setting the class_init pointer.

Although "_interface_init" instead of "_class_init" would make sense, and would make the initializer name match the class struct name.

> 2) "Iface" is a very wierd abbreviation

Agreed. "Interface" is better. (I was just copying existing convention.)

> 3) i think we should check for TYPE_PREREQ!=0 in case the user doesn't want to
> use it and passes in 0.

I thought about that before and figured the user could just pass G_TYPE_OBJECT in that case. But this is probably nicer.

(In reply to comment #11)
> (In reply to comment #10)
> > 1) the initializer name should be _default_init if that's the function pointer
> > it implements.
> 
> Ok... but it's not called _default_init. It's setting the class_init pointer.

right, but there is no "class" for interfaces, other than the default interface vtable that gets copied over when an interface is setup for a particular object class via GInterfaceInitFunc. so the only reason for someone to supply a function pointer in the GTypeInfo.class_init slot for interfaces would be to provide an initializer for the default interface structure (e.g. to default-initialize interface functions). i hope this makes my point clearer... ;)

> > 3) i think we should check for TYPE_PREREQ!=0 in case the user doesn't want to
> > use it and passes in 0.
> 
> I thought about that before and figured the user could just pass G_TYPE_OBJECT
> in that case. But this is probably nicer.

well, interfaces are not neccessarily bound to be implemented only for G_TYPE_OBJECT derived types (allthough that's probably the only _practical_ use currently).

Created attachment 118322 [details] [review]
updated G_DEFINE_INTERFACE patch

Incorporates the previous suggestions: Iface->Interface, class_init->default_init, allow 0 for TYPE_PREREQ. Also moves the docs inline.

Created attachment 118323 [details] [review]
patch to make gio use G_DEFINE_INTERFACE

Trying to port gio to use G_DEFINE_INTERFACE turns up some problems:

    - since they use "FooIface" in their public API, but the macro only
      handles "FooInterface", we have to add a typedef. (A #define wouldn't
      work, unless we wanted to "#define Interface Iface".) Maybe if we
      really like Interface and hate Iface we'd want to move the typedefs
      to the .h files and deprecate the Iface names.

    - Several gio interfaces define signals rather than just filling in the
      vtable, making the name "default_init" less accurate.

but it works

ping?

I tried to ping tim for a comment but he hasn't commented here, but as per comment #10 timj seems ok with commiting this. I think it looks good too, so I commited this.

I agree that default_init is not super-accurate since we also sometimes set up signals there, but this is not so critical imho, its more important to get this in so people can start using it.

(In reply to comment #10)
> 2) "Iface" is a very wierd abbreviation, it might be ok for non-API words like
> variables, but for programming interfaces we should use proper nameing like
> "Interface¨.

It might be a bit late, but as discussed on IRC with alex lately, I don't really agree on this, mostly for the sake of consistency:

1/ The current usage is to use the (maybe weird but so common) Iface suffix for interface types, at least in Gtk+ and glib, and probably in a lot of other related libraries.

2/ Along with those name types come the usual "accessor" macro (for instance G_FILE_GET_IFACE) and the eventual _IFACE cast macro. For the sake of consistency, those should probably be renamed as well, if the -iface suffix is evil.

3/ "Interface" is even longer than "Iface", which, combined with the _GET_IFACE stuff, will make the function/macro calls and iface-related code even more verbose.

4/ As stated in comment 14, using the new macro with the Interface suffix forces one to define a whole lot of new typedefs, while not being able to drop the old ones, for API compat. It's quite opposite to reducing the code base. Also some tools might be bound to the Iface names.

So for the sake of consistency and convenience, I'd rather go with Iface as a suffix than Interface.


Also, not related but still, comment 14 seems to suggest that class_init would indeed be more adapted than base_init, but to me it is useful to have a reimlementation of an interface by a child class to default to the already-implemented function, and to allow chaining to the parent's implementation of said iface function (this is to avoid having to write all the static functions just to chain up if you're overriding a method from an interface one of your parents already implemented).

A practical example for the chainup thing:

I have a PeasConfigurableIface which provides two methods: is_configurable and get_configure_dialog.

I have a PeasPlugin which implements it this way: is_configurable returns true if get_configure_dialog is not null.

Now I have python bindings which define a PeasPythonPlugin Gobject (in C) and "overrides" is_configurable to return true if there is a python method called "create_configure_dialog" in the python object, because the C object's method has been changed to the python wrapper.

What I'd like here is to make it possible for a PeasPythonPlugin subclass to re-implement PeasConfigurableIface and to see its create_configure_dialog overriden while keeping the existing behaviour on the is_configurable method. This is all for convenience, of course.

(In reply to comment #17)
> It might be a bit late

well, it's already been committed using "Interface", but it's not stable yet, so...

> Also, not related but still, comment 14 seems to suggest that class_init would
> indeed be more adapted than base_init, but to me it is useful to have a
> reimlementation of an interface by a child class to default to the
> already-implemented function, and to allow chaining to the parent's
> implementation of said iface function

That is what using class_init does, and that's what we do, though the method
is called foo_default_init, not foo_class_init, for reasons I don't understand.