[Telepathy] API Draft for high level tubes in tp-qt4

[Dario Freddi]

Hi Olli, first of all thanks for the thorough review.

On Tuesday 11 May 2010 18:36:02 Olli Salli wrote:
> On Sat, May 1, 2010 at 8:43 PM, Dario Freddi <drf54321 at gmail.com> wrote:
> > On Saturday 01 May 2010 16:19:10 Dario Freddi wrote:
> >> Hello,
> >> 
> >> I managed to implement a preliminary version of StreamTubes. There's
> >> still a critical problem in the accept phase I cannot get over and I'd
> >> like you to take a look as well. But one thing at a time.
> > 
> > Quick update: I switched using Unix sockets in IncomingTubeChannel while
> > the problem gets settled, and everything is working nicely :) I made the
> > sockets exchange some random text messages and printing them to stdout.
> > Also, to demonstrate that everything is working, the sender is using a
> > standard QTcpServer with default listening arguments, while the receiver
> > is using, as already said, a unix socket.
> 
> Finally had time to take a more thorough look at your code. Some
> observations:
> 
> Account::createStreamTube: about including TargetHandle == 0 in the
> request when contact is NULL: the spec states about TargetHandle that
> "If this is present in a channel request, it must be nonzero". So, I
> think you should return a PendingChannelRequest which fails
> immediately if the contact isn't valid. However, I realize what you
> did is what the other Account methods are doing as well - Andre, can
> you give me a heads up on why this is so? Should we just change them
> all in fact?

I agree with you they should all be changed - although, given that it should 
be a global change, I think it would be better to have my stuff merged first 
with this approach, and then change everything with a single commit - or 
otherwise have the other methods changed before the merge.

> 
> Account::create*DBusTube: please comment them out as long as they
> aren't actually implemented ;) (That is, if a merge is planned at this
> point). Also, I think "SingleDBusTube" could be clearer as
> "P2PDBusTube", as that's the terminology we've otherwise used.

Oops :D they actually should not be there, sorry. I will push -f later when 
I'll fix all the other things you pointed out.

> 
> TubeChannel: I think it indeed makes sense to not have a separate
> TubeState feature (which you've commented out already), as any user of
> a tube channel will be interested in the state. As a side note, the
> warning output in tubeState() actually says FeatureChatState must be
> requested ;)

Ops ^^'

> In fact, to have the separate feature would be in a sense
> similar to having Connection's Status as an optional introspectable,
> which is clearly wrong.

Ok - so I assume it is right to have this bundled into 
TubeChannel::FeatureCore (which will be renamed to FeatureTube as Simon 
suggested)?

> 
> The documentation for FeatureCore says it's implicitly added to the
> set of features for isReady() and becomeReady() - is this really true?
> I see the FT channel docs say the same, but I don't think such a
> mechanism actually exists (while it would be useful). However,
> becomeReady does have a default parameter of Channel::FeatureCore, but
> that doesn't extend to subclasses. Andre? Maybe we could make
> ReadinessHelper always assume all registered introspectables with
> critical == true as requested, or something?

I leave the ball to Andre here.

> 
> As for StreamTubeChannel, the separate FeatureMonitorConnections makes
> sense - although I'd call it ConnectionMonitoring, to keep with the
> noun naming of most other Features.

Roger that.

> Not many applications are likely
> to care about the signals, and we should prevent the unneeded wakeups
> in those cases. However, one thing I'd like to be explored here is
> implementing QObject::connectNotify to make the connection as-needed,
> which would lead to both less chances of getting it wrong (expecting
> the signals while the feature isn't requested) and possibly better
> performance (per-signal granularity instead of connecting all three).

Well, connectNotify just lets the class know when a connection has been 
estabilished. It could be useful for spitting out warnings in case you 
connected to a signal which is not supposed to be emitted as the corresponding 
feature has not yet been enabled, but its purpose is barely limited to letting 
you know when $something connected to $signal, and act accordingly.

I agree it should be implemented to spit out warnings, though.

> 
> If we still go down the Feature route we might as well offer a
> connections() method, with the StreamTubeChannel object internally
> keeping track of the changes, keeping the signals for change
> notification of that set.

Ok - do you want to add a separate FeatureConnections for that, I suppose? I 
also suppose the connections method should eventually return a list of 
Contacts?

> Also, considering newRemote/newLocal is only
> ever emitted for outgoing / incoming tubes respectively, maybe they
> should be moved to the directional subclasses?

Makes a lot of sense.

> One more thing is you
> should consider building up Contacts for the NewRemoteConnection
> signal like Channel does for the group membership changes - it
> shouldn't be too hard here, but will allow applications to live with
> one less asynchronous call step.

Good idea - I will. Having the signals being too tightly mapped to the low 
level API was one of my primary concerns, this is definitely a good step 
forward.

> 
> OutgoingStreamTube: You should move PendingOpenTube out from the
> public header. That is, unless you're planning to add some useful API
> to it, maybe access to the Offer params, in which case you should
> change the offer() methods to admit they return PendingOpenTube *
> (also requires you to make them have the PendingFailure functionality
> by themselves).

Yes, I wanted to do it but somehow forgot it, sorry for that. Although, since 
it needs to be processed by moc, I'd need a separate private header. What is 
the naming convention for that? I saw there's a stream-media-channel-
internal.h, so I suppose it's <name>-internal.h

> Also, the offer methods seem to return NotCapable if
> the CM doesn't support the requested socket type / access control
> combo. NotCapable actually means "the *contact in question* doesn't
> have sufficient capabilities for the operation to succeed" -
> NotImplemented would be more appropriate here. Also, the remote "not
> IPv4/6" case should be InvalidArgument. Maybe go over your error usage
> in the other classes as well.

Thanks for the clarification - I will.

> 
> IncomingStreamTube: I think PendingIODevice needs to be changed to
> PendingStreamTubeConnection or somesuch with the option of also
> getting the Address out of it in addition to the IO device to cater
> for applications NOT using a qt iodevice for their network I/O, at
> least not very directly - I imagine these are fairly common when eg. a
> protocol library with a non-qt API is used. For example, krdc with
> telepathy support passes around KUrl instances that the tubes support
> has constructed from the address the tube Accept returns, and
> eventually it passes this to libvncclient using a bare C API.
> Similarly, the device() on IncomingStreamTube should be complemented
> with an address() as well.

Makes sense - both the naming and the additional parameters.

> 
> Maybe setAllowedAddress actually removes the need for the access
> control parameter?

Maybe. There is still the Credential case which should be handled, even if 
there could probably be a similar way for doing it. Maybe if I had some 
enlightenments on how the Credential control works (as I don't really get what 
it stands for at the moment) I could come up with something.

Given the above is solved, I'm all for removing the parameter.

Is such a thing applicable for Outgoing tubes as well? If so, those functions 
might be moved down to TubeChannel and simpify the whole deal, maybe even by 
having private classes inheriting one from the other thanks to the power of 
Q_DECLARE_PRIVATE.

> We could have having called setAllowedAddress
> successfully mean that the application wants Port access control,
> otherwise it's Localhost. We'd need a unsetAllowedAddress() and proper
> documentation though, but seems like a good idea at first otherwise
> though.

Correct - we definitely need it.

> 
> Similar abuse of the NotCapable error occurs in IncomingStreamTube too.

Ok, will fix as well.

> 
> The examples seem ok from a quick look.

Great.

I will try to address the obvious points while I'll wait for your comments on 
the other things I raised above: done that, I will force push again to my 
branch.

P.S.: About that, would you prefer a particular commit scheme for the final 
patches?

> 
> ---
> 
> Br,
> Olli Salli

-- 
-------------------

Dario Freddi
KDE Developer
GPG Key Signature: 511A9A3B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/telepathy/attachments/20100511/4296b5be/attachment-0001.pgp>