[Bug 21594] Implement being a Handler

[bugzilla-daemon at freedesktop.org]

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





--- Comment #11 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-05-19 06:24:07 PST ---
> 
> > --- 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.


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