Currently the top level singleton is TpAccountManager, but ideally all TpProxy subclasses should be created through the factory. I've got here a branch that does: - TpProxy::constructed assert that it has a factory. - TpClientFactory is the top level singleton. Others tp_foo_dup() are implemented by taking the factory singleton, then taking foo from that factory. - All TpDBusDaemon are replaced by GDBusConnection since that object wasn't doing anything anymore. http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factory
> + self = g_weak_ref_get (&singleton); [No action required] You get bonus penguin points for preemptively using a thread-safe GWeakRef, but to be fully thread-aware, all reads and explicit writes to @singleton would need to be protected by a lock. I might implement that, just for completeness. > * @must_have_unique_name: If set %TRUE by a subclass, the #TpProxy > - * constructor will fail if a well-known bus name is given > + * constructed will fail if a well-known bus name is given [smcv will fix] I'd prefer this to say something vague about it being a programming error. > - bus_daemon = tp_tests_dbus_daemon_dup_or_die (); > - tp_proxy_invalidate ((TpProxy *) bus_daemon, &invalidation_reason); > - > - has_props = tp_proxy_check_interface_by_id ((TpProxy *) bus_daemon, > - TP_IFACE_QUARK_DBUS_DAEMON, &error); > - > - /* Borrowing the interface should fail because the proxy is invalidated. */ > - g_assert (!has_props); > - g_assert (error != NULL); > - g_assert_cmpuint (error->domain, ==, invalidation_reason.domain); > - g_assert_cmpint (error->code, ==, invalidation_reason.code); > - g_assert_cmpstr (error->message, ==, invalidation_reason.message); [smcv will fix] We shouldn't just delete test coverage, but this can hopefully be stuck onto an existing test. > --- a/telepathy-glib/introspection.am > +++ b/telepathy-glib/introspection.am > @@ -36,7 +36,6 @@ TelepathyGLib_1_gir_FILES = \ ... > - $(srcdir)/dbus-daemon.c $(srcdir)/dbus-daemon.h \ [smcv will fix] This is an introspection regression, but hopefully if we move the GValue and a{sv} utilities out of dbus.h, we can introspect it OK.
(In reply to comment #1) > > - $(srcdir)/dbus-daemon.c $(srcdir)/dbus-daemon.h \ > > [smcv will fix] > > This is an introspection regression, but hopefully if we move the GValue and > a{sv} utilities out of dbus.h, we can introspect it OK. No, my mistake, we introspect dbus.[ch] already.
Created attachment 96667 [details] [review] 1/3] Add channel-filter to documentation
Created attachment 96668 [details] [review] 2/3] TpProxyClass: disobeying must_have_unique_name is programming error
Created attachment 96669 [details] [review] 3/3] Restore lost test coverage from get-interface-after-invalidate test
Created attachment 96679 [details] [review] [4/4] TpClientFactory: consistently use _ensure_ when creating objects Xavier was going for "singletons use _dup_, objects that are per-path use _ensure_" but we agreed that using _ensure_ for both was less confusing.
+1 for your extra patches.
Added one more commit: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=factory&id=09505790cfbcc97e0f4cc51b720d6dc7684bd2f9
Reviewed: http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=next-factory (I split up the _ensure commit) Unreviewed: http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=next-factory2 Unimplemented: Gabble/Salut/Haze/Idle/Rakia Folks TPAW Empathy
Haze does not need changes beyond the next-dbus branch from Bug #28782. Salut, Rakia and Idle are straightforward: http://cgit.freedesktop.org/~smcv/telepathy-idle/log/?h=next-factory http://cgit.freedesktop.org/~smcv/telepathy-rakia/log/?h=next-factory http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=next-factory
MC from next-gdbus to next-factory2: - "McdMaster: take a TpClientFactory at construct-time": priv->factory should be unref in dispose instead of priv->dbus_daemon. I would rather keep a ref on both tbh, but since the daemon is going away later, I'm ok. Thats what you do already in McdAccount.
(In reply to comment #11) > - "McdMaster: take a TpClientFactory at construct-time": priv->factory > should be unref in dispose instead of priv->dbus_daemon. Makes sense, I'll fix that. Any thoughts on MC's next-tests, next-import, next-dbus, which precede this one? (The installed-tests are not strictly critical-path, but test coverage is important for quality, and I would really like to have them in the initial uploads of MC 6 and each CM to Debian, so that they don't have to go through new-package processing for a second time when I add installed-tests later. I want to get the MC version right, because I want to base the other CMs' tests on it.)
(In reply to comment #11) > - "McdMaster: take a TpClientFactory at construct-time": priv->factory > should be unref in dispose instead of priv->dbus_daemon. priv->client_factory was already unreffed in dispose. The fact that priv->dbus_daemon changed from "owned ref" to "borrowed ref" in this commit means I should have removed the tp_clear_object (&priv->dbus_daemon) that was also in dispose. I didn't (but perhaps a refleak elsewhere cancelled out this error, or perhaps I missed the crash because it was only after testing had completed). However, I'm not going to bother rebasing to fix that (unless you insist), since we've deleted TpDBusDaemon anyway. I agree that in general "owned ref" is a better pattern. While checking that the endpoint of next-factory2 didn't have any crashes during testing, I did find that the test build of MC was crashing during shutdown for an unrelated reason. With that fixed (patch in a moment), it isn't crashing.
Created attachment 96775 [details] [review] mc-debug-server: if we have several reasons to stop, don't crash Previously, when the session and (fake) system bus closed (simultaneously, because they are in fact the same dbus-daemon), we would call mcd_mission_abort() for the first one. That results in a call to tp_clear_object (&mcd), and then the second call to mcd_mission_abort (mcd) would crash with a critical warning, because MCD_IS_MISSION (NULL) is false.
Gabble is also easy: http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=next-factory
TPAW (compiles, cannot test until I've done Folks and Empathy): http://cgit.freedesktop.org/~smcv/telepathy-account-widgets/log/?h=next-factory
Folks (rebased on a merge from current master): http://cgit.freedesktop.org/~smcv/folks/log/?h=next-factory
(In reply to comment #8) > Added one more commit: > http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/ > ?h=factory&id=09505790cfbcc97e0f4cc51b720d6dc7684bd2f9 I applied this, but then reverted it, because it removes dup_*_features() functions which are certainly useful (even if subclassing isn't). I think I'm going to do a whole-stack test *without* this patch, then think about which parts of it make sense to pull in.
(In reply to comment #18) > (In reply to comment #8) > > Added one more commit: > > http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/ > > ?h=factory&id=09505790cfbcc97e0f4cc51b720d6dc7684bd2f9 > > I applied this, but then reverted it, because it removes dup_*_features() > functions which are certainly useful (even if subclassing isn't). We don't need dup_*_features() vfuncs, factory subclasses can call _add_*_features() just fine. AFAIK, those vfunc are there to not return TpTextChannel specific features when getting the features of a TpCallChannel. But I don't think that's an issue, TpProxy will filter features that it doesn't know, I think. > I think I'm going to do a whole-stack test *without* this patch, then think > about which parts of it make sense to pull in. fair enough.
(In reply to comment #19) > We don't need dup_*_features() vfuncs, factory subclasses can call > _add_*_features() just fine. Empathy doesn't, which is enough to stall this temporarily. I'd be OK with removing this unnecessary API surface area if it is in fact unnecessary, but the way to prove it's unnecessary is to test a whole-system where it isn't used, and that clearly hasn't been done yet. I don't think this is a high priority compared with Bug #76855 and Bug #76369. > TpProxy will filter features that it doesn't know, I think. It does, with a comment saying "this doesn't seem ideal". If we want to rely on this as an API design factor, we should document it.
Not tested yet, but compiles and passes "make check": http://cgit.freedesktop.org/~smcv/empathy/log/?h=next-factory
(In reply to comment #21) > Not tested yet, but compiles and passes "make check": > http://cgit.freedesktop.org/~smcv/empathy/log/?h=next-factory It needed some additional fixes to the debug window, but commit 1dfa999 seems to work. The complete set, then: Reviewed: http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=next-factory Reviewed, added attachment #96775 [details] [review], no changes in response to review, please check: http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=next-factory2 Unreviewed: http://cgit.freedesktop.org/~smcv/telepathy-idle/log/?h=next-factory http://cgit.freedesktop.org/~smcv/telepathy-rakia/log/?h=next-factory http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=next-factory http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=next-factory http://cgit.freedesktop.org/~smcv/telepathy-account-widgets/log/?h=next-factory http://cgit.freedesktop.org/~smcv/folks/log/?h=next-factory http://cgit.freedesktop.org/~smcv/empathy/log/?h=next-factory (No changes to Haze needed.)
(In reply to comment #22) > (In reply to comment #21) > > Not tested yet, but compiles and passes "make check": > > http://cgit.freedesktop.org/~smcv/empathy/log/?h=next-factory - http://cgit.freedesktop.org/~smcv/empathy/commit/?h=next-factory&id=7a735da21f170d75808cf32899c5322e116cc34a: You can use g_application_get_dbus_connection() instead of g_bus_get_sync(). You can also use HasNameOwner. Checking on my ubuntu 14.04, unity takes "org.gnome.Shell" name probably for some compat. So that empathy checks seems wrong, but that's not a regression from your branch. - http://cgit.freedesktop.org/~smcv/empathy/commit/?h=next-factory&id=77f6895f743fd0d5d9fad7dafb327bee3753486b: the factory singleton is leaked then in empathy_init(). Wondering how that was working before, because it does unref the AM... Maybe empathy_factory_dup() should set itself as default if tp_client_factory_can_set_default() is true? > Reviewed, added attachment #96775 [details] [review] [review], no changes in response > to review, please check: > http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=next- > factory2 +1 for the whole branch. > Unreviewed: > http://cgit.freedesktop.org/~smcv/telepathy-idle/log/?h=next-factory +1 > http://cgit.freedesktop.org/~smcv/telepathy-rakia/log/?h=next-factory +1 > http://cgit.freedesktop.org/~smcv/telepathy-salut/log/?h=next-factory - http://cgit.freedesktop.org/~smcv/telepathy-salut/commit/?h=next-factory&id=7b7810b5f295b4d769f96b695b2e1eae82c6b476: why do you init in each test instead of once in main? > http://cgit.freedesktop.org/~smcv/telepathy-gabble/log/?h=next-factory +1 > http://cgit.freedesktop.org/~smcv/telepathy-account-widgets/log/?h=next- > factory +1 > http://cgit.freedesktop.org/~smcv/folks/log/?h=next-factory +1
Note that UOA does not build at all currently, it has not been ported to new MC plugin API. I should get to it at some point. Or just let canonical make the patches.
(In reply to comment #23) > http://cgit.freedesktop.org/~smcv/empathy/commit/?h=next- > factory&id=7a735da21f170d75808cf32899c5322e116cc34a: You can use > g_application_get_dbus_connection() instead of g_bus_get_sync(). """ If GApplication is not using D-Bus then this function will return NULL. This includes the situation where the D-Bus backend would normally be in use but we were unable to connect to the bus. """ ... so I don't see much advantage over g_bus_get_sync(), which returns the same singleton if everything is OK, and can do better error signalling otherwise? I'd be OK with swapping this if you feel that it's important though. > You can also use HasNameOwner. GetNameOwner requires us to check for an error. NameHasOwner requires us to check for an error and also parse a boolean from a (b) tuple. I decided I preferred the former :-) > Checking on my ubuntu 14.04, unity takes > "org.gnome.Shell" name probably for some compat. So that empathy checks > seems wrong, but that's not a regression from your branch. If Unity is running, the presence Indicator is just as good as the Shell's presence widget? So in either case we don't want a GtkStatusIcon. (Non-critical-path, in any case.) > http://cgit.freedesktop.org/~smcv/empathy/commit/?h=next- > factory&id=77f6895f743fd0d5d9fad7dafb327bee3753486b: the factory singleton > is leaked then in empathy_init(). Yes, deliberately. I don't think we can fix that without adding an empathy_uninit() or something. > Wondering how that was working before, > because it does unref the AM... Maybe empathy_factory_dup() should set > itself as default if tp_client_factory_can_set_default() is true? *shrug* In practice I don't think it matters? The factory isn't going to be freed until we're about to exit anyway. > http://cgit.freedesktop.org/~smcv/telepathy-salut/commit/?h=next- > factory&id=7b7810b5f295b4d769f96b695b2e1eae82c6b476: why do you init in each > test instead of once in main? A vague feeling that significant code in a GTest's main() should be avoided, because it's executed even if all we're doing is "list test cases".
ok, good with me.
(In reply to comment #24) > Note that UOA does not build at all currently, it has not been ported to new > MC plugin API. I should get to it at some point. Or just let canonical make > the patches. Fine, I'll go with "best-effort porting only" for that bit.
Fixed in git for 0.99.10
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.