[Bug 29606] Reduce number of needed Account becomeReady calls

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Olli Salli <ollisal at gmail.com> 2010-09-02 11:29:17 PDT ---
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.

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