[Bug 28366] Add DBusTube interface support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #10 from Olli Salli <ollisal at gmail.com> 2011-06-09 22:55:42 PDT ---
(In reply to comment #8)
> 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

The slot is now shared, which is good, but you still have the two identical
constructors.

You've now fixed the invalidation vs bogus state change issue in
PendingDBusTube, but I believe the similar bug in the
PendingStreamTubeConnection class remains. Could you fix that too, please? This
branch or a separate one doesn't really matter.

+ * This signal will be emitted only if the tube is a group tube (not p2p), and
if the
+ * FeatureBusNameMonitoring feature has been enabled. You usually want to wait
until the
+ * aforementioned feature is ready before connecting to this signal.

What's the reason for which one would want to wait? I can't think of any.

+ * Return whether creating a DBusTube channel, using the given \a serviceName,
by providing a
+ * contact identifier is supported.

As docs for ContactCapabilities::dbusTubes() this is not very appropriate: it's
not about "providing a contact identifier": these ContactCapabilities are
specific to *this particular contact*. So something like "Return whether
creating a DBusTube channel with the given service targeting this contact is
expected to succeed\n".

I see the corresponding StreamTube method description is also similarly
misleading, could you fix that as well?

+            } else {
+                warning() << "Trying to remove a bus name which has not been
retrieved previously!";
+            }

You could print the bus name in question.

Now as for the stuff which is still missing/broken:

 * Be sure to track the pending request to retrieve your outgoing DBus tube
upon success.

These kinds of very, very misleading (given how the channel requesting and
dispatching big picture really works) comments are still there in both
OutgoingDBusTubeChannel and OutgoingStreamTubeChannel.

Also, the long descriptions are still promoting the use of explicit becomeReady
calls over using the factories. And linking to the account readiness section -
why not the general async model section, for example? If you rebase on top of
current master, you'll find the other documentation in this area improved a
lot.

 * \note When using QHostAddress::Any, the allowedPort parameter is ignored.
 *

This is also still there in the acceptTube docs, although there is no such
parameter. Actually, please go through my initial comments on the documentation
again in general to see if you missed anything else like these.

DBusTubeChannel::FeatureBusNameMonitoring still has absolutely horrible
semantics, namely:
 - if you don't enable it, you'll get a random initial set of names, which
doesn't stay up to date, forever stuck in busNames()
 - if you enable it later than you enabled FeatureCore, you have lost all
changes in between
 - and actually, as even with a single becomeReady call / using factories with
both set, FeatureBusNameMonitoring is only introspected after Core, there is a
race condition: you'll in any case lose whichever changes happen between
introspecting Core, and getting connected to the change notification signal
when introspecting Monitoring!

You *MUST* only request/extract the property *after* connecting to the change
notification signal, in the introspection function for
FeatureBusNameMonitoring. This way all the above issues will be fixed. You can
use the "new" requestPropertyDBusNames() method for nicely getting the single
property once needed.

The above change to not make FeatureCore consider the DBusNames property at all
also has a favorable consequence: the remaining two properties are actually
immutable (although somebody has forgot to write that in the spec. Anyway, they
don't have change notification signals or any other way to change.). This means
that if the base class Channel's immutableProperties() has those properties,
you don't need to call GetAll at all for Core, just extract them.

In case you feel like improving the code further, while doing the introspection
changes to properly introspect DBusNames and consider immutableProperties() for
the remaining Core properties, you could start using
DBusTubeChannelInterface::requestAllProperties() instead of an explicit
PropertiesInterface::GetAll call.

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