Bug 29606

Posted the current state of my WIP branch to address this up for comments (see the URL). The branch has Account & Connection factories, a DBusProxyFactory caching factory parent-class, a lot of required utility code improvements, a placeholder fixed implementation of ChannelFactory and using AccountFactory in AccountManager.

Will still have to add to the branch
 - documentation for added API
 - some finer-granularity unit tests (so that the added code has >80% line coverage, preferably >90%)
 - using AccountFactory and ConnectionFactory in other places besides AccountManager (but not yet ClientRegistrar, I think, there is bug #29609 for that)

- shared-ptr.dox
+ * (which might be be disabled by eg. the -fno-rtti flag of the GNU G++ compiler).
be be

- dbus-proxy-factory.cpp
Missing space after "namespace Tp {"

- Always declare constructors/destructors first in class definitions and then the other methods. Same for implementation. In classes that the constructors are not public, declare them first in the protected/private sectors and then the other methods, but always implement them before the methods impl. If the class has a create method declare/implement it first.
  Should we add this to HACKING?

Eg.:
  class Foo
  public:
    Foo();
    ~Foo();
    // other methods

  class Foo
  public:
    create();
    ~Foo();
    // other methods
  protected/private:
    Foo();
    // other methods

- In a Private class please first declare methods and then declare attributes. Same rule for constructors/destructors as above.

- Prefer to declare getters first and then setters as used everywhere else
  Eg. DBusProxyFactory::features() should be declared before DBusProxyFactory::addFeature(s)

- Avoid using "get" in method names unless it is really necessary (name collision for example).
  Eg.: DBusProxyFactory::getCachedProxy should be renamed to cachedProxy(). Same for getProxy.

- Telepathy/Global already includes QtGlobal, so no need to include both.

- Makefile.am
     _gen/dbus-proxy.moc.hpp \
+       _gen/dbus-proxy-factory-internal.moc.hpp \
Use 4 spaces here

- dbus-proxy-factory-internal.h
There is an extra indentation in the declaration of DBusProxyFactory::Cache, check other classes

Also the ifndef BUILDING_TPQT4 is not needed as the file is not installed, but it won't hurt, so do it as you wish.

Please make Cache a class inside DBusProxyFactory::Private and declare the private class inside dbus-proxy-factory-internal.h.

You may also want to receive DBusProxyFactory *parent in DBusProxyFactory::Private constructor and pass it as a parent to Cache, so there is no need delete the cache in the destructor.

- dbus-proxy-factory.cpp
+    connect(dynamic_cast<DBusProxy*>(obj.data()),
+            SIGNAL(invalidated(Tp::DBusProxy*,QString,QString)),
You probably want to dynamic_cast it before using connect and warn in case it returns NULL. Same applies to other places you use dynamic_cast without checking the return value. Ignore this in places that you are 100% sure it won't return NULL.

Also use spaces here, "..., QString, QString"

+    // No features requested or they are all ready - optimize a bit by not calling ReadinessHelper
+    PendingReady *readyOp = new PendingReady(mPriv->features, proxyQObject, proxyQObject);
+    readyOp->setFinished();
+    return readyOp;
Please use ReadinessHelper here if you want object->isReady() to return true when no features are requested.

- I think Account/ConnectionFactory classes deserve their own header/source file.

- Account/ConnectionFactory classes have an extra indentation also. Also implement create/constructor/destructor in this order as explained above.

- No need to add DBusProxyFactory to types.h if there is no DBusProxyFactoryPtr declaration.

- Do we really want AccountFactory::coreFactory, I would add another constructor/create that takes the features as a param, like create(bus, features). I just didn't like the coreFactory method :/

- AccountFactory::stockFreshFactory. Please don't add a new name for things like create, it will just make users more confuse IMHO, just add AccountFactory::create(bus) and create(bus, features) if needed. I know this is temporary, but in case you want to add this as public API I would rename it first.
  The current channel-factory is internal, so feel free to remove/add methods as you wish, like removing the current create method.

- The patch "Use the SharedPtr upcasting constructor everywhere" also adds specificFeaturesFor support. But no need to rebase this

- How to ensure that the factories received as param have the same busConnection as AM for example? We probably want to _at least_ warn in case they differ. Imagine someone creating an AM with sessionBus and the factories with systemBus, it won't work really.

- I would rename finalBusNameFrom to uniqueNameFrom to be consistent with what we already have in DBusProxy.

+    static AccountManagerPtr create(const ChannelFactoryConstPtr &channelFactory,
+            const ConnectionFactoryConstPtr &connectionFactory =
+                ConnectionFactory::create(QDBusConnection::sessionBus()),
+            const AccountFactoryConstPtr &accountFactory =
+                AccountFactory::coreFactory(QDBusConnection::sessionBus()));
I wouldn't add default params here, mainly to force people to actually pass the factories as param. When we break API we can think of having default factories for Core Features everywhere, but having just a channelFactory not being default now seems a bit awkward.


After seeing this, we _really_ want to make DBusProxy inherit ReadyObject and RefCounted in the API/ABI break, the whole casting all over makes the code look horrible :/.


So after seeing the code I came up with some questions that I believe that we should think about before continue doing it.

Problems that we want to solve with factories:
1 - Being able to get objects with a set of features ready in all accessors that return such an object
2 - Being able to share object instances, so for example a handler would use the same connection object as an account (in case they match) if someone is using both AM and client in the code.

Problems that I see with the factories approach:
- If someone creates a factory and passes it to AM for example and use a different factory to create the ClientRegistrar, the objects won't be shared, so we still have problem 2
- Simon pointed that some places one may want objects with certain features that are not required in other places. In order to do that we would need to use different factories which would break 2.

So I was thinking about a solution to this and came up with the following idea, correct me if I am wrong:
- We should have a global cache of objects that should be identified by dbusConnection+uniqueName+objectpath.
  To be able to cover #1 we would pass features as param in the constructor of AM, Account, Connection, ClientRegistrar, etc. The objects returned by these object would have these configured features ready.
  That would cover all situations, even the one that Simon pointed as we would reuse objects everywhere. If I have a connection with FeatureRoster ready already and someone wants to create a ClientRegistrar with the connection having the FeatureCore only, there is no problem, as it won't have to introspect Roster again (it's already there), and in case both want FeatureRoster for eg, ReadinessHelper will take care of not creating/introspecting the object again if the introspection started already.

In other words, I believe we should make factories private to the class and have global objects that should be used everywhere.

I am not sure if I made myself clear, but I believe now after looking at this, that we are trying to solve some issues and find other issues to be worried about.

I'll reply to the parts I don't agree first and then fix the things I agree with (the ones I'm not commenting on here) to give you room to respond.

(In reply to comment #2)
> In classes that the constructors
> are not public, declare them first in the protected/private sectors and then
> the other methods, but always implement them before the methods impl. If the
> class has a create method declare/implement it first.

Really? I mean really? Wouldn't it make more sense to have them in the same order as the class declaration does? I see you've changed TpQt4 to follow this arbitrary rule where they're not in the same order in other places though so I'll reorder my added classes for now, but let's discuss if we should make the implementations ordered identically to the declarations globally somewhere in the future, though.

> Please make Cache a class inside DBusProxyFactory::Private and declare the
> private class inside dbus-proxy-factory-internal.h.

Why? It'd just make the names horribly long, wouldn't it? Also, moc doesn't need to scan the Private class and I don't want to expose it to the Cache implementation anyway. It's not like having the "class Cache" forward-declaration in the public class declaration would cause any recompilation - the class body is not there after all (which COULD cause them).

Actually, OptionalInterfaceFactory keeps its entire cache class declared in the public header... I think that's a bit bad, because any change is going to cause almost a library-wide recompilation, but that's not an issue for just having a forward declaration like DBusProxyFactory::Private.

> 
> You may also want to receive DBusProxyFactory *parent in
> DBusProxyFactory::Private constructor and pass it as a parent to Cache, so
> there is no need delete the cache in the destructor.

DBusProxyFactory is not a QObject, so it can't be a parent of another QObject (and I don't want to make it one just for this).

> 
> - dbus-proxy-factory.cpp
> +    connect(dynamic_cast<DBusProxy*>(obj.data()),
> +            SIGNAL(invalidated(Tp::DBusProxy*,QString,QString)),
> You probably want to dynamic_cast it before using connect and warn in case it
> returns NULL. Same applies to other places you use dynamic_cast without
> checking the return value. Ignore this in places that you are 100% sure it
> won't return NULL.

Well, connect() already warns if you give it NULL I think. However, I did Q_ASSERT wherever I wanted to be extra sure. The code you're quoting is called from a place which already Q_ASSERTs.

Anyway, all of these dynamic casts will disappear (and be just implicit upcasts) in the API/ABI break, thankfully.

> +    // No features requested or they are all ready - optimize a bit by not
> calling ReadinessHelper
> +    PendingReady *readyOp = new PendingReady(mPriv->features, proxyQObject,
> proxyQObject);
> +    readyOp->setFinished();
> +    return readyOp;
> Please use ReadinessHelper here if you want object->isReady() to return true
> when no features are requested.
> 

The intention is that no features (not even core) means that the factory should not make the class ready ie. make no features ready. We need this a) to be able to emulate the pre-factory behavior b) for eg. account editing UIs, which probably want to pass a factory that doesn't make Connections ready to their AM instance.

So, in short, I DON'T want object->isReady() to return true.

This deserves a mention in the doc comments to come, though, obviously.

> - I think Account/ConnectionFactory classes deserve their own header/source
> file.
> 

Ok, I'll also put FixedFeatureFactory in its own then. The thing is, the division between the base class and the subclasses sort of went back and forth during development - at first, Acc/ConnFact were just implementing two virtual methods, with an oneliner implementation each!

> - Do we really want AccountFactory::coreFactory, I would add another
> constructor/create that takes the features as a param, like create(bus,
> features). I just didn't like the coreFactory method :/

The intention here (which I'll surely explain in the docs) is that coreFactory when passed to AM will produce the same behavior as AM did in the pre-factory era.

Adding a Features constructor (with a default param of none probably) is a good idea though, will do that, and make coreFactory use it. Doing this another option would be to have the AM default parameter be AccountFactory::create(QDBusConnection::sessionBus(), Account::FeatureCore) but I think as a default parameter that surely is long and convoluted to the point of being unreadable (consider all of the three factory params being like this).

I guess it could be called createWithPreFactoryBehavior or something, but that's a) convoluted b) doesn't encode what the actual behavior is. Thus the current naming which is similar to eg. QDBusConnection::sessionBus() (that's not createForSessionBus() either, is it?)

> 
> - AccountFactory::stockFreshFactory. Please don't add a new name for things
> like create, it will just make users more confuse IMHO, just add
> AccountFactory::create(bus) and create(bus, features) if needed. I know this is
> temporary, but in case you want to add this as public API I would rename it
> first.

Similarly, stockFreshFactory is *NOT* a synonym for create(). It's intended to produce the current behavior of returning the tp-qt4 Channel subclasses as appropriate, but with no features set.

In the future when I make ChannelFactory be a bit more dynamic, it will be possible to specify the subclasses to construct and features to make ready on them at runtime. Then, create() would return a empty factory with no subclasses/features set (so always just Channel), and you could build it yourself - but stockFreshFactory would still be there if one wants to start from the TpQt4 default subclasses instead (and we want that to be able to specify it as the default factory in other classes' constructors without a ridiculously convoluted sequence of like passing all the channel class fixed properties and a constructor function for them in the default parameter declaration).

I think the class has a comment just above stockFreshFactory that I don't want to declare a create() constructor yet exactly because you can't customize it anyway beyond stockFreshFactory() yet - but rest assured, create() will be added once ChannelFactory is able to act differently from stockFreshFactory(). Returning what stockFreshFactory() now returns from a create() method would put us in a situation where the future create() starts being silently incompatible, as it no longer fills the default behavior in.

>   The current channel-factory is internal, so feel free to remove/add methods
> as you wish, like removing the current create method.

I was a bit wary of doing this as it had been declared TELEPATHY_QT4_EXPORT (accidentally?). Although there are no headers installed, it's still part of the .so this way, so I'd think any binary compatibility checkers distributions / other downstream have would whine at us for removing/changing the current static ChannelFactory::create() (and we'd have to explain to each and every one of them that headers weren't installed, exporting it was accidental and nobody probably ever used it etc).

At ABI break though, sure, let's do it.

That said, I don't even want to remove the current create() yet before ChannelFactory is used in the DBusProxyFactory way everywhere in the library itself.

> - How to ensure that the factories received as param have the same
> busConnection as AM for example? We probably want to _at least_ warn in case
> they differ. Imagine someone creating an AM with sessionBus and the factories
> with systemBus, it won't work really.

QDBusConnection doesn't have a == operator so this is a bit hard to do. Maybe QDBusConnection::name() could be used, though, but is that reliable? At least it would indicate different connections to the same bus, which would otherwise work, as different - but that's something we might want to not allow anyway, though.

Also, the only reason that the DBusProxyFactory even has a bus param is that I don't want its cache key to additionally have something unique to a dbus connection, as it seems that a single factory will only ever be used for a single dbus connection anyway even if it didn't have the param.

I didn't add a sessionBus() default param to the factory constructors exactly because I feared users would forget to pass the bus to them even if they used a different bus for eg. the AM by the way.

So, what should we do? Assert on name() being the same? I'm OK with that, if that's bound to work.

> 
> - I would rename finalBusNameFrom to uniqueNameFrom to be consistent with what
> we already have in DBusProxy.
> 

My intention was that this is not necessarily the unique name. In fact, for AccountFactory it will just be the well-known name. The intention in the naming is that it should be the final name the object will have when constructed, after any normalization/uniquefying. Will document appropriately, of course. Is this acceptable?

> +    static AccountManagerPtr create(const ChannelFactoryConstPtr
> &channelFactory,
> +            const ConnectionFactoryConstPtr &connectionFactory =
> +                ConnectionFactory::create(QDBusConnection::sessionBus()),
> +            const AccountFactoryConstPtr &accountFactory =
> +                AccountFactory::coreFactory(QDBusConnection::sessionBus()));
> I wouldn't add default params here, mainly to force people to actually pass the
> factories as param. When we break API we can think of having default factories
> for Core Features everywhere, but having just a channelFactory not being
> default now seems a bit awkward.
> 

Actually, channelFactory is effectively default too. Note that we still have the old create() and create(bus) variants we have to be ABI compatible with. Remember that C++ default params work caller-side - having create(bus = defaultparam) only generates code for create(bus) and the compiler then auto-fills the default parameter in the caller code if it's not provided, for example. Here I've updated the create() and create(bus) variants to pass the default channel factory in addition to the default connection and account factories, which is exactly the same API usage wise as if channelFactory had a default param too.

Of course, in the API/ABI break, we should ditch the variants without the factory params and just have all-default parameter variants. I actually explained this in a API/ABI break TODO comment just a few lines before the declaration you quoted, didn't I?

> 
> So after seeing the code I came up with some questions that I believe that we
> should think about before continue doing it.
> 
> Problems that we want to solve with factories:
> 1 - Being able to get objects with a set of features ready in all accessors
> that return such an object
> 2 - Being able to share object instances, so for example a handler would use
> the same connection object as an account (in case they match) if someone is
> using both AM and client in the code.
> 
> Problems that I see with the factories approach:
> - If someone creates a factory and passes it to AM for example and use a
> different factory to create the ClientRegistrar, the objects won't be shared,
> so we still have problem 2

ClientRegistrar will have a constructor which takes an AM and starts using its factories, which solves this without any extra complexity. The constructor will be documented as "always use this if you're using an AM", but that's for another branch and bug #29609. Just wait for it...

> - Simon pointed that some places one may want objects with certain features
> that are not required in other places. In order to do that we would need to use
> different factories which would break 2.

Simon pointed out that this is a problem if we have library-global state, and then have uncooperative modules (eg. plugins the host application can't control) which then mess each other up.

However, the solution me and Simon agreed on in the semantics branch was not having any library-global state (even bus-wide pseudo-singletons) and instead make ClientRegistrar/AbstractClient take the AM as a constructor param. This enables those hostile plugins to each construct their own AM and ClientRegistrar with different sets of features. Note that they should not share the objects anyway if we consider them not being able to coordinate their usage of the objects sufficiently - otherwise they'd still be able to mess each other up.

Also, we agreed that anywhere a single AM/ClientRegistrar is used (a single plugin, or a host application which provides restricted access to an AM / ClientRegistrar it constructs for its plugins) we can assume they can be coordinated to not mess up the objects. And think about it a while until you say "they're going to mess up each other inevitably, we have to guard against it" :) As a bit of a straw man, that would be almost equivalent to saying that we shouldn't allow passing a QDBusConnection from eg. Account to Connection when constructing it, because the Connection may then mess up Account by disconnecting it.

> 
> So I was thinking about a solution to this and came up with the following idea,
> correct me if I am wrong:
> - We should have a global cache of objects that should be identified by
> dbusConnection+uniqueName+objectpath.

No we shouldn't, read above ^^ :)

>   To be able to cover #1 we would pass features as param in the constructor of
> AM, Account, Connection, ClientRegistrar, etc. The objects returned by these
> object would have these configured features ready.

This won't work at all, leads to constructor explosion. Consider all the possibilities about configuring ChannelFactory in the future. It needs to be configurable on a per-set-of-immutable-properties basis, and at least the subclass and the features should be configurable for each. So we'd need to add all of this configurability to ctors everywhere where an object potentially constructing a Channel is (directly or indirectly) contained. This would mean currently AccountManager, Account, Connection, PendingChannel, ClientRegistry and ChannelDispatchOperation at least.

Also, we can't even add the Channel subclass, features construction options yet because we don't have the implementation to do so yet, so we'd have to now add ctor variants which can take Account and Connection construction options, and then deprecate those and add the ones which also take the convoluted Channel construction options instead. If it will be in a factory, we can just have the placeholder non-configurable factory param in the constructors and then add configurability to the factory to be passed through that same constructor.

Consider adding yet another configurable to the picture - the prepare() virtual allows saying "I want to always groupAddSelfHandle() whenever I'm local pending in a channel that's constructed for me" in ChannelFactory or a subclass thereof. Making this kind of configuration option without factories would mean deprecating all of the then-current AccountManager, Account, Connection, PendingChannel, ClientRegistry and ChannelDispatchOperation constructors and adding variants where you can specify this. Granted, this problem is not that obvious at this stage as I've only yet made AccountManager thus configurable, and none of the objects it (recursively) constructs.

With factories, adding something like alwaysJoinWhenLocalPending is just a simple matter of adding that configuration option to ChannelFactory. As the AM would pass its ChannelFactory to Account, Account to Connection and any future requestChannelAndHandleIt machinery, etc, the adjustment would propagate to the entire object tree without any changes to any constructors, no deprecations etc - just adding a setter and an accessor to ChannelFactory.

In other words, having every proxy construction configurable contained in the factory is really good because we can just then pass the factory around without having to understand the semantics of what the user has asked from the factory. The only semantic is that "we get a PendingReady from the factory, and when it completes, we can present the object to the user in the state the user asked for".

>   That would cover all situations, even the one that Simon pointed as we would
> reuse objects everywhere. If I have a connection with FeatureRoster ready
> already and someone wants to create a ClientRegistrar with the connection
> having the FeatureCore only, there is no problem, as it won't have to
> introspect Roster again (it's already there), and in case both want
> FeatureRoster for eg, ReadinessHelper will take care of not
> creating/introspecting the object again if the introspection started already.
> 
> In other words, I believe we should make factories private to the class and
> have global objects that should be used everywhere.

Naa... :) That is actually approximately equivalent to one of the evolutions of my suggested solution approach to bug #29451 but as explained above and in the later bug #29451 comments I don't think that's a good idea anymore.

To reiterate: if the parts of an application can't be guaranteed to be able to coordinate which features it wants everywhere, it can't be guaranteed to be able to safely handle shared instances of that object either. So we should exactly share what we specify features in a shared fashion for.

Also note that the factories as passed to AM (and in the future also other proxy) ctors are ConstPtr params, and that eg. FixedFeatureFactory only ever allows adding features, not removing them. This means nobody can a) add conflicting features to make ready in an AM's factory instance accessing it through AM->accountFactory() etc and b) an application constructing the factory can't mess up 

We should of course document that the application shouldn't allow access to the non-const pointers to the factories after passing them to the AM/whatever constructor, at least not to potentially untrusted plugins. I don't think this is much of a problem though, as the use will naturally fall into the following pattern I think:

{
   AccountFactoryPtr accFact = AccountFactory::create(bus)

}

Sorry for the missing semicolons, my custom keyboard layout and opera together don't allow typing that in bugzilla comments (because it's the altgr+capslock keys on a typical PC qwerty layout :))

If you like, we can add something like isReadonly() / setReadonly() to the 

> 
> I am not sure if I made myself clear, but I believe now after looking at this,
> that we are trying to solve some issues and find other issues to be worried
> about.

Please get back to me if you still disagree after my above rationale. I'll surely add some of the rationale to the documentation even if you agree, though.

OK it seems I noobed completing some paragraphs there :P let's retry:

(In reply to comment #3)
> 
> Also note that the factories as passed to AM (and in the future also other
> proxy) ctors are ConstPtr params, and that eg. FixedFeatureFactory only ever
> allows adding features, not removing them. This means nobody can a) add
> conflicting features to make ready in an AM's factory instance accessing it
> through AM->accountFactory() etc and b) an application constructing the factory
> can't mess up 

its other parts expecting a certain set of features (and having done addFeature(s)) accordingly. Also, it's possible that the application checks the features set on a factory against a whitelist and/or a blacklist before passing them over to the AM/ClientRegistrar/whatever, if it wants - this is assuming it allows some arbitrary self-contained modules to set any features in the first place.

> 
> We should of course document that the application shouldn't allow access to the
> non-const pointers to the factories after passing them to the AM/whatever
> constructor, at least not to potentially untrusted plugins. I don't think this
> is much of a problem though, as the use will naturally fall into the following
> pattern I think:
> 
> {
>    AccountFactoryPtr accFact = AccountFactory::create(bus)
> 
> }

I mean:
private: void initAM()
{
ChannelFactoryPtr chanFact = ChannelFactory::create(bus)
// Do chanFact manipulation, whatever the API will be like

ConnectionFactoryPtr connFact = ConnectionFactory::create(bus)
connFact->addFeatures(Connection::FeatureCore | Connection::FeatureRoster)

AccountFactoryPtr accFact = AccountFactory::create(bus)
accFact->addFeature(Account::FeatureAvatar)

mPriv->am = AccountManager::create(bus, chanFact, connFact, accFact)
}

So after the end of that scope/function, the factories will go out of scope and the only way to access them is the const access through the AM.

> 
> Sorry for the missing semicolons, my custom keyboard layout and opera together
> don't allow typing that in bugzilla comments (because it's the altgr+capslock
> keys on a typical PC qwerty layout :))
> 
> If you like, we can add something like isReadonly() / makeReadonly() to the 

factories though, and make AM etc do makeReadonly() on the factories passed to them and then make the addFeature etc setters warn/assert if isReadonly().I don't think they should return a bool or anything though as it's not really a runtime error, it's a programming error. This would make it even more "secure" but maybe it's going overboard a bit already.

(In reply to comment #3)
> >   That would cover all situations, even the one that Simon pointed as we would
> > reuse objects everywhere. If I have a connection with FeatureRoster ready
> > already and someone wants to create a ClientRegistrar with the connection
> > having the FeatureCore only, there is no problem, as it won't have to
> > introspect Roster again (it's already there), and in case both want
> > FeatureRoster for eg, ReadinessHelper will take care of not
> > creating/introspecting the object again if the introspection started already.
> > 
> > In other words, I believe we should make factories private to the class and
> > have global objects that should be used everywhere.
> 
> Naa... :) That is actually approximately equivalent to one of the evolutions of
> my suggested solution approach to bug #29451 but as explained above and in the
> later bug #29451 comments I don't think that's a good idea anymore.
> 

Also, doing this only solves requesting features - it doesn't solve requesting a specific (application-configurable) (sub)class. If the objects are all in some library-global cache, what if the factory/AM/whatever for one plugin/module wants subclass SkypeStyleCallUIConnection and the one for another wants say XChatStyleIRCUIConnection - if we only have one instance, being able to request different features from it doesn't help much if one module wants it to be a SkypeStyleCallUIConnection and the other a XChatStyleIRCUIConnection.

And this is just assuming the only things ever configurable using factories will be the subclasses and the features on them...

Agreed with Andre that my above rationale indeed made the design decisions make sense. I've now fixed the ones I did agree with, though. Here's some more commentary:

(In reply to comment #2)
> - shared-ptr.dox
> + * (which might be be disabled by eg. the -fno-rtti flag of the GNU G++
> compiler).
> be be
> 

Fixed

> - dbus-proxy-factory.cpp
> Missing space after "namespace Tp {"

Fixed

> 
> - Always declare constructors/destructors first in class definitions and then
> the other methods. Same for implementation. In classes that the constructors
> are not public, declare them first in the protected/private sectors and then
> the other methods, but always implement them before the methods impl. If the
> class has a create method declare/implement it first.
>   Should we add this to HACKING?
> 
> Eg.:
>   class Foo
>   public:
>     Foo();
>     ~Foo();
>     // other methods
> 
>   class Foo
>   public:
>     create();
>     ~Foo();
>     // other methods
>   protected/private:
>     Foo();
>     // other methods

Fixed, although I'd still rather prefer the implementation order to follow the declaration order!

> 
> - In a Private class please first declare methods and then declare attributes.
> Same rule for constructors/destructors as above.

Fixed

> 
> - Prefer to declare getters first and then setters as used everywhere else
>   Eg. DBusProxyFactory::features() should be declared before
> DBusProxyFactory::addFeature(s)

Fixed

> 
> - Avoid using "get" in method names unless it is really necessary (name
> collision for example).
>   Eg.: DBusProxyFactory::getCachedProxy should be renamed to cachedProxy().
> Same for getProxy.

Fixed

> 
> - Telepathy/Global already includes QtGlobal, so no need to include both.
> 

I like to not rely on assumptions like "I include a header which PROBABLY includes Tp/Global which PROBABLY includes QtGlobal" so I left it as-is. Cpp processing include guards, making the include a no-op is almost exactly free compared to the actual C++ compilation anyway...

> - Makefile.am
>      _gen/dbus-proxy.moc.hpp \
> +       _gen/dbus-proxy-factory-internal.moc.hpp \
> Use 4 spaces here

Fixed

> 
> - dbus-proxy-factory-internal.h
> There is an extra indentation in the declaration of DBusProxyFactory::Cache,
> check other classes

Fixed

> 
> Also the ifndef BUILDING_TPQT4 is not needed as the file is not installed, but
> it won't hurt, so do it as you wish.

It's a sanity check like any other, so left as-is

> - dbus-proxy-factory.cpp
> +    connect(dynamic_cast<DBusProxy*>(obj.data()),
> +            SIGNAL(invalidated(Tp::DBusProxy*,QString,QString)),
> You probably want to dynamic_cast it before using connect and warn in case it
> returns NULL. Same applies to other places you use dynamic_cast without
> checking the return value. Ignore this in places that you are 100% sure it
> won't return NULL.

Added some Q_ASSERT when dynamic_casting as appropriate.

> 
> Also use spaces here, "..., QString, QString"

http://techbase.kde.org/Policies/Library_Code_Policy#Signal_and_Slot_Normalization, so nope. We should change the rest of the library accordingly, though, but I don't think a "change all at once" maneuver benefits much in this case (as it's not visible in public headers or anything).

> - I think Account/ConnectionFactory classes deserve their own header/source
> file.
> 

Fixed, also moved FixedFeatureFactory

> - Account/ConnectionFactory classes have an extra indentation also. Also
> implement create/constructor/destructor in this order as explained above.
> 

Fixed

> - No need to add DBusProxyFactory to types.h if there is no DBusProxyFactoryPtr
> declaration.
> 

Fixed

>   The current channel-factory is internal, so feel free to remove/add methods
> as you wish, like removing the current create method.
> 

I made ::create deprecated, private and to have friends so we won't have problems having removed it in the future because somebody started using it now that we install the header.

> - How to ensure that the factories received as param have the same
> busConnection as AM for example? We probably want to _at least_ warn in case
> they differ. Imagine someone creating an AM with sessionBus and the factories
> with systemBus, it won't work really.

I made the constructor emit a warning if the QDBusConnection name() on a factory is different from the proxy one.

Factory infrastructure and using Account/ConnectionFactory in AccountManager/Account merged to master, will be in 0.3.9.