Bug 27872

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.

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.

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

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

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

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

*** Bug 28155 has been marked as a duplicate of this bug. ***

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

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

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.

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

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.

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

That was actually a similar issue in the CDO test. I pushed a fixed.
All tests are happy now :)

Yes, this looks good. Ship it!

Merged \o/

Will be in 0.11.6