Bug 29973 (TpClientChannelFac.)

I don't like A) because when the user get the channel object I think it should be ready to use.

B) and C) could work but B) as the small advantage of being usable even when we need an object right away. We can always prepare it later if needed. But I'm not sure what are the use cases for that?

Also, what would do TpDefaultChannelFactory exactly? Only create TpChannel object (no subclass) and prepare TP_CHANNEL_FEATURE_CORE?

Let's say we have high level object for text channel and stream tube. We'll have a TpTextChannelFactory object creating TpTextChannel objects and preparing TP_CHANNEL_TEXT_FEATURE_CORE and another factory TpStreamTubeChannelFactory creating TpStreamTubeChannel objects and preparing TP_CHANNEL_STREAM_TUBE_FEATURE_CORE ?

Then, if I'm a tube application handling stream tube and text channels, does that mean I'll have to defiine my factory implementing the TpClientChannelFactory interface creating and preparing both kind of channels (using TpStreamTubeChannelFactory and TpTextChannelFactory to do so) ?

See also Bug #29451 for general discussion on this topic. Adding Olli to cc here because he's largely been driving the design in Qt-land.

(In reply to comment #1)

There is also an option D), which is the same as A) plus, in pseudo-template notation:

list<GQuark> tp_client_channel_factory_get_desired_features (factory);

(where list could mean GArray or 0-terminated-C-array or something)

> I don't like A) because when the user get the channel object I think it should
> be ready to use.

... although TpBaseClient prepares features separately anyway, and it's my main target use-case right now.

If we implement A) now, we can easily turn it into B) or D) afterwards, so that would be my vote.

The denizens of #introspection tell me that increasing the size of a GInterface is considered to be a compatible change, so we can always add more methods, as long as there is something sensible that can be done if an implementation of the interface has no implementation for the method. A prepare_async method's default implementation could just be "succeed in an idle" or "prepare the core feature", and a get_desired_features method could just return the core feature, for instance.

(Olli: FYI, prepare_async is telepathy-glib's equivalent of becomeReady.)

> B) and C) could work but B) as the small advantage of being usable even when we
> need an object right away. We can always prepare it later if needed. But I'm
> not sure what are the use cases for that?

For a Channel, not a whole lot; for a Connection, since one of the features is "become connected", yes we do potentially care about unprepared objects.

(In reply to comment #2)
> Also, what would do TpDefaultChannelFactory exactly? Only create TpChannel
> object (no subclass) and prepare TP_CHANNEL_FEATURE_CORE?

It would be entirely reasonable to have a TpBasicChannelFactory which always creates a TpChannel with CORE, and a TpDefaultChannelFactory which creates the most-derived subclass in telepathy-glib, and prepares whatever feature is most sensible (initially the same as the Basic version, but switching, say, stream tubes to be TpStreamTubeChannel would be a compatible change).

Synonyms for Basic, if you don't like it: Core, Minimal.

Synonyms for Default, if you don't like that (or if Basic might in fact be the default): Normal, Conventional, Automatic.

This would avoid potential layering problems where the channel factory must "know about" every TpChannel subclass in telepathy-glib, by putting the GInterface at a layer just above TpChannel itself, but the default implementation at a layer much higher than that (above all the subclasses).

> creating TpStreamTubeChannel objects and preparing
> TP_CHANNEL_STREAM_TUBE_FEATURE_CORE ?

This argument matters more for Text channels and preparing GLib equivalents of Tp::TextChannel::FeatureMessageQueue (the incoming message queue) and Tp::TextChannel::FeatureMessageSending (the outgoing message capabilities), which we could consider conflating into one feature.

It's not actually very relevant for stream tubes, since I think Danielle intended for stream tubes to be somewhat stateless, and for Offer() and Accept() to be orthogonal async methods which are not features (they have side-effects and need extra information, so they can't usefully be features, AIUI). If someone other than the handler has changed the stream tube's state then (a) they're wrong, and (b) the async methods will just fail (which should probably invalidate the channel as a side-effect, since it's no longer useful).

FYI, my most recent design for factory API in TpQt4 is (without the Qt/C++ subleties):

/*
 * Actually, most SharedPtr<DBusProxy> is really currently SharedPtr<RefCounted> because of totally bonged class hierarchy, will change in API/ABI break
 */

class DBusFactory
{
protected non-virtual methods:
    SharedPtr<DBusProxy> cachedProxy(busName, objectPath)
    {
        // Return proxy from the cache or NULL if not present
    }

    PendingReady *nowHaveProxy(proxy)
    {
        // Lots of dynamic casts and asserts etc because the current class hierarchy sucks

        Features specificFeatures = featuresFor(proxy) // Will be fixed for Acc/Conn, but variable for Channel, see later
        if (proxy not in cache) {
            // Put to cache, make sure it's removed from cache if invalidated or destroyed in user code ie. weak pointer and invalidated() signal connection
        }

        if (prepare(proxy) != NULL && not yet completed the prepare of for this instance)
            return PendingReady which first waits for the prepare() op to complete, then makes the specificFeatures ready, if any
        else
            return PendingReady which just makes specificFeatures ready, if any - if no features are set, not even Core is made ready, which is of benefit to eg. account editing UIs which probably don't want to make Connections ready although they're tampering with their respective Connections
    }

virtual methods:
    Features featuresFor(proxy) = 0
    PendingOperation *prepare(proxy) // By default, returns NULL PendingOp, this is for future expansion / application-specific "always perform this" actions only - obviously we could do such actions in the class constructors themself if we wanted
}

class FixedFeatureFactory : DBusProxyFactory
{
public non-virtual methods:
    Features features()

    addFeature(feature)
    addFeatures(features)

    // By default, no features, not even Core

virtual implementations:
    featuresFor(proxy) {
        UNUSED(proxy)
        return set features
    }

    // No prepare()
}

class AccountFactory : FixedFeatureFactory
{
    PendingReady *proxy(busName, objectPath, connFactory, chanFactory)
    {
        SharedPtr<DBusProxy> proxy = cachedProxy(busName, objectPath)
        if (!proxy)
            proxy = Account::create(busName, objectPath, connFactory, chanFactory)

        return nowHaveProxy(proxy)
    }
}

class ConnectionFactory : FixedFeatureFactory
{
    PendingReady *proxy(busName, objectPath, chanFactory) // Note that actually chanFactory won't be used much at all in tp-qt4 Connection, but still there for direct ensureChannel / createChannel and other misc
    {
        // Very similar to AccountFactory::proxy()
    }
}

class ChannelFactory : DBusProxyFactory
{
    // To enable channel class -specific subclasses
    void setClassCtorFunction(QVariantMap channelClass, Ctor ctor)
    // Ctor is essentially any function like ChannelPtr ctor(connectionProxy, channelObjectPath, immutableProperties) ie. what TextChannel::create, etc. are like anyway - so no need for any templated glue-generating magic (although that'd be possible too if we want it at some stage)

    // To enable channel class -specific features
    Features classFeatures(QVariantMap channelClass)
    void addClassFeatures(QVariantMap channelClass, Features features)

    PendingReady *proxy(connectionProxy, channelObjectPath, immutableProperties)
    {
        // Similar to Acc/ConnFactory proxy() but uses immutableProperties-specific construction
    }

    ChannelFactoryPtr stockFreshFactory()
    {
         return ChannelFactory which constructs Channel subclasses provided in TpQt4 proper (hence "stock") but makes no features ready on them (equivalent to the old pre-factory behavior)
    }

virtual implementations:
    Features featuresFor(proxy)
    {
        // Digs the immutable properties out of the proxy, and returns channel class -specific features accordingly
    }
}

etc.

where PendingReady is a class like

class PendingReady
{
    // Well because the current object hierarchy is bonged, this is SharedPtr<RefCounted> until the API/ABI break, which sucks
    SharedPtr<DBusProxy> proxy()

signals:
    finished(this)
}

So you have the option of extracting the proxy() as soon as you get the PendingReady out of the factory, but also can delay doing that until finished(which is when the set features and prepare op are both finished). So this makes it most like A) of your options. FWIW I think it's important, if only for library internal use, that one can get to the proxy even before it's fully ready.

(In reply to comment #3)
> There is also an option D), which is the same as A) plus, in pseudo-template
> notation:
> 
> list<GQuark> tp_client_channel_factory_get_desired_features (factory);

Then it's up to the user of the factories (TpBaseClient for example) to prepare the features specified by the factory?


> > I don't like A) because when the user get the channel object I think it should
> > be ready to use.
> 
> ... although TpBaseClient prepares features separately anyway, and it's my main
> target use-case right now.

Good point.

> If we implement A) now, we can easily turn it into B) or D) afterwards, so that
> would be my vote.

Make sense. I started to implement this.

> > B) and C) could work but B) as the small advantage of being usable even when we
> > need an object right away. We can always prepare it later if needed. But I'm
> > not sure what are the use cases for that?
> 
> For a Channel, not a whole lot; for a Connection, since one of the features is
> "become connected", yes we do potentially care about unprepared objects.

Right. Should we have a general interface to create channel and connection (TpClientFactory?) or define a TpClientConnectionFactory later? An object can easily implement both interfaces if needed.

Oth, if interfaces defaults to tp_channel_new_from_properties() and tp_connection_new() we could have a single interface and it won't be an issue if only one head is actually implemented by the objecT.

I guess the tp-qt4 design is a bit overkill for tp-glib, right?

> (In reply to comment #2)
> > Also, what would do TpDefaultChannelFactory exactly? Only create TpChannel
> > object (no subclass) and prepare TP_CHANNEL_FEATURE_CORE?
> 
> It would be entirely reasonable to have a TpBasicChannelFactory which always
> creates a TpChannel with CORE, and a TpDefaultChannelFactory which creates the
> most-derived subclass in telepathy-glib, and prepares whatever feature is most
> sensible (initially the same as the Basic version, but switching, say, stream
> tubes to be TpStreamTubeChannel would be a compatible change).
> 
> Synonyms for Basic, if you don't like it: Core, Minimal.

I like this idea. TpBasicChannelFactory and TpDefaultChannelFactory sounds good to me.

> > creating TpStreamTubeChannel objects and preparing
> > TP_CHANNEL_STREAM_TUBE_FEATURE_CORE ?
> 
> This argument matters more for Text channels and preparing GLib equivalents of
> Tp::TextChannel::FeatureMessageQueue (the incoming message queue) and
> Tp::TextChannel::FeatureMessageSending (the outgoing message capabilities),
> which we could consider conflating into one feature.

I think TpDefaultChannelFactory should those features as most client which are going to handle text channels will propably care.

(In reply to comment #5)
> (In reply to comment #3)
> > There is also an option D), which is the same as A) plus, in pseudo-template
> > notation:
> > 
> > list<GQuark> tp_client_channel_factory_get_desired_features (factory);
> 
> Then it's up to the user of the factories (TpBaseClient for example) to prepare
> the features specified by the factory?

Actually that make sense as TpBaseClient now has API to define extra channel features to prepare. This API will become less usefull once we'll have the channel factory btw.

I implemented A in
http://git.Collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-factory-29973

Not sure if TpBasicChannelFactory should be public or not.

Also, should be we have a tp_base_client_set_channel_factory() (to be called before calling _register()) or should we make it construct only?
Not sure if we want to have a  _with_am_and_channel_factory() variant for each TpSimple*.

> +GType
> +tp_client_channel_factory_get_type (void)

G_DEFINE_INTERFACE; also, please explicitly include GObject as an interface prerequisite.

TpBasicChannelFactory should either move to a -internal.h header and have all its methods be non-ABI (underscore-prefixed), or be documented. I think it's reasonable for it to be public, to be honest - that makes it easier to explain what the default behaviour is, if it's the default.

(In reply to comment #7)
> Also, should be we have a tp_base_client_set_channel_factory() (to be called
> before calling _register())

Yes, I think so.

(In reply to comment #5)
> Right. Should we have a general interface to create channel and connection
> (TpClientFactory?) or define a TpClientConnectionFactory later? An object can
> easily implement both interfaces if needed.

My vote would be one GInterface per thing being manufactured. We could consider renaming TpBasicChannelFactory to TpBasicObjectFactory and making it manufacture all of them, though :-)

(Adding more GInterfaces to TpBasicObjectFactory is not an ABI break, obviously)

> Oth, if interfaces defaults to tp_channel_new_from_properties() and
> tp_connection_new() we could have a single interface and it won't be an issue
> if only one head is actually implemented by the objecT.

I don't feel entirely happy about this, but I can't quite articulate why... let's both think about it some more.

> I like this idea. TpBasicChannelFactory and TpDefaultChannelFactory sounds good
> to me.

... unless we (might) default to the basic/minimal version anywhere, in which case replace Default with Automatic or something :-P

http://git.Collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-factory-29973
has been rebased

(In reply to comment #8)
> > +GType
> > +tp_client_channel_factory_get_type (void)
> 
> G_DEFINE_INTERFACE; also, please explicitly include GObject as an interface
> prerequisite.

done.

> TpBasicChannelFactory should either move to a -internal.h header and have all
> its methods be non-ABI (underscore-prefixed), or be documented. I think it's
> reasonable for it to be public, to be honest - that makes it easier to explain
> what the default behaviour is, if it's the default.

It's now public.

> (In reply to comment #7)
> > Also, should be we have a tp_base_client_set_channel_factory() (to be called
> > before calling _register())
> 
> Yes, I think so.

done.

> (In reply to comment #5)
> > Right. Should we have a general interface to create channel and connection
> > (TpClientFactory?) or define a TpClientConnectionFactory later? An object can
> > easily implement both interfaces if needed.
> 
> My vote would be one GInterface per thing being manufactured. We could consider
> renaming TpBasicChannelFactory to TpBasicObjectFactory and making it
> manufacture all of them, though :-)

Make sense. I didn't rename it but can easily if we follow this path.

> (Adding more GInterfaces to TpBasicObjectFactory is not an ABI break,
> obviously)
> 
> > Oth, if interfaces defaults to tp_channel_new_from_properties() and
> > tp_connection_new() we could have a single interface and it won't be an issue
> > if only one head is actually implemented by the objecT.
> 
> I don't feel entirely happy about this, but I can't quite articulate why...
> let's both think about it some more.

Right, one interface per proxy type and a BasicObjectFactory implementing each interface sounds good.

> > I like this idea. TpBasicChannelFactory and TpDefaultChannelFactory sounds good
> > to me.
> 
> ... unless we (might) default to the basic/minimal version anywhere, in which
> case replace Default with Automatic or something :-P

We now have TpBasicChannelFactory and TpAutomaticChannelFactory.

Marking as an Empathy 3.0 blocker (bug #30178) as we'll need it to properly support the Call API: https://bugzilla.gnome.org/show_bug.cgi?id=629903

Ok, so my remaning questions at this stage are:

Should I go for B ( tp_client_channel_factory_prepare_channel_async) or D (
tp_client_channel_factory_get_desired_features ()).

Are we happy with TpBasicChannelFactory and TpAutomaticChannelFactory as names? Should we rename TpBasicChannelFactory to TpBasicObjectFactory?

Should TpBaseClient default to TpBasicChannelFactory or TpAutomaticChannelFactory ?

(In reply to comment #11)
> Ok, so my remaning questions at this stage are:
> 
> Should I go for B ( tp_client_channel_factory_prepare_channel_async) or D (
> tp_client_channel_factory_get_desired_features ()).

Either way we're going to need an async call that can potentially fail if any channel preparation is going to take place.

> Are we happy with TpBasicChannelFactory and TpAutomaticChannelFactory as names?
> Should we rename TpBasicChannelFactory to TpBasicObjectFactory?

ObjectFactory is wrong, since it can't create any object. ProxyFactory?

(In reply to comment #12)
> > Are we happy with TpBasicChannelFactory and TpAutomaticChannelFactory as names?
> > Should we rename TpBasicChannelFactory to TpBasicObjectFactory?
> 
> ObjectFactory is wrong, since it can't create any object. ProxyFactory?

Make sense. I renamed it to TpBasicProxyFactory

(In reply to comment #11)
> Ok, so my remaning questions at this stage are:
> 
> Should I go for B ( tp_client_channel_factory_prepare_channel_async) or D (
> tp_client_channel_factory_get_desired_features ()).

Re-reading this bug, I think my intention with (D) was probably that users of the factory (TpBaseClient, etc.) would asynchronously prepare the indicated features before telling anyone about the channel. Sorry if that wasn't clear.

(B) is slightly more API, and makes TpBaseClient's current feature handling entirely redundant (TpBaseClient would end up having to do two async calls per object, one on the factory and one on the object, until we break API) but it's more flexible if we want the possibility of doing async actions that aren't actually preparing features.

You don't necessarily need to implement either of those to have a useful object, I don't think...

> Are we happy with TpBasicChannelFactory and TpAutomaticChannelFactory as names?
> Should we rename TpBasicChannelFactory to TpBasicObjectFactory?

I'd be happy with TpBasicProxyFactory, and either TpAutomaticChannelFactory or TpAutomaticProxyFactory depending which direction you think you'll go with it.

> Should TpBaseClient default to TpBasicChannelFactory or
> TpAutomaticChannelFactory ?

TpAutomaticChannelFactory, I think.

(In reply to comment #14)
> (In reply to comment #11)
> > Ok, so my remaning questions at this stage are:
> > 
> > Should I go for B ( tp_client_channel_factory_prepare_channel_async) or D (
> > tp_client_channel_factory_get_desired_features ()).
> 
> Re-reading this bug, I think my intention with (D) was probably that users of
> the factory (TpBaseClient, etc.) would asynchronously prepare the indicated
> features before telling anyone about the channel. Sorry if that wasn't clear.
> 
> (B) is slightly more API, and makes TpBaseClient's current feature handling
> entirely redundant (TpBaseClient would end up having to do two async calls per
> object, one on the factory and one on the object, until we break API) but it's
> more flexible if we want the possibility of doing async actions that aren't
> actually preparing features.
> 
> You don't necessarily need to implement either of those to have a useful
> object, I don't think...

Indeed. Let's do that once we'll have a TpChannel sub-class introducing new features.

> > Are we happy with TpBasicChannelFactory and TpAutomaticChannelFactory as names?
> > Should we rename TpBasicChannelFactory to TpBasicObjectFactory?
> 
> I'd be happy with TpBasicProxyFactory, and either TpAutomaticChannelFactory or
> TpAutomaticProxyFactory depending which direction you think you'll go with it.

Atm I have TpBasicProxyFactory and TpAutomaticChannelFactory.
I guess it makes sense (if only to be symetric) to rename it to TpAutomaticProxyFactory ?

> > Should TpBaseClient default to TpBasicChannelFactory or
> > TpAutomaticChannelFactory ?
> 
> TpAutomaticChannelFactory, I think.

done.

I think this branch is ready for review, we just have to decide of the name of the automatic factory.

http://git.Collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-factory-29973

(In reply to comment #15)
> > I'd be happy with TpBasicProxyFactory, and either TpAutomaticChannelFactory or
> > TpAutomaticProxyFactory depending which direction you think you'll go with it.
> 
> Atm I have TpBasicProxyFactory and TpAutomaticChannelFactory.
> I guess it makes sense (if only to be symetric) to rename it to
> TpAutomaticProxyFactory ?

What I was getting at with "depending which direction" is: do you expect that TpAutomaticThingFactory will gain functionality to do automatic manufacture for all object types, or just channels?

I'd tend to vote for "all known object types" (so you'd call it ProxyFactory and it'll keep gaining GInterfaces over time), but perhaps you can see a reason not to?

> http://git.Collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/channel-factory-29973

Queued...

Makes sense; I renamed it.

> +tp_base_client_set_channel_factory
> +tp_base_client_get_channel_factory

Hmm. Does this mean that if you want to have control over both TpChannel and, say, TpChannelDispatchOperation, you're going to have to call both set_channel_factory and set_cdo_factory?

> + * The current version of #TpAutomaticProxyFactory guarantees to create the
> + * following objects:
> + *  - if channel is of type TP_IFACE_CHANNEL_TYPE_STREAM_TUBE, a
> + *  #TpStreamTubeChannel
> + *  - for all the other channel types, a #TpChannel

The constant should have a %.

This is a little vague about what happens now vs. what we guarantee in future. Perhaps this:

    #TpAutomaticProxyFactory will currently create #TpChannel objects
    as follows:

    - a #TpStreamTubeChannel, if the channel is of type
      %TP_IFACE_CHANNEL_TYPE_STREAM_TUBE;
    - a plain #TpChannel, otherwise

    It is guaranteed that the objects returned by future versions
    will be either the class that is currently used, or a more specific
    subclass of that class.

    TpProxy subclasses other than TpChannel are not currently supported.

Should we have a tp_automatic_proxy_factory_dup()? I expect it'll be quite commonly-used.

I think there should be a "see also" link to #TpBasicProxyFactory, and vice versa.

TpBasicProxyFactory should indicate its intended future behaviour:

    This factory implements the #TpClientChannelFactory interface to create
    plain #TpChannel objects. Unlike #TpAutomaticProxyFactory, it will
    not create higher-level subclasses like #TpStreamTubeChannel.

    TpProxy subclasses other than TpChannel are not currently supported.

> +struct _TpClientChannelFactoryInterface {
> +    GTypeInterface parent;
> ...
> +    /*<private>*/
> +    GCallback _padding[7];

The GObject people tell me padding isn't actually necessary for a GInterface, because they're assumed to be variable-size.

>  #include <telepathy-glib/stream-tube-channel.h>
>  #include <telepathy-glib/stream-tube-connection.h>
> +#include <telepathy-glib/client-channel-factory.h>
> +#include <telepathy-glib/automatic-proxy-factory.h>
> +#include <telepathy-glib/basic-proxy-factory.h>

I'd prefer these in alphabetical order.

(In reply to comment #18)
> > +tp_base_client_set_channel_factory
> > +tp_base_client_get_channel_factory
> 
> Hmm. Does this mean that if you want to have control over both TpChannel and,
> say, TpChannelDispatchOperation, you're going to have to call both
> set_channel_factory and set_cdo_factory?

Humm, we could have a set_proxy_factory that will be used for all proxy creation in TpBaseClient. But that implies that if we add more type of proxy later, we'll have to check that the factory implement the new interface before using it (which is fine). set_factory will take a GObject * (or gpointer?) as argument.

If you think it's fine, then I'm happy to do the change. I think it makes sense actually, custom factory can easily implement more than one interface if needed.

> > + * The current version of #TpAutomaticProxyFactory guarantees to create the
> > + * following objects:
> > + *  - if channel is of type TP_IFACE_CHANNEL_TYPE_STREAM_TUBE, a
> > + *  #TpStreamTubeChannel
> > + *  - for all the other channel types, a #TpChannel
> 
> The constant should have a %.

fixed.

> This is a little vague about what happens now vs. what we guarantee in future.
> Perhaps this:
> 
>     #TpAutomaticProxyFactory will currently create #TpChannel objects
>     as follows:
> 
>     - a #TpStreamTubeChannel, if the channel is of type
>       %TP_IFACE_CHANNEL_TYPE_STREAM_TUBE;
>     - a plain #TpChannel, otherwise
> 
>     It is guaranteed that the objects returned by future versions
>     will be either the class that is currently used, or a more specific
>     subclass of that class.
> 
>     TpProxy subclasses other than TpChannel are not currently supported.

updated.

> Should we have a tp_automatic_proxy_factory_dup()? I expect it'll be quite
> commonly-used.

Should I replace _new() by _dup() and so define this object as a singleton? I guess that's ok

> I think there should be a "see also" link to #TpBasicProxyFactory, and vice
> versa.

done.

> TpBasicProxyFactory should indicate its intended future behaviour:
> 
>     This factory implements the #TpClientChannelFactory interface to create
>     plain #TpChannel objects. Unlike #TpAutomaticProxyFactory, it will
>     not create higher-level subclasses like #TpStreamTubeChannel.
> 
>     TpProxy subclasses other than TpChannel are not currently supported.

done.

> > +struct _TpClientChannelFactoryInterface {
> > +    GTypeInterface parent;
> > ...
> > +    /*<private>*/
> > +    GCallback _padding[7];
> 
> The GObject people tell me padding isn't actually necessary for a GInterface,
> because they're assumed to be variable-size.

removed.

> >  #include <telepathy-glib/stream-tube-channel.h>
> >  #include <telepathy-glib/stream-tube-connection.h>
> > +#include <telepathy-glib/client-channel-factory.h>
> > +#include <telepathy-glib/automatic-proxy-factory.h>
> > +#include <telepathy-glib/basic-proxy-factory.h>
> 
> I'd prefer these in alphabetical order.

sorted.

(In reply to comment #19)
> > Hmm. Does this mean that if you want to have control over both TpChannel and,
> > say, TpChannelDispatchOperation, you're going to have to call both
> > set_channel_factory and set_cdo_factory?
> 
> Humm, we could have a set_proxy_factory that will be used for all proxy
> creation in TpBaseClient. But that implies that if we add more type of proxy
> later, we'll have to check that the factory implement the new interface before
> using it (which is fine). set_factory will take a GObject * (or gpointer?) as
> argument.

If you can think of a reasonably nice way to do this right now, go for it!

If you can't right now, we could always deprecate set_channel_factory later, in favour of a future set_proxy_factory.

> > Should we have a tp_automatic_proxy_factory_dup()? I expect it'll be quite
> > commonly-used.
> 
> Should I replace _new() by _dup() and so define this object as a singleton? I
> guess that's ok

Only if you don't think it'll ever be useful to subclass TpAutomaticProxyFactory. I think subclassability plus a _dup() convenience function is probably more useful here than the singleton-in-constructed() pattern, tbh?

One issue I can think of which isn't relevant for Channel, but is for some of the other proxies, is: should the proxy factory have a TpDBusDaemon property? I'd actually be inclined to say the answer is "no, all the methods should either take a TpDBusDaemon, or a 'smaller' object like TpAccountManager, as an argument".

(In reply to comment #20)
> (In reply to comment #19)
> > > Hmm. Does this mean that if you want to have control over both TpChannel and,
> > > say, TpChannelDispatchOperation, you're going to have to call both
> > > set_channel_factory and set_cdo_factory?
> > 
> > Humm, we could have a set_proxy_factory that will be used for all proxy
> > creation in TpBaseClient. But that implies that if we add more type of proxy
> > later, we'll have to check that the factory implement the new interface before
> > using it (which is fine). set_factory will take a GObject * (or gpointer?) as
> > argument.
> 
> If you can think of a reasonably nice way to do this right now, go for it!
> 
> If you can't right now, we could always deprecate set_channel_factory later, in
> favour of a future set_proxy_factory.

I agreed on IRC, I keept the current API.

> > > Should we have a tp_automatic_proxy_factory_dup()? I expect it'll be quite
> > > commonly-used.
> > 
> > Should I replace _new() by _dup() and so define this object as a singleton? I
> > guess that's ok
> 
> Only if you don't think it'll ever be useful to subclass
> TpAutomaticProxyFactory. I think subclassability plus a _dup() convenience
> function is probably more useful here than the singleton-in-constructed()
> pattern, tbh?

Agreed, I added _dup() functions.

> One issue I can think of which isn't relevant for Channel, but is for some of
> the other proxies, is: should the proxy factory have a TpDBusDaemon property?
> I'd actually be inclined to say the answer is "no, all the methods should
> either take a TpDBusDaemon, or a 'smaller' object like TpAccountManager, as an
> argument".

I agree and that fits with the current implementation of proxies.

Ship it!

Merged to master. Will be in 0.13.2