Hi, 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 <drf54...@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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ telepathy mailing list telepathy@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/telepathy