Bug 29699

Summary: Tp::ChannelRequest incorrectly checks immutable properties
Product: Telepathy Reporter: Will Thompson <will>
Component: tp-qtAssignee: Olli Salli <ollisal>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: blocker    
Priority: medium CC: artemgarmash, ollisal
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/oggis/telepathy-qt4.git;a=shortlog;h=refs/heads/cr-enhancements
Whiteboard:
i915 platform: i915 features:

Description Will Thompson 2010-08-20 03:51:00 UTC
In response to a reported problem about Tp::ChannelRequest not becoming ready, I had a poke around its implementation.

Every property on ChannelRequest is immutable, so there should be no reason for introspectMain() to call GetAll() again.

However, extractMainProps(), called from the ctor, expects the QVariantMap's keys to be unqualified names like "Account" rather than qualified names like "org.freedesktop....ChannelRequest.Account", as they are documented to be by <http://telepathy.freedesktop.org/spec/Client_Interface_Requests.html#org.freedesktop.Telepathy.Client.Interface.Requests.AddRequest>.

So yeah, this seems broken. :)
Comment 1 Olli Salli 2010-08-24 10:31:26 UTC
Fix merged to master. Will be in 0.3.8.
Comment 2 Artem Garmash 2010-09-16 06:39:09 UTC
With the fix, request object provided in addRequest still can't be used to get immutable properties. Can introspectMain be called from ctor right away in order to be able to use request in addRequest without waiting?
Comment 3 Artem Garmash 2010-09-17 04:53:01 UTC
The request argument is still unusable in addRequest. With the current implementation it's never become ready and actually you can see from log that another instance created for handleChannels.
If introspectMain() is called right from ChannelRequest::Private ctor, immutable properties usable right away.
Comment 4 Olli Salli 2010-09-19 23:51:42 UTC
The point of passing immutable properties never was that the object would immediately be ready by the time the constructor returned. The immutable properties were only ever intended to be an optimization for not having to do GetAll in most cases when the introspection process kicks off. The thing is that the supplied immutable properties may be insufficient and therefore the normal introspection has to run - hence the application has to do becomeReady and wait for it to finish (although it would finish immediately with sufficient immutable props), just to handle the case of insufficient props correctly.

Besides, the CR still has to make its Account ready, which doesn't occur immediately in most cases. HOWEVER, a related feature addition in 0.3.10 makes the ChannelRequest given to you in addRequest reuse the Account from the rest of your application, and only makes it ready if nobody else hasn't already. This means less DBus traffic and it becoming ready quicker, but still, *you need to call becomeReady*.

To reiterate: to make ReadyObjects like ChannelRequest become ready, *you have to call becomeReady()* in general. However, the factory stuff from 0.3.9/0.3.10 removes the need to do this for Account and Connection objects - you can instead set specific features on an Acc/ConnFactory to be always made ready for you before Accs/Conns are signaled anywhere, including the Client methods.

I guess I could add a separate special-case thing similar to factories which would essentially be a boolean you set on your AbstractClientHandler implementation, to make the addRequest ChannelRequests always be ready using becomeReady() before signaling an addRequest. It's either the library or the app doing that BUT if you do it in your application, you can actually prepare for errors in making the CR ready - the best we can do in the library is to have another iWasGoingToGiveYouACRButItFailedWithThisError() hook to your application, which I don't think is very clean. If you think this is still a bug and we should add that boolean-y thing - which would mean that error-out in making it ready would never signal anything to your application, reopen with prio:high, severity:enhancement.
Comment 5 Artem Garmash 2010-09-22 06:55:24 UTC
With the current tp-qt4, the ChannelRequest from addRequest() becomes ready after handleChannels(). What's the point?
All UI needs from it are account object path and targetID to map to window. Could they be provided right away, e.g. by having immutableProperties() for ChannelRequest, like for Channel.
Comment 6 Olli Salli 2010-09-27 01:53:34 UTC
(In reply to comment #5)
> With the current tp-qt4, the ChannelRequest from addRequest() becomes ready
> after handleChannels(). What's the point?

This is not always true. You get addRequest as soon as the request is made, but handleChannels only when the CM has set up the channel etc - when e.g. requesting XMPP MUCs, the delay between the two might be significant. However, I agree that the situation could be made better. I'll work some spec additions in soon which enable the ChannelRequest (and also ChannelDispatchOperations in other Client APIs) to be introspected faster.

> All UI needs from it are account object path and targetID to map to window.
> Could they be provided right away, e.g. by having immutableProperties() for
> ChannelRequest, like for Channel.

This is certainly possible. Put in my next 2-week TODO. I guess QString accountObjectPath(), QString targetID() etc would be nicer to use, though, so I'm probably going to go that way.
Comment 7 Artem Garmash 2010-09-27 06:04:32 UTC
> This is certainly possible. Put in my next 2-week TODO. I guess QString
> accountObjectPath(), QString targetID() etc would be nicer to use, though, so
> I'm probably going to go that way.

At least from the messaging point of view, TargetHandleType and PersistentID would be needed as well for group chat support...
Comment 8 Olli Salli 2010-09-27 06:50:11 UTC
OK, I'll expose the bare variant map but also include the friendly accessors then.
Comment 9 Olli Salli 2010-10-03 11:01:30 UTC
Branch offering early access to the CR up for review...

Essentially, it makes you able to use account() etc straight after the ChannelRequest is created (even in the same mainloop iteration, such as in the addRequest handler), as long as those properties are in the provided immutable properties map (like they always should be with up-to-date Mission Control).
Comment 10 Olli Salli 2010-10-04 10:40:11 UTC
Branch merged to master. Will be in 0.3.11, due momentarily.

You can now access whatever was provided by the immutable properties even in the same mainloop iteration you get the ChannelRequest, even if becomeReady hasn't been called. The object still won't be fully ready however - missing information is fetched and eg. the Account made ready according to the factory settings only when the becomeReady operation completes. Nevertheless, you can use it to map with the Account and some of the desired channel properties passed in the request as a key.

There also is a bare variant map of immutable properties, similar to how Channel::immutableProperties() works.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.