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

[Olli Salli]

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?

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.

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 ;) 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.

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?

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

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. Also, considering newRemote/newLocal is only
ever emitted for outgoing / incoming tubes respectively, maybe they
should be moved to the directional subclasses? 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.

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). 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.

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.

Maybe setAllowedAddress actually removes the need for the access
control parameter? 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.

Similar abuse of the NotCapable error occurs in IncomingStreamTube too.

The examples seem ok from a quick look.

---

Br,
Olli Salli