Bug 21594

(In reply to comment #0)
> Andre has a branch. Some quick review comments:
> 
> * AbstractClientHandler::isListeningRequests() makes no sense grammatically and
> neither does the constructor parameter listenRequests. wantsRequestNotification
> would be better English
Fixed

> * Didn't we agree that a newly created ChannelRequest should not be trying to
> download its properties, at least in cases where the immutable properties
> aren't supplied ahead of time? Most Observers won't care what the CR's
> properties are, so they shouldn't have to make an extra round-trip for them
> 
> * When constructing unique names for Clients, note that (ulong) is not a safe
> way to cast a pointer to a unique integer on LLP64 architectures like x86-64
> Windows: perhaps cast it to intptr_t, which is a typedef for the integer type
> of the same size as a pointer (e.g. long long on x86-64 Windows)
Fixed

> * It's 2009, please don't check in new code with © 2008
Fixed
 
> * How would you register the same Client name to be an observer, an approver
> and a handler using this ClientRegistrar API? I don't see how you could...
> 
Fixed. Clients should use ClientObject and set the observer, approver and handler and then call ClientRegistrar::registerClient.

(In reply to comment #0)
> * Should the library be keeping track of the AbstractClientHandler's handled
> channels, rather than requiring the Client to do it? (Add them when
> HandleChannels accepts them, remove them when they close - to be
> spec-compliant, it would need to take the union of the handled channels for
> every Client that's sharing a unique name, see the spec for details)
Done

Very slight review so far:

AbstractClientApprover:
/* TODO add more methods */

What's with the comments in ChannelRequest::Private::Private?
+        QSet<uint>() << 0,                                           // makesSenseForStatuses
+        Features(),                                                  // dependsOnFeatures
+        QStringList(),                                               // dependsOnInterfaces

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?

+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.

+    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?

+    // export o.f,T,Client interface

those commas should be dots.

(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 :)

(In reply to comment #4)
> (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.

Okay.

> > 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.

Okay.

> > 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.

Well, the spec guarantees that when a Handler has AddRequest() called on it, the UserActionTime and the requested channel properties are included in the requested channel properties. I guess a client using this tp-qt4 API can just assume that ::userActionTime() and ::requests() will do the right thing in that situation. (Of course, they have to cope with a broken MC anyway.) Okay, this seems right.

> > 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().

So it is.

> > +    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

Okay!

More review to follow...

In ChannelRequest::Private::Private():
+    // readinessHelper->becomeReady(Features() << FeatureCore);

This should probably be deleted rather than commented out.

+ * The client registrar will be responsible for proper exporting the client
+ * D-Bus interfaces, based on inheritance.

I can't really parse this. I think I'd say “The client registrar will export the appropriate D-Bus interfaces, based on the abstract classes subclassed by \param client” or something.

MethodInvocationContext only supports returning <= 8 values? This seems like an unfortunate arbitrary limitation. Can it be raised without an ABI break?

Why is Q_DISABLE_COPY(MethodInvocationContext) in the private: section? Other classes have it as the first thing inside class ____ {

 ---

That's my review finished, up to 8c26060d417a68ca65affb73edb5828bd056dea0.

Simon's comments have all been fixed. So I guess the above four points are all that's left.

No more merge blockers from my second pass through reviewing, but these would all be nice to fix:

I'd like to see a test with the "happy path" for AddRequest - AddRequest adds a request, and HandleChannels notes that the channels satisfy it.

> --- a/TelepathyQt4/pending-operation.h
> +++ b/TelepathyQt4/pending-operation.h
> @@ -64,7 +64,7 @@ class PendingOperation : public QObject
>      Q_DISABLE_COPY(PendingOperation)
>  
>  public:
> -    virtual ~PendingOperation();
> +    ~PendingOperation();

Why? (I believe this is actually virtual anyway, because the parent class's destructor is virtual: am I right? If so it seems sensible to make this explicit)

+    static ClientRegistrarPtr create();
+    static ClientRegistrarPtr create(const QDBusConnection &bus);

Why not one method with a default argument? That'd make it clearer, I think?

In ClientRegistrar:
> +        busName.append(QString(".%1._%2")
> +                .arg(mPriv->bus.baseService()
> +                    .replace(':', '_')
> +                    .replace('.', "._"))
> +                .arg((intptr_t) client.data()));

If it's trivial to do, could we format the intptr_t in hex? That'd make it easier to debug what's going on, probably (gdb etc. never show object addresses in decimal). If it's not trivial, don't bother.

In AbstractClientHandler:
> +    virtual void addRequest(const ChannelRequestPtr &request);
> +    virtual void removeRequest(const ChannelRequestPtr &request,
> +            const QString &errorName, const QString &errorMessage);

Should these be signals?

(In reply to comment #6)
> Why is Q_DISABLE_COPY(MethodInvocationContext) in the private: section? Other
> classes have it as the first thing inside class ____ {

It's a matter of taste - the macro expands to a private copy-constructor that asserts, or some such. It includes a private: prefix, so whatever section you're in before the macro, you'll be in a private: section afterwards: as a result, it's good style to put it inside a private: section (like the one implicitly started by the beginning of the class declaration) so the implicit becoming-private has no effect.

I've been considering it to be boilerplate similar to Q_OBJECT, and so putting it at the top next to Q_OBJECT (which has a similar auto-private effect), but in the private: section at the end works equally well.

(In reply to comment #6)
> In ChannelRequest::Private::Private():
> +    // readinessHelper->becomeReady(Features() << FeatureCore);
> 
> This should probably be deleted rather than commented out.
Fixed.

> + * The client registrar will be responsible for proper exporting the client
> + * D-Bus interfaces, based on inheritance.
> 
> I can't really parse this. I think I'd say “The client registrar will export
> the appropriate D-Bus interfaces, based on the abstract classes subclassed by
> \param client” or something.
Fixed.

> MethodInvocationContext only supports returning <= 8 values? This seems like an
> unfortunate arbitrary limitation. Can it be raised without an ABI break?
This is a chosen limitation. QDBusPendingReply already supports only <=8 values, so we chose to use the same. Also 8 is a common default value used in C++ templates supporting multiple values.

> Why is Q_DISABLE_COPY(MethodInvocationContext) in the private: section? Other
> classes have it as the first thing inside class ____ {
As there are some typedefs there, I prefer to move the declaration to the private section, it looked better to me, but I can change that if you prefer.

>  ---
> 
> That's my review finished, up to 8c26060d417a68ca65affb73edb5828bd056dea0.
> 
> Simon's comments have all been fixed. So I guess the above four points are all
> that's left.
> 

(In reply to comment #9)
> (In reply to comment #6)
> > MethodInvocationContext only supports returning <= 8 values? This seems like an
> > unfortunate arbitrary limitation. Can it be raised without an ABI break?
> This is a chosen limitation. QDBusPendingReply already supports only <=8
> values, so we chose to use the same.

Oh, okay.

> > Why is Q_DISABLE_COPY(MethodInvocationContext) in the private: section? Other
> > classes have it as the first thing inside class ____ {
> As there are some typedefs there, I prefer to move the declaration to the
> private section, it looked better to me, but I can change that if you prefer.

Nope, it's fine as is; I didn't know the relevant background.

I think this is all my review comments covered!

> 
> > --- a/TelepathyQt4/pending-operation.h
> > +++ b/TelepathyQt4/pending-operation.h
> > @@ -64,7 +64,7 @@ class PendingOperation : public QObject
> >      Q_DISABLE_COPY(PendingOperation)
> >  
> >  public:
> > -    virtual ~PendingOperation();
> > +    ~PendingOperation();
> 
> Why? (I believe this is actually virtual anyway, because the parent class's
> destructor is virtual: am I right? If so it seems sensible to make this
> explicit)
Nope, that's not true, if the class itself have virtual methods, it should have
a virtual destructor. This is to make sure inherited class destructors get
called when deleting the base class.

> +    static ClientRegistrarPtr create();
> +    static ClientRegistrarPtr create(const QDBusConnection &bus);
> 
> Why not one method with a default argument? That'd make it clearer, I think?
Yeah, I thought about it, but I prefer to do it this way, as we are already
doing the same in many classes, so I believe we should do it all at once, when
we do it, to have a standardize API.

> In ClientRegistrar:
> > +        busName.append(QString(".%1._%2")
> > +                .arg(mPriv->bus.baseService()
> > +                    .replace(':', '_')
> > +                    .replace('.', "._"))
> > +                .arg((intptr_t) client.data()));
> 
> If it's trivial to do, could we format the intptr_t in hex? That'd make it
> easier to debug what's going on, probably (gdb etc. never show object addresses
> in decimal). If it's not trivial, don't bother.
Fixed.

> In AbstractClientHandler:
> > +    virtual void addRequest(const ChannelRequestPtr &request);
> > +    virtual void removeRequest(const ChannelRequestPtr &request,
> > +            const QString &errorName, const QString &errorMessage);
> 
> Should these be signals?
They can't be signals as AbstractClientHandler cannot be a QObject as we use
Multiple inheritance (MI), and there is no way to MI QObjects.

Support added on version 0.1.5