Bug 27872

Summary: TpBaseClient Handler support
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Guillaume Desmottes <guillaume.desmottes>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium CC: danielle
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/base-handler-27872
Whiteboard: review+
i915 platform: i915 features:
Bug Depends on: 25236    
Bug Blocks: 25512, 27873    

Description Simon McVittie 2010-04-28 08:45:25 UTC
Cloned from Bug #25236. There was skeletal support for being a Handler, but it was deleted before merging; it should be reinstated, and filled in.

Tentatively assigning to Guillaume, who implemented the Observer equivalent.
Comment 1 Guillaume Desmottes 2010-05-17 03:21:41 UTC
I'm almost done with this but some help regarding the test would be welcome as I don't understand wtf is going on.

See last commit of:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-glib;a=shortlog;h=refs/heads/base-handler-27872

The test doesn't pass, for some reason tp_dbus_daemon_unregister_object doesn't actually unregister the channel (so the invalidated signal is not fired). Calling g_object_unref *instead* of unregister works fine but calling it *after* unregister doesn't work either.
Comment 2 Simon McVittie 2010-05-18 03:43:48 UTC
Sorry I didn't pass this on sooner. Sjoerd suggested on the way back from UDS that for as long as there are HandledChannels, telepathy-glib should ensure that there is a Client "head" with an empty list of HandlerChannelFilters - it could have name "Auto" or something, and the uniquify flag - so that MC can perform crash-recovery by interrogating that "head".

However, this is a bit problematic to implement at the moment, because what would that Handler head's HandleChannels method do? The only sane version, I think, would be for it to raise NotImplemented.

This is awkward because when told to Ensure a channel that already exists, MC will currently pick one of the Client bus names that is a Handler on the appropriate unique name (arbitrarily), and ask it to re-handle the channel. If it happens to pick the Auto "head", Ensure will not have its desired behaviour of raising the window (or whatever).

That's arguably a bug - it should prefer the "head" that accepted the channel the first time - but is somewhat fiddly to fix, and until now it hasn't really been significant (in practice I expect that most/all? Handlers will only have one real implementation of HandleChannels anyway, and all the others will defer to it).

We could achieve a similar practical result at the moment by having the insides of telepathy-glib monitor the set of TpBaseClient instances that are Handlers (per DBusConnection), and if the last one is about to be removed, only pop up the Auto "head" at that point. This would mean that people who "do it right" never get Ensured channels dispatched to the Auto "head".

Relatedly, at the moment the only way to remove a TpBaseClient is to release the last reference to it - I should file a bug for an unregister method.

I think we can safely declare that unregistering the object and its bus name manually is not a supported action, and that only unreffing the object or calling its unregister method are supported.
Comment 3 Simon McVittie 2010-05-18 04:30:01 UTC
(In reply to comment #2)
> This is awkward because when told to Ensure a channel that already exists, MC
> will currently pick one of the Client bus names that is a Handler on the
> appropriate unique name (arbitrarily)
...
> That's arguably a bug

Bug #24645, in fact.

> Relatedly, at the moment the only way to remove a TpBaseClient is to release
> the last reference to it - I should file a bug for an unregister method.

Bug #28155, which I think might actually be what you need for Comment #1 too.
Comment 4 Guillaume Desmottes 2010-05-18 05:46:32 UTC
(In reply to comment #2)
> Sorry I didn't pass this on sooner. Sjoerd suggested on the way back from UDS
> that for as long as there are HandledChannels, telepathy-glib should ensure
> that there is a Client "head" with an empty list of HandlerChannelFilters - it
> could have name "Auto" or something, and the uniquify flag - so that MC can
> perform crash-recovery by interrogating that "head".

As we agreed on IRC that this could be done later I opened bug #28158 to track this feature.
Comment 5 Guillaume Desmottes 2010-05-18 05:48:30 UTC
(In reply to comment #3)
> > Relatedly, at the moment the only way to remove a TpBaseClient is to release
> > the last reference to it - I should file a bug for an unregister method.
> 
> Bug #28155, which I think might actually be what you need for Comment #1 too.

How so? The problem is when unregistering a channel in the test, not the client.
Comment 6 Simon McVittie 2010-05-24 03:38:58 UTC
> +  tp_dbus_daemon_unregister_object (tp_base_connection_get_dbus_daemon (
> +        test->base_connection), test->text_chan_service);
> +  /*
> +  g_object_unref (test->text_chan_service);
> +  test->text_chan_service = NULL;
> +  */

Aha! Forcibly taking a channel off D-Bus won't trigger client-side invalidation (how could it? the client doesn't know it's disappeared!). You need to make it emit Closed somehow, before it's removed from D-Bus. (After it's removed, emitting TpSvcChannel::closed doesn't emit Closed any more, for obvious reasons.)

For hysterical raisins, causing most channel implementations to be disposed has the side-effect of emitting Closed, but that's not really right.

I've fixed that, and the test now passes:

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/handler

With that change I reckon it's reviewable, so I'll review the rest of your branch now.
Comment 7 Simon McVittie 2010-05-24 03:42:15 UTC
*** Bug 28155 has been marked as a duplicate of this bug. ***
Comment 8 Simon McVittie 2010-05-24 04:31:35 UTC
+++ b/tests/dbus/base-client.c
> +#include <telepathy-glib/handle-channels-context-internal.h>

Why? If possible I'd prefer it if regression tests didn't use internal API.

> +  param_spec = g_param_spec_uint64 ("user-action-time",

This (and friends) should probably be an int64 that happens to be positive, even though we get a uint64 from D-Bus (which was a mistake in tp-spec).

It'd be good to document it in terms of the stuff you recently merged to tp-spec, too (with the 0 and G_MAXINT64 special cases).

Non-blocking trivia
===================

> +      g_warning ("Disposing a context in the %s state",

Could be WARNING().

> +  void (*chain_up) (GObject *) =
> +    ((GObjectClass *) \
> +      tp_handle_channels_context_parent_class)->constructed;

Unnecessary backslash.

> +  GList *l;

In future I'd prefer it if you'd avoid lower-case L as a variable name - it looks a lot like 1.

> +#include "telepathy-glib/handle-channels-context-internal.h"
> +#include "telepathy-glib/handle-channels-context.h"

Please put the public header first; the point of doing that is to verify that the public header is self-contained.
Comment 9 Guillaume Desmottes 2010-05-24 08:04:45 UTC
(In reply to comment #6)
> I've fixed that, and the test now passes:

Great. I merged your branch to mine but the test now crashes:

** CRITICAL **: dbus_g_connection_unregister_g_object: assertion `registrations != NULL' failed

#4  0x000000000040c266 in channel_get_interfaces (iface=0x20b5c10, context=0x20b5c00) at textchan-null.c:489


(In reply to comment #8)
> +++ b/tests/dbus/base-client.c
> > +#include <telepathy-glib/handle-channels-context-internal.h>
> 
> Why? If possible I'd prefer it if regression tests didn't use internal API.

We use it to easily access to the ctx semi-private attributes (conn, account, channels, etc).
Note that we do that for the Observer and Approver as well.

> > +  param_spec = g_param_spec_uint64 ("user-action-time",
> 
> This (and friends) should probably be an int64 that happens to be positive,
> even though we get a uint64 from D-Bus (which was a mistake in tp-spec).
> 
> It'd be good to document it in terms of the stuff you recently merged to
> tp-spec, too (with the 0 and G_MAXINT64 special cases).

done.

> Non-blocking trivia
> ===================
> 
> > +      g_warning ("Disposing a context in the %s state",
> 
> Could be WARNING().

done.

> > +  void (*chain_up) (GObject *) =
> > +    ((GObjectClass *) \
> > +      tp_handle_channels_context_parent_class)->constructed;
> 
> Unnecessary backslash.

removed.

> > +  GList *l;
> 
> In future I'd prefer it if you'd avoid lower-case L as a variable name - it
> looks a lot like 1.

That's used in a lot of place in telepathy.

> > +#include "telepathy-glib/handle-channels-context-internal.h"
> > +#include "telepathy-glib/handle-channels-context.h"
> 
> Please put the public header first; the point of doing that is to verify that
> the public header is self-contained.

done.
Comment 10 Simon McVittie 2010-05-24 09:08:44 UTC
Your changes look good.

(In reply to comment #9)
> (In reply to comment #6)
> > I've fixed that, and the test now passes:
> 
> Great. I merged your branch to mine but the test now crashes:
> 
> ** CRITICAL **: dbus_g_connection_unregister_g_object: assertion `registrations
> != NULL' failed
> 
> #4  0x000000000040c266 in channel_get_interfaces (iface=0x20b5c10,
> context=0x20b5c00) at textchan-null.c:489

I don't know why it'd be doing that, but will investigate a bit.

> (In reply to comment #8)
> > +++ b/tests/dbus/base-client.c
> > > +#include <telepathy-glib/handle-channels-context-internal.h>
> > 
> > Why? If possible I'd prefer it if regression tests didn't use internal API.
> 
> We use it to easily access to the ctx semi-private attributes (conn, account,
> channels, etc).
> Note that we do that for the Observer and Approver as well.

That's an acceptable use, but I'd prefer it commented.

> > > +  GList *l;
> > 
> > In future I'd prefer it if you'd avoid lower-case L as a variable name - it
> > looks a lot like 1.
> 
> That's used in a lot of place in telepathy.

Yeah, wjt overruled this too. Fair enough.
Comment 11 Guillaume Desmottes 2010-05-25 01:30:46 UTC
(In reply to comment #10)
> > (In reply to comment #8)
> > > +++ b/tests/dbus/base-client.c
> > > > +#include <telepathy-glib/handle-channels-context-internal.h>
> > > 
> > > Why? If possible I'd prefer it if regression tests didn't use internal API.
> > 
> > We use it to easily access to the ctx semi-private attributes (conn, account,
> > channels, etc).
> > Note that we do that for the Observer and Approver as well.
> 
> That's an acceptable use, but I'd prefer it commented.

I added a comment.
Comment 12 Guillaume Desmottes 2010-05-25 01:38:52 UTC
I merged your latest patch to my branch and the test works fine now. \o/

I think I fixed all your comments. For me the branch is good to go.
Comment 13 Guillaume Desmottes 2010-05-25 01:53:27 UTC
(In reply to comment #12)
> I merged your latest patch to my branch and the test works fine now. \o/
> 
> I think I fixed all your comments. For me the branch is good to go.

Hum actually when running all the test one seems to fail (don't know how to spot the one failing).
Comment 14 Guillaume Desmottes 2010-05-25 02:07:27 UTC
That was actually a similar issue in the CDO test. I pushed a fixed.
All tests are happy now :)
Comment 15 Simon McVittie 2010-05-25 03:29:50 UTC
Yes, this looks good. Ship it!
Comment 16 Guillaume Desmottes 2010-05-25 03:37:19 UTC
Merged \o/

Will be in 0.11.6

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.