[Bug 31583] Expose TpProxyFeature in the API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #17 from Will Thompson <will.thompson at collabora.co.uk> 2011-04-15 10:15:37 PDT ---
(In reply to comment #15)
> (In reply to comment #14)
> > a5adfbb:
> > 
> > @@ -649,7 +676,8 @@ get_pending_messages_cb (TpProxy *proxy,
> >      gpointer user_data,
> >      GObject *weak_object)
> >  {
> > -  TpTextChannel *self = user_data;
> > +  TpTextChannel *self = (TpTextChannel *) weak_object;
> > 
> > We may as well use a checked cast if we're casting at all. But...
> 
> Those are more time consuming, so I try to limit them.

Avoiding repeated checked casts in the same function is fine: if you've already
checked the cast to TpTextChannel, then casting to GObject is fine. But a
callback's user_data seems like a perfect place to use a checked cast.

If you insist on not using TP_TEXT_CHANNEL(), then don't use any explicit cast
at all, since user_data is a gpointer. (Is 'self' the same pointer as 'proxy'
here?)

> > +typedef struct
> > +{
> > +  GList *parts_list;
> > +  GSimpleAsyncResult *result;
> > +} IdentifyMessagesCtx;
> > +
> > 
> > If you use the GSimpleAsyncResult object as the weak_object, you don't need to
> > faff with context structs, since you can fish the TpTextChannel out of it using
> > g_simple_async_result_get_source_object.
> > 
> > (Since the GSimpleAsyncResult refs the channel anyway, this doesn't change the
> > weak_object-y behaviour at all.)
> 
> Oh, good idea. done.

You could use free_parts_list as the destroy callback for
tp_connection_get_contacts_by_id() and tp_connection_get_contacts_by_handle().

> > -              0, NULL, got_pending_senders_contact_by_id_cb, parts_list,
> > -              free_parts_list, G_OBJECT (self));
> > +              0, NULL, got_pending_senders_contact_by_id_cb, ctx,
> > +              (GDestroyNotify) identify_messages_free, G_OBJECT (self));
> > 
> > You seem to have changed the calls, but not updated the callbacks for the
> > changed type of the user_data. So I'm not sure how this can possibly work.
> 
> Yeah you're right, it's fixed now. Unfortunatelly this code was untested :(
> (testing isn't trivial as that's the fallback path used with CMs not
>  implementing newer spec (ImmportaHandles) which is automatically implemented
> by TpBaseConnection so with all tp-glib CMs...)

Fun.

Olli dealt with this for some other stuff by deliberately breaking the
DBus.Properties implementation for an interface; if you were keen, you could do
the same to force the fallback?

> > I wonder if this loop's body would be clearer as a switch.
> 
> Don't think so as we have to check feature->can_retry as well.

Okay.

> > Also, wait, what? You're upgrading unwanted features to wanted features if the
> > object has the necessary interfaces? Am I missing something here? Are the enum
> > members mis-documented?
> 
> Yeah, as the feature as been requested (using tp_proxy_prepare_async()), if
> the interfaces/deps are good, the property is now WANTED .

I still don't really understand. I'll take another look on Monday.

> > so what's wrong with a function that allocates and returns a zero-terminated
> > quark array? It could even be varargs for people who want to depend on more
> > than one feature.
> 
> Humm, then we'd have to allocate the arrays on the heap rather than on the
> stack, and they will be leaked which won't make valgrind happy. :\

Fair enough.

> > 53d656:
> > 
> > +  g_type_init ();
> > +  tp_tests_abort_after (10);
> > +  tp_debug_set_flags ("all");
> > +
> > +  g_test_init (&argc, &argv, NULL);
> > +  g_test_bug_base ("http://bugs.freedesktop.org/show_bug.cgi?id=");
> > 
> > Every single test starts with this. Can we have tp_tests_init (&argc, &argv); ?
> 
> Done. I recorded in such way we can easily cherry pick the commits to master
> if needed.

Great.

> > Your tests' prepare_foo_async functions should assert that the dependent
> > features have been prepared before they're called. I see that you do check that
> > the interfaces are checked, and that children of unpreparable features aren't
> > told to prepare; adding an assertion to feature B's preparation function
> > wouldn't hurt.
> 
> Humm I'm not sure to understand. prepare_b_async() does check if A is
> prepared.

I'll double-check this on Monday too.

> > +    /* List of TpProxyPrepareRequest. The first requests are the core one,
> > +     * sorted from the most upper super class to the subclass core features.
> > +     * This is needed to guarantee than subclass features are not prepared
> > +     * until the super class features have been prepared. */
> >      GList *prepare_requests;
> > 
> > This is a hobbyhorse of mine, but: this should be a GQueue (nb. you can use
> > GQueue as opposed to GQueue * if you're into that sort of thing). We prepend in
> > one place, and append in another place, so GQueue is genuinely a more
> > appropriate data structure.
> 
> done.

In
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=commitdiff;h=855f03acf403ad9359d199f22294498169e4710e
you used a GQueue * in the struct, not a GQueue...

I mean, it's fine, it works, but…

Also:

-  for ( ; iter != NULL; iter = g_list_delete_link (iter, iter))
+  for (iter = tmp->head; iter != NULL; iter = g_list_next (iter))
     {
       tp_proxy_prepare_request_finish (iter->data, error);
     }
+

This could become while ((proxy = g_queue_pop_head (iter)) != NULL) {
tp_proxy_prepare_request_finish (proxy, error); }. Not a big deal though.

> > I think it's really sketchy that proxy.c has connection-specific stuff in it. I
> > don't think any of this stuff should be in there. Can't the connection have the
> > special knowledge about which features should be prepared just before
> > announcing Connected, and call tp_proxy_prepare_async() for them all?
> 
> It's more subtile than that. prepare_before_signalling_connected_async()
> doesn't mean we have to prepare the feature before announcing CONNECTED but
> that IF the feature is already prepared, then we have to give it a chance to
> update itself before annoucning CONNECTED.

Ah, I see.

> I agree that the connection special case in proxy.c isn't great, but tbh
> prepare_before_signalling_connected_async is already connection specific any
> way. I discussed this with smcv when I started working on this and he agreed
> that modifying proxy.c was probably the way to go.
> 
> I guess the alternative would be to expose details about interfaces to
> connection.c and move mosf of the code there?

Pragmatically, let's stick with the current solution for now.

-- 
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.