[Bug 29606] Reduce number of needed Account becomeReady calls

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29606

--- Comment #2 from Andre Moreira Magalhaes <andrunko at gmail.com> 2010-08-30 14:45:16 PDT ---
- 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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.