[Bug 28367] Add StreamTube interface support

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=28367

--- Comment #11 from Dario Freddi <drf54321 at gmail.com> 2010-08-02 12:01:53 PDT ---
(In reply to comment #10)
> Here follows a quick review, further review will be done after these "issues"
> are handled:
> 
> General notes:
> - On documentation use the constants such as 
> TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE instead of using the string literal.

Roger that

> - Indicate on documentation that method X requires FeatureX, see other docs for
> example. The account class documentation is very complete

Yup

> - \return instead of \returns

Yup

> - Remove __k__ and similar from all methods, vars, etc.

Hm, I thought I already did that, I probably forgot something on my way. Ok
though.

> - Use the include directives as done in other classes, meaning that all
> included 
> are at the top of the file, following a certain order, check other classes as 
> reference.

Will do

> - Remove the connectNotify stuff, we don't do that in any other class. The user 
> should know what he is doing and the docs should state which features the
> signals
> depend on.

Ok - although this was an experimental thing I agreed to do with Olli - maybe
we should investigate in the future on using it everywhere?

> - Do not use Q_PRIVATE_SLOT, just do as done in other class using "private
> Q_SLOTS"

Ok

> - Use include QtSomething instead of QtFoo/QtSomething

Ok

> - Never use public vars, add accessors for all members that may be accessed.

Hmm, are there any public vars around? If that's the case it's probably a
mistake.

> - Do not use Q_D, Q_Q, we may change that in the future but for now we don't
> use 
> it anywhere, so let's just stick to mPriv for standardization

Here comes a problem - to simplify some things, I added inheritance between
some private classes. This is possible in a decent way only by using
Q_DECLARE_PRIVATE and Q_D. Q_Q can indeed be removed, but I cannot say the same
for Q_D without reworking stuff a bit. Anyway, if you're keen on standardizing
stuff, it should just be a matter of exposing some more stuff in the public
classes as protected methods - then Q_D can safely go away.

> - Methods should be declared first, then vars.

Ok

> - Try to initialize mPriv members inside mPriv constructor instead of doing
> mPriv->foo = bla inside class constructor.

This is done for the very same reason as stated above: when inheriting from
private classes, initialization of member variables has to be done after the
construction phase.

> 
> On TubeChannel::Private constructor, when defining a introspectable, make it 
> like this:
>     ReadinessHelper::Introspectable introspectableTube(
>         QSet<uint>() << 0,                                           //
> makesSenseForStatuses
>         Features() << Channel::FeatureCore,                          //
> dependsOnFeatures (core)
>         QStringList() << TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE, //
> dependsOnInterfaces
>         (ReadinessHelper::IntrospectFunc) &Private::introspectTube,
>         this);
> 
> Doing this there is no need to check if the interface is present on 
> Private::introspectTube. Just do:
>     Client::ChannelInterfaceTubeInterface *tubeInterface = 
>         parent->tubeInterface(BypassInterfaceCheck);

Ok - thanks for the info.

> 
> +                   
> QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER)),
> This should be tube instead of ft.

Whooops.

Anyway - I'll be away for one week from tomorrow. As soon as I will be back,
I'll fix the obvious issues (the only controversial one is the Q_D one) and
push again.

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