[Bug 28366] Add DBusTube interface support
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Mon May 30 17:48:14 CEST 2011
https://bugs.freedesktop.org/show_bug.cgi?id=28366
Olli Salli <ollisal at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
URL| |http://cgit.collabora.com/g
| |it/user/drf/telepathy-qt4.g
| |it/log/?h=dbus-tubes
Status Whiteboard| |review-
--- Comment #7 from Olli Salli <ollisal at gmail.com> 2011-05-30 08:48:11 PDT ---
Dario has had a branch at the URL for some time now. I originally dismissed it
because it wasn't beer. Sorry about that.
I've had enough beer since, so it's review time.
> handling accept and offer methods, they might become more useful whenever we'll
> add methods for returning Qt objects such as QDBusServer.
Actually I think we should only ever return QDBusConnections. The CM implements
the p2p dbus transport server side for both incoming and outgoing dbus tubes.
(That's why both accept and offer return an address to the CM server, unlike
stream tubes where you offer a server of your own). The QDBusConnection
accessor doing connectToBus() can then co-exist with the QString address
accessor in the DBusTubeChannel base class.
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?
Please add a DBusTube channel class to RequestableChannelClassSpec, and using
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?
> static const Feature FeatureBusNamesMonitoring;
FeatureBusNameMonitoring would be more proper English.
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.
> 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.
if (parent->targetHandleType() == static_cast<uint>(Tp::HandleTypeRoom)) {
parent->connect(dbusTubeInterface,
SIGNAL(DBusNamesChanged(Tp::DBusTubeParticipants,Tp::UIntList)),
parent,
SLOT(onDBusNamesChanged(Tp::DBusTubeParticipants,Tp::UIntList)));
} else {
As you weren't following the signal before, you might have lost changes to it
since the initial property download. Remember that additional features can be
activated later, not only in one bunch when making an initial becomeReady call.
Hence you must refresh the current state of the property after connecting by
requestPropertyDBusNames'ing it, and only finish the feature introspection once
that is done.
Additionally, I don't like the fact that whatever DBusNames were there at the
time of the original introspection of FeatureDBusTube will be stuck (e.g.
contacts reffed) in the private busNames variable forever even if they leave if
FeatureBusNameMonitoring is not activated. It should only be populated when
enabling that feature in the first place.
* Returns the service name which will be used over the tube. This should be a
* well-known and valid DBus service name, in the form "org.domain.service".
Tubes
* providing invalid service names might cause non-predictable behavior.
*
This comment in DBusTubeChannel::serviceName() rather makes more sense when
specifying the service name when requesting a channel. A properly requested
channel will always have such a service name set.
* If the tube has been opened, this function returns the private bus address
you should be listening
* to for using this tube.
You don't listen to the tube, you connect to it.
* This function returns all the known active connections since
FeatureConnectionMonitoring has
* been enabled. For this method to return all known connections, you need to
make
* FeatureConnectionMonitoring ready before accepting or offering the tube.
Accidentally refers to a StreamTubeChannel property, in addition to the quite
awkward semantics resulting from not reintrospecting the bus names property
properly.
I'd also like a mention here that this will be empty for p2p channels no matter
what one does, because it's trivial to deduce the bus names for ourselves and
the other party.
Relatedly, I'd make
warning() << "FeatureBusNamesMonitoring does not make sense in a P2P
context";
debug only, as one can't easily know whether a tube is a group tube or p2p one
before introspecting Channel::FeatureCore, which will usually only be
introspected at the same time as this feature.
mPriv->extractProperties(props);
debug() << "Got reply to Properties::GetAll(DBusTubeChannel)";
Please reverse these lines; that way there's a hint that we're crashing in the
response handling if the extraction is buggy. General rule: report the D-Bus
event that causes logic to be run before running the logic.
> void DBusTubeChannel::onDBusNamesChanged(const Tp::DBusTubeParticipants &added,
Queue, queue. Also the meaning of the "real" sets seems to be lost here: you're
intended to weed out redundant events (such as adding somebody who already is
there, or removing somebody who wasn't there in the first place) using them.
Check the other code using "real" sets in more detail for reference.
* \fn void DBusTubeChannel::busNamesChanged(const QHash< ContactPtr, QString >
&added, const QList< ContactPtr > &removed)
*
* Emitted when the participants of this tube change
Only for multi-user tubes, and when the feature has been enabled.
* \c IncomingDBusTubeChannel is an high level wrapper for managing Telepathy
interface
* #TELEPATHY_INTERFACE_CHANNEL_TYPE_DBUS_TUBE.
* In particular, this class is meant to be used as a comfortable way for
accepting incoming requests.
Well not quite. This description gives the hint that one'd use this in the
Approver for accepting/rejecting incoming tubes. One doesn't use it for that:
the Handler uses it for getting a bus connection to start communicating with
when somebody offers you a tube (and it has been accepted by an Approver, if
there is one).
* Tp::IncomingDBusTubeChannelPtr::dynamicCast(channel);
Promote qObjectCast instead of dynamicCast, please.
I wonder if we should have the handleChannels example at all, actually. First
of all, it shows nothing which wouldn't be applicable to any other kind of
channel (it's after all just casting to the subclass based on the channel
class), second: we should finish bug 35085 and promote using it instead of
implementing custom handlers. This reminds me, whoever continues bug 35084
should also remove any similar docs from StreamTubeChannel...
The feature preparation things are likewise in conflict with 1) the practice of
using factories currently considered ideal and 2) the fact that the bug 35085
API should do this for you anyway. 2) applies to the acceptTube doc as well.
* To learn more on how to use introspectable and features, please see \ref
account_ready_sec.
account?
* QString address = op->address();
* // Do some stuff here
Well, some stuff would be to connect some kind of dbus transport (such as
QDBusConnection, or DBusGConnection) to that address. This part will remain
even after bug 35085 API has been added: it'll just signal you addresses to
connect to, or after Qt 4.8 is released, QDBusConnections if you tell it you
want those.
*
* \note When using QHostAddress::Any, the allowedPort parameter is ignored.
*
acceptTube doesn't have such a parameter anyway, unlike the StreamTubeChannel
accept as tcp socket method.
I'd also like to hint that you can only use credentials if you can tell your
dbus transport (unlike, say, QDBus's) to actually pass the SCM_CRED(ENTIAL)S
message.
The actual accept method impl looks fine.
The example considerations from ISTC apply to OutgoingStreamTubeChannel as
well. It's cool to direct people to use Account to request the channel, though!
EXCEPT:
* Be sure to track the pending request to retrieve your outgoing DBus tube
upon success.
The Handler gets the tube, not any arbitrary "you". In particular, people
should not try to implement tube apps by using the PendingChannel::channel()
accessor, which is provided strictly for observation purposes (and documented
thus), and then acting like they were the tube Handler. Once again, once bug
35085 is done, in most uses the request should just eventually result in the
DBusTubeServer or alike class giving you an address/connection to export your
object on, or perhaps exporting it for you.
* This method creates a brand new private DBus connection, and offers it
through the tube.
*
Well it moreso sets up a private DBus connection to the channel target(s).
* \param parameters A dictionary of arbitrary Parameters to send with the tube
offer.
* Please read the specification for more details.
There's not that much more to read in the spec. It'd suffice to say that this
is what ends up in parameters() of the corresponding IncomingStreamTubeChannel
in the other end.
In PendingDBusTubeAccept::onTubeStateChanged():
// The tube is ready: let's inject the address into the tube itself
This comment looks a bit stale, as the address has been injected earlier on.
Otherwise the pending operations look OK, except for the aforementioned
needless duplication between the two, and it being questionable to have a
public class for them at all because you can anyway get the same information
from the channel object once the PendingOperation has finished.
--
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
More information about the telepathy-bugs
mailing list