[Bug 28366] Add DBusTube interface support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #8 from Olli Salli <ollisal at gmail.com> 2011-06-07 04:56:29 PDT ---
(In reply to comment #7)
> Dario has had a branch at the URL for some time now. I originally dismissed it
> because it wasn't beer. Sorry about that.

He's updated it. I'll re-review now. Hopefully it's not WIP at this point or
anything - I guess I'll find out in a bit.

Commenting on my own post so it acts sort of like a checklist for having taken
care of everything.

> As the paradigm is the same for both incoming and outgoing tubes, I'd use just
> one Tp::PendingDBusTube class which waits for accept/offer to finish and the
> tube to go Open (or fail doing that), to avoid duplication in both the API and
> the implementation. Or perhaps you only need to return PendingOperation, with
> the address/connection accessible through the Tp::DBusTubeChannel instance once
> that finishes?

This has now been done, and sure enough the logic is exactly the same for both.
However, there still is a separate onOfferFinished and onAcceptFinished, and
separate constructors - why are these needed? The only difference I can see in
them is the different debug message (which could be moved to the "not open yet"
case and say something like "Waiting for the tube to be opened", as the other
case debugs anyway already). So I'd rather remove this duplication too.

However, I see another issue now: PendingDBusTube::onTubeStateChanged is
expecting the tube state changes to either local pending or open. However, for
outgoing tubes it surely won't ever change to local pending; but it WILL change
to remote pending until the remote accepts it, which would currently trigger
the "error handling": The code seems to be using the other states as some kind
of error mechanism; this is incorrect. There is no "failed" state. Rather, the
class should connect to the invalidated signal of the channel: if the offer is
rejected or some other error happens, the channel will be invalidated.

I note PendingStreamTubeConnection also expects to receive the errors through
some bogus tube state change: that won't happen. Could fix that as well,
please, by additionally connecting to the channel's invalidated signal when it
starts waiting for a state change?

> 
> Please add a DBusTube channel class to RequestableChannelClassSpec, and using

Done-ish, however:

+RequestableChannelClassSpec RequestableChannelClassSpec::dbusTube(const
QString &serviceName)
+{
+    static RequestableChannelClassSpec spec;
+
+    if (!spec.isValid()) {
+        RequestableChannelClass rcc;
+        rcc.fixedProperties.insert(TP_QT4_IFACE_CHANNEL +
QLatin1String(".ChannelType"),
+                TP_QT4_IFACE_CHANNEL_TYPE_STREAM_TUBE);

Compare the first and last lines please :P

> it, capability accessors similar to the stream tube ones to ContactCapabilities
> and ConnectionCapabilities (with service name instead of service). Or is this
> somehow not feasible?

Not done! Oh well, I guess this was still WIP anyway then.

> 
> >     static const Feature FeatureBusNamesMonitoring;
> 
> FeatureBusNameMonitoring would be more proper English.

Fixed now, good!

> 
> Otherwise, the public APIs look good to me.
> 
> > path: root/TelepathyQt4/dbus-tube-channel-internal.h
> 
> We only use separate internal headers if we need to run moc on the private
> class(es), which is not the case here.

Good, you've fixed this. As a bonus this made the address setting better
decoupled and thus cleaner.

> 
> >         busNames.insert(parent->connection()->contactManager()->
> >             lookupContactByHandle(i.key()), i.value());
> 
> This won't work in the general case, as we discussed back when the stream tube
> channel API was being implemented - you have to asynchronously build the
> contacts instead. I noticed that the lookupContactByHandle function is nowadays
> public: this is totally incorrect (Andre agrees). I've filed bug 37748 as a
> result.

This is now mostly done, however there's a small issue:

+        // Add it to our connections hash
+        foreach (const Tp::ContactPtr &contact, contacts) {
+            mPriv->busNames.insertMulti(contact, busName);
+            added.insert(contact, busName);
+        }

Aside from the copy-pasted comment (we have bus names here, not connections),
you shouldn't be using insertMulti: each contact can only ever have one bus
name, as the DBusNames property isn't a multi-map (map with lists as values)
either. Also, you shouldn't emit a signal if a removed contact wasn't in the
current bus names set: you should rather print a warning in that case, because
the service is misbehaving.

The rest is still not done I see. OK, so this was quite WIP.

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