|
Description
Simon McVittie
2012-09-13 16:05:44 UTC
In "account: replace every other mention of Valid with Usable"
- /* %NULL if the account is valid; a valid error for reporting over the
- * D-Bus if the account is invalid.
+ /* %NULL if the account is usable; a usable error for reporting over the
+ * D-Bus if the account is unusable.
Slightly overzealous search & replace: "a valid error" is still right.
- /* If invalid or something, force it to AVAILABLE - we want the auto
+ /* If unusable or something, force it to AVAILABLE - we want the auto
* presence type to be an online status */
Likewise.
In "rename SimplePresence interface to Presence" This is going to have a non-trivial merge with Bug #54780. Sorry... (I think we should make this lot into a new 'next' branch, then merge the new master into it, tbh.) In "replace tp_{connection,account,channel}_new calls with ensure factory calls"
+ g_object_get (self,
+ "factory", &factory,
+ NULL);
Ugh. If the assumption is that every McpDispatchOperation has a "factory" property, I think it'd be better to give it a dup_factory() method or make the property part of the GInterface.
However, I think it'd be better still if we made the factory an explicit parameter to the function. We're going to be breaking the plugin API/ABI anyway, because it depends on telepathy-glib which is breaking API/ABI. I think plugins can be expected to supply their own factory - they might well have a different set of desirable features, or even a different desired subclass.
[src/mcd-account-manager.c]
+ factory = tp_automatic_client_factory_new (priv->dbus_daemon);
The AM should have one TpAutomaticClientFactory - or, preferably, TpSimpleClientFactory - and cache it. Otherwise, there's no point in using a factory, since objects aren't going to be unique.
In "replace now sealed struct member accessing with getters" I think this could usefully be done on master. I'll prepare a patch based on yours (I was actually about to do part of it for other reasons anyway). > + params = tp_protocol_borrow_params (protocol); This function broke ABI between master and next, and I think Xavier might be removing it altogether now. I think I'm going to just use dup_params() so the same code works in both - efficiency is not exactly MC's strong point anyway. > mcd_account_check_parameters This still frees protocol with the TpConnectionManagerProtocol free-function. That's not going to work. I see you fixed it in the next patch, though. > check_one_parameter_update (McdAccount *account, > TpConnectionManagerProtocol *protocol, Similar. Again, the next patch fixes it. > + if (!tp_conn || tp_proxy_get_invalidated (TP_PROXY (tp_conn)) != NULL) return; I'm not going to incorporate this patch without chopping off the "return" onto its own line :-) In "stop calling deprecated self handle TpConnection functions" This looks as though it ought to be OK for master too. In "exec-with-log.sh: add gdb wrapper" This could usefully reach master. In "service-point: stop using deprecated APIs and add a small test"
+ g_print ("adding %s to the string list\n", *ptr);
I think you left your debugging code in.
I think the complexity of this patch is an indication that Conn.I.ServicePoint should change like this on next:
Service_Point: (us) type, ID -> (uus) type, handle, ID
Service_Point_Info: ((us)as) type, ID, aliases -> ((uus)a{su}) or something
I've only skim-read the patch, but it looks basically sensible, and the fact that you add a test is a good sign.
+# MC used to critical if more than one emergency service point was
+# given by the CM. That's silly, so let's test it.
Almost as if this had never been tested...
In "tools: update directory snapshot to tp-glib" I'm pretty sure we don't need all of these. I'm really quite sure we don't need shave.mk... -TELEPATHY_GLIB_SRCDIR = $(top_srcdir)/../telepathy-glib -maintainer-update-from-telepathy-glib: +TELEPATHY_SPEC_SRCDIR = $(top_srcdir)/../telepathy-spec +maintainer-update-from-telepathy-spec: set -e && cd $(srcdir) && \ for x in $(EXTRA_DIST); do \ - if test -f $(TELEPATHY_GLIB_SRCDIR)/tools/$$x; then \ - cp $(TELEPATHY_GLIB_SRCDIR)/tools/$$x $$x; \ + if test -f $(TELEPATHY_SPEC_SRCDIR)/tools/$$x; then \ + cp $(TELEPATHY_SPEC_SRCDIR)/tools/$$x $$x; \ fi; \ done Other GLib projects are meant to copy stuff in from telepathy-glib, not from the spec. Or if we're moving to a model of "copy the entire directory around", just delete this stanza... Some of this branch, on master: http://cgit.freedesktop.org/~smcv/telepathy-mission-control/log/?h=future-proofing (4 commits) > [src/mcd-account-manager.c] > + factory = tp_automatic_client_factory_new (priv->dbus_daemon); > > The AM should have one TpAutomaticClientFactory - or, preferably, > TpSimpleClientFactory - and cache it. Otherwise, there's no point in using a > factory, since objects aren't going to be unique. I'd like to add a MC-wide TpSimpleClientFactory on master, but I haven't done it yet. (In reply to comment #5) > In "stop calling deprecated self handle TpConnection functions" > > This looks as though it ought to be OK for master too. I'd like to put this on master, but two tests fail, and I haven't debugged it yet. Before starting next branch everywhere, please make master build with: AC_DEFINE(TP_SEAL_ENABLE, [], [Prevent to use sealed variables]) AC_DEFINE(TP_DISABLE_SINGLE_INCLUDE, [], [Disable single header include]) AC_DEFINE(TP_VERSION_MIN_REQUIRED, TP_VERSION_0_20, [Ignore post 0.20 deprecations]) AC_DEFINE(TP_VERSION_MAX_ALLOWED, TP_VERSION_0_20, [Prevent post 0.20 APIs]) I'm getting closer: Tests run: 73; tests failed: 11 Created attachment 87050 [details] [review] [master 1/6] dispatch-before-connected test: simulate ServerTLSConnection We might have had some sort of draft in mind when we wrote this, or these names might just be random strings. Let's be a bit closer to what this test was designed for. Created attachment 87051 [details] [review] [master 2/6] capabilities test: emulate Call1, not MediaSignalling MediaSignalling is dead, long live Call1. --- Yeah I know this isn't a particularly good emulation - it's good enough to test MC :-) Created attachment 87052 [details] [review] [master 3/6] contact-caps test: remove test coverage for obsolete Capabilities Created attachment 87053 [details] [review] [master 4/6] already-has-obsolete test: remove This didn't test what it was meant to test (see the next commit for details) which made it basically the same as dispatch-obsolete. In Telepathy 1.0 the "no Requests" case won't exist anyway. Created attachment 87054 [details] [review] [master 5/6] already-has-channel test: alter how we delay preparation We used to delay GetInterfaces and Get(CONNECTION, Interfaces), but in recent telepathy-glib, we just use GetAll(CONNECTION), so this was not effective. However, this was masked by the fact that the ServicePoints code redundantly calls GetInterfaces. :-( GetAll is also what we use to replace GetStatus, so connection won't proceed if we delay that. However, we can also set up the situation we're trying to test by making GetAll(CONNECTION) return promptly, and delaying GetAll(REQUESTS) instead. --- I called the parameter "implement_get_channels" because I thought "implement_get_all_requests" was too unwieldy, and the high-level operation it represents is getting Requests.Channels. Created attachment 87055 [details] [review] [master 6/6] Make ServicePoint setup more sensible (In reply to comment #17) > Created attachment 87055 [details] [review] > [master 6/6] Make ServicePoint setup more sensible The missing long commit message: mcd_connection_service_point_setup shouldn't call GetInterfaces - TpConnection already knows how to do that. As currently implemented, mcd_connection_service_point_setup() is only called from the status-changed callback for CONNECTED, which telepathy-glib guarantees not to call until core features are ready. However, it's a lot more clearly correct if we do this in on_connection_ready(). Created attachment 87057 [details] [review] [next 01/22] Allow client names to start with an underscore There's no good reason not to, and telepathy-spec 0.99.1 says they can. --- This comes after a merge of master (with attachment #87050 [details] [review] to attachment #87055 [details] [review]) into next. The only conflicts were in constants.py. Created attachment 87058 [details] [review] [next 02/22] require telepathy-glib-1 Drop TP_DISABLE_SINGLE_INCLUDE: it no longer does anything (the equivalent functionality is on by default). Created attachment 87059 [details] [review] [next 03/22] update tools from telepathy-glib 0.99.2 Created attachment 87060 [details] [review] [next 04/22] adjust for rename of TpSimpleClientFactory to TpClientFactory Created attachment 87061 [details] [review] [next 05/22] account: rename Valid to Usable Based on a patch by Jonny Lamb. --- So was the rest of this branch, really, but this is the only one to be based on an actual cherry-pick - MC had diverged far enough that most of Jonny's branch was inapplicable. Created attachment 87062 [details] [review] [next 06/22] rename SimplePresence interface to Presence in MC itself Created attachment 87063 [details] [review] [next 07/22] tests: replace SimplePresence with Presence Created attachment 87064 [details] [review] [next 08/22] Re-namespace bus names, and bump version to "Mission Control 6" Created attachment 87065 [details] [review] [next 09/22] Bump plugin API/ABI and generally be parallel-installable --- The documentation isn't parallel installable, and neither are mc-tool or mc-wait-for-name, but they can wait. Created attachment 87066 [details] [review] [next 10/22] tp_connection_manager_get_protocol lost its "_object" suffix Created attachment 87067 [details] [review] [next 11/22] Adjust contact features to Telepathy-1 API: quarks, not an enum Created attachment 87068 [details] [review] [next 12/22] turn CreateChannelWithHints into CreateChannel Correspondingly, stop implementing SupportsRequestHints: there has been no version of Telepathy 1 that lacked this functionality. Created attachment 87069 [details] [review] [next 13/22] switch from Messages to unified Text API Created attachment 87070 [details] [review] [next 14/22] Re-namespace remaining o.fd.T names in the actual source code Created attachment 87071 [details] [review] [next 15/22] Re-namespace remaining o.fd.T names in tests Created attachment 87072 [details] [review] [next 16/22] Fix StreamTube namespaces in regression test .client Created attachment 87073 [details] [review] [next 17/22] SucceededWithChannel is just Succeeded now Created attachment 87074 [details] [review] [next 18/22] Upgrade to the Hidden1 interface from telepathy-spec 0.99.2 In a future version we should make telepathy-glib generate this interface, but this will do for now. Created attachment 87075 [details] [review] [next 19/22] Remove some tests for obsolete functionality Created attachment 87076 [details] [review] [next 20/22] SimulatedConnection: simulate Telepathy 1 a bit better - the Requests interface is mandatory - GetSelfHandle, GetStatus, GetInterfaces, RequestHandles, InspectHandles, HoldHandles, ListChannels have gone away - so have GetPresences, GetAliasFlags, GetAliases, GetAvatarRequirements - SelfHandleChanged is now SelfContactChanged - SelfID exists now I haven't implemented Get(ALIASING, "AliasFlags") because MC doesn't actually use or need it. Created attachment 87077 [details] [review] [next 21/22] AliasesChanged now takes an a{us}, not an a(us) Created attachment 87078 [details] [review] [next 22/22] SimulatedChannel: fill in extra immutable properties Not all of our regression tests fill in all the necessary immutable properties for a modern TpChannel to prepare; some of them cut corners. Fill in the gaps. Comment on attachment 87050 [details] [review] [master 1/6] dispatch-before-connected test: simulate ServerTLSConnection Review of attachment 87050 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87051 [details] [review] [master 2/6] capabilities test: emulate Call1, not MediaSignalling Review of attachment 87051 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87052 [details] [review] [master 3/6] contact-caps test: remove test coverage for obsolete Capabilities Review of attachment 87052 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87053 [details] [review] [master 4/6] already-has-obsolete test: remove Review of attachment 87053 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87054 [details] [review] [master 5/6] already-has-channel test: alter how we delay preparation Review of attachment 87054 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87055 [details] [review] [master 6/6] Make ServicePoint setup more sensible Review of attachment 87055 [details] [review]: ----------------------------------------------------------------- I didn't even know about this iface. ++ Comment on attachment 87057 [details] [review] [next 01/22] Allow client names to start with an underscore Review of attachment 87057 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87058 [details] [review] [next 02/22] require telepathy-glib-1 Review of attachment 87058 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87059 [details] [review] [next 03/22] update tools from telepathy-glib 0.99.2 Review of attachment 87059 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87060 [details] [review] [next 04/22] adjust for rename of TpSimpleClientFactory to TpClientFactory Review of attachment 87060 [details] [review]: ----------------------------------------------------------------- ++ (In reply to comment #46) > I didn't even know about this iface. ++ I'm not really surprised - even though you're one of the most central Telepathy maintainers. Ladies and gentlemen, the Telepathy API! :-) Comment on attachment 87061 [details] [review] [next 05/22] account: rename Valid to Usable Review of attachment 87061 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87062 [details] [review] [next 06/22] rename SimplePresence interface to Presence in MC itself Review of attachment 87062 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87063 [details] [review] [next 07/22] tests: replace SimplePresence with Presence Review of attachment 87063 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87064 [details] [review] [next 08/22] Re-namespace bus names, and bump version to "Mission Control 6" Review of attachment 87064 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87065 [details] [review] [next 09/22] Bump plugin API/ABI and generally be parallel-installable Review of attachment 87065 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87066 [details] [review] [next 10/22] tp_connection_manager_get_protocol lost its "_object" suffix Review of attachment 87066 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87067 [details] [review] [next 11/22] Adjust contact features to Telepathy-1 API: quarks, not an enum Review of attachment 87067 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87068 [details] [review] [next 12/22] turn CreateChannelWithHints into CreateChannel Review of attachment 87068 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87069 [details] [review] [next 13/22] switch from Messages to unified Text API Review of attachment 87069 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87070 [details] [review] [next 14/22] Re-namespace remaining o.fd.T names in the actual source code Review of attachment 87070 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87071 [details] [review] [next 15/22] Re-namespace remaining o.fd.T names in tests Review of attachment 87071 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87072 [details] [review] [next 16/22] Fix StreamTube namespaces in regression test .client Review of attachment 87072 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87073 [details] [review] [next 17/22] SucceededWithChannel is just Succeeded now Review of attachment 87073 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87074 [details] [review] [next 18/22] Upgrade to the Hidden1 interface from telepathy-spec 0.99.2 Review of attachment 87074 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87075 [details] [review] [next 19/22] Remove some tests for obsolete functionality Review of attachment 87075 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87076 [details] [review] [next 20/22] SimulatedConnection: simulate Telepathy 1 a bit better Review of attachment 87076 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87077 [details] [review] [next 21/22] AliasesChanged now takes an a{us}, not an a(us) Review of attachment 87077 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 87078 [details] [review] [next 22/22] SimulatedChannel: fill in extra immutable properties Review of attachment 87078 [details] [review]: ----------------------------------------------------------------- ++ All good! Fixed in git for 5.99.2 |
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.