Bug 28367

Please find the code here http://git.collabora.co.uk/?p=user/drf/telepathy-qt4.git;a=shortlog;h=refs/heads/high-level-tubes

Adding myself to CC list.

Implemented two trivial fixes suggested by Andre and Olli:

32996d2171a4b961fa53ae8ea96351ea33a6b803: remove __k__ prefix from private slots

b3417c76585424a46cb875beede45ca59168611d: Merge newLocalConnection and newRemoteConnection into StreamTubeChannel::newConnection

Review of your latest commits:

Updating parameters on successful offer(): I think you did it in a really complicated fashion :( Why not just stash the parameter map the app gives to offer() in the PendingOpenTube instance and then if the underlying processes are successful, upload it to StreamTubeChannel::parameters before doing setFinished(), via a protected method (or a public one on the inherited private class?). This would make the parameters update happen *before finished() is emitted* - currently parameters getting updated to the real value happens an arbitrary time after the PendingOpenTube finishes (when the Get parameter call happens to return), with no corresponding signal or anything to indicate "it now has the correct value". And no, I wouldn't like to add another signal for this :)

Another thing where we actually might need real reintrospection for would be if somebody else does offer() and we are just monitoring the tube. This would require us to react to the TubeStateChanged to Offered and delay emitting it until we've re-introspected the param. However, the current implementation doesn't cater for it either. But, I don't think this use-case of monitoring a tube getting offered AND needing to use the parameters for something actually even exists, so I'm fine with fixing the needless reintrospection and consequent indeterministic parameters update in the common case as outlined above. Maybe put a FIXME in there explaining that parameters doesn't correctly update if you're not the one offering the tube though.

Connection mapping functions: I guess you're correct about returning the hash itself - after all, thanks to qt containers being implicitly shared this is common practice in many other Qt/KDE APIs (in other places of TpQt4 too), and allows using other functions in the hash api besides the lookup...

Naming changes ++, but please also change AllowedAddress to SpecifiedAddress and reference the fact that supportsWithSpecifiedAddress also means "does connection tracking work" in the supports function doc too.

More to come...

(In reply to comment #3)
> Review of your latest commits:
> 
> Updating parameters on successful offer(): I think you did it in a really
> complicated fashion :( Why not just stash the parameter map the app gives to
> offer() in the PendingOpenTube instance and then if the underlying processes
> are successful, upload it to StreamTubeChannel::parameters before doing
> setFinished(), via a protected method (or a public one on the inherited private
> class?). This would make the parameters update happen *before finished() is
> emitted* - currently parameters getting updated to the real value happens an
> arbitrary time after the PendingOpenTube finishes (when the Get parameter call
> happens to return), with no corresponding signal or anything to indicate "it
> now has the correct value". And no, I wouldn't like to add another signal for
> this :)

So true :) Fixed in 2c9b755f850d41f781e8e3f67299f228ce23dcd4

> 
> Another thing where we actually might need real reintrospection for would be if
> somebody else does offer() and we are just monitoring the tube. This would
> require us to react to the TubeStateChanged to Offered and delay emitting it
> until we've re-introspected the param. However, the current implementation
> doesn't cater for it either. But, I don't think this use-case of monitoring a
> tube getting offered AND needing to use the parameters for something actually
> even exists, so I'm fine with fixing the needless reintrospection and
> consequent indeterministic parameters update in the common case as outlined
> above. Maybe put a FIXME in there explaining that parameters doesn't correctly
> update if you're not the one offering the tube though.

I actually did not touch anything here as I did not get the whole drill

> 
> Connection mapping functions: I guess you're correct about returning the hash
> itself - after all, thanks to qt containers being implicitly shared this is
> common practice in many other Qt/KDE APIs (in other places of TpQt4 too), and
> allows using other functions in the hash api besides the lookup...

Cool

> 
> Naming changes ++, but please also change AllowedAddress to SpecifiedAddress
> and reference the fact that supportsWithSpecifiedAddress also means "does
> connection tracking work" in the supports function doc too.

Done in e3ff9d7b4e3f40dcf8e6ad5d59be0aced76a9c27

> 
> More to come...

Waiting :)

(In reply to comment #4)
> (In reply to comment #3)
> 
> So true :) Fixed in 2c9b755f850d41f781e8e3f67299f228ce23dcd4

Indeed, the fix is spot on, ++ ;)

> 
> > 
> > Another thing where we actually might need real reintrospection for would be if
> > somebody else does offer() and we are just monitoring the tube. This would
> > require us to react to the TubeStateChanged to Offered and delay emitting it
> > until we've re-introspected the param. However, the current implementation
> > doesn't cater for it either. But, I don't think this use-case of monitoring a
> > tube getting offered AND needing to use the parameters for something actually
> > even exists, so I'm fine with fixing the needless reintrospection and
> > consequent indeterministic parameters update in the common case as outlined
> > above. Maybe put a FIXME in there explaining that parameters doesn't correctly
> > update if you're not the one offering the tube though.
> 
> I actually did not touch anything here as I did not get the whole drill
> 

What I mean is, similarly to the allKnownContactsChanged() stuff we previously worked into TpQt4, currently if some other application (the Handler) using the (D-Bus level) Channel does Offer, and you're an application using TpQt4 (most likely an Observer) and having an OutgoingStreamTubeChannel for said channel, you won't get the Parameters updated.

What SHOULD happen in this case is, the Observing TpQt4 delays emitting tubeStateChanged to Offered/Open UNTIL it has re-introspected the Parameters property (since it has been changed on the CM object by some other application, not us doing offer*(), similarly to how the known contacts could change because some other application did a roster modify operation). Still, I'm going to opt for the FIXME comment describing this and not bothering with it now, as the use case seems rather slim - I think the only applications caring about Parameters should be the ones having the actual socket and needing to match with the bootstrap stuff (handshakes / HELLO-style message / alike) the parameters represent, and these applications should be the Handler / the one doing offer() anyway.

(In reply to comment #5) 
> Indeed, the fix is spot on, ++ ;)

:)

> 
> What I mean is, similarly to the allKnownContactsChanged() stuff we previously
> worked into TpQt4, currently if some other application (the Handler) using the
> (D-Bus level) Channel does Offer, and you're an application using TpQt4 (most
> likely an Observer) and having an OutgoingStreamTubeChannel for said channel,
> you won't get the Parameters updated.

Ok - now I see the point.

> 
> What SHOULD happen in this case is, the Observing TpQt4 delays emitting
> tubeStateChanged to Offered/Open UNTIL it has re-introspected the Parameters
> property (since it has been changed on the CM object by some other application,
> not us doing offer*(), similarly to how the known contacts could change because
> some other application did a roster modify operation). Still, I'm going to opt
> for the FIXME comment describing this and not bothering with it now, as the use
> case seems rather slim - I think the only applications caring about Parameters
> should be the ones having the actual socket and needing to match with the
> bootstrap stuff (handshakes / HELLO-style message / alike) the parameters
> represent, and these applications should be the Handler / the one doing offer()
> anyway.

I agree - it seems overkill to me at this stage, and not really useful. I'll add a FIXME later.

It's starting to be in a really good shape... But I'm still going to bother you with some small finishing touches :P

One thing I previously didn't notice is you should have pretty include headers (TelepathyQt4/ClassName) for all of your public classes. Currently there seems to be one for just the "main class" of a header (eg. IncomingStreamTubeChannel). You should also have pretty include headers for the "auxiliary" classes like PendingStreamTubeConnection, obviously just including the same header as the main include, in this case incoming-stream-tube-channel.h.

IncomingStreamTubeChannel::acceptTubeAsTcpSocket(): You allow the case of allowedAddress != Any but allowedPort == 0 - which doesn't make sense, and MIGHT fail on the CM, but not necessarily, and would then only fail obscurely when trying to actually connect to the CM socket. So either error out if an address is provided but not a port, or better yet don't have default parameters but a separate overload taking neither and specifying Any, 0 - leading to you having to specify exactly neither or both.

With these, I'm good for a merge (phew). Andre will likely want to review this stuff as well though - so stay tuned for his comments. In the meantime, I'll move on to looking at your dbus stuff. Is there a chance the improvements we've done here would be applicable there too btw? You know what to do if so ;)

(In reply to comment #7)
> It's starting to be in a really good shape... But I'm still going to bother you
> with some small finishing touches :P

Hehe, np, I'm as picky as hell, so I expect others to be as picky as me :P

> 
> One thing I previously didn't notice is you should have pretty include headers
> (TelepathyQt4/ClassName) for all of your public classes. Currently there seems
> to be one for just the "main class" of a header (eg.
> IncomingStreamTubeChannel). You should also have pretty include headers for the
> "auxiliary" classes like PendingStreamTubeConnection, obviously just including
> the same header as the main include, in this case
> incoming-stream-tube-channel.h.

Correct. Added in 8cbaff8dbcd862056b75a086405cd4a5b645336d.

> 
> IncomingStreamTubeChannel::acceptTubeAsTcpSocket(): You allow the case of
> allowedAddress != Any but allowedPort == 0 - which doesn't make sense, and
> MIGHT fail on the CM, but not necessarily, and would then only fail obscurely
> when trying to actually connect to the CM socket. So either error out if an
> address is provided but not a port, or better yet don't have default parameters
> but a separate overload taking neither and specifying Any, 0 - leading to you
> having to specify exactly neither or both.

This is correct. After thinking about it a small while, I came to the conclusion that having a separate overload would make more sense. However, to save code and make things more consistent, I also fixed the behavior of the other overload (check if port is valid). This way, the overload with no parameters just calls acceptTubeAsTcpSocket(QHostAddress::Any, 0). I also specified in the docs all the possible behaviors and how the two overloads interact. This is in 89e1787569842039d8d8e899b1fa413d8590cd71.

> 
> With these, I'm good for a merge (phew).

\o/!!

> Andre will likely want to review this
> stuff as well though - so stay tuned for his comments.

Yup, level 2 incoming :)

> In the meantime, I'll
> move on to looking at your dbus stuff. Is there a chance the improvements >we've
> done here would be applicable there too btw? You know what to do if so ;)

Possibly. Check your mail in the next hours to be sure ;)

Just joking, the hashes are different.

BTW, pushed some improvements to my dbus-tubes branch ;)

Here follows a quick review, further review will be done after these "issues" are handled:

General notes:
- On documentation use the constants such as 
TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE instead of using the string literal.
- Indicate on documentation that method X requires FeatureX, see other docs for
example. The account class documentation is very complete
- \return instead of \returns
- Remove __k__ and similar from all methods, vars, etc.
- Use the include directives as done in other classes, meaning that all included 
are at the top of the file, following a certain order, check other classes as 
reference.
- Remove the connectNotify stuff, we don't do that in any other class. The user 
should know what he is doing and the docs should state which features the signals
depend on.
- Do not use Q_PRIVATE_SLOT, just do as done in other class using "private Q_SLOTS"
- Use include QtSomething instead of QtFoo/QtSomething
- Never use public vars, add accessors for all members that may be accessed.
- Do not use Q_D, Q_Q, we may change that in the future but for now we don't use 
it anywhere, so let's just stick to mPriv for standardization
- Methods should be declared first, then vars.
- Try to initialize mPriv members inside mPriv constructor instead of doing
mPriv->foo = bla inside class constructor.

On TubeChannel::Private constructor, when defining a introspectable, make it 
like this:
    ReadinessHelper::Introspectable introspectableTube(
        QSet<uint>() << 0,                                           // makesSenseForStatuses
        Features() << Channel::FeatureCore,                          // dependsOnFeatures (core)
        QStringList() << TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE, // dependsOnInterfaces
        (ReadinessHelper::IntrospectFunc) &Private::introspectTube,
        this);

Doing this there is no need to check if the interface is present on 
Private::introspectTube. Just do:
    Client::ChannelInterfaceTubeInterface *tubeInterface = 
        parent->tubeInterface(BypassInterfaceCheck);

+                    QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER)),
This should be tube instead of ft.

(In reply to comment #10)
> Here follows a quick review, further review will be done after these "issues"
> are handled:
> 
> General notes:
> - On documentation use the constants such as 
> TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE instead of using the string literal.

Roger that

> - Indicate on documentation that method X requires FeatureX, see other docs for
> example. The account class documentation is very complete

Yup

> - \return instead of \returns

Yup

> - Remove __k__ and similar from all methods, vars, etc.

Hm, I thought I already did that, I probably forgot something on my way. Ok though.

> - Use the include directives as done in other classes, meaning that all
> included 
> are at the top of the file, following a certain order, check other classes as 
> reference.

Will do

> - Remove the connectNotify stuff, we don't do that in any other class. The user 
> should know what he is doing and the docs should state which features the
> signals
> depend on.

Ok - although this was an experimental thing I agreed to do with Olli - maybe we should investigate in the future on using it everywhere?

> - Do not use Q_PRIVATE_SLOT, just do as done in other class using "private
> Q_SLOTS"

Ok

> - Use include QtSomething instead of QtFoo/QtSomething

Ok

> - Never use public vars, add accessors for all members that may be accessed.

Hmm, are there any public vars around? If that's the case it's probably a mistake.

> - Do not use Q_D, Q_Q, we may change that in the future but for now we don't
> use 
> it anywhere, so let's just stick to mPriv for standardization

Here comes a problem - to simplify some things, I added inheritance between some private classes. This is possible in a decent way only by using Q_DECLARE_PRIVATE and Q_D. Q_Q can indeed be removed, but I cannot say the same for Q_D without reworking stuff a bit. Anyway, if you're keen on standardizing stuff, it should just be a matter of exposing some more stuff in the public classes as protected methods - then Q_D can safely go away.

> - Methods should be declared first, then vars.

Ok

> - Try to initialize mPriv members inside mPriv constructor instead of doing
> mPriv->foo = bla inside class constructor.

This is done for the very same reason as stated above: when inheriting from private classes, initialization of member variables has to be done after the construction phase.

> 
> On TubeChannel::Private constructor, when defining a introspectable, make it 
> like this:
>     ReadinessHelper::Introspectable introspectableTube(
>         QSet<uint>() << 0,                                           //
> makesSenseForStatuses
>         Features() << Channel::FeatureCore,                          //
> dependsOnFeatures (core)
>         QStringList() << TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE, //
> dependsOnInterfaces
>         (ReadinessHelper::IntrospectFunc) &Private::introspectTube,
>         this);
> 
> Doing this there is no need to check if the interface is present on 
> Private::introspectTube. Just do:
>     Client::ChannelInterfaceTubeInterface *tubeInterface = 
>         parent->tubeInterface(BypassInterfaceCheck);

Ok - thanks for the info.

> 
> +                   
> QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER)),
> This should be tube instead of ft.

Whooops.

Anyway - I'll be away for one week from tomorrow. As soon as I will be back, I'll fix the obvious issues (the only controversial one is the Q_D one) and push again.

Hey, I pushed to my branch all the needed changes. While I was at it, I took the chance to push a quick fix to my cmake branch, cherry pick please.

However, I fixed everything you pointed out except for a pair of things. Moreover, some of the things you pointed out (such as __k__ and the FILE_TRANSFER stuff) were not present in the latest revision - maybe I forgot to push some time ago. Anyway.

(In reply to comment #10)
> - On documentation use the constants such as 
> TELEPATHY_INTERFACE_CHANNEL_INTERFACE_TUBE instead of using the string literal.
> - Indicate on documentation that method X requires FeatureX, see other docs for
> example. The account class documentation is very complete

While I was at it, I took the chance to improve vastly the whole documentation, adding a paragraph about usage in both outgoing and incoming stream tubes, in a similar fashion to account.

> - Do not use Q_PRIVATE_SLOT, just do as done in other class using "private
> Q_SLOTS"

I actually did not do this, for the reason that this would have required to move all of the private slots into the public classes. Given that we're aiming to a more Qtish library, I think this would have been a wasted effort, as I'd really like to see every private slot in public classes become a Q_PRIVATE_SLOT once the refactoring is done. If you don't agree, though, I can change this anyway.

> - Use include QtSomething instead of QtFoo/QtSomething

I did it, but please remember that Qt itself strongly recommends to use QtFoo/QtSomething over QtSomething at least in public headers - probably another thing to change in the future.

Cheers!

Just for coherence with the spec, I would rename the "serviceName" parameter of the Account::createStreamTube methods and in several other places just to "service".
This might avoid confusion as the corresponding property is org.freedesktop.Telepathy.Channel.Type.StreamTube.Service (while for example dbustubes have a property org.freedesktop.Telepathy.Channel.Type.DBusTube.ServiceName)

Andre, I pushed to my branch the changes you requested. Please note that in some cases (channel factory for example), I stayed consistent with coding style in the file: as you said, it's better to change them all at once.

Let me know if there are still some issues.

NOTICE: Now that the cmake branch has been merged onto master, I rebased this branch on master, and it won't be rebased from now until merge, as there is no need to.

Here comes the long awaited update to the high-level-tubes branch - hope we can get this merged before christmas!

So, changes to tp-qt4 had been quite radical, to the point where the previous commits did not even compile and required substantial changes. For this very reason, and since I don't actually like having a non-compiling history, I opted for amending all of the previous commits to make them compile correctly, and then perform some additional fixes on top of them.

There are now two new branches, which are the candidates for review: high-level-tubes-0.5 and high-level-tubes-0.4. Names are rather self explainatory: code-wise the two branches are the very same, one is based upon master and the other on telepathy-qt-0.4.

Again, most of the changes were just a mere update to the new API: a new hot path is in channel-factory and channel-class-spec, as they are the only real new code compared to the previous branch.

P.S.: I could not get to test the examples yet due to some issues with DBus I'm experiencing now, however I decided to submit the review already to save some time.

So here goes my 2 cents:

Minor coding style issues:
- On private classes always declare the methods before the variables

- On class members initialization the "," should be in the same line as the member initialization and not in a new line.

- Add a new line (in incoming-stream-tube-channel.h):
+class QIODevice;
+namespace Tp
+{

Same thing here (in outgoing-stream-tube-channel.h):
+class QLocalServer;
+namespace Tp
+{

Check other places also.

- Remove trailing whitespaces:
I can see at least one in TubeChannel::Private::introspectTube:
+    Client::ChannelInterfaceTubeInterface *tubeInterface = 

- Remove the empty line after StreamTubeChannel::mPriv declaration.

- I like to name params in the slots declarations, but only do this if you are in the mood

- When using templates, only add spaces if really required.
+    QHash< uint, Tp::ContactPtr > contactsForConnections() const;
should be
+    QHash<uint, Tp::ContactPtr> contactsForConnections() const;

- Move the signals documentation to the bottom of the classes.

- Don't use \brief in methods documentation, the first line is considered the \brief doc.

- Lots of documentation only have \return (for eg. PendingStreamTubeConnection::addressType). Please add a brief description also in the first line.

- I would like to see this TODO and also any other TODO removed and implemented before merging:
+    // TODO: Use error to provide more detailed error messages
+    Q_UNUSED(error)

Other minor details:
- Make IncomingStreamTubeChannel::device() const, same for PendingStreamTubeConnection::device()

- When rebasing against master there will be a conflict in account.h, as Olli just added hints support there. Please fix this and add a default "hints = ChannelRequestHints()" to the createStreamTube methods and pass it to PendingChannelRequest. 
  Ps.: No need to add new methods that take hints as param, just change the current methods to get a hints default param, as this is not in our API/ABI yet.

- Make it Private *mPriv instead of Private * const here (and in other places as well):
+    Private * const mPriv;
+
+    friend class PendingStreamTubeConnection;
  Ps.: Also move the "friend class PendingStreamTubeConnection" declaration to be located immediately after the private keyword. Check channel.h for examples. Do the same for all "friend class" declarations other than "friend struct Private"

- Make sure all public classes have a fancy header (not sure if there is any missing).

- No need to have a virtual destructor in PendingStreamTubeConnection::Private, please also check other places where it is not needed.

I think that is pretty much it for now. I will double-check again when the branch is updated. Almost there.

Hi Andre and thanks for reviewing.

Please pull from my branch, I updated it accordingly, and added a single commit for each point you mentioned (which implies that everything was fixed :) ). I also rebased it atop of master and fixed the conflicts which arose.

P.S.: I just updated the high-level-tubes-0.5 branch as I don't know if this should be 0.4 material, let me know if that is the case.

Cheers!

0.4 is bugfixes only at this point, so this is indeed only needed for 0.5.

channel-factory.h:

 208     // When merged, Tube channels should have export/import variants too like FT has for send/receive

You've added those methods already, so please remove this comment!

channel-class-spec.h:

 186     static ChannelClassSpec outgoingStreamTube(const QVariantMap &additionalProperties = QVariantMap());
 187     static ChannelClassSpec incomingStreamTube(const QVariantMap &additionalProperties = QVariantMap());

Add const QString &service = QString() as the first argument (and don't insert the key at all if the default value was passed, rather than inserting the empty QString). This is because most often ChannelClassSpec will be used when specifying Handler filters, and trying to be a Handler for all possible services on stream tubes makes no sense (as the Handler does the actual service-specific communication over the tube), so it should be convenient to specify the service. However, the argument shouldn't be mandatory, because it's likely that Observers and perhaps also Approvers want to observe / approve tubes in a generic fashion.

I also second Daniele's suggestion about changing your use of serviceName to mean the TCP-like service the StreamTube connects to to just "service" as above; "service name" has a strong association with the well-known names of D-Bus services, which might be misleading in this case, as we're also going to add D-Bus tubes.

OutgoingStreamTubeChannel:

  50     PendingOperation *offerTcpSocket(QTcpServer *server, const QVariantMap &parameters);
  54     PendingOperation *offerUnixSocket(QLocalServer *server, const QVariantMap &parameters,

Couldn't the Q*Server args be pointers to const?

Otherwise, it's VeryGood (tm) that the API allows passing in bare addresses, not just QSomethingServers, as otherwise you couldn't implement much anything with it (for example, the tube-enabled krfb uses libvncserver (a C library) to actually listen to incoming VNC connections, and consequently there's no QTcpServer.

This is, however, in stark contrast with...

IncomingStreamTubeChannel:

Sorry for not noticing this earlier (by earlier I mean up to a year ago when I first looked at this!), but the same "not everything uses the Qt network classes" requirement holds here as well.

Currently, while you're exposing the bare address in PendingStreamTubeConnection, which is Good, you're also auto-connecting a QTcpSocket/QLocalSocket to that address, when the tube state goes Open, which is Bad.

Consider an application which needs to use the bare address, because they use a protocol library which doesn't communicate over QIODevices, but rather takes the address as a parameter and uses internal implementation means to actually establish a socket connection accordingly. This is true of all libraries I've used in my entire programming "career", actually, I guess because libraries usually aim to be as generic as possible, while QIODevices require e.g. running the Qt event loop.

Now, it might appear that the application can pass the address to the library. Well, it can, but at that point we've already auto-connected a QTcpSocket to that address! If the CM and remote service are gracious enough to allow multiple connections, this might not cause more than a log entry in the remote service like "somebody just connected to us, but then disconnected without sending anything", but it can also prevent the real connection using the bare address passed to the library from succeeding.

Hence, we can't auto-connect a socket as part of the accept() PendingOp. Rather, that op should just finish successfully once the tube state goes Open, or with a failure if the tube channel is closed before that happens. If the former happens, it should provide access to the address. At that point, any protocol library or the application's own implementation can be easily used to connect using that address information.

We can still offer convenience for getting a connected QIODevice to that address, but that needs to be in an another PendingOp you get after the first one (getting the address) finishes. That said, I don't think it's really that useful: it's nothing Telepathy-specific, it'd just be an abstraction to connecting a socket which is either an UNIX socket or a TCP one.

Also I'd like to see RCCSpec and CapabilitiesBase methods for stream tubes. These are not essential though: I'd like to see this branch finally merged and not bitrotting further as a first priority, we can add the capability stuff in an another quick branch.

Oh, and we also recently decided to declare all private slots and private methods TELEPATHY_QT4_NO_EXPORT to trim down the .so symbol table further. So you should do that for the private methods added here as well.

Hi Olli and thanks for the review. Just like I did with Andre's comments, I addressed each point you brought to attention in a separate commit. I am commenting what is worth commenting below:

(In reply to comment #19)
> channel-class-spec.h:
> 
>  186     static ChannelClassSpec outgoingStreamTube(const QVariantMap
> &additionalProperties = QVariantMap());
>  187     static ChannelClassSpec incomingStreamTube(const QVariantMap
> &additionalProperties = QVariantMap());
> 
> Add const QString &service = QString() as the first argument (and don't insert
> the key at all if the default value was passed, rather than inserting the empty
> QString).

Did that, although I am not really sure of the implementation, so I'd ask you to review carefully this specific commit.

> Sorry for not noticing this earlier (by earlier I mean up to a year ago when I
> first looked at this!), but the same "not everything uses the Qt network
> classes" requirement holds here as well.

Indeed. I ended up removing all the logic for connecting to a QIODevice, and changed the exposed methods in IncomingTubeChannel to reflect the ones in the PendingOperation. And I also agree on not providing a way for building a QIODevice, given how easy it is (the example was updated accordingly).

> Also I'd like to see RCCSpec and CapabilitiesBase methods for stream tubes.
> These are not essential though: I'd like to see this branch finally merged and
> not bitrotting further as a first priority, we can add the capability stuff in
> an another quick branch.

I agree with doing this after the merge, and I will take care of it.

P.S.: To both of you, would you also mind trying out the example and tell me if it works for you?

(In reply to comment #21)
> (In reply to comment #19)
> > channel-class-spec.h:
> > 
> >  186     static ChannelClassSpec outgoingStreamTube(const QVariantMap
> > &additionalProperties = QVariantMap());
> >  187     static ChannelClassSpec incomingStreamTube(const QVariantMap
> > &additionalProperties = QVariantMap());
> > 
> > Add const QString &service = QString() as the first argument (and don't insert
> > the key at all if the default value was passed, rather than inserting the empty
> > QString).
> 
> Did that, although I am not really sure of the implementation, so I'd ask you
> to review carefully this specific commit.

Good thing that you asked. The ChannelClassSpec keys should be qualified D-Bus property names; thus the following is invalid:

+    if (!service.isEmpty()) {
+        spec.setProperty(QLatin1String("service"), service);
+    }

The key should rather be QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_STREAM_TUBE ".Service"). Please update for all instances of this.

> P.S.: To both of you, would you also mind trying out the example and tell me if
> it works for you?

I didn't test-run the example; however, as the example uses the Connection low-level functionality in a very unexemplary fashion, let's leave it out for the moment. We should try and make the existing examples more exemplary; but at the very least let's not add any new examples showing naive/incorrect usage of Telepathy.

A proper example should be a Handler Client, and request the channels from the Account, integrating properly with the Channel Dispatcher. The Request and Handle API Andre is currently finishing up will make writing such an example easier.

I also think the final example should employ another kind of a convenience API: a Handler base class for tube clients. I'd expect many applications using TpQt4 solely for the purpose of exporting their services over a Telepathy Tube - we should make implementing that as easy as possible. We planned such an API a bit already last September at the KDE Telepathy sprint, but the idea would be quite similar to the one proposed for Text Observers in bug #28753.

So, the end result: let's fix that ChannelClassSpec service property declaration; and merge the branch to master, but without the current test. Then, let's work on adding the remaining bits to make it possible to write a correct (in the sense that it co-operates with the ChannelDispatcher) but yet very simple and clean example (thanks to the added convenience APIs). Obviously, let's still keep the current test around in a branch as a basis for those further developments.

(In reply to comment #22)
> A proper example should be a Handler Client, and request the channels from the
> Account, integrating properly with the Channel Dispatcher. The Request and
> Handle API Andre is currently finishing up will make writing such an example
> easier.
> 

Of course, the above would be for the initiator side, while

> I also think the final example should employ another kind of a convenience API:
> a Handler base class for tube clients. I'd expect many applications using TpQt4
> solely for the purpose of exporting their services over a Telepathy Tube - we
> should make implementing that as easy as possible. We planned such an API a bit
> already last September at the KDE Telepathy sprint, but the idea would be quite
> similar to the one proposed for Text Observers in bug #28753.
> 

would make implementing the receiver properly easy.

I am going to fix the last bits now, and remove the example in a separate last commit.

Changes pushed to branch

OK to merge! (Do you have write access to the new freedesktop.org tp-qt4 repo, or shall I merge it?)

I need to solve a small issue with fd.o's admins and then I can merge it myself :)

Reviewed and merged to master. Will be in release 0.5.9 due today.