[Bug 21594] Implement being a Handler

[bugzilla-daemon at freedesktop.org]

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





--- Comment #5 from Will Thompson <will.thompson at collabora.co.uk>  2009-05-19 04:19:45 PST ---
(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...


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