[Bug 21594] Implement being a Handler

[bugzilla-daemon at freedesktop.org]

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





--- Comment #7 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-05-19 05:28:39 PST ---
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?


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