[Bug 28367] Add StreamTube interface support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #4 from Dario Freddi <drf54321 at gmail.com> 2010-06-06 05:52:56 PDT ---
(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 :)

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