GNOME Bugzilla – Bug 696267
Add a provider for chat-only (Telepathy) accounts
Last modified: 2014-04-10 13:53:41 UTC
The intent of the patches submitted under bug #695885 is to provide a Online Accounts view customised for Empathy, by showing only those accounts and providers which provides IM services. The patch attached builds on that feature to replace empathy-accounts by invoking Online Accounts panel. I choose to follow how the UOA panel is invoked. With bug #696054 g-c-c panels will be DBus-activatable and if preferable I could go that route. This is just a first step: the XMPP/IRC/etc. unbranded providers for g-o-a will follow and once they're in place we'll plan how to handle the migration of existing accounts.
Created attachment 239449 [details] [review] accounts-dialog: Launch the Online Accounts panel if using GOA
The code looks good to me. However this is for 3.9 and we did not branch empathy yet AFAIK. So needs to wait before merging. (In reply to comment #0) > This is just a first step: the XMPP/IRC/etc. unbranded providers for g-o-a > will follow and once they're in place we'll plan how to handle the > migration of existing accounts. Is there a GOA bug about this? I don't think we can merge your patch until GOA side is ready. For the migration, empathy already have UOA migration code, we should do similar code for GOA.
(In reply to comment #2) > Is there a GOA bug about this? I don't think we can merge your patch until GOA > side is ready. For the migration, empathy already have UOA migration code, we > should do similar code for GOA. As far as I understand, it is blocking on a bunch of gnome-control-center patches that should land post-3.8.0.
(In reply to comment #3) > (In reply to comment #2) > > Is there a GOA bug about this? I don't think we can merge your patch until GOA > > side is ready. For the migration, empathy already have UOA migration code, we > > should do similar code for GOA. Bug 696281 created. > As far as I understand, it is blocking on a bunch of gnome-control-center > patches that should land post-3.8.0. Yep, sorry for being unclear. I've submitted the patch just to validate the approach, it's still a somewhat long road before being able to merge it. :)
I'm going to work on this in the next month or so.
Comment on attachment 239449 [details] [review] accounts-dialog: Launch the Online Accounts panel if using GOA Marking the patch as obsolete as it's not mergeable yet.
I have a branch to fix this for GOA at: http://cgit.collabora.com/git/user/bari/gnome-online-accounts.git/log/ There are also a few patches for gnome-control-center: http://cgit.collabora.com/git/user/bari/gnome-control-center.git/log/ There are also a few screenshots here: - http://people.collabora.com/~bari/tmp-goa-empathy/accounts.png ← g-c-c with IM accounts. - http://people.collabora.com/~bari/tmp-goa-empathy/edit-connection-parameters.png ← Edit connection parameters dialog - http://people.collabora.com/~bari/tmp-goa-empathy/edit-personal-details.png ← Edit personal details dialog - http://people.collabora.com/~bari/tmp-goa-empathy/add-account1.png ← what you get when you add an account and press other (with all the IM account types visible) - http://people.collabora.com/~bari/tmp-goa-empathy/add-account2.png ← after selecting jabber in the previous dialog The dialogs for the various account types come from empathy and are a bit ugly, but we could make some incremental improvements, maybe starting from xmpp and irc. There is only one feature missing, that is the white list for account types. I couldn't figure out a nice way to implement this, but it shouldn't take long. Bastien would like the avatar button to be in a library, so that the “Users” panel and the goa one could share this code. I will open a bug about this, but I'm not sure in which library it should be (he suggested libgnome-desktop) and I'm not sure if we need this for the first version or if we can do it later (I guess it depends on the version of Gnome we want this bug for).
Attaching the patches from bari/im-accounts here for review.
Created attachment 252346 [details] [review] daemon: avoid ID collisions if accounts are created in the same second
Created attachment 252347 [details] [review] providerfactory: add abstract class to create providers dynamically
Created attachment 252348 [details] [review] provider: add an extension point for GoaProviderFactory implementations
Created attachment 252349 [details] [review] provider: make goa_provider_get_for_provider_type() handle factories too
Created attachment 252350 [details] [review] provider: add goa_provider_get_all_async() and _finish()
Created attachment 252351 [details] [review] provider: add GOA_PROVIDER_GROUP_CHAT
Created attachment 252352 [details] [review] build: add m4/ax_config_dir.m4 for the AX_CONFIG_DIR() function
Created attachment 252353 [details] [review] build: add the telepathy-account-widgets submodule
Created attachment 252354 [details] [review] build: use Telepathy's libraries in goabackend
Created attachment 252355 [details] [review] telepathy: add a stub for the Telepathy provider
Created attachment 252357 [details] [review] telepathy: implement build_object
Created attachment 252358 [details] [review] telepathy: add a switch to enable/disable chat
Created attachment 252359 [details] [review] telepathy: add methods to instantiate the provider with the right values
Created attachment 252361 [details] [review] telepathy: implement get_provider_type() correctly
Review of attachment 252346 [details] [review]: Otherwise looks good. ::: src/daemon/goadaemon.c @@ +704,3 @@ generate_new_id (GoaDaemon *daemon) { + static gint counter = 0; Maybe use guint? @@ +715,3 @@ + counter++; + The extra newline doesn't seem necessary.
Review of attachment 252347 [details] [review]: ::: src/goabackend/goaproviderfactory.h @@ +85,3 @@ + /*< private >*/ + /* Padding for future expansion */ + gpointer goa_reserved[16]; I don't think this is required because this is a private type that is not exported to the outside world. Only goaprovider.h is public. I know that some of the private headers still have these expansion slots, but that is a relic of the past, and should be cleaned up in my opinion. @@ +99,3 @@ + GList **out_providers, + GAsyncResult *result, + GError **error); Any reason the _finish can't return a GList * instead of a gboolean? Returning a GList * looks more idiomatic to me, but I can be convinced otherwise. I am guessing that you are doing this for 2 reasons: (1) To differentiate cases where there has been no error, yet not providers were found. (2) To avoid a deep_copy if the user is not interested in the GList *. For (1), the caller can check the GError instead of the return value. For (2), if you are using GTask then the deep_copy won't be needed, and we can make it mandatory for the caller to free the GList* if it is calling _finish. This is not a blocker, though.
Review of attachment 252348 [details] [review]: Looks good.
Review of attachment 252349 [details] [review]: ::: src/goabackend/goaprovider.c @@ +864,3 @@ GoaProvider *ret; + g_return_val_if_fail (provider_type != NULL, NULL); Should ideally be in a different commit.
(In reply to comment #24) > I know that some of the private headers still have these expansion slots, but > that is a relic of the past, and should be cleaned up in my opinion. Ah ok. I was a bit confused by that and I was not sure what GOA considered public and what not. > @@ +99,3 @@ > + GList > **out_providers, > + GAsyncResult > *result, > + GError > **error); > > Any reason the _finish can't return a GList * instead of a gboolean? Returning > a GList * looks more idiomatic to me, but I can be convinced otherwise. I actually wrote this function to return a GList first (and also some functions in tp-aw). Then I look more around in the code and found other functions (I don't remember where, for sure in empathy, but I think also in GOA or g-c-c) that returned a boolean instead of a GList*, so I thought that returning a boolean would have been more idiomatic and maybe also have other advantages. > I am guessing that you are doing this for 2 reasons: > (1) To differentiate cases where there has been no error, yet not providers > were found. > (2) To avoid a deep_copy if the user is not interested in the GList *. I guess those were the reasons why the code I looked at was doing this. If you prefer I can revert for my old version returning a GList directly. (Tbh I don't really care about 2. It's not a significant performance difference.)
Review of attachment 252350 [details] [review]: ::: src/goabackend/goaprovider.c @@ +1024,3 @@ ensure_builtins_loaded (); + data = g_new0 (GetAllData, 1); As a general rule, when allocating fixed-sized structs g_slice_new0() and g_slice_free() are better. (Obviously this code is not performance-critical!) @@ +1115,3 @@ + * + * Note that this function will run the default main loop while retrieving + * the list of providers. Why? For a truly sync operation shouldn't we be creating a new thread default context and a running a main loop on it? I am guessing that you are doing this to make it user for the GUI. So that it can call goa_provider_get_all and not freeze. Can't we just ask the GUI to use the async variant in that case? ::: src/goabackend/goaprovider.h @@ +175,3 @@ guint goa_provider_get_credentials_generation (GoaProvider *provider); +GList *goa_provider_get_all (void); Should ideally be in a different commit.
(In reply to comment #27) > (In reply to comment #24) > > @@ +99,3 @@ > > + GList > > **out_providers, > > + GAsyncResult > > *result, > > + GError > > **error); > > > > Any reason the _finish can't return a GList * instead of a gboolean? Returning > > a GList * looks more idiomatic to me, but I can be convinced otherwise. > > I actually wrote this function to return a GList first (and also some functions > in tp-aw). Then I look more around in the code and found other functions (I > don't remember where, for sure in empathy, but I think also in GOA or g-c-c) > that returned a boolean instead of a GList*, so I thought that returning a > boolean would have been more idiomatic and maybe also have other advantages. Ok. > > I am guessing that you are doing this for 2 reasons: > > (1) To differentiate cases where there has been no error, yet not providers > > were found. > > (2) To avoid a deep_copy if the user is not interested in the GList *. > > I guess those were the reasons why the code I looked at was doing this. > If you prefer I can revert for my old version returning a GList directly. (Tbh > I don't really care about 2. It's not a significant performance difference.) Don't worry. We can revisit this when we port things to GTask.
Review of attachment 252351 [details] [review]: Passing note unrelated to this bug: we might end up deprecating GoaProviderGroup in future because we now have GoaProviderFeatures.
Review of attachment 252352 [details] [review]: ::: m4/ax_config_dir.m4 @@ +1,2 @@ +dnl Copied from Audacity 1.3.10 which itself is licensed under the GPL v2 or +dnl any later version Do we need to ask permission for using it in a LGPLv2+ code base?
Review of attachment 252353 [details] [review]: Note that I am no expert on Git submodules. :-) ::: autogen.sh @@ +28,3 @@ +cd telepathy-account-widgets +sh autogen.sh --no-configure +cd .. I think this whole snippet can be replaced by: git submodule update --init --recursive. See gnome-photos for an example.
Review of attachment 252354 [details] [review]: Looks good.
Review of attachment 252355 [details] [review]: Looks good.
Review of attachment 252357 [details] [review]: ::: src/goabackend/goatelepathyprovider.c @@ +125,3 @@ + GoaChat *chat = NULL; + gboolean chat_enabled; + gboolean ret = FALSE; It is better to retain the style that is followed all over the code base. ie.: account = NULL; chat = NULL; ret = FALSE; This is how you did it in the previous patch, but now you changed it. :-)
Review of attachment 252358 [details] [review]: ::: src/goabackend/goatelepathyprovider.c @@ +194,3 @@ + _("Use for"), + "chat-disabled", + _("C_hat")); po/POTFILES.in needs an update. :-)
Review of attachment 252359 [details] [review]: ::: src/goabackend/goatelepathyprovider.c @@ +289,3 @@ + GOA_TYPE_TELEPATHY_PROVIDER, GoaTelepathyProviderPrivate); + + provider->priv = priv; We can assign directly to provider->priv. @@ +302,3 @@ + { + if (priv->protocol_name != NULL) + g_error ("You cannot set \"protocol-name\" if you set \"protocol\""); You need to g_free (priv->protocol_name), otherwise you will leak it in the next line.
Review of attachment 252361 [details] [review]: Looks good.
Created attachment 252386 [details] [review] telepathyfactory: add GoaProviderFactory for dynamic Telepathy providers
Created attachment 252387 [details] [review] provider: initialise the Telepathy provider factory
Created attachment 252388 [details] [review] tplinker: add a stub for GoaTpAccountLinker
Created attachment 252389 [details] [review] daemon: start the Telepathy linker when we acquire the bus name
Created attachment 252390 [details] [review] tplinker: keep track of both GOA and Telepathy accounts
Created attachment 252391 [details] [review] tplinker: create GOA accounts for new Telepathy accounts
Created attachment 252392 [details] [review] tplinker: handle deletion of both GOA and Telepathy accounts
Created attachment 252393 [details] [review] tplinker: check for GOA accounts with no corresponding Telepathy account
Created attachment 252395 [details] [review] tplinker: sync the enabled status between Telepathy and GOA accounts
Created attachment 252396 [details] [review] tplinker: make sure all the TpAccounts have the features we need
Created attachment 252397 [details] [review] tplinker: ignore chat accounts handled by other providers
Created attachment 252398 [details] [review] tplinker: allow to ignore some accounts to make debugging easier
Created attachment 252399 [details] [review] tplinker: cache the protocol and icon names in the key file
Created attachment 252400 [details] [review] daemon: set icon and provider name after building the object
Created attachment 252401 [details] [review] telepathy: return a different provider name based on the IM protocol
Created attachment 252402 [details] [review] telepathy: implement the add_account method
Created attachment 252403 [details] [review] telepathy: implement the refresh_account method
Review of attachment 252386 [details] [review]: ::: src/goabackend/goatelepathyfactory.c @@ +53,3 @@ + g_return_if_fail (GOA_IS_TELEPATHY_FACTORY (factory)); + + return (GoaProvider *) goa_telepathy_provider_new_from_protocol_name (provider_name); Was there any reason to use a C-style cast instead of GOA_PROVIDER (...) ?
Review of attachment 252387 [details] [review]: Looks good.
Review of attachment 252350 [details] [review]: ::: src/goabackend/goaprovider.c @@ +1040,3 @@ + /* The extensions are loaded in the reverse order we used in + * ensure_builtins_loaded. */ + g_queue_reverse (&data->ret); Can't we push_head instead of push_tail and reverse?
Review of attachment 252388 [details] [review]: ::: src/daemon/goatpaccountlinker.c @@ +52,3 @@ +{ + return g_object_new (GOA_TYPE_TP_ACCOUNT_LINKER, NULL); +} The public methods are usually after the GObject boilerplate. Can you move it to the end of the file? @@ +60,3 @@ + + if (priv->goa_client == NULL || + priv->account_manager == NULL || Is it really necessary to check priv->account_manager ? It has already been set in goa_tp_account_linker_init. @@ +127,3 @@ +{ + G_OBJECT_CLASS (goa_tp_account_linker_parent_class)->finalize (object); +} This can be removed.
Review of attachment 252389 [details] [review]: Looks good.
Comments #23-#26 done. (In reply to comment #28) > As a general rule, when allocating fixed-sized structs g_slice_new0() and > g_slice_free() are better. (Obviously this code is not performance-critical!) Done. > @@ +1115,3 @@ > + * > + * Note that this function will run the default main loop while retrieving > + * the list of providers. > > Why? For a truly sync operation shouldn't we be creating a new thread default > context and a running a main loop on it? > > I am guessing that you are doing this to make it user for the GUI. So that it > can call goa_provider_get_all and not freeze. Can't we just ask the GUI to use > the async variant in that case? But we cannot remove the function and I don't particularly like the idea of making that function freeze waiting for something async to happen. I wanted to deprecate the sync function, but I forgot (and so I also forgot to use the async one in g-c-c). I can do it now if you want. > ::: src/goabackend/goaprovider.h > @@ +175,3 @@ > guint goa_provider_get_credentials_generation (GoaProvider > *provider); > > +GList *goa_provider_get_all (void); > > Should ideally be in a different commit. Done. (In reply to comment #31) > Review of attachment 252352 [details] [review]: > > ::: m4/ax_config_dir.m4 > @@ +1,2 @@ > +dnl Copied from Audacity 1.3.10 which itself is licensed under the GPL v2 or > +dnl any later version > > Do we need to ask permission for using it in a LGPLv2+ code base? We actually use this macro in LGPL components. I will check. (In reply to comment #32) > Review of attachment 252353 [details] [review]: > > Note that I am no expert on Git submodules. :-) > > ::: autogen.sh > @@ +28,3 @@ > +cd telepathy-account-widgets > +sh autogen.sh --no-configure > +cd .. > > I think this whole snippet can be replaced by: > git submodule update --init --recursive. > > See gnome-photos for an example. I changed it to use git submodule update --init --recursive, but the part you quoted in your comment cannot be removed. gnome-photos doesn't use it as libgd doesn't have a configure.ac, just a Makefile.am. Comments #35-#36 done. (In reply to comment #37) > Review of attachment 252359 [details] [review]: > > ::: src/goabackend/goatelepathyprovider.c > @@ +289,3 @@ > + GOA_TYPE_TELEPATHY_PROVIDER, GoaTelepathyProviderPrivate); > + > + provider->priv = priv; > > We can assign directly to provider->priv. Done. > @@ +302,3 @@ > + { > + if (priv->protocol_name != NULL) > + g_error ("You cannot set \"protocol-name\" if you set \"protocol\""); > > You need to g_free (priv->protocol_name), otherwise you will leak it in the > next line. I changed it to use git submodule update --init --recursive, but the part you quoted in your comment cannot be removed. gnome-photos doesn't use it as libgd doesn't have a configure.ac, just a Makefile.am. (In reply to comment #56) > Review of attachment 252386 [details] [review]: > > ::: src/goabackend/goatelepathyfactory.c > @@ +53,3 @@ > + g_return_if_fail (GOA_IS_TELEPATHY_FACTORY (factory)); > + > + return (GoaProvider *) goa_telepathy_provider_new_from_protocol_name > (provider_name); > > Was there any reason to use a C-style cast instead of GOA_PROVIDER (...) ? Hm, not sure why I did it. Maybe because we are sure it's the right type so there's no need for an extra check. Anyway, I changed it. (In reply to comment #58) > Review of attachment 252350 [details] [review]: > > ::: src/goabackend/goaprovider.c > @@ +1040,3 @@ > + /* The extensions are loaded in the reverse order we used in > + * ensure_builtins_loaded. */ > + g_queue_reverse (&data->ret); > > Can't we push_head instead of push_tail and reverse? At first I used a GList, but the code became much simpler using a GQueue. I think I just forgot to change that bit. It's fixed now.
Review of attachment 252390 [details] [review]: ::: src/daemon/goatpaccountlinker.c @@ +272,3 @@ + g_clear_pointer (&priv->goa_accounts, g_hash_table_unref); + g_clear_pointer (&priv->goa_accounts, g_hash_table_unref); One of them should be priv->tp_accounts.
(In reply to comment #61) Thanks a lot for the quick response! > > @@ +1115,3 @@ > > + * > > + * Note that this function will run the default main loop while retrieving > > + * the list of providers. > > > > Why? For a truly sync operation shouldn't we be creating a new thread default > > context and a running a main loop on it? > > > > I am guessing that you are doing this to make it user for the GUI. So that it > > can call goa_provider_get_all and not freeze. Can't we just ask the GUI to use > > the async variant in that case? > > But we cannot remove the function and I don't particularly like the idea of > making that function freeze waiting for something async to happen. > I wanted to deprecate the sync function, but I forgot (and so I also forgot to > use the async one in g-c-c). I can do it now if you want. There is no urgency to deprecate the sync variant. It can be useful if someone wants to call it from a thread. eg., say someone is implementing a async operation using a *_run_in_thread and wants to use it there. That is why it should be wrapped around a new thread default context so that is a real sync operation. However, if g-c-c is using it from the main thread then it should be using the async variant. > > ::: m4/ax_config_dir.m4 > > @@ +1,2 @@ > > +dnl Copied from Audacity 1.3.10 which itself is licensed under the GPL v2 or > > +dnl any later version > > > > Do we need to ask permission for using it in a LGPLv2+ code base? > > We actually use this macro in LGPL components. I will check. Ok. > > ::: autogen.sh > > @@ +28,3 @@ > > +cd telepathy-account-widgets > > +sh autogen.sh --no-configure > > +cd .. > > > > I think this whole snippet can be replaced by: > > git submodule update --init --recursive. > > > > See gnome-photos for an example. > > I changed it to use git submodule update --init --recursive, but the part you > quoted in your comment cannot be removed. gnome-photos doesn't use it as libgd > doesn't have a configure.ac, just a Makefile.am. Ok.
(In reply to comment #31) > Review of attachment 252352 [details] [review]: > > ::: m4/ax_config_dir.m4 > @@ +1,2 @@ > +dnl Copied from Audacity 1.3.10 which itself is licensed under the GPL v2 or > +dnl any later version > > Do we need to ask permission for using it in a LGPLv2+ code base? It doesn't matter because the m4 file doesn't end up in the binary, is not distributed with the compiled software and is not installed. Just distributing it together with the source is fine according to the GPL (search for mere aggregation in the GPL text).
(In reply to comment #59) > Review of attachment 252388 [details] [review]: > > The public methods are usually after the GObject boilerplate. Can you move it > to the end of the file? Done. > @@ +60,3 @@ > + > + if (priv->goa_client == NULL || > + priv->account_manager == NULL || > > Is it really necessary to check priv->account_manager ? It has already been set > in goa_tp_account_linker_init. tp_account_manager_dup() could in theory fail and return NULL. > @@ +127,3 @@ > +{ > + G_OBJECT_CLASS (goa_tp_account_linker_parent_class)->finalize (object); > +} > > This can be removed. Done. (In reply to comment #62) > Review of attachment 252390 [details] [review]: > > ::: src/daemon/goatpaccountlinker.c > @@ +272,3 @@ > > + g_clear_pointer (&priv->goa_accounts, g_hash_table_unref); > + g_clear_pointer (&priv->goa_accounts, g_hash_table_unref); > > One of them should be priv->tp_accounts. Done.
Review of attachment 252391 [details] [review]: Looks good.
Review of attachment 252392 [details] [review]: Looks good. Thanks for putting those comments.
Review of attachment 252393 [details] [review]: ::: src/daemon/goatpaccountlinker.c @@ +387,3 @@ + NULL, /* cancellable */ + NULL, /* callback */ + NULL); /* user data */ Can't we pass goa_account_removed_by_us_cb as the callback? Might be useful for debugging.
Review of attachment 252395 [details] [review]: ::: src/daemon/goatpaccountlinker.c @@ +104,3 @@ + goa_enabled = !goa_account_get_chat_disabled (goa_account); + + tp_enabled = tp_account_is_enabled (tp_account); You can consider removing the empty newline to make it consistent with tp_account_chat_enabled_changed_cb. @@ +147,3 @@ + priv->block_goa_account_chat_disabled_changed = TRUE; + goa_account_set_chat_disabled (goa_account, !tp_enabled); + priv->block_goa_account_chat_disabled_changed = FALSE; Why not use g_signal_handlers_block_by_func instead?
Review of attachment 252396 [details] [review]: ::: src/daemon/goatpaccountlinker.c @@ +543,3 @@ + + initialized = TRUE; + Here is a crazy idea: what about calling this from class_init?
Review of attachment 252397 [details] [review]: Looks good.
Review of attachment 252398 [details] [review]: Looks good.
(In reply to comment #68) > Can't we pass goa_account_removed_by_us_cb as the callback? Might be useful for > debugging. Done. (In reply to comment #69) > Review of attachment 252395 [details] [review]: > > You can consider removing the empty newline to make it consistent with > tp_account_chat_enabled_changed_cb. Done. > @@ +147,3 @@ > + priv->block_goa_account_chat_disabled_changed = TRUE; > + goa_account_set_chat_disabled (goa_account, !tp_enabled); > + priv->block_goa_account_chat_disabled_changed = FALSE; > > Why not use g_signal_handlers_block_by_func instead? Hm, I thought there was a reason for that, but it works with g_signal_handlers_block_by_func(). Fixed. (In reply to comment #70) > Review of attachment 252396 [details] [review]: > > Here is a crazy idea: what about calling this from class_init? Done.
Review of attachment 252399 [details] [review]: Looks good.
Review of attachment 252400 [details] [review]: Looks good.
Review of attachment 252399 [details] [review]: Oh, wait a minute. Can the values in the key file be really different from the one that we can get from TpawProtocol? Maybe I am missing something.
Review of attachment 252401 [details] [review]: ::: src/goabackend/goatelepathyprovider.c @@ +108,1 @@ Can the value from the key file be really different from the one from TpawProtocol. (The value in the key file seems to be coming from TpAccount.)
(In reply to comment #76) > Review of attachment 252399 [details] [review]: > > Oh, wait a minute. Can the values in the key file be really different from the > one that we can get from TpawProtocol? Maybe I am missing something. The provider get the TpawProtocol only if it's instantiated when creating a new account. That is, g-c-c is going to get GOA to create the provider with goa_telepathy_provider_new_from_protocol(). In the daemon, the providers are loaded from the key file and instantiated with goa_telepathy_provider_new_from_protocol_name(). So there is no TpawProtocol around.
Review of attachment 252402 [details] [review]: Looks good.
Review of attachment 252403 [details] [review]: Looks good.
Created attachment 252525 [details] [review] telepathy: add a button to change the connection parameters
Created attachment 252526 [details] [review] telepathy: add a button to edit personal information
Created attachment 252527 [details] [review] telepathyfactory: filter out Google Talk and Facebook
Created attachment 252528 [details] [review] utils: add goa_utils_init_icon_paths()
Created attachment 252529 [details] [review] provider: initialise the icon paths in _init()
Created attachment 252531 [details] [review] daemon: initialise the icon paths
Created attachment 252532 [details] [review] telepathy: implement get_provider_icon()
Review of attachment 252525 [details] [review]: Looks good to me, assuming that the designers are fine with it.
(In reply to comment #63) > There is no urgency to deprecate the sync variant. It can be useful if someone > wants to call it from a thread. eg., say someone is implementing a async > operation using a *_run_in_thread and wants to use it there. That is why it > should be wrapped around a new thread default context so that is a real sync > operation. Fixed in http://cgit.collabora.com/git/user/bari/gnome-online-accounts.git/commit/?id=2bcde555da1b4f3acdba773fd8542df9a8fcdbf8
Review of attachment 252526 [details] [review]: Looks good to me, assuming that the designers are happy with it.
Review of attachment 252527 [details] [review]: ::: src/goabackend/goatelepathyfactory.c @@ +85,3 @@ + facebook_quark = g_quark_from_static_string ("facebook"); + google_talk_quark = g_quark_from_static_string ("google-talk"); I wonder if we can avoid keeping this list updated manually. eg., I like the way we check based on the storage for incoming TpAccounts. This is not a blocker. :-)
Review of attachment 252529 [details] [review]: I think the 3 goa_utils_init_icon_paths patches can be squashed together. They are small enough and should still be easily understandable. ::: src/goabackend/goaprovider.c @@ +152,3 @@ + /* Make sure that the icon paths are initialized so that + * goa_provider_get_provider_icon() will always work */ + goa_utils_init_icon_paths (); What about moving it to the class_init? Isn't there something called a shared library constructor? Can't we do it from the constructor of libgoabacked.so? Upto you.
Review of attachment 252528 [details] [review]: See previous comment.
Review of attachment 252531 [details] [review]: See previous comment.
Review of attachment 252532 [details] [review]: Looks good.
Review of attachment 252527 [details] [review]: ::: src/goabackend/goatelepathyfactory.c @@ +106,3 @@ +#endif + + provider = goa_telepathy_provider_new_from_protocol (l->data); Should be "protocol" instead of "l->data".
(In reply to comment #78) > (In reply to comment #76) > > Review of attachment 252399 [details] [review] [details]: > > > > Oh, wait a minute. Can the values in the key file be really different from the > > one that we can get from TpawProtocol? Maybe I am missing something. > > The provider get the TpawProtocol only if it's instantiated when creating a new > account. That is, g-c-c is going to get GOA to create the provider with > goa_telepathy_provider_new_from_protocol(). > > In the daemon, the providers are loaded from the key file and instantiated with > goa_telepathy_provider_new_from_protocol_name(). So there is no TpawProtocol > around. I see. Am I right in understanding that either priv->protocol or priv->protocol_name is valid in GoaTelepathyProvider? So without the entry in the key file and a TpawProtocol, we will atleast have priv->protocol_name to fallback on. Do we actually have a case where the service-specific icon is not the same as the protocol icon? Or are we just trying to guard against the future? We already have native GOA providers for things where this could be a problem -- Google / Facebook vs Jabber. Is there anything else? I am trying to avoid adding yet another (we already had some, which should be fixed) call to g_key_file_load_from_file from a sync method that can be running in the main thread unless it is really important.
Review of attachment 252352 [details] [review]: Changing the status to ACN based on Marco's reply.
(In reply to comment #91) > Review of attachment 252527 [details] [review]: > > ::: src/goabackend/goatelepathyfactory.c > @@ +85,3 @@ > > + facebook_quark = g_quark_from_static_string ("facebook"); > + google_talk_quark = g_quark_from_static_string ("google-talk"); > > I wonder if we can avoid keeping this list updated manually. eg., I like the > way we check based on the storage for incoming TpAccounts. Hm, I don't think so. I have no ideas at the moment. (In reply to comment #92) > Review of attachment 252529 [details] [review]: > > I think the 3 goa_utils_init_icon_paths patches can be squashed together. They > are small enough and should still be easily understandable. Ok. > ::: src/goabackend/goaprovider.c > @@ +152,3 @@ > + /* Make sure that the icon paths are initialized so that > + * goa_provider_get_provider_icon() will always work */ > + goa_utils_init_icon_paths (); > > What about moving it to the class_init? > > Isn't there something called a shared library constructor? Can't we do it from > the constructor of libgoabacked.so? Upto you. For now I haven't move it. I will just check if you can have functions called when loading a shared library, but I have no idea if it's possible (why would lots of libraries have _init() functions if it were possible?). (In reply to comment #96) > Review of attachment 252527 [details] [review]: > > ::: src/goabackend/goatelepathyfactory.c > @@ +106,3 @@ > +#endif > + > + provider = goa_telepathy_provider_new_from_protocol (l->data); > > Should be "protocol" instead of "l->data". Done. (In reply to comment #97) > I see. Am I right in understanding that either priv->protocol or > priv->protocol_name is valid in GoaTelepathyProvider? So without the entry in > the key file and a TpawProtocol, we will atleast have priv->protocol_name to > fallback on. Almost. protocol_name is always valid; the _constructed method sets priv->protocol_name from priv->protocol. > Do we actually have a case where the service-specific icon is not the same as > the protocol icon? Or are we just trying to guard against the future? We > already have native GOA providers for things where this could be a problem -- > Google / Facebook vs Jabber. Is there anything else? The main cases are Google and Facebook, but we filter those. It would be possible that Facebook or Google show up in the list if GOA was compiled with those disabled. > I am trying to avoid adding yet another (we already had some, which should be > fixed) call to g_key_file_load_from_file from a sync method that can be running > in the main thread unless it is really important. Hm, I will think about it.
Created attachment 252559 [details] [review] daemon: avoid ID collisions if accounts are created in the same second
Created attachment 252560 [details] [review] providerfactory: add abstract class to create providers dynamically
Created attachment 252561 [details] [review] provider: add an extension point for GoaProviderFactory implementations
Created attachment 252562 [details] [review] provider: make goa_provider_get_for_provider_type() handle factories too
Created attachment 252563 [details] [review] provider: check that provider_type is not NULL in get_for_provider_type()
Created attachment 252564 [details] [review] provider: add goa_provider_get_all_async() and _finish() This also reimplements goa_provider_get_all() to use goa_provider_get_all_async().
Created attachment 252565 [details] [review] provider: style: align the declaration of _get_all() with the others
Created attachment 252566 [details] [review] provider: add GOA_PROVIDER_GROUP_CHAT
Created attachment 252567 [details] [review] build: add m4/ax_config_dir.m4 for the AX_CONFIG_DIR() function
Created attachment 252568 [details] [review] build: add the telepathy-account-widgets submodule
Created attachment 252569 [details] [review] build: use Telepathy's libraries in goabackend
Created attachment 252570 [details] [review] telepathy: add a stub for the Telepathy provider
Created attachment 252571 [details] [review] telepathy: implement build_object
Created attachment 252572 [details] [review] telepathy: add a switch to enable/disable chat
Created attachment 252573 [details] [review] POTFILES.in: add goatelepathyprovider.c
Created attachment 252574 [details] [review] telepathy: add methods to instantiate the provider with the right values
Created attachment 252575 [details] [review] telepathy: implement get_provider_type() correctly
Created attachment 252576 [details] [review] telepathyfactory: add GoaProviderFactory for dynamic Telepathy providers
Created attachment 252577 [details] [review] provider: initialise the Telepathy provider factory
Created attachment 252578 [details] [review] tplinker: add a stub for GoaTpAccountLinker
Created attachment 252579 [details] [review] daemon: start the Telepathy linker when we acquire the bus name
Created attachment 252580 [details] [review] tplinker: keep track of both GOA and Telepathy accounts
Created attachment 252581 [details] [review] tplinker: create GOA accounts for new Telepathy accounts
Created attachment 252582 [details] [review] tplinker: handle deletion of both GOA and Telepathy accounts
Created attachment 252583 [details] [review] tplinker: check for GOA accounts with no corresponding Telepathy account A Telepathy account could be deleted while goa-daemon is not running, so we need to check for that and delete the GOA account too.
Created attachment 252584 [details] [review] tplinker: sync the enabled status between Telepathy and GOA accounts
Created attachment 252585 [details] [review] tplinker: make sure all the TpAccounts have the features we need
Created attachment 252586 [details] [review] tplinker: ignore chat accounts handled by other providers We don't want chat accounts handled by other providers (like Facebook or Google ones) to appear twice, once through the “telepathy” provider and once through the service-specific one.
Created attachment 252587 [details] [review] tplinker: allow to ignore some accounts to make debugging easier If $GOA_TELEPATHY_DEBUG_ACCOUNT_FILTER is set then only accounts containing that string in their ID are handled.
Created attachment 252588 [details] [review] tplinker: cache the protocol and icon names in the key file
Created attachment 252589 [details] [review] daemon: set icon and provider name after building the object If provider name and type can change based on the GoaObject, but, if the methods are called before the object is built, there is no way to do anything useful.
Created attachment 252590 [details] [review] telepathy: return a different provider name based on the IM protocol
Created attachment 252591 [details] [review] telepathy: implement the add_account method
Created attachment 252592 [details] [review] telepathy: implement the refresh_account method
Created attachment 252594 [details] [review] telepathy: add a button to change the connection parameters
Created attachment 252595 [details] [review] telepathy: add a button to edit personal information
Created attachment 252596 [details] [review] telepathyfactory: filter out Google Talk and Facebook
Created attachment 252597 [details] [review] utils: add goa_utils_init_icon_paths()
Created attachment 252598 [details] [review] telepathy: implement get_provider_icon()
Created attachment 252599 [details] [review] DO NOT MERGE! WIP to fixup with: tplinker: sync the enabled status between Telepathy and GOA accounts == WORK IN PROGRESS, THIS NEEDS TO BE SQUASHED == Rishi, I left this patch not squashed with the "sync the enabled status..." one so you can take a look at it separately before I squash it. This fixes a small bug I haven't noticed before as it depends on some timings that can vary.
Created attachment 252600 [details] [review] DO NOT MERGE: fixup! telepathy: add a button to change the connection parameters
Created attachment 252601 [details] [review] DO NOT MERGE: squash! telepathy: add a button to edit personal information
(In reply to comment #99) > > What about moving it to the class_init? > > > > Isn't there something called a shared library constructor? Can't we do it from > > the constructor of libgoabacked.so? Upto you. > > For now I haven't move it. I will just check if you can have functions called > when loading a shared library, but I have no idea if it's possible (why would > lots of libraries have _init() functions if it were possible?). Yes, it's possible, and that's why g_type_init() has been deprecated. This does not mean that it is going to be simple, as the G_DEFINE_CONSTRUCTOR() macro is not exported by GLib as it may need compiler-by-compiler adaptations: For reference, here is how GType uses it: https://git.gnome.org/browse/glib/tree/gobject/gtype.c#n4309 And here's the commit that introduce the macro: https://git.gnome.org/browse/glib/commit/?id=968f4e8d
Review of attachment 252559 [details] [review]: Looks good.
Comment on attachment 252559 [details] [review] daemon: avoid ID collisions if accounts are created in the same second Looks good.
Created attachment 252607 [details] [review] utils: add goa_utils_init_icon_paths()
Comment on attachment 252560 [details] [review] providerfactory: add abstract class to create providers dynamically Looks good.
(In reply to comment #142) > Yes, it's possible, and that's why g_type_init() has been deprecated. > > This does not mean that it is going to be simple, as the G_DEFINE_CONSTRUCTOR() > macro is not exported by GLib as it may need compiler-by-compiler adaptations: Ok, then I just moved the call to _class_init :)
Comment on attachment 252561 [details] [review] provider: add an extension point for GoaProviderFactory implementations Looks good. Unchanged from last round.
Comment on attachment 252562 [details] [review] provider: make goa_provider_get_for_provider_type() handle factories too Looks good.
Comment on attachment 252563 [details] [review] provider: check that provider_type is not NULL in get_for_provider_type() Looks good.
Comment on attachment 252564 [details] [review] provider: add goa_provider_get_all_async() and _finish() Looks good.
Comment on attachment 252565 [details] [review] provider: style: align the declaration of _get_all() with the others Looks good.
Comment on attachment 252566 [details] [review] provider: add GOA_PROVIDER_GROUP_CHAT Looks good. Unchanged from the last round.
Comment on attachment 252567 [details] [review] build: add m4/ax_config_dir.m4 for the AX_CONFIG_DIR() function Looks good. Unchanged from the last round.
Comment on attachment 252568 [details] [review] build: add the telepathy-account-widgets submodule Looks good.
Comment on attachment 252569 [details] [review] build: use Telepathy's libraries in goabackend Looks good. Unchanged from the last round.
Comment on attachment 252570 [details] [review] telepathy: add a stub for the Telepathy provider Looks good. Unchanged from the last round.
Comment on attachment 252571 [details] [review] telepathy: implement build_object Looks good.
Comment on attachment 252572 [details] [review] telepathy: add a switch to enable/disable chat Looks good.
Comment on attachment 252573 [details] [review] POTFILES.in: add goatelepathyprovider.c Looks good.
Review of attachment 252574 [details] [review]: You forgot one small bit from the last review. :-) ::: src/goabackend/goatelepathyprovider.c @@ +304,3 @@ + { + if (priv->protocol_name != NULL) + g_error ("You cannot set \"protocol-name\" if you set \"protocol\""); You need to g_free (priv->protocol_name), otherwise you will leak it in the next line.
Comment on attachment 252575 [details] [review] telepathy: implement get_provider_type() correctly Looks good.
Comment on attachment 252576 [details] [review] telepathyfactory: add GoaProviderFactory for dynamic Telepathy providers Looks good.
Comment on attachment 252577 [details] [review] provider: initialise the Telepathy provider factory Looks good. Unchanged from the last round.
Comment on attachment 252578 [details] [review] tplinker: add a stub for GoaTpAccountLinker Looks good.
Comment on attachment 252579 [details] [review] daemon: start the Telepathy linker when we acquire the bus name Looks good. Unchanged from the last round.
Comment on attachment 252580 [details] [review] tplinker: keep track of both GOA and Telepathy accounts Looks good.
Comment on attachment 252581 [details] [review] tplinker: create GOA accounts for new Telepathy accounts Looks good.
Comment on attachment 252582 [details] [review] tplinker: handle deletion of both GOA and Telepathy accounts Looks good. Unchanged from the last round.
Comment on attachment 252583 [details] [review] tplinker: check for GOA accounts with no corresponding Telepathy account Looks good.
Comment on attachment 252584 [details] [review] tplinker: sync the enabled status between Telepathy and GOA accounts Looks good.
Comment on attachment 252585 [details] [review] tplinker: make sure all the TpAccounts have the features we need Looks good.
Comment on attachment 252586 [details] [review] tplinker: ignore chat accounts handled by other providers Looks good. Unchanged from the last round.
Comment on attachment 252587 [details] [review] tplinker: allow to ignore some accounts to make debugging easier Looks good. Unchanged from the last round.
(In reply to comment #99) > > Do we actually have a case where the service-specific icon is not the same as > > the protocol icon? Or are we just trying to guard against the future? We > > already have native GOA providers for things where this could be a problem -- > > Google / Facebook vs Jabber. Is there anything else? > > The main cases are Google and Facebook, but we filter those. > It would be possible that Facebook or Google show up in the list if GOA was > compiled with those disabled. We can ignore that case. - If the distributor really wanted to disable Google and Facebook integration, it would have disabled them in Telepathy too so they don't show up. - If it is just someone who is playing with the compile-time options in GOA, then he/she can live with non-ideal icons.
Review of attachment 252588 [details] [review]: There is a pending issue from the last round. ::: src/daemon/goatpaccountlinker.c @@ +187,3 @@ + tp_account_get_protocol_name (tp_account)); + g_variant_builder_add (&details, "{ss}", "IconName", + tp_account_get_icon_name (tp_account)); I think we should not keep them in the key file because I haven't seen a case where the values guessed from the protocol_name are so bad that they will confuse the user. Historically, the 2nd GoaObject parameter was added to get_provider_icon and get_provider_name to possibly differentiate between ordinary Google accounts and Google Apps accounts. However we never did that, so they have remained unused.
Comment on attachment 252589 [details] [review] daemon: set icon and provider name after building the object Looks good. Unchanged from the last round.
Review of attachment 252590 [details] [review]: There is a pending issue from the last round. ::: src/goabackend/goatelepathyprovider.c @@ +105,3 @@ + return ret; + } + } I think we should simplify this to: return g_strdup (tpaw_protocol_name_to_display_name (priv->protocol_name));
Comment on attachment 252591 [details] [review] telepathy: implement the add_account method Looks good. Unchanged from the last round.
Comment on attachment 252592 [details] [review] telepathy: implement the refresh_account method Looks good. Unchanged from the last round.
Comment on attachment 252594 [details] [review] telepathy: add a button to change the connection parameters Looks good. Unchanged from the last round.
Comment on attachment 252595 [details] [review] telepathy: add a button to edit personal information Looks good. Unchanged from the last round.
Comment on attachment 252596 [details] [review] telepathyfactory: filter out Google Talk and Facebook Looks good.
Review of attachment 252598 [details] [review]: ::: src/goabackend/goatelepathyprovider.c @@ +251,3 @@ + else + return tpaw_protocol_icon_name (priv->protocol_name); +} This is the same issue about whether we should be keeping IconName in the key file or not. I think we shouldn't and this should be simply: return tpaw_protocol_icon_name (priv->protocol_name);
Comment on attachment 252607 [details] [review] utils: add goa_utils_init_icon_paths() Looks good.
Review of attachment 252599 [details] [review]: Ok, fine. Feel free to squash it.
Review of attachment 252600 [details] [review]: Ok, fine. Feel free to squash it.
Review of attachment 252601 [details] [review]: Ok, fine. Feel free to squash it.
Review of attachment 252574 [details] [review]: Oh, sorry. That is a g_error which will abort the process.
Created attachment 252623 [details] [review] telepathy: return a different provider name based on the IM protocol
Created attachment 252625 [details] [review] telepathy: implement get_provider_icon()
Created attachment 252627 [details] [review] telepathy: implement get_provider_icon()
Created attachment 252628 [details] [review] POTFILES.skip: ignore files in telepathy-account-widgets/
Review of attachment 252623 [details] [review]: Looks good.
Review of attachment 252627 [details] [review]: Looks good.
Review of attachment 252628 [details] [review]: Nice. I wouldn't have thought of that.
Created attachment 252637 [details] [review] goadaemon: use goa_provider_get_all_async() instead of the sync version
Created attachment 252638 [details] [review] goadaemon: use goa_provider_get_all_async() instead of the sync version
Created attachment 252733 [details] [review] configure.ac: pass --with-icondir to tpaw's configure
Created attachment 252734 [details] [review] configure.ac: pass --with-gettext-package to tpaw's configure
Created attachment 252735 [details] [review] WIP! fixup! telepathy: implement the add_account method Rishi, this makes the UI look more like Allan wants it.
Created attachment 252736 [details] [review] telepathy: improve the spacing in the various dialogs
Created attachment 252737 [details] [review] telepathy: add a hack to avoid the personal details dialog from moving The personal details dialog loads its content asynchronously and then adapts its size to the content. This means that sometimes you can see the dialog moving after the content is loaded. This hack just adds a small timeout (not noticeable by the user) to hide the problem. The is no guarantee that the timeout is long enough, but it should be enough for most users (and a longer timeout would be noticeable by the user). In a future version we should add a way for the user info panel to signal us when the content is loaded. Alternatively we could create the panel when an account is selected, before the user clicks on the "Personal details" button.
Review of attachment 252638 [details] [review]: Looks good.
Review of attachment 252733 [details] [review]: Looks good.
Review of attachment 252734 [details] [review]: Looks good.
Created attachment 252748 [details] [review] provider: make goa_provider_get_all() async
Review of attachment 252735 [details] [review]: Ok, fine. You can squash it now.
Review of attachment 252736 [details] [review]: Looks good.
Review of attachment 252737 [details] [review]: ::: src/goabackend/goatelepathyprovider.c @@ +625,3 @@ + + g_main_loop_quit (data->loop); + return FALSE; Please use G_SOURCE_REMOVE instead of FALSE. It is more readable.
Review of attachment 252748 [details] [review]: Looks good. Please bump the gnome-control-center dependency on gnome-online-accounts to 3.9.90.
Created attachment 252753 [details] [review] telepathy: add a hack to avoid the personal details dialog from moving The personal details dialog loads its content asynchronously and then adapts its size to the content. This means that sometimes you can see the dialog moving after the content is loaded. This hack just adds a small timeout (not noticeable by the user) to hide the problem. The is no guarantee that the timeout is long enough, but it should be enough for most users (and a longer timeout would be noticeable by the user). In a future version we should add a way for the user info panel to signal us when the content is loaded. Alternatively we could create the panel when an account is selected, before the user clicks on the "Personal details" button.
Review of attachment 252753 [details] [review]: Looks good.
Merged :) 28f38a78522d613e84c71be67d6a603a6a5d405b is the most recent commit in this branch.
*** Bug 653269 has been marked as a duplicate of this bug. ***
*** Bug 727965 has been marked as a duplicate of this bug. ***