After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 655799 - Use new client factory
Use new client factory
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
2.33.x
Other Linux
: Normal enhancement
: ---
Assigned To: empathy-maint
empathy-maint
Depends on: 655815
Blocks:
 
 
Reported: 2011-08-02 11:28 UTC by Guillaume Desmottes
Modified: 2011-08-18 10:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Port to new tp-glib client factory (60.01 KB, patch)
2011-08-02 14:20 UTC, Guillaume Desmottes
none Details | Review
Port to new tp-glib client factory (60.16 KB, patch)
2011-08-02 14:57 UTC, Guillaume Desmottes
none Details | Review
Depend on telepathy-glib 0.15.5 (747 bytes, patch)
2011-08-18 09:48 UTC, Guillaume Desmottes
committed Details | Review
Port to new tp-glib client factory (38.41 KB, patch)
2011-08-18 09:48 UTC, Guillaume Desmottes
none Details | Review
Port to new tp-glib client factory (46.31 KB, patch)
2011-08-18 10:15 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2011-08-02 11:28:52 UTC
tp-glib master introduced a new more generic kind of factory. We should use it once it's released.
Comment 1 Guillaume Desmottes 2011-08-02 14:06:25 UTC
Not sure if that's a merge blocker but we should be able to share objects with folks: bug #655815.
Comment 2 Guillaume Desmottes 2011-08-02 14:20:54 UTC
Created attachment 193078 [details] [review]
Port to new tp-glib client factory

- EmpathyChannelFactory has been changed to EmpathyClientFactory and inherit
from TpAutomaticClientFactory.
- We now always use the _with_am variant of TpSimple* constructors
- We always use empathy_account_manager_dup() instead of
tp_account_manager_dup()
- Replace empathy_get_account_for_connection() by tp_connection_get_account()
- The factory is passed to EmpathyTpChat and TpyCallChannel

This should ensure that all the TpProxy and TpContact objects created in
Empathy are shared and use EmpathyClientFactory.
Comment 3 Xavier Claessens 2011-08-02 14:28:52 UTC
(In reply to comment #1)
> Not sure if that's a merge blocker but we should be able to share objects with
> folks: bug #655815.

I would say that's blocker, it would duplicate all TpContact objects, right? Well if not merge blocker, at least 3.2 blocker.
Comment 4 Xavier Claessens 2011-08-02 14:41:23 UTC
Review of attachment 193078 [details] [review]:

::: libempathy/empathy-chatroom-manager.c
@@ +581,3 @@
     }
 
+  am = empathy_account_manager_dup ();

you can use priv->account_manager initialized just above

::: libempathy/empathy-client-factory.c
@@ +173,3 @@
+      TP_SIMPLE_CLIENT_FACTORY (factory));
+  g_object_unref (factory);
+

note that you could cache the manager ptr in static here instead of calling ensure_account_manager() all the time, because that one would lookup into a GHashTable containing all TpProxy subclasses. Or that optimization could be done in TpSimpleClientFactory itself tbh.
Comment 5 Guillaume Desmottes 2011-08-02 14:57:20 UTC
(In reply to comment #4)
> Review of attachment 193078 [details] [review]:
> 
> ::: libempathy/empathy-chatroom-manager.c
> @@ +581,3 @@
>      }
> 
> +  am = empathy_account_manager_dup ();
> 
> you can use priv->account_manager initialized just above

done.

> ::: libempathy/empathy-client-factory.c
> @@ +173,3 @@
> +      TP_SIMPLE_CLIENT_FACTORY (factory));
> +  g_object_unref (factory);
> +
> 
> note that you could cache the manager ptr in static here instead of calling
> ensure_account_manager() all the time, because that one would lookup into a
> GHashTable containing all TpProxy subclasses. Or that optimization could be
> done in TpSimpleClientFactory itself tbh.

done.
Comment 6 Guillaume Desmottes 2011-08-02 14:57:26 UTC
Created attachment 193083 [details] [review]
Port to new tp-glib client factory

- EmpathyChannelFactory has been changed to EmpathyClientFactory and inherit
from TpAutomaticClientFactory.
- We now always use the _with_am variant of TpSimple* constructors
- We always use empathy_account_manager_dup() instead of
tp_account_manager_dup()
- Replace empathy_get_account_for_connection() by tp_connection_get_account()
- The factory is passed to EmpathyTpChat and TpyCallChannel

This should ensure that all the TpProxy and TpContact objects created in
Empathy are shared and use EmpathyClientFactory.
Comment 7 Xavier Claessens 2011-08-03 09:51:02 UTC
Patch looks good, IMO it is ready to be merged once we sorted out bug #655815.
Comment 8 Guillaume Desmottes 2011-08-18 09:48:50 UTC
Created attachment 194112 [details] [review]
Depend on telepathy-glib 0.15.5

Needed for new factory API.
Comment 9 Guillaume Desmottes 2011-08-18 09:48:55 UTC
Created attachment 194113 [details] [review]
Port to new tp-glib client factory

- EmpathyChannelFactory has been changed to EmpathyClientFactory and inherit
from TpAutomaticClientFactory.
- We now always use the _with_am variant of TpSimple* constructors
- We define our own factory as default.
- Replace empathy_get_account_for_connection() by tp_connection_get_account()
- The factory is passed to EmpathyTpChat and TpyCallChannel
- Use tp_simple_client_factory_ensure_account() instead of
tp_account_manager_ensure_account().
- Rely on the factory to prepare connection features.

This should ensure that all the TpProxy and TpContact objects created in
Empathy are shared and use EmpathyClientFactory.
Comment 10 Xavier Claessens 2011-08-18 10:08:45 UTC
Review of attachment 194113 [details] [review]:

missing the new factory files

::: libempathy/empathy-chatroom-manager.c
@@ -605,3 @@
 
-  tp_base_client_add_connection_features_varargs (priv->observer,
-      TP_CONNECTION_FEATURE_CAPABILITIES, NULL);

that feature is not set on the new factory.

::: tests/empathy-chatroom-manager-test.c
@@ +110,3 @@
         { "name2", "room2", FALSE, TRUE }};
 
+  account_manager = tp_account_manager_dup_singleton ();

does that build? s/_singleton// afaik.
Comment 11 Guillaume Desmottes 2011-08-18 10:15:26 UTC
(In reply to comment #10)
> Review of attachment 194113 [details] [review]:
> 
> missing the new factory files

Oooops; added.

> ::: libempathy/empathy-chatroom-manager.c
> @@ -605,3 @@
> 
> -  tp_base_client_add_connection_features_varargs (priv->observer,
> -      TP_CONNECTION_FEATURE_CAPABILITIES, NULL);
> 
> that feature is not set on the new factory.

it is in the EmpathyClientFactory.

> ::: tests/empathy-chatroom-manager-test.c
> @@ +110,3 @@
>          { "name2", "room2", FALSE, TRUE }};
> 
> +  account_manager = tp_account_manager_dup_singleton ();
> 
> does that build? s/_singleton// afaik.

Those disables have been disabled, so yes. Anyway, fixed.
Comment 12 Guillaume Desmottes 2011-08-18 10:15:39 UTC
Created attachment 194114 [details] [review]
Port to new tp-glib client factory

- EmpathyChannelFactory has been changed to EmpathyClientFactory and inherit
from TpAutomaticClientFactory.
- We now always use the _with_am variant of TpSimple* constructors
- We define our own factory as default.
- Replace empathy_get_account_for_connection() by tp_connection_get_account()
- The factory is passed to EmpathyTpChat and TpyCallChannel
- Use tp_simple_client_factory_ensure_account() instead of
tp_account_manager_ensure_account().
- Rely on the factory to prepare connection features.

This should ensure that all the TpProxy and TpContact objects created in
Empathy are shared and use EmpathyClientFactory.
Comment 13 Xavier Claessens 2011-08-18 10:22:32 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > ::: libempathy/empathy-chatroom-manager.c
> > @@ -605,3 @@
> > 
> > -  tp_base_client_add_connection_features_varargs (priv->observer,
> > -      TP_CONNECTION_FEATURE_CAPABILITIES, NULL);
> > 
> > that feature is not set on the new factory.
> 
> it is in the EmpathyClientFactory.

ah, ok. Other features (contacts alias, etc) are still prepared manually I guess?

Branch looks good to me, thanks.
Comment 14 Guillaume Desmottes 2011-08-18 10:30:17 UTC
Attachment 194112 [details] pushed as 7fbf3e0 - Depend on telepathy-glib 0.15.5
Attachment 194114 [details] pushed as 9ddd25f - Port to new tp-glib client factory