Created attachment 15471 [details] [review]
the patch

Hi Gustavo,

Looks interesting; the #define should be all upper case, as all
#defines ought to be (esp. those that double evaluate), the '_' prefix
is only necessary for library local symbols.

OTOH - I'm prolly being dim, but I can't see how this can cope with
the 'circular query-interface' problem; also, do you think we could
achieve the same end by adding a spoof BonoboObject that just had a
valid CORBA_Object on it (and/or type id) - to save having the memory
overhead of the AggregateITem (in addition to the other allocations in
there).

  Would it be safe to g_object_new (BONOBO_TYPE_OBJECT, NULL); then
set the objref to the foreign interface?  If we could pull that off,
that would be perfect. I'm willing to give it a try.

  About 'circular query-interface' problem, I don't think there is
one. Not in the gnome-python implementation I'm working on, at least.
 The python implementation of Bonobo::Object automatically adds all
intefaces to the BonoboObject when it is aggregated with the first
BonoboObject. It's kind of hard to explain :)  I'm going to upload the
gnome-python patch, though it is not very well tested yet...

I don't know if it would be safe to do g_object_new (foo,NULL) - and
then set the ref; we should certainly provide a wrapper method for
doing that - so we can isolate the behavior somewhere sensible ;-)
Prolly we want to add a sub-class for future reference:

struct BonoboObjectForeign {
     struct BonoboObject;
     gpointer priv;
};

and add some vtable methods for future expansion; and make it a really
thin sub-class of BonoboObject - and put the hooks there.

I'd be most happy with something like that; how does that sound ?

[ we may need to hook various internals to make this happen, but as an
outline it sounds great to me ].

Created attachment 15680 [details] [review]
BonoboForeignObject

  Ok, so now I subclassed BonoboObject (but only GObject subclassing,
not "Bonobo" subclassing).

  There is a problem with do_corba_setup, called from BonoboObject
instance init function.  I had to use a hack to prevent it from being
called with BonoboForeignObject.  Similar problem and similar
workaround regarding bonobo_object_corba_deactivate.  It is
unfortunate that we can't use g_type_is_a to check if an instance is
of class BonoboForeignObject, because during the instance
initialization function the object has no type!  This is a known and
old problem with GObject.

  I think there are no hooks required.  There is already a "destroy"
signal, to find out when the foreign object should be destroyed, and
bonobo_object_ref(), bonobo_object_unref(),
bonobo_object_query_local_interface() to implement the Unknown
interface in the foreign servant.

The class of the instance passed to the instance_init() function is
the class the instance_init() function belongs to.  So with a GObject
inheritance heirachy of A -> B -> C, it gets initialised as:
  set class of instance to A
  A.instance_init(instance)
  set class of instance to B
  B.instance_init(instance)
  set class of instance to C
  C.instance_init(instance)

This means that instances passed to A's instance_init() will always
look like instances of A.  If you want to find out what the actual
class of the instance is, you can look at the second argument to the
instance_init() function.  All you need to do is adjust the
instance_init function prototype to take an additional GObjectClass
argument.  So in the above example, A's instance init func is called as:
  A.instance_init(instance, C)

This might help you detect when initialising a BonoboForeignObject in
BonoboObject's instance_init() function.  Does that sound about right?

One other thing:  It is probably a good idea for BonoboForeignObject
to CORBA_Object_duplicate() the reference passed to its constructor. 
That would make it more consistent with other Gnome/CORBA APIs.

Hi Gustavo,

I'm much happier with this; please can you commit - modulo binning the
'IsForeignObject' test - and using James' suggestion for that, also we
do (prolly) want to CORBA_Object_duplicate internally at the API level
there.

Apart from that it looks really good; please do commit with ChangeLog
ASAP :-) [ oh, and some API docs on bonobo_foreign_object_new () would
be good ]. Also, perhaps a helper:

bonobo_foreign_object_add (BonoboObject *obj, CORBA_Object obj);

might be a useful helper ?

Great work.

  Ok, I'll CORBA_Object_duplicate, and add helper function.
  About what james says about instance_init and classes and such
stuff, I already knew that, and I'm already using that. :)  If you'll
notice, I added a boolean field to the BonoboObject class that is set
to FALSE in BonoboObject's class and TRUE in BonoboForeignObject's class.
  Now, I just realized, there is there a better way to differentiate
the class types, using g_type_is_a(G_TYPE_FROM_CLASS(klass),
BONOBO_TYPE_FOREIGN_OBJECT).  This way I will not have to touch the
BonoboObjectClass.
  I wish this stuff would be fixed in GObject once and for all.  This
feels very much like a workaround of a fundamental GObject problem. :(
  I prefer to not commit right away. It is better to do the python
stuff first, to see if this suits python's needs.  The good news is
that the python stuff becomes very simple with this; I will be
deleting a lot of code! :-)  In fact, a pure python implementation of
bonobo.Unknown is probably possible! ;)

Michael: the helper function you mention sounds a lot like a method on
BonoboObject, in which case it should probably be named as such. 
Maybe bonobo_object_add_foreign_object() or
bonobo_object_aggregate_foreign() ?

  James, I agree, but I think it should be named
bonobo_object_add_foreign_interface(), analogous to
bonobo_object_add_interface().
  I'll be working on this tonight.  I have a feeling the helper
function won't be used by gnome-python, but I'll do it anyway for
completeness.

Well ;-) you're right;

but the thing is that I want to avoid making the foreign object stuff
too promenant, in documentation, headers, etc. and forcing the header
to be included everywhere sounds dodgy to me.

But as you like - I have no particularly strong feelings here.

  Don't worry, the header doesn't have to be included.

    bonobo-object.h:
#ifdef BONOBO_PRIVATE
void bonobo_object_add_foreign_interface (BonoboObject *object,
CORBA_Object interface);
#endif

    bonobo-object.c:
#include <bonobo/bonobo-foreign-object.h>
  ...etc...

Well, you can make the function documentation go in the same section
as BonoboForeignObject pretty easily (by listing it in the right place
in the -sections.txt file).

I don't think marking the function as private makes sense.  The API is
designed to be used by things outside of libbonobo, and it seems
simple enough that maintaining it as a stable API shouldn't be a problem.

I will see if the C++ guys feel about the API, as it might be quite
useful for them as well.

I don't want to make the method private; simply discourage people from
seeing it / mis-using it horribly; it'd be great to get input from the
C++ people as well though.

  Just committed the following patch.  Michael, I didn't add a wrapper
function, because it seemed more convenient, when doing the python
stuff, to use BonoboForeignObject directly.  However, the
bonobo-foreign-object.h header file needs to be explicitly included;
it is not listed in libbonobo.h.

Created attachment 15754 [details] [review]
foreign object impl.

Created attachment 15778 [details] [review]
I screwed up :/ this a post-fix

I guess this should be closed now...