[Bug 21594] Implement being a Handler

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=21594





--- Comment #4 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-05-18 06:09:33 PST ---
(In reply to comment #3)
> Very slight review so far:
> 
> AbstractClientApprover:
> /* TODO add more methods */
This will be fixed when we implement Approver support. For now we only have
Handler and Observer support.

> What's with the comments in ChannelRequest::Private::Private?
> +        QSet<uint>() << 0,                                           //
> makesSenseForStatuses
> +        Features(),                                                  //
> dependsOnFeatures
> +        QStringList(),                                               //
> dependsOnInterfaces
These comment are the meaning of the params used by ReadinessHelper class, it
means the ChannelRequest introspection core feature does not depend on any
other feature or interface to be introspected.

> I can't see how you're supposed to construct a ChannelRequest without having
> its immutable properties. It doesn't start readying itself, and there's no
> constructor that lets you omit the properties. Maybe this is just incomplete?
Actually you can, in some places you just receive the ChannelRequest object
path, for example Handler.HandleChannels or Handler.RemoveRequest.
In order for the user to read it's properties it should call
ChannelRequest::becomeReady that will return a PendingReady operation that will
emit finished when the object is ready to use.
In some cases where you already have the immutableProperties some methods will
return the proper property value when called, but there is no guarantee about
that.

> +ClientHandlerAdaptor::ClientHandlerAdaptor(const QDBusConnection &bus,
> +        AbstractClientHandler *client,
> +        QObject *parent)
> +    : QDBusAbstractAdaptor(parent),
> +      mBus(bus),
> +      mClient(client)
> +{
> +    QList<ClientHandlerAdaptor *> &handlerAdaptors =
> +        mAdaptorsForConnection[qMakePair(mBus.name(), mBus.baseService())];
> +    handlerAdaptors.append(this);
> +}
> 
> + * One way to create a ClientRegistrar object is to just call the create
> method.
> + * For example:
> + *
> + * \code ClientRegistrarPtr cr = ClientRegistrar::create(); \endcode
> + *
> + * You can also provide a D-Bus connection as a QDBusConnection:
> + *
> + * \code ClientRegistrarPtr cr =
> ClientRegistrar::create(QDBusConnection::sessionBus()); \endcode
> 
> Which bus does create() use? It should be documented.
It uses QDBusConnection::sessionBus. It's documented on the create method
documentation.

 * Create a new client registrar object using QDBusConnection::sessionBus().

> +    QString busName = QLatin1String("org.freedesktop.Telepathy.Client.");
> 
> it's not latin 1! it's ascii! And just below that a QString is appended. Is
> that right?
If Qt is not compiled without ascii support you will not be able to assign a
ascii string to a QString, so this is the proper way to do it in Qt world
>From Qt docs:

"Applications that define QT_NO_CAST_FROM_ASCII (as explained in the QString
documentation) don't have access to QString's const char * API. To provide an
efficient way of specifying constant Latin-1 strings, Qt provides the
QLatin1String, which is just a very thin wrapper around a const char *."

> +    // export o.f,T,Client interface
> 
> those commas should be dots.
Fixed :)


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