|
Description
Simon McVittie
2013-08-29 16:04:00 UTC
Created attachment 84850 [details] [review] 01/16] McdMaster: use modern idiom for priv Created attachment 84851 [details] [review] 02/16] mcd_master_create_manager: unvirtualize There's no point in this being virtual: there are no subclasses, and it isn't API any more. Created attachment 84852 [details] [review] Remove various useless virtual method padding This part of Mission Control is no longer API. Created attachment 84853 [details] [review] mcd_operation_take_mission, _remove_mission: do not be virtual Nothing overrides them, they're no longer API, and one day I'd like to get rid of McdOperation entirely. Created attachment 84854 [details] [review] 05/16] Remove support for checking conditions on accounts We no longer have API for external transport providers, and we've never defined any conditions on the transport provider we do have. --- I haven't removed the Account.Conditions interface yet, but I probably will. Created attachment 84855 [details] [review] 06/16] McdMaster: remove support for writing account-manager property Nothing constructs a McdMaster with a non-default McdAccountManager. Created attachment 84856 [details] [review] 07/16] McdConnectivityMonitor: take responsibility for monitoring use-conn I want to get rid of the kludge-transport. Created attachment 84857 [details] [review] 08/16] Remove the concept of binding accounts to transports We only have one transport now, so there are two options: either the account is a special one that "needs dispatch" and also bypasses the transport check (in practice this means cellular accounts), or it is connected via the Internet. Binding accounts to transports never made a vast amount of sense. If we have many parallel Internet connections (e.g. wired + wifi + cellular + VPN), Mission Control can't know which one a Connection is relying on - only the connection manager can know that - so we have to err on the side of disconnecting everything on a connectivity change anyway. A possible refinement in future Tp versions would be a way for a CM to tell MC "this connection does its own connectivity-monitoring, so don't kick it when we get disconnected". For now, we assume that everything needs the Internet, unless it's one of the accounts that wouldn't previously have been bound to a connection (i.e. _mcd_account_needs_dispatch() returns FALSE). Created attachment 84858 [details] [review] 09/16] Put the connectivity monitor in the McdAccountManager --- Minor technical debt, repaid later in the patch series: + McdKludgeTransport *self = g_object_new (MCD_TYPE_KLUDGE_TRANSPORT, NULL); + + /* Strictly speaking this should be done with properties, but I'm + * going to delete this class soon anyway. */ + self->priv->minotaur = connectivity_monitor; + tp_g_signal_connect_object (self->priv->minotaur, "state-change", + (GCallback) monitor_state_changed_cb, self, 0); Created attachment 84859 [details] [review] 10/16] McdAccount: have a reference to the connectivity monitor Created attachment 84860 [details] [review] 11/16] Move "waiting for connectivity" functionality into McdAccount Created attachment 84861 [details] [review] 12/16] McdAccount: respond to connectivity status changes Instead of proxying through the transport plugin and the McdMaster, just watch the McdConnectivityMonitor. Much simpler! Created attachment 84862 [details] [review] 13/16] mcd_master_connect_automatic_accounts: move to McdAccountManager Iterating over all accounts? Looks like a job for the account manager. Created attachment 84863 [details] [review] 14/16] _mcd_master_account_replace_transport: inline into caller and simplify There's only one transport plugin, it only has one transport, and its state is basically boolean, so we can delete a lot of code. Created attachment 84864 [details] [review] 15/16] Remove the remains of McdTransportPlugin Created attachment 84865 [details] [review] 16/16] Remove unused signal McdAccount::connection-process Comment on attachment 84850 [details] [review] 01/16] McdMaster: use modern idiom for priv Review of attachment 84850 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84851 [details] [review] 02/16] mcd_master_create_manager: unvirtualize Review of attachment 84851 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84852 [details] [review] Remove various useless virtual method padding Review of attachment 84852 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84853 [details] [review] mcd_operation_take_mission, _remove_mission: do not be virtual Review of attachment 84853 [details] [review]: ----------------------------------------------------------------- you didn't remove those from the .h file as well? Comment on attachment 84854 [details] [review] 05/16] Remove support for checking conditions on accounts Review of attachment 84854 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84855 [details] [review] 06/16] McdMaster: remove support for writing account-manager property Review of attachment 84855 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84856 [details] [review] 07/16] McdConnectivityMonitor: take responsibility for monitoring use-conn Review of attachment 84856 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84857 [details] [review] 08/16] Remove the concept of binding accounts to transports Review of attachment 84857 [details] [review]: ----------------------------------------------------------------- Patch looks ok but that means we'll have to WONTFIX bug #41148 Comment on attachment 84858 [details] [review] 09/16] Put the connectivity monitor in the McdAccountManager Review of attachment 84858 [details] [review]: ----------------------------------------------------------------- ::: src/mcd-account-manager.c @@ +1657,4 @@ > > DEBUG (""); > > + priv->minotaur = mcd_connectivity_monitor_new (); shouldn't you unref it when disposing? Comment on attachment 84859 [details] [review] 10/16] McdAccount: have a reference to the connectivity monitor Review of attachment 84859 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84860 [details] [review] 11/16] Move "waiting for connectivity" functionality into McdAccount Review of attachment 84860 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84861 [details] [review] 12/16] McdAccount: respond to connectivity status changes Review of attachment 84861 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84862 [details] [review] 13/16] mcd_master_connect_automatic_accounts: move to McdAccountManager Review of attachment 84862 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84863 [details] [review] 14/16] _mcd_master_account_replace_transport: inline into caller and simplify Review of attachment 84863 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 84864 [details] [review] 15/16] Remove the remains of McdTransportPlugin Review of attachment 84864 [details] [review]: ----------------------------------------------------------------- ++ nice cleaning! Comment on attachment 84865 [details] [review] 16/16] Remove unused signal McdAccount::connection-process Review of attachment 84865 [details] [review]: ----------------------------------------------------------------- ++ (In reply to comment #20) > mcd_operation_take_mission, _remove_mission: do not be virtual > > you didn't remove those from the .h file as well? Will do. (In reply to comment #24) > 08/16] Remove the concept of binding accounts to transports > > Patch looks ok but that means we'll have to WONTFIX bug #41148 I'm not sure that having MC try to understand account/transport binding ever made sense. I'll reply on that bug. I could see whether I can make MC disconnect and reconnect everything when the default route changes, if that would help? That'd be a good first approximation (and is what this transport API was meant to do, except I'm not sure it worked like that even on Maemo, and it certainly didn't work for our NM/ConnMan support). (In reply to comment #25) > 09/16] Put the connectivity monitor in the McdAccountManager > > shouldn't you unref it when disposing? Maybe. I'll check. (In reply to comment #20) > mcd_operation_take_mission, _remove_mission: do not be virtual > you didn't remove those from the .h file as well? No, that's correct. The functions still exist, and are called from elsewhere - they're just not virtual. (I deleted the current extern mcd_..._mission wrappers, renamed the static base-class implementations from _mcd_..._mission to mcd_..._mission and made them extern.) > (In reply to comment #24) > > 08/16] Remove the concept of binding accounts to transports > > > > Patch looks ok but that means we'll have to WONTFIX bug #41148 > > I'm not sure that having MC try to understand account/transport binding ever > made sense. I'll reply on that bug. Are you convinced by Will's and my reasoning? > (In reply to comment #25) > > 09/16] Put the connectivity monitor in the McdAccountManager > > > > shouldn't you unref it when disposing? > > Maybe. I'll check. Yes, and I should also have unreffed it when a McdAccount is disposed. Fixed that... Created attachment 85130 [details] [review] [08/16 v2] Put the connectivity monitor in the McdAccountManager --- v2: unref it in dispose. Created attachment 85131 [details] [review] [09/16 v2] McdAccount: have a reference to the connectivity monitor --- v2: unref it in dispose. Comment on attachment 85130 [details] [review] [08/16 v2] Put the connectivity monitor in the McdAccountManager Review of attachment 85130 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 85131 [details] [review] [09/16 v2] McdAccount: have a reference to the connectivity monitor Review of attachment 85131 [details] [review]: ----------------------------------------------------------------- ++ (In reply to comment #35) > > (In reply to comment #24) > > > 08/16] Remove the concept of binding accounts to transports > > > > > > Patch looks ok but that means we'll have to WONTFIX bug #41148 > > > > I'm not sure that having MC try to understand account/transport binding ever > > made sense. I'll reply on that bug. > > Are you convinced by Will's and my reasoning? Yep; I closed the bug. Feel free to merge your patches. Merged \o/ |
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.