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

Dario Freddi drf54321 at gmail.com
Wed Jun 2 17:39:41 PDT 2010


pushed stuff to my branch, here we go :)

On Monday 31 May 2010 20:39:21 Olli Salli wrote:
> On Fri, May 14, 2010 at 7:52 PM, Dario Freddi <drf54321 at gmail.com> wrote:
> > Hello everyone,
> > 
> > I addressed most of your concerns and implemented most of your
> > suggestions.
> > 
> > Thanks, and waiting for the usual feedback :)
> Okay, coming back for probably the last round of reviewing this
> stuff... Again, sorry for the immense delay.
> TubeChannel::parameters(): Note that currently this is only ever
> populated for incoming tubes. Parameters for outgoing tubes are set in
> offer(), not on channel construction, so introspecting them after
> creation and not updating afterwards won't do. The solution would be
> to implement a mechanism (protected function in TubeChannel most
> likely?) to update the parameters after Offer() has successfully
> returned.

Fixed in 08e001ce4279a701dac1b3858bc9efabcace8fbd

> OutgoingStreamTubeChannel: <nitpick> I think
> offerTubeAs{Tcp,Unix}Socket is slightly awkward naming. You are not
> offering a tube as a socket, be it tcp or unix, instead, you're
> offering said socket as a tube ;) Given that "tube" is in the class
> name already, I think the most intuitive naming would be
> offer{Tcp,Unix}Socket - that says you're offering a tcp/unix socket
> using the tube.</nitpick> However, acceptTubeAsTcp/UnixSocket is fine
> and logical.

Fixed in 790605f521d126e7769bd8c3ae40780126560a1d

> StreamTubeChannel::support*(): They assume Localhost is supported if
> ANY address control scheme is supported for a given protocol; while
> this might usually be true, the spec guarantees it only for IPv4. So
> for the other two, please do check for Localhost being present like
> you're currently doing for Port and Credentials. Also, I think you
> should use these methods in the offer() methods instead of
> reimplementing the checks for consistency, it's currently actually
> implemented correctly (checking for Localhost) in the offer() methods
> although it's not in support*()!

Fixed in 08dc73e8993b36a47c651f1aa0447f75aa2900e2. I actually changed the API 
again here and implemented a set of supports*(). They are many, I'd say a lot, 
but I think this approach is more consistent and does not throw in weird and 
confusing enums,

> OutgoingStreamTubeChannel::offer / PendingStreamTubeConnection (unix
> socket variant): While the D-Bus API has QByteArray for the unix
> socket addresses, as QLocalSocket and QLocalServer both use QString I
> think that's what we should expose to the user too - preventing them
> from mixing the various charset interpretation choices you can do the
> conversion with. One thing is, you should probably use Latin1
> conversions everywhere (as you're doing in eg. onTubeStateChanged)
> instead of UTF-8.

Fixed in fc1679ba4cadab3d6ea00b22e2ec7d925c94d77e

> And yeah, do comment out connections(), or implement it fully - the
> only thing you need to add is to declare a struct
> StreamTube::Connection with publics id, contact and
> sourceAddress/credentialByte with the last two carrying around data of
> the used access control to error out in the accessor if it's not
> appropriate. Just exposing the connection id is useless, as you can't
> get the useful information related to an id in any way.
> However, I see a bigger problem with the connection tracking - you use
> lookupContactByHandle(). That function can freely return you NULL if
> the contact in question happens to be not cached - only by doing the
> asynchronous contactsForHandles() call can you always build it. You
> should do something similar to how Channel handles the Group signals -
> put a suitable context of the signal to a FIFO queue (needs to be
> shared between the new*Connection and connectionClosed handling), then
> always build the contact using contactsForHandles() for the head of
> the queue, after that PendingContacts finishes move to the next queue
> item etc. What you shouldn't do is allow the contact building to
> overlap, this way you could eg. reorder a connection getting removed
> and appearing in the wrong order.

Implemented/Fixed in 0abaf81048986cce3afac3eb58a6222a18949ce1. I created a 
custom FIFO queue which might be nice to use somewhere else. I also changed 
the signals to be shaped like we agreed on IRC.

Implemented in the examples a debug output for showing that tracking works - 
and it actually does :)

Nitpick: I had to cherry pick my fix to SocketAddress* structs as I forgot to 
rebase my branch on it, so skip that commit away: it will disappear from that 
unfortunate position when I'll rebase on master anyway.

> If this contact building in a FIFO queue stuff seems a bit too much,
> feel free to just skip it but in that case remove / comment out ALL of
> the connection tracking - there's not much use for signals which might
> contain the contact (the useful bit) or NULL arbitrarily.
> Please update the documentation accordingly with the changes, eg.
> there are still mentions of setAllowedAddress etc. and the accept() /
> offer() variants should probably refer to their corresponding
> support() methods, etc. Probably some more of these occurrences of
> docs lagging behind the actual impl lurking around.

Fixed in 63f939c3e97d4d857df7dc2ae73f90b87eb11f13

> Seems like that'd be it for this round. This is a fairly extensive and
> invasive chunk of work to design/review - thankfully, once we have
> this stuff settled here for good, the DBus tube stuff should be much
> more clearer conceptually.

I should add that examples have been updated as well, and everything is still 
working nicely :)

P.S.: I still did not add the missing function to account due to the fact that 
I have some doubts on how I should implement a Group Stream Tube. Should I do 
it exactly the same way it's done in createConference*?
Also, I will rename the functions to create{P2P,Group}StreamTube.


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/20100603/5bba72d9/attachment.pgp>

More information about the telepathy mailing list