Bug 38142

What I don't like with current code:

1) TpBasicProxyFactory is supposed to _implement_ TpClientChannelFactoryInterface, but it actually have no code at all, since the interface already implement the methods. I think the interface should print warning and return NULL if one of its method is called but not actually implemented by the object.

2) TpAutomaticProxyFactory and TpBasicProxyFactory are separate objects implementing the same interfaces. That means that if we want to add a TpClientAccountFactoryInterface it will have to be reimplemented in both objects, even though Automatic would have nothing to add compared to Basic in that case I think. IMO TpAutomaticProxyFactory should be SUBCLASS of TpBasicProxyFactory, in a way that if the Automatic has nothing to add to the Basic behaviour, it simply fallback to parent implementation.

3) struct _TpBasicProxyFactory {GObject parent;}; --> no place to add a "gpointer priv" without breaking ABI. But we need to add a priv struct for holding the proxy cache for uniqueness...

4) it mentions "proxy" in the name reffering to TpProxy, but if we want to use it for TpContact that's not true... no a real issue though :)

What I suggest:

1) create TpSimpleClientFactory, subclass of GObject, that will have virtual methods like _create_channel(); _create_connection(); _dup_features_for_channel(); with default implementation to return tp_channel_new(); tp_connection_new(). It will also have a TpProxy *tp_simple_client_factory_proxy_lookup(self, object_path); that gives the cached proxy if already exists. Technically that would be a 'protected' method in C++ terms used by subclasses that reimplement some methods to see if a proxy already exists, but doesn't hurt to be public.

2) create TpAutomaticClientFactory, subclass of TpSimpleClientFactory, that will reimplement _create_channel() returning tp_text_channel_new() and fallback to chainup to parent implementation (just like we chainup in GObject constructed, dispose, etc). It will also reimplement _dup_features_for_channel(); by first chaining up to parent to get the CORE features, then add more into the array before retuning.

3) TpUserClientFactory, subclass of TpAutomaticClientFactory, that will reimplement _dup_features_for_connection() by first chaining up to parent impl to get the features from the automatic factory, then add features asked by user using something like tp_user_client_factory_add_features_for_connections().

4) EmpathyClientFactory, subclass of TpUserClientFactory, that will do reimplement methods for creating EmpathyTpChat and EmpathyContact subclasses, but relies on parent implementation for all the rest.



This proposal does not need interfaces, all is virtual methods. It has the disadvantage of replacing the limited factory code we already have (they would be deprecated). I personally prefer my design but I'm surely open to comments :)

Forgot that we'll also need to add methods like

tp_proxy_set_factory(TpProxy *proxy, TpSimpleClientFactory *factory);

that will be called by the factory when creating a new proxy subclass (TpConnection, TpChannel, etc). Like that TpAccount would use that factory to create its TpConnection object, and prepare it.


With my proposal, a typical app would do (supposing my contact-list branch is merged and uses factories)

factory = tp_user_factory_dup_singleton();
tp_user_factory_add_account_manager_features (factory, [TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT]);
tp_user_factory_add_account_features (factory, [TP_ACCOUNT_FEATURE_CONNECTION]);
tp_user_factory_add_connection_features (factory, [TP_CONNECTION_FEATURE_CONTACT_LIST]);
tp_user_factory_add_contact_features (factory, [TP_CONTACT_FEATURE_ALIAS]);


manager = tp_user_factory_create_account_manager(factory);
manager_features = tp_user_factory_dup_account_manager_features(factory, manager);
tp_proxy_prepare_async (manager, manager_features, callback);


void callback(manager)
{
  /* everything is ready as described by the factory, so we can iterate on manager's accounts, connection, contacts, all with sync APIs and no other callback */
}

Oh, and finally: TpProxy strong-ref its factory, but factories weak-ref its proxies (and also drop from its cache on invalidated).

(In reply to comment #1)
> What I don't like with current code:
> 
> 1) TpBasicProxyFactory is supposed to _implement_
> TpClientChannelFactoryInterface, but it actually have no code at all, since the
> interface already implement the methods. I think the interface should print
> warning and return NULL if one of its method is called but not actually
> implemented by the object.

That's an ABI break; previously-working code can stop working.

In general, a new virtual method added to an interface or base class can have either of these semantics:

* Method is optional, interface/base class does something
  vaguely sensible (typically "do nothing" or "raise GError", but in
  this case "instantiate basic object") if it wasn't provided, callers of
  the new virtual method don't need to check before calling it

* Method is optional, callers of the method must check (somehow) before
  calling it and not call it if it's unimplemented/unsuitable, which means
  there must be some way to check whether it's implemented or not
  (capability flags, maybe)

It can't be mandatory without breaking the interface's ABI.

> IMO TpAutomaticProxyFactory should be SUBCLASS of
> TpBasicProxyFactory

ABI break, unless sizeof (TpBasicProxyFactory) == sizeof (TpAutomaticProxyFactory.parent) (i.e. no priv pointer), in which case it's OK.

Happily, TpBasicProxyFactory has no priv pointer, so this rearrangement would be an option.

> 3) struct _TpBasicProxyFactory {GObject parent;}; --> no place to add a
> "gpointer priv" without breaking ABI. But we need to add a priv struct for
> holding the proxy cache for uniqueness...

You can do private access the slow way, without a cache (priv): just call G_OBJECT_INSTANCE_GET_PRIVATE_DATA (or whatever the macro is called) every time you need to get it.

The advantage of using a GInterface (as opposed to an abstract or non-abstract base class) for, well, interfaces, is that you can multiple-inherit from interfaces but you can't multiple-inherit from objects. An object can implement ClientChannelFactory and ClientConnectionFactory simultaneously.

If we use a base class for factories, that base class has to know about every flavour of of proxy that there will ever be. (It can 

> 1) create TpSimpleClientFactory, subclass of GObject, that will have virtual
> methods like _create_channel(); _create_connection();
> _dup_features_for_channel(); with default implementation to return
> tp_channel_new(); tp_connection_new(). It will also have a TpProxy
> *tp_simple_client_factory_proxy_lookup(self, object_path); that gives the
> cached proxy if already exists.

This could just be TpBasicProxyFactory, probably.

> Technically that would be a 'protected' method
> in C++ terms used by subclasses that reimplement some methods to see if a proxy
> already exists, but doesn't hurt to be public.

protected and public are very similar, in practice - they're part of your API and ABI (because library consumers can call them).

> 3) TpUserClientFactory, subclass of TpAutomaticClientFactory, that will
> reimplement _dup_features_for_connection() by first chaining up to parent impl
> to get the features from the automatic factory, then add features asked by user
> using something like tp_user_client_factory_add_features_for_connections().

If the user is going to set their desired features via methods, there's no need for a subclass - just fold this into the Automatic (or even Basic) class. The user will have to create their own instance, call methods on it, then pass it around (instead of relying on a singleton-generating function like tp_dup_default_proxy_factory() or something), but they'd have to do that anyway.

If the user is going to set their desired features via subclassing then they might as well subclass the Automatic or Basic class directly.

> 4) EmpathyClientFactory, subclass of TpUserClientFactory, that will do
> reimplement methods for creating EmpathyTpChat and EmpathyContact subclasses,
> but relies on parent implementation for all the rest.

This probably does want to be a subclass.

> This proposal does not need interfaces, all is virtual methods.

Which is good because?

(In reply to comment #2)
> factory = tp_user_factory_dup_singleton();
> tp_user_factory_add_account_manager_features (factory,
> [TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT]);

Why is it a singleton if it has state?

(In reply to comment #0)
> Maybe TpContact as well (not technically a proxy, but same concept, and could
> open the door for EmpathyContact being subclass of TpContact).

It'd be useful to have the factory manufacture contacts in order to request features, even if we don't allow subclassing.

One reason we don't allow subclassing is that allowing independent construction is incompatible with per-(connection,handle) uniqueness of contact objects, but factories can help to take care of uniqueness. Another reason to not allow independent construction is to make contacts guaranteed-invisible to user code until they've been set up enough for their handle/identifier to be known and persistent.

If instead of a virtual method to construct a Contact, we have a (class property that is|class method that returns) a GType that extends TP_TYPE_CONTACT, then we can have subclassing without independent construction (and also guarantee that all the contacts on a particular connection are of the same class, which would reduce confusion!).

> 2) Do we want it to be able to upgrade all its existing proxies with more
> features when the user provide them? tpqt4 only let user add features to be
> prepared on new proxies, but existing ones would need to be upgraded manually
> if user need it.

Unless we have a concrete use-case for add_features_async(), I'd vote for tpqt4's behaviour (which keeps adding features as a synchronous operation).

(In reply to comment #2)
> Forgot that we'll also need to add methods like
> 
> tp_proxy_set_factory(TpProxy *proxy, TpSimpleClientFactory *factory);

I think I'd prefer a construct-time property - proxies that can change factory mid-life seem likely to be confusing. (Unfortunately, TpChannelRequest already has a mutable factory, as does TpAccountChannelRequest - although the latter could inherit its default factory from the TpAccount, maybe?)

(In reply to comment #4)
> (In reply to comment #1)
> > What I don't like with current code:
> > 
> > 1) TpBasicProxyFactory is supposed to _implement_
> > TpClientChannelFactoryInterface, but it actually have no code at all, since the
> > interface already implement the methods. I think the interface should print
> > warning and return NULL if one of its method is called but not actually
> > implemented by the object.
> 
> That's an ABI break; previously-working code can stop working.
> 
> In general, a new virtual method added to an interface or base class can have
> either of these semantics:
> 
> * Method is optional, interface/base class does something
>   vaguely sensible (typically "do nothing" or "raise GError", but in
>   this case "instantiate basic object") if it wasn't provided, callers of
>   the new virtual method don't need to check before calling it
> 
> * Method is optional, callers of the method must check (somehow) before
>   calling it and not call it if it's unimplemented/unsuitable, which means
>   there must be some way to check whether it's implemented or not
>   (capability flags, maybe)
> 
> It can't be mandatory without breaking the interface's ABI.

Right, I know that would be ABI break. That's one (small) reason why my proposal does not reuse those API but add completely new and deprecate the rest.

> > IMO TpAutomaticProxyFactory should be SUBCLASS of
> > TpBasicProxyFactory
> 
> ABI break, unless sizeof (TpBasicProxyFactory) == sizeof
> (TpAutomaticProxyFactory.parent) (i.e. no priv pointer), in which case it's OK.
> 
> Happily, TpBasicProxyFactory has no priv pointer, so this rearrangement would
> be an option.

I know that's an ABI break, just like above, that's another reason to create new API instead of reusing this.

> > 3) struct _TpBasicProxyFactory {GObject parent;}; --> no place to add a
> > "gpointer priv" without breaking ABI. But we need to add a priv struct for
> > holding the proxy cache for uniqueness...
> 
> You can do private access the slow way, without a cache (priv): just call
> G_OBJECT_INSTANCE_GET_PRIVATE_DATA (or whatever the macro is called) every time
> you need to get it.

as above: yet another reason to deprecate those API and add new one instead of trying to extend it and be annoyed by ABI constrains.

> The advantage of using a GInterface (as opposed to an abstract or non-abstract
> base class) for, well, interfaces, is that you can multiple-inherit from
> interfaces but you can't multiple-inherit from objects. An object can implement
> ClientChannelFactory and ClientConnectionFactory simultaneously.
> 
> If we use a base class for factories, that base class has to know about every
> flavour of of proxy that there will ever be. (It can 

TpBaseClientFactory surely knows about all flavors of TpProxy. If app (empathy) create its own, it can subclass TpBaseClientFactory to add its own proxy construct (with empathy_factory_create_foo), and all other proxies will still be implemented as well, no need to reimplement the interfaces.

The big reason I don't like the interface way, is that you can't easilly reimplement an interface method in your subclass but still chainup to parent's implementation of that same interface. Or is that possible/easy/nice with GObject??

> > 3) TpUserClientFactory, subclass of TpAutomaticClientFactory
> 
> If the user is going to set their desired features via methods, there's no need
> for a subclass - just fold this into the Automatic (or even Basic) class. The
> user will have to create their own instance, call methods on it, then pass it
> around (instead of relying on a singleton-generating function like
> tp_dup_default_proxy_factory() or something), but they'd have to do that
> anyway.

I like that! So in my proposal: remove TpUserClientFactory and add tp_simple_client_factory_add_*_features(). That makes even more important for factory subclasses to chainup to parent implementation to get the user-set features and yet add the special features of EmpathyTpChat for example.

> If the user is going to set their desired features via subclassing then they
> might as well subclass the Automatic or Basic class directly.

While technically that's an option, in practice doing GObject subclasses is so boring I would like to avoid apps doing it. Unless really necessary, like temporally for EmpathyTpChat. Also defining desired features by subclassing factory means we won't be able to add features at run-time.

> > This proposal does not need interfaces, all is virtual methods.
> 
> Which is good because?

Said above: chaining to parent implementation to "extend" instead of "replace" the behavior of methods. But maybe there is something I don't know in GInterface that makes this possible using interfaces?

(In reply to comment #5)
> (In reply to comment #2)
> > factory = tp_user_factory_dup_singleton();
> > tp_user_factory_add_account_manager_features (factory,
> > [TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT]);
> 
> Why is it a singleton if it has state?

Right, no need to be singleton, even if in practice apps would generally have only one instance.

> (In reply to comment #0)
> > Maybe TpContact as well (not technically a proxy, but same concept, and could
> > open the door for EmpathyContact being subclass of TpContact).
> 
> It'd be useful to have the factory manufacture contacts in order to request
> features, even if we don't allow subclassing.
> 
> One reason we don't allow subclassing is that allowing independent construction
> is incompatible with per-(connection,handle) uniqueness of contact objects, but
> factories can help to take care of uniqueness. Another reason to not allow
> independent construction is to make contacts guaranteed-invisible to user code
> until they've been set up enough for their handle/identifier to be known and
> persistent.

right, so unlike tp_simple_client_factory_create_channel() that returns a TpChannel in sync call, for contacts that would be tp_simple_client_factory_create_contacts_async(). Since the TpConnection inside the factory is unique, that would guarantee the uniqueness of contacts just like now too.

This makes me think that constructor methods on the factory could assert that given objects comes from that same factory. For example:

TpChannel *tp_simple_client_factory_create_channel (self, conn, object-path)
{
  g_return_val_if_fail (tp_proxy_get_factory (conn) == self, NULL);
  ...
}

With such requirement, TpContact objects created from a factory are guaranteed to all come from the same TpConnection object and so be per-factory unique as any other proxy.

> > 2) Do we want it to be able to upgrade all its existing proxies with more
> > features when the user provide them? tpqt4 only let user add features to be
> > prepared on new proxies, but existing ones would need to be upgraded manually
> > if user need it.
> 
> Unless we have a concrete use-case for add_features_async(), I'd vote for
> tpqt4's behaviour (which keeps adding features as a synchronous operation).

fair enough.

> (In reply to comment #2)
> > Forgot that we'll also need to add methods like
> > 
> > tp_proxy_set_factory(TpProxy *proxy, TpSimpleClientFactory *factory);
> 
> I think I'd prefer a construct-time property - proxies that can change factory
> mid-life seem likely to be confusing. (Unfortunately, TpChannelRequest already
> has a mutable factory, as does TpAccountChannelRequest - although the latter
> could inherit its default factory from the TpAccount, maybe?)

Right, that should be construct-only property. Sadly tp_*_new does not take the factory in args, so that means we need to create tp_*_new_with_factory()... well, actually that's good :)

Created attachment 47808 [details]
python example of API I would like

This is a python example of how I would like this API to work

Here is an implementation of my proposal: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factory

I've ported my contact-list branch to use the factory, the full branch is now there: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contact-list

What my branch is missing is deprecate the old proxyFactory stuff and make TpBaseClient and friends use the new one. But it needs to keep compatibility with old factories so that could get a little bit tricky.

(In reply to comment #11)
> What my branch is missing is deprecate the old proxyFactory stuff and make
> TpBaseClient and friends use the new one. But it needs to keep compatibility
> with old factories so that could get a little bit tricky.

done, ready for review.

Created attachment 49036 [details] [review]
Add TpSimpleClientFactory

This is a new object replacing TpBasicProxyFactory but can construct
any known TpProxy subclasses, guarantee their uniqueness per object-path
and keep a user-defined set of features to prepare on them.

Created attachment 49037 [details] [review]
Add TpAutomaticClientFactory

This is a subclass of TpSimpleClientFactory that creates specialized
TpChannel objects, like TpTextChannel and TpStreamTubeChannel

Here is a cherry-pick of the relevant commits. It only add the new factory objects, but does not use them yet.

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factories

Created attachment 49070 [details] [review]
Expose internally a TpContact constructor

Created attachment 49071 [details] [review]
Add TpSimpleClientFactory

This is a new object replacing TpBasicProxyFactory but can construct
any known TpProxy subclasses, guarantee their uniqueness per object-path
and keep a user-defined set of features to prepare on them.

Created attachment 49072 [details] [review]
Add TpAutomaticClientFactory

This is a subclass of TpSimpleClientFactory that creates specialized
TpChannel objects, like TpTextChannel and TpStreamTubeChannel

Created attachment 49073 [details] [review]
Add a comment explaining why we cannot let make TpContact subclasses yet

Review of attachment 49070 [details] [review]:

Internal files have to be added to IGNORE_HFILES in docs/reference/Makefile.am

::: telepathy-glib/contact-internal.h
@@ +25,3 @@
+G_BEGIN_DECLS
+
+TpContact *_tp_contact_new (TpConnection *connection, TpHandle handle,

one par per line.

Review of attachment 49071 [details] [review]:

::: telepathy-glib/proxy.c
@@ +1340,3 @@
+   * TpProxy:factory:
+   *
+   * The #TpSimpleClientFactory that have been used to create this proxy,

"that has been used"

@@ +1401,3 @@
+ * Returns: (transfer none): the same value as #TpProxy:factory property
+ *
+ * Since: 0.UNRELEASED

We usually use this pattern for getters's doc:


 * Return the #TpTextChannel:is-sms-channel property
 *
 * Returns: the value of #TpTextChannel:is-sms-channel property

::: telepathy-glib/simple-client-factory-internal.h
@@ +30,3 @@
+
+TpAccount *_tp_account_new_with_factory (TpDBusDaemon *bus_daemon,
+    const gchar *object_path, GError **error, TpSimpleClientFactory *factory);

one arg per line.

::: telepathy-glib/simple-client-factory.c
@@ +33,3 @@
+ * construct its #TpConnection, it will ensure that all desired features are
+ * prepared.
+ *

This should describe the type of objects that will be created (mostly interesting for TpChannel but still) and the features asked to be prepared.

@@ +99,3 @@
+{
+  TpDBusDaemon *dbus;
+  GHashTable *proxy_cache;

Please describe this hash table.

@@ +129,3 @@
+    gpointer proxy)
+{
+  if (proxy == NULL)

g_return_if_fail()? Why would it be NULL?

@@ +135,3 @@
+      (gpointer) tp_proxy_get_object_path (proxy), proxy);
+  tp_g_signal_connect_object (proxy, "invalidated",
+      G_CALLBACK (proxy_invalidated_cb), self, 0);

You rely on "invalidated" being fired when the proxy is destroyed. Is that guaranteed? If it is at least add a comment (but I think wjt and/or smcv would like to change that at some point). If it's not you should add a weak pointer.

@@ +220,3 @@
+
+  array = g_array_new (FALSE, FALSE, sizeof (TpContactFeature));
+  g_array_append_vals (array, self->priv->desired_contact_features->data,

You could use g_array_sized_new()

@@ +256,3 @@
+    {
+    case PROP_DBUS_DAEMON:
+      g_assert (self->priv->dbus == NULL);

we usually use:

      g_assert (self->priv->dbus == NULL);  /* construct only */

@@ +270,3 @@
+  TpSimpleClientFactory *self = (TpSimpleClientFactory *) object;
+
+  g_assert (self->priv->dbus != NULL);

g_assert (TP_IS_DBUS_DAEMON (..));

@@ +318,3 @@
+  self->priv->desired_contact_features = g_array_new (FALSE, FALSE,
+      sizeof (TpContactFeature));
+  contact_feature = TP_CONTACT_FEATURE_SUBSCRIPTION_STATES;

Any reason to only request this feature?

@@ +381,3 @@
+ * @self: a #TpSimpleClientFactory object
+ *
+ * <!-- -->

Same comment about getters' doc.

@@ +401,3 @@
+ *
+ * If a #TpAccountManager proxy has already been created using this method,
+ * it will be returned. Otherwise a new one will be created.

Phrasing is a bit weird. Make it more like tp_account_manager_dup's doc?

@@ +411,3 @@
+ *
+ * Returns: (transfer full): a new or existing #TpAccountManager proxy;
+ *  see tp_account_manager_new().

"a reference on a new or existing #TpAccountManager.." ?

@@ +426,3 @@
+    return g_object_ref (manager);
+
+  manager = TP_SIMPLE_CLIENT_FACTORY_GET_CLASS (self)->create_account_manager (self);

this line is too long.

@@ +439,3 @@
+ *
+ * If a #TpAccount proxy has already been created using this method for the
+ * given @object_path, it will be returned. Otherwise a new one will be created.

Same comment, I find the "has already been created using this method" a bit weird; and false actually as one may have been created but has been destroyed since.

@@ +740,3 @@
+ * responsability to keep a strong ref as long as needed.
+ *
+ * For this to work properly @connection must have immortal handles.

tp_connection_has_immortal_handles () has to return TRUE for @connection?

@@ +772,3 @@
+      connection, handle, identifier);
+
+  if (contact != NULL)

Why would it be NULL?

@@ +808,3 @@
+ * @n_features: The number of features in @features (may be 0)
+ * @features: (array length=n_features) (allow-none): an array of desired
+ *  features (may be %NULL if @n_features is 0)

Any reason why contact features are not GQuark? Should we open a API-break bug about this?

@@ +821,3 @@
+void
+tp_simple_client_factory_add_contact_features (TpSimpleClientFactory *self,
+    guint n_features, const TpContactFeature *features)

one arg per line.

::: telepathy-glib/simple-client-factory.h
@@ +40,3 @@
+
+    /* TpAccountManager */
+    TpAccountManager * (*create_account_manager) (TpSimpleClientFactory *self);

no dup_features() for this one?

@@ +101,3 @@
+
+/* TpAccountManager */
+TpAccountManager *tp_simple_client_factory_dup_account_manager (

_dup_ sounds like a bad name to me. We usually use it for property getters allocating memory or reffing an object. _ensure_ may be better here.

Review of attachment 49072 [details] [review]:

Looks good.

Review of attachment 49073 [details] [review]:

Can't we already use it if CM supports immortal handles?

Updated the branch with more commits, to propagate the factory to all our proxies and use it to reuse TpAccount, TpConnection, TpChannelRequest and TpChannelDispatchOperation everywhere. So internal code will never create those proxies directly, but always through a factory.

(In reply to comment #24)
> Review of attachment 49073 [details] [review]:
> 
> Can't we already use it if CM supports immortal handles?

Probably possible, yes. But let's do that later when the initial work is merged, OK? That's why I've noted it with a FIXME :)

(In reply to comment #21)
> Review of attachment 49070 [details] [review]:
> 
> Internal files have to be added to IGNORE_HFILES in docs/reference/Makefile.am
> 
> ::: telepathy-glib/contact-internal.h
> @@ +25,3 @@
> +G_BEGIN_DECLS
> +
> +TpContact *_tp_contact_new (TpConnection *connection, TpHandle handle,
> 
> one par per line.

Didn't know that one. Fixed.

Created attachment 49077 [details] [review]
Add _tp_proxy_ensure_factory

Created attachment 49078 [details] [review]
Ensure TpAccountManager, TpAccount and TpConnection always have a factory

Propagate the factory Manager->Account->Connection and fallback to create
an automatic factory in case they were not created through a factory.

Created attachment 49079 [details] [review]
Add tp_simple_client_factory_dup_channel_request()

Created attachment 49080 [details] [review]
Ensure TpChannelRequest always has a factory

Use it to share its TpAccount and TpConnection

Created attachment 49081 [details] [review]
 Add tp_simple_client_factory_dup_channel_dispatch_operation()

Created attachment 49082 [details] [review]
Ensure TpChannelDispatchOperation always has a factory

Use it to share its TpAccount and TpConnection, and also TpChannel
in the future

Review of attachment 49077 [details] [review]:

Looks good

Review of attachment 49078 [details] [review]:

Looks good

Review of attachment 49079 [details] [review]:

::: telepathy-glib/channel-request.c
@@ +657,3 @@
+{
+  if (self->priv->immutable_properties == NULL && immutable_properties != NULL)
+    self->priv->immutable_properties = g_hash_table_ref (immutable_properties);

g_object_notify ();

Review of attachment 49080 [details] [review]:

Looks good.

Review of attachment 49081 [details] [review]:

Looks good (modulo the usual doc comments)

Review of attachment 49082 [details] [review]:

Looks good.

::: telepathy-glib/channel-dispatch-operation.c
@@ +1260,3 @@
 
+/* This is temporary solution to share TpChannel objects until
+ * TpSimpleClientFactory can be used for that */

Add a FIXME?

(In reply to comment #26)
> (In reply to comment #24)
> > Review of attachment 49073 [details] [review] [details]:
> > 
> > Can't we already use it if CM supports immortal handles?
> 
> Probably possible, yes. But let's do that later when the initial work is
> merged, OK? That's why I've noted it with a FIXME :)

sure.

(In reply to comment #22)
> Review of attachment 49071 [details] [review]:
> 
> ::: telepathy-glib/proxy.c
> @@ +1340,3 @@
> +   * TpProxy:factory:
> +   *
> +   * The #TpSimpleClientFactory that have been used to create this proxy,
> 
> "that has been used"

Fixed

> @@ +1401,3 @@
> + * Returns: (transfer none): the same value as #TpProxy:factory property
> + *
> + * Since: 0.UNRELEASED
> 
> We usually use this pattern for getters's doc:
> 
>  * Return the #TpTextChannel:is-sms-channel property
>  *
>  * Returns: the value of #TpTextChannel:is-sms-channel property

Not everywhere, see for example tp_account_get_connection(). I think both are good enough. I'm lazy to edit everywhere tbh... do you consider this important?

> ::: telepathy-glib/simple-client-factory-internal.h
> @@ +30,3 @@
> +
> +TpAccount *_tp_account_new_with_factory (TpDBusDaemon *bus_daemon,
> +    const gchar *object_path, GError **error, TpSimpleClientFactory *factory);
> 
> one arg per line.

Fixed everywhere 

> ::: telepathy-glib/simple-client-factory.c
> @@ +33,3 @@
> + * construct its #TpConnection, it will ensure that all desired features are
> + * prepared.
> + *
> 
> This should describe the type of objects that will be created (mostly
> interesting for TpChannel but still) and the features asked to be prepared.

That doc is even partly wrong: TpDBusDaemon is not created through factory (was in previous version of the branch) and TpAccount does not not prepare features on its TpConnection (not yet, that's part of other commits in contact-list).

It also should document the recommended way of subclassing it.

> @@ +99,3 @@
> +{
> +  TpDBusDaemon *dbus;
> +  GHashTable *proxy_cache;
> 
> Please describe this hash table.

Fixed

> @@ +129,3 @@
> +    gpointer proxy)
> +{
> +  if (proxy == NULL)
> 
> g_return_if_fail()? Why would it be NULL?

In each _dup_ method, creation can fail and return NULL. I've make the check in insert_proxy() instead of repeating in each method.

> @@ +135,3 @@
> +      (gpointer) tp_proxy_get_object_path (proxy), proxy);
> +  tp_g_signal_connect_object (proxy, "invalidated",
> +      G_CALLBACK (proxy_invalidated_cb), self, 0);
> 
> You rely on "invalidated" being fired when the proxy is destroyed. Is that
> guaranteed? If it is at least add a comment (but I think wjt and/or smcv would
> like to change that at some point). If it's not you should add a weak pointer.

It is guaranteed to be emitted from TpProxy::dispose, until a future API break. AFAIK it is done like that everywhere, no?

> @@ +220,3 @@
> +
> +  array = g_array_new (FALSE, FALSE, sizeof (TpContactFeature));
> +  g_array_append_vals (array, self->priv->desired_contact_features->data,
> 
> You could use g_array_sized_new()

Since I use g_array_append_vals() it will set the size only once, afaik. Fixed anyway.

> @@ +256,3 @@
> +    {
> +    case PROP_DBUS_DAEMON:
> +      g_assert (self->priv->dbus == NULL);
> 
> we usually use:
> 
>       g_assert (self->priv->dbus == NULL);  /* construct only */

Fixed

> @@ +270,3 @@
> +  TpSimpleClientFactory *self = (TpSimpleClientFactory *) object;
> +
> +  g_assert (self->priv->dbus != NULL);
> 
> g_assert (TP_IS_DBUS_DAEMON (..));

Fixed

> @@ +318,3 @@
> +  self->priv->desired_contact_features = g_array_new (FALSE, FALSE,
> +      sizeof (TpContactFeature));
> +  contact_feature = TP_CONTACT_FEATURE_SUBSCRIPTION_STATES;
> 
> Any reason to only request this feature?

Hm, that comes from the contact-list branch, because we get that feature for free as it's always pushed by ContactList iface... Dropped for now.

> @@ +401,3 @@
> + *
> + * If a #TpAccountManager proxy has already been created using this method,
> + * it will be returned. Otherwise a new one will be created.
> 
> Phrasing is a bit weird. Make it more like tp_account_manager_dup's doc?

Fixed

> @@ +411,3 @@
> + *
> + * Returns: (transfer full): a new or existing #TpAccountManager proxy;
> + *  see tp_account_manager_new().
> 
> "a reference on a new or existing #TpAccountManager.." ?

Fixed

> @@ +426,3 @@
> +    return g_object_ref (manager);
> +
> +  manager = TP_SIMPLE_CLIENT_FACTORY_GET_CLASS (self)->create_account_manager
> (self);
> 
> this line is too long.

fixed

> @@ +439,3 @@
> + *
> + * If a #TpAccount proxy has already been created using this method for the
> + * given @object_path, it will be returned. Otherwise a new one will be
> created.
> 
> Same comment, I find the "has already been created using this method" a bit
> weird; and false actually as one may have been created but has been destroyed
> since.

Fixed

> @@ +740,3 @@
> + * responsability to keep a strong ref as long as needed.
> + *
> + * For this to work properly @connection must have immortal handles.
> 
> tp_connection_has_immortal_handles () has to return TRUE for @connection?

Fixed

> @@ +772,3 @@
> +      connection, handle, identifier);
> +
> +  if (contact != NULL)
> 
> Why would it be NULL?

Indeed it can't be NULL, that's just copy/paste pattern from proxies where we can get an error. Fixed.

> @@ +808,3 @@
> + * @n_features: The number of features in @features (may be 0)
> + * @features: (array length=n_features) (allow-none): an array of desired
> + *  features (may be %NULL if @n_features is 0)
> 
> Any reason why contact features are not GQuark? Should we open a API-break bug
> about this?

TpContact internally uses flags, but smcv was scared if we have more than 32 features. Indeed using quarks is more consistent. +1 to make that change in future break.

> @@ +821,3 @@
> +void
> +tp_simple_client_factory_add_contact_features (TpSimpleClientFactory *self,
> +    guint n_features, const TpContactFeature *features)
> 
> one arg per line.

fixed.

> ::: telepathy-glib/simple-client-factory.h
> @@ +40,3 @@
> +
> +    /* TpAccountManager */
> +    TpAccountManager * (*create_account_manager) (TpSimpleClientFactory
> *self);
> 
> no dup_features() for this one?

TpAccountManager has no features other than CORE atm. We can easily add that method when/if needed.

> @@ +101,3 @@
> +
> +/* TpAccountManager */
> +TpAccountManager *tp_simple_client_factory_dup_account_manager (
> 
> _dup_ sounds like a bad name to me. We usually use it for property getters
> allocating memory or reffing an object. _ensure_ may be better here.

Bah, see for example tp_account_manager_dup(). dup is common for getting singletons too

(In reply to comment #36)
> Review of attachment 49079 [details] [review]:
> 
> ::: telepathy-glib/channel-request.c
> @@ +657,3 @@
> +{
> +  if (self->priv->immutable_properties == NULL && immutable_properties !=
> NULL)
> +    self->priv->immutable_properties = g_hash_table_ref
> (immutable_properties);
> 
> g_object_notify ();

It is supposed a construct-only property, so no one is expecting a notify signal.

(In reply to comment #39)
> ::: telepathy-glib/channel-dispatch-operation.c
> @@ +1260,3 @@
> 
> +/* This is temporary solution to share TpChannel objects until
> + * TpSimpleClientFactory can be used for that */
> 
> Add a FIXME?

fixed

(In reply to comment #41)
> > @@ +1401,3 @@
> > + * Returns: (transfer none): the same value as #TpProxy:factory property
> > + *
> > + * Since: 0.UNRELEASED
> > 
> > We usually use this pattern for getters's doc:
> > 
> >  * Return the #TpTextChannel:is-sms-channel property
> >  *
> >  * Returns: the value of #TpTextChannel:is-sms-channel property
> 
> Not everywhere, see for example tp_account_get_connection(). I think both are
> good enough. I'm lazy to edit everywhere tbh... do you consider this important?

Not really.

> > ::: telepathy-glib/simple-client-factory.c
> > @@ +33,3 @@
> > + * construct its #TpConnection, it will ensure that all desired features are
> > + * prepared.
> > + *
> > 
> > This should describe the type of objects that will be created (mostly
> > interesting for TpChannel but still) and the features asked to be prepared.
> 
> That doc is even partly wrong: TpDBusDaemon is not created through factory (was
> in previous version of the branch) and TpAccount does not not prepare features
> on its TpConnection (not yet, that's part of other commits in contact-list).

> It also should document the recommended way of subclassing it.

Indeed and it should mention TpContact as well.

> > @@ +135,3 @@
> > +      (gpointer) tp_proxy_get_object_path (proxy), proxy);
> > +  tp_g_signal_connect_object (proxy, "invalidated",
> > +      G_CALLBACK (proxy_invalidated_cb), self, 0);
> > 
> > You rely on "invalidated" being fired when the proxy is destroyed. Is that
> > guaranteed? If it is at least add a comment (but I think wjt and/or smcv would
> > like to change that at some point). If it's not you should add a weak pointer.
> 
> It is guaranteed to be emitted from TpProxy::dispose, until a future API break.
> AFAIK it is done like that everywhere, no?

I think so, but best to double check and at least add a comment.

> > @@ +808,3 @@
> > + * @n_features: The number of features in @features (may be 0)
> > + * @features: (array length=n_features) (allow-none): an array of desired
> > + *  features (may be %NULL if @n_features is 0)
> > 
> > Any reason why contact features are not GQuark? Should we open a API-break bug
> > about this?
> 
> TpContact internally uses flags, but smcv was scared if we have more than 32
> features. Indeed using quarks is more consistent. +1 to make that change in
> future break.

Open a bug prefixed with [API break] so we won't forget?

> > ::: telepathy-glib/simple-client-factory.h
> > @@ +40,3 @@
> > +
> > +    /* TpAccountManager */
> > +    TpAccountManager * (*create_account_manager) (TpSimpleClientFactory
> > *self);
> > 
> > no dup_features() for this one?
> 
> TpAccountManager has no features other than CORE atm. We can easily add that
> method when/if needed.

I'm still wondering if we shouldn't already add it for the sake of
consistency. Also, old code, if any, supposed to do the prepartion won't work
if we don't include it now.

> > @@ +101,3 @@
> > +
> > +/* TpAccountManager */
> > +TpAccountManager *tp_simple_client_factory_dup_account_manager (
> > 
> > _dup_ sounds like a bad name to me. We usually use it for property getters
> > allocating memory or reffing an object. _ensure_ may be better here.
> 
> Bah, see for example tp_account_manager_dup(). dup is common for getting
> singletons too

Yes, but those are not singleton: one single instance of an object
type. We usually use "ensure" for function either creating a new object or
re-using an existing one, which is exactly what these methods are doing.

(In reply to comment #42)
> > ::: telepathy-glib/channel-request.c
> > @@ +657,3 @@
> > +{
> > +  if (self->priv->immutable_properties == NULL && immutable_properties !=
> > NULL)
> > +    self->priv->immutable_properties = g_hash_table_ref
> > (immutable_properties);
> > 
> > g_object_notify ();
> 
> It is supposed a construct-only property, so no one is expecting a notify
> signal.

Strictly speaking construct-only doesn't imply immutable. I agree that's not
very useful but it doesn't hurt either.

(In reply to comment #44)
> > > @@ +135,3 @@
> > > +      (gpointer) tp_proxy_get_object_path (proxy), proxy);
> > > +  tp_g_signal_connect_object (proxy, "invalidated",
> > > +      G_CALLBACK (proxy_invalidated_cb), self, 0);
> > > 
> > > You rely on "invalidated" being fired when the proxy is destroyed. Is that
> > > guaranteed? If it is at least add a comment (but I think wjt and/or smcv would
> > > like to change that at some point). If it's not you should add a weak pointer.
> > 
> > It is guaranteed to be emitted from TpProxy::dispose, until a future API break.
> > AFAIK it is done like that everywhere, no?
> 
> I think so, but best to double check and at least add a comment.

Verified and it is indeed emitted from tp_proxy_dispose(). I added a comment telling we assume that.

> > > @@ +808,3 @@
> > > + * @n_features: The number of features in @features (may be 0)
> > > + * @features: (array length=n_features) (allow-none): an array of desired
> > > + *  features (may be %NULL if @n_features is 0)
> > > 
> > > Any reason why contact features are not GQuark? Should we open a API-break bug
> > > about this?
> > 
> > TpContact internally uses flags, but smcv was scared if we have more than 32
> > features. Indeed using quarks is more consistent. +1 to make that change in
> > future break.
> 
> Open a bug prefixed with [API break] so we won't forget?

bug #39248

> > > ::: telepathy-glib/simple-client-factory.h
> > > @@ +40,3 @@
> > > +
> > > +    /* TpAccountManager */
> > > +    TpAccountManager * (*create_account_manager) (TpSimpleClientFactory
> > > *self);
> > > 
> > > no dup_features() for this one?
> > 
> > TpAccountManager has no features other than CORE atm. We can easily add that
> > method when/if needed.
> 
> I'm still wondering if we shouldn't already add it for the sake of
> consistency. Also, old code, if any, supposed to do the prepartion won't work
> if we don't include it now.

added for completeness of the API

> > > @@ +101,3 @@
> > > +
> > > +/* TpAccountManager */
> > > +TpAccountManager *tp_simple_client_factory_dup_account_manager (
> > > 
> > > _dup_ sounds like a bad name to me. We usually use it for property getters
> > > allocating memory or reffing an object. _ensure_ may be better here.
> > 
> > Bah, see for example tp_account_manager_dup(). dup is common for getting
> > singletons too
> 
> Yes, but those are not singleton: one single instance of an object
> type. We usually use "ensure" for function either creating a new object or
> re-using an existing one, which is exactly what these methods are doing.

ok, renamed to _ensure_

> (In reply to comment #42)
> > > ::: telepathy-glib/channel-request.c
> > > @@ +657,3 @@
> > > +{
> > > +  if (self->priv->immutable_properties == NULL && immutable_properties !=
> > > NULL)
> > > +    self->priv->immutable_properties = g_hash_table_ref
> > > (immutable_properties);
> > > 
> > > g_object_notify ();
> > 
> > It is supposed a construct-only property, so no one is expecting a notify
> > signal.
> 
> Strictly speaking construct-only doesn't imply immutable. I agree that's not
> very useful but it doesn't hurt either.

Well, in this case it is construct-only immutable-properties... but ok, added the notify :)



All comments are now fixed in the branch, except for better description of TpSimpleClientFactory

(In reply to comment #45)
> All comments are now fixed in the branch, except for better description of
> TpSimpleClientFactory

This is now done, ready for final review and merge !

You should add in TpSimpleClientFactory's doc which features are asked to be
prepared by default.


 * @create_account_manager: virtual method used to create a #TpAccountManager;
 *  see tp_simple_client_factory_dup_account_manager()
tp_simple_client_factory_dup_account_manager() doesn't exist any more.

Ditto for the others.


static TpConnection *
create_connection_impl (TpSimpleClientFactory *self,
    const gchar *object_path,
    GError **error)

Shoudln't we do something like:

static TpConnection *
create_connection_impl (TpSimpleClientFactory *self,
    const gchar *bus_name,
    const gchar *object_path,
    GError **error)

to stay coherent with tp_connection_new and allows user to create TpConnection
from their bus name rather than their path?


I do beliver that oa{sv} (object-path and immutable properties) is the right
way to advertise objects and that we should use it everywhere in
Telepathy. As we are "about" to break the D-Bus API maybe we could add a
"GHashTable *immutable_propertiers" argument to all of these methods to be
future proof?

A few lines are still too long, you should configure your editor to mark them
in red or something:
  tp_clear_pointer (&self->priv->desired_account_manager_features, g_array_unref);
  connection = TP_SIMPLE_CLIENT_FACTORY_GET_CLASS (self)->create_connection (self,
tp_simple_client_factory_ensure_channel_dispatch_operation (TpSimpleClientFactory *self,

(In reply to comment #47)
> You should add in TpSimpleClientFactory's doc which features are asked to be
> prepared by default.

Fixed

>  * @create_account_manager: virtual method used to create a #TpAccountManager;
>  *  see tp_simple_client_factory_dup_account_manager()
> tp_simple_client_factory_dup_account_manager() doesn't exist any more.
> 
> Ditto for the others.

Fixed

> static TpConnection *
> create_connection_impl (TpSimpleClientFactory *self,
>     const gchar *object_path,
>     GError **error)
> 
> Shoudln't we do something like:
> 
> static TpConnection *
> create_connection_impl (TpSimpleClientFactory *self,
>     const gchar *bus_name,
>     const gchar *object_path,
>     GError **error)
> 
> to stay coherent with tp_connection_new and allows user to create TpConnection
> from their bus name rather than their path?

There is not a single usage of that in the whole tp-glib, I've verified. IMO that's just making stuff more complicate for no benefit. And it is more coherent with other objects (TpAccount, TpChannel) which takes only an object-path. IMO constructing a TpConnection from a bus_name is just legacy, the spec always give the object path.

> I do beliver that oa{sv} (object-path and immutable properties) is the right
> way to advertise objects and that we should use it everywhere in
> Telepathy. As we are "about" to break the D-Bus API maybe we could add a
> "GHashTable *immutable_propertiers" argument to all of these methods to be
> future proof?

IMO that would be too early generalization. The spec does not provide immutable-properties table for those objects, so user would wonder why we have that currently useless arg. But we could keep this in mind for future API break if needed.

> A few lines are still too long, you should configure your editor to mark them
> in red or something:
>   tp_clear_pointer (&self->priv->desired_account_manager_features,
> g_array_unref);
>   connection = TP_SIMPLE_CLIENT_FACTORY_GET_CLASS (self)->create_connection
> (self,
> tp_simple_client_factory_ensure_channel_dispatch_operation
> (TpSimpleClientFactory *self,

I do have a 80 col lines, but I'm not that strict on the limit usually :p
Fixed.

(In reply to comment #48)
> > I do beliver that oa{sv} (object-path and immutable properties) is the right
> > way to advertise objects and that we should use it everywhere in
> > Telepathy. As we are "about" to break the D-Bus API maybe we could add a
> > "GHashTable *immutable_propertiers" argument to all of these methods to be
> > future proof?
> 
> IMO that would be too early generalization. The spec does not provide
> immutable-properties table for those objects, so user would wonder why we have
> that currently useless arg. But we could keep this in mind for future API break
> if needed.

http://telepathy.freedesktop.org/spec-snapshot/Channel_Request.html#Signal:SucceededWithChannel
does

Ok, fixed

(In reply to comment #45)
> > > > ::: telepathy-glib/simple-client-factory.h
> > > > @@ +40,3 @@
> > > > +
> > > > +    /* TpAccountManager */
> > > > +    TpAccountManager * (*create_account_manager) (TpSimpleClientFactory
> > > > *self);
> > > > 
> > > > no dup_features() for this one?
> > > 
> > > TpAccountManager has no features other than CORE atm. We can easily add that
> > > method when/if needed.
> > 
> > I'm still wondering if we shouldn't already add it for the sake of
> > consistency. Also, old code, if any, supposed to do the prepartion won't work
> > if we don't include it now.
> 
> added for completeness of the API

Humm shouldn't we do the same for TpChannelRequest and
TpChannelDispatchOperation? Not sure.

Those can't even be overriden in the factory. I think they really are just internal objects for TpBaseClient tbh. I was even wondering if I should make tp_simple_client_factory_ensure_channel_request() internal only... should I?

ok, made those functions private for now, so we can easily make them public later if needed.

+++ b/telepathy-glib/automatic-client-factory-internal.h
@@ -0,0 +1,45 @@
+/*
+ * Automatic client factory internal

Gee, what a helpful description of the file. ;)

I rearranged the arguments of these three functions to put the GError ** last, and put the factory first. They're methods on the factory, no?

+ * SECTION:automatic-client-factory
+ * @title: TpAutomaticClientFactory
+ * @short_description: factory creating higher level objects

That description tells me even less than the class name. “Factory for specialized TpChannel subclasses” sounds better. I've changed this.

+#define chainup ((TpSimpleClientFactoryClass *) \
+    tp_automatic_client_factory_parent_class)

This is a new one, I like it in theory—but I think I would have used

  static TpSimpleClientFactoryClass *chainup = NULL;

  ... class_init {
    ...
    chainup = (TpSimpleClientFactoryClass *) tp_automatic_client_factory_parent_class;
  }

Maybe that's just me.

/* FIXME: This is temporary solution to share TpChannel objects until
 * TpSimpleClientFactory can be used for that */
void
_tp_channel_dispatch_operation_ensure_channels (TpChannelDispatchOperation *self,
    GPtrArray *channels)

Why can't it be used for that now? How temporary is temporary? What would actually need to be done to fix this FIXME?

TpAutomaticClientFactory doesn't seem to show up in the generated documentation for me. At least some of these classes should have code samples for how to actually used them. I'm lost in a sea of very similarly-named classes whose documentation describes them as “Factory creating higher level objects” or “Factory creating higher level proxy objects”.

I have pushed http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=factories which fixes some documentation nits—plus a build failure and g_object_unref (NULL) which I found within 30 seconds of cloning the branch ;).

Thanks for that contribution :)

(In reply to comment #54)
> +++ b/telepathy-glib/automatic-client-factory-internal.h
> @@ -0,0 +1,45 @@
> +/*
> + * Automatic client factory internal
> 
> Gee, what a helpful description of the file. ;)

suggestion?

> I rearranged the arguments of these three functions to put the GError ** last,
> and put the factory first. They're methods on the factory, no?

Looks better indeed. You should do the same for those defined in simple-client-factory-internal.h too :)

> + * SECTION:automatic-client-factory
> + * @title: TpAutomaticClientFactory
> + * @short_description: factory creating higher level objects
> 
> That description tells me even less than the class name. “Factory for
> specialized TpChannel subclasses” sounds better. I've changed this.

Atm it does specialized subclasses only for TpChannel but more could be added later. Guess we can change the doc when/if we actually do more than channels.

> +#define chainup ((TpSimpleClientFactoryClass *) \
> +    tp_automatic_client_factory_parent_class)
> 
> This is a new one, I like it in theory—but I think I would have used
> 
>   static TpSimpleClientFactoryClass *chainup = NULL;
> 
>   ... class_init {
>     ...
>     chainup = (TpSimpleClientFactoryClass *)
> tp_automatic_client_factory_parent_class;
>   }
> 
> Maybe that's just me.

I find my idea simpler, tbh. Since it is static casting anyway, there is no real benefit in keeping a ptr to the parent class.

> /* FIXME: This is temporary solution to share TpChannel objects until
>  * TpSimpleClientFactory can be used for that */
> void
> _tp_channel_dispatch_operation_ensure_channels (TpChannelDispatchOperation
> *self,
>     GPtrArray *channels)
> 
> Why can't it be used for that now? How temporary is temporary? What would
> actually need to be done to fix this FIXME?

because this branch is a subset of what I have in contact-list branch. I wanted to get this part already merged and keep using the old channel factory for now.

> TpAutomaticClientFactory doesn't seem to show up in the generated documentation
> for me. At least some of these classes should have code samples for how to
> actually used them. I'm lost in a sea of very similarly-named classes whose
> documentation describes them as “Factory creating higher level objects” or
> “Factory creating higher level proxy objects”.

TpBasicProxyFactory, TpAutomaticProxyFactory and TpClientChannelFactory will all be deprecated soon. TpSimpleClientFactory and TpAutomaticClientFactory replaces them.

> I have pushed
> http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=factories
> which fixes some documentation nits—plus a build failure and g_object_unref
> (NULL) which I found within 30 seconds of cloning the branch ;).

Thanks, already cherry-picked one commit that can go to master right away.

b9bbb42842ce5a1ddfab555da34511ad82fe9431 -> squashed into my branch
e0393dbedb3c2a16cd150a966181dfffcae12e40 -> merged into master
439e7d7f8703d200c5fc9a5dc48aaf9e69dbb345 -> squashed into my branch and did the same for the _new_with_factory() in simple-client-factory-internal.h

I didn't merge doc changes yet since that seems WIP.

(In reply to comment #54)
> TpAutomaticClientFactory doesn't seem to show up in the generated documentation
> for me.

Fixed

Once factories branch gets merged, here is the next step: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factories-continued

It adds feature on AccountManager/Account to expose only prepared Account/Connection.

(In reply to comment #55)
> Thanks for that contribution :)
> 
> (In reply to comment #54)
> > +++ b/telepathy-glib/automatic-client-factory-internal.h
> > @@ -0,0 +1,45 @@
> > +/*
> > + * Automatic client factory internal
> > 
> > Gee, what a helpful description of the file. ;)
> 
> suggestion?

“Internal constructors for TpChannel subclasses” maybe? I've changed it to this. I've changed various others too.

My point is really: why bother adding this comment if it's just the filename with s/-/ /? Ditto @short_description: describing TpSimpleClientFactory as “simple client factory” doesn't add anything. What's a client, what's a factory and how is this one simple? I replaced this with “a factory for #TpContact<!-- -->s and plain subclasses of #TpProxy”.

+      /* A common usage is request_and_handle, in that case EnsureChannel
+       * returns only the object-path of the ChannelRequest but not properties.
+       * The TpChannelRequest will be created with no properties, then when
+       * handling we get the properties and we reuse the same TpChannelRequest
+       * object, and we can give it the immutable-properties. */

Eh? This doesn't sound right. The client library should know the immutable properties of the ChannelRequest that EnsureChannel() returns because it specified them when it called EnsureChannel().

There are a pile more patches on my branch.

Thanks for the doc tweaks!

I've cherry-picked 37bb2dde30f6d8cb66e7c5a8a87c884a6d52ec5c and ec40be527645607f91b30beaad1b2a38258d60ee and merged into master since they are unrelated.

(In reply to comment #59)
> +      /* A common usage is request_and_handle, in that case EnsureChannel
> +       * returns only the object-path of the ChannelRequest but not
> properties.
> +       * The TpChannelRequest will be created with no properties, then when
> +       * handling we get the properties and we reuse the same TpChannelRequest
> +       * object, and we can give it the immutable-properties. */
> 
> Eh? This doesn't sound right. The client library should know the immutable
> properties of the ChannelRequest that EnsureChannel() returns because it
> specified them when it called EnsureChannel().

I don't know how this is supposed to work, but fact is TpBaseClient creates TpChannelRequest objects without giving immutable properties, then create another with the immutable_properties. If you remove that workaround, you'll see unit tests failing. I should probably add a FIXME in that comment and open a bug to fix TpBaseClient so it always gives the immutable-properties? I would like to not block on this, factories have been waiting for too long already.

Do you have any other comments, is your branch ready to get merged now?

Great, factories branch merged into master. Now that one needs review: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factories-continued

And the last bits for this bug: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factories-continued2

+void
+_tp_connection_set_account (TpConnection *self,
+    TpAccount *account)
+{
+  if (self->priv->account == account)
+    return;
+
+  g_assert (self->priv->account == NULL);

I'd probably throw in an assertion that 'account' is non-NULL, too.

+static void
+account_prepare_cb (GObject *object,
+    GAsyncResult *res,
+    gpointer user_data)
+{
+  TpAccount *account = (TpAccount *) object;
+  GSimpleAsyncResult *result = user_data;
+  GError *error = NULL;
+
+  account_set_public (account);
+
+  if (!tp_proxy_prepare_finish (object, res, &error))
+    {
+      DEBUG ("Failed to prepare account: %s", error->message);
+      g_simple_async_result_take_error (result, error);
+    }

I'm surprised that you can call g_simple_async_result_take_error() more than once on the same result without it criticaling. But it seems you can.

+ * Note that the returned #TpAccount<!-- -->s are not guaranteed to have any
+ * features pre-prepared, including %TP_ACCOUNT_FEATURE_CORE unless feature
+ * %TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT has been prepared.
+ *

How about:

  * Note that the returned #TpAccount<!-- -->s are not guaranteed to have any
  * features pre-prepared (not even %TP_ACCOUNT_FEATURE_CORE) unless
  * %TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT has been prepared on @self.
  *

Ditto all the other places. As written, “blah, including %TP_ACCOUNT_FEATURE_CORE unless feature” with no second comma after the feature name is misleading.

I actually think at least CORE should be prepared on any account advertised by TpAccountManager, and TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT should hence probably not exist. The only downside I can see is that you'd get a bunch of calls to GetAll() to prepare each new account (and all accounts when you first turn tp-glib on), but that's what almost every application using TpAccountManager is going to do anyway, surely? And really, the bug is that the “new account” D-Bus signal doesn't include its properties, and that there's no D-Bus call to get all the properties for the AccountManager and all of its accounts. (http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager would be appropriate here.)

The TpAccountManager code is made much more complicated by having these two modes.

+  return GPOINTER_TO_UINT (g_object_get_qdata ((GObject *) account,
+      ACCOUNT_PREPARED_KEY)) != TRUE;

I don't like this very much. I find boolean comparisons of booleans confusing. How about g_object_get_qdata (...) != NULL? Or should it be == NULL?

But, really, I'd be tempted to keep a set of accounts which we're preparing, in parallel to the existing two sets of valid and invalid accounts, rather than glueing labels onto accounts in those two sets.


+ * When this feature is prepared, it is guaranteed that all #TpAccount will
+ * always be prepared.

No, it's not. It's guaranteed that all #TpAccount<!-- -->s produced by this object will be prepared.

                         If the account manager was created using a
+ * #TpSimpleClientFactory, the same factory will be used to create #TpAccount
+ * objects and to determin desired account features.

determine.

+ * #TpAccountManager::account-validity-changed will be delayed until all
+ * features (at least %TP_ACCOUNT_FEATURE_CORE) are prepared. See
+ * tp_simple_client_factory_add_account_manager_features() to define which
+ * features needs to be prepared.

features need.


+  /* Delay signal emition until account is prepared if we want to expose only

emission.

  Add new feature TP_ACCOUNT_FEATURE_CONNECTION

  It delays "connection" property change notification until the TpConnection
  is prepared with factory's desired features.

Hmm. I guess this is less weird than TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT.

+   * It is not guaranteed that the returned #TpConnection object is ready
+   * unless feature %TP_ACCOUNT_FEATURE_CONNECTION has been prepared.

…unless %TP_ACCOUNT_FEATURE_CONNECTION has been prepared on @self.

 static void
+set_connection_prepare_cb (GObject *object,
+    GAsyncResult *res,
+    gpointer user_data)
+{
+  TpConnection *connection = (TpConnection *) object;
+  TpAccount *self = user_data;
+  GError *error = NULL;
+
+  if (!tp_proxy_prepare_finish (object, res, &error))
+    {
+      DEBUG ("Error preparing connection: %s", error->message);
+      g_clear_error (&error);
+      goto OUT;
+    }
+
+  /* Connection could have changed again while we were preparing it */
+  if (self->priv->connection == connection)
+    {
+      self->priv->connection_prepared = TRUE;
+      g_object_notify ((GObject *) self, "connection");
+    }
+
+OUT:
+  g_object_unref (self);
+}

Overuse of 'goto OUT' like this irritates me. I'd put the second half into an else {}, which I think is clearer. But hey.

+ * #TpSimpleClientFactory, the same factory will be used to create #TpConnection
+ * object and to determin desired connection features. Change notification of

determine.

-      channel = tp_client_channel_factory_create_channel (
-          self->priv->channel_factory, connection, chan_path, chan_props,
-          &error);
+      if (self->priv->channel_factory != NULL)
+        channel = tp_client_channel_factory_create_channel (
+            self->priv->channel_factory, connection, chan_path, chan_props,
+            &error);
+      else
+        channel = tp_simple_client_factory_ensure_channel (
+            tp_proxy_get_factory (self->priv->account_mgr), connection,
+            chan_path, chan_props, &error);

This, and the equivalent for dupping features, is repeatedly duplicated.

Is tp_proxy_get_factory() guaranteed to return non-NULL if self->priv->channel_factory == NULL? Why?


-  g_return_if_fail (TP_IS_CLIENT_CHANNEL_FACTORY (factory));
 
   tp_clear_object (&self->priv->channel_factory);
 
-  self->priv->channel_factory = g_object_ref (factory);
+  if (factory != NULL)
+    self->priv->channel_factory = g_object_ref (factory);

You should probably not remove the type check.

(In reply to comment #63)
> +void
> +_tp_connection_set_account (TpConnection *self,
> +    TpAccount *account)
> +{
> +  if (self->priv->account == account)
> +    return;
> +
> +  g_assert (self->priv->account == NULL);
> 
> I'd probably throw in an assertion that 'account' is non-NULL, too.

done

> + * Note that the returned #TpAccount<!-- -->s are not guaranteed to have any
> + * features pre-prepared, including %TP_ACCOUNT_FEATURE_CORE unless feature
> + * %TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT has been prepared.
> + *
> 
> How about:
> 
>   * Note that the returned #TpAccount<!-- -->s are not guaranteed to have any
>   * features pre-prepared (not even %TP_ACCOUNT_FEATURE_CORE) unless
>   * %TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT has been prepared on @self.
>   *
> 
> Ditto all the other places. As written, “blah, including
> %TP_ACCOUNT_FEATURE_CORE unless feature” with no second comma after the feature
> name is misleading.

done

> I actually think at least CORE should be prepared on any account advertised by
> TpAccountManager, and TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT should hence probably
> not exist. The only downside I can see is that you'd get a bunch of calls to
> GetAll() to prepare each new account (and all accounts when you first turn
> tp-glib on), but that's what almost every application using TpAccountManager is
> going to do anyway, surely? And really, the bug is that the “new account” D-Bus
> signal doesn't include its properties, and that there's no D-Bus call to get
> all the properties for the AccountManager and all of its accounts.
> (http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager
> would be appropriate here.)

We could make that feature being CORE at any time, tbh. I think it's fine to merge this like it is for now, and revisit when/if we have spec that gives all the needed info? It is really easy now to prepare extra features, you just have to list them at startup on your factory, so having extra features isn't that annoying ;)

> The TpAccountManager code is made much more complicated by having these two
> modes.
> 
> +  return GPOINTER_TO_UINT (g_object_get_qdata ((GObject *) account,
> +      ACCOUNT_PREPARED_KEY)) != TRUE;
> 
> I don't like this very much. I find boolean comparisons of booleans confusing.
> How about g_object_get_qdata (...) != NULL? Or should it be == NULL?

I can remove the GPOINTER_TO_UINT and do ==NULL (meaning: if they key is not set the account is internal)

> But, really, I'd be tempted to keep a set of accounts which we're preparing, in
> parallel to the existing two sets of valid and invalid accounts, rather than
> glueing labels onto accounts in those two sets.

Right that would be cleaner, I'll prepare a patch.

> + * When this feature is prepared, it is guaranteed that all #TpAccount will
> + * always be prepared.
> 
> No, it's not. It's guaranteed that all #TpAccount<!-- -->s produced by this
> object will be prepared.

fixed

>                          If the account manager was created using a
> + * #TpSimpleClientFactory, the same factory will be used to create #TpAccount
> + * objects and to determin desired account features.
> 
> determine.

fixed

> + * #TpAccountManager::account-validity-changed will be delayed until all
> + * features (at least %TP_ACCOUNT_FEATURE_CORE) are prepared. See
> + * tp_simple_client_factory_add_account_manager_features() to define which
> + * features needs to be prepared.
> 
> features need.

fixed

> +  /* Delay signal emition until account is prepared if we want to expose only
> 
> emission.
> 
>   Add new feature TP_ACCOUNT_FEATURE_CONNECTION 
> 
>   It delays "connection" property change notification until the TpConnection
>   is prepared with factory's desired features.
> 
> Hmm. I guess this is less weird than TP_ACCOUNT_MANAGER_FEATURE_ACCOUNT.
> 
> +   * It is not guaranteed that the returned #TpConnection object is ready
> +   * unless feature %TP_ACCOUNT_FEATURE_CONNECTION has been prepared.
> 
> …unless %TP_ACCOUNT_FEATURE_CONNECTION has been prepared on @self.

It is a property doc, so there is no @self defined AFAIK. I changed to

   * Note that the returned #TpConnection is not guaranteed to have any
   * features pre-prepared (not even %TP_CONNECTION_FEATURE_CORE) unless
   * %TP_ACCOUNT_FEATURE_ACCOUNT has been prepared on the account


>  static void
> +set_connection_prepare_cb (GObject *object,
> +    GAsyncResult *res,
> +    gpointer user_data)
> +{
> +  TpConnection *connection = (TpConnection *) object;
> +  TpAccount *self = user_data;
> +  GError *error = NULL;
> +
> +  if (!tp_proxy_prepare_finish (object, res, &error))
> +    {
> +      DEBUG ("Error preparing connection: %s", error->message);
> +      g_clear_error (&error);
> +      goto OUT;
> +    }
> +
> +  /* Connection could have changed again while we were preparing it */
> +  if (self->priv->connection == connection)
> +    {
> +      self->priv->connection_prepared = TRUE;
> +      g_object_notify ((GObject *) self, "connection");
> +    }
> +
> +OUT:
> +  g_object_unref (self);
> +}
> 
> Overuse of 'goto OUT' like this irritates me. I'd put the second half into an
> else {}, which I think is clearer. But hey.

I've overused the goto to keep the same pattern than various other functions where it does a goto OUT on error (or return). But arguably that could be a pattern only me does? :P tbh in this case I find the else if{} a bit ugly with the comment in the middle...

> + * #TpSimpleClientFactory, the same factory will be used to create
> #TpConnection
> + * object and to determin desired connection features. Change notification of
> 
> determine.

done

(In reply to comment #64)
> -      channel = tp_client_channel_factory_create_channel (
> -          self->priv->channel_factory, connection, chan_path, chan_props,
> -          &error);
> +      if (self->priv->channel_factory != NULL)
> +        channel = tp_client_channel_factory_create_channel (
> +            self->priv->channel_factory, connection, chan_path, chan_props,
> +            &error);
> +      else
> +        channel = tp_simple_client_factory_ensure_channel (
> +            tp_proxy_get_factory (self->priv->account_mgr), connection,
> +            chan_path, chan_props, &error);
> 
> This, and the equivalent for dupping features, is repeatedly duplicated.

Right, I could factor-out a function for that. I'll prepare a patch.

> Is tp_proxy_get_factory() guaranteed to return non-NULL if
> self->priv->channel_factory == NULL? Why?

tp_proxy_get_factory() is always guaranteed to return non-NULL on TpAccountManager (and some other TpProxy) because they have _tp_proxy_ensure_factory() in their constructed(). AM rely on it to create its TpAccount objects too.

> 
> -  g_return_if_fail (TP_IS_CLIENT_CHANNEL_FACTORY (factory));
> 
>    tp_clear_object (&self->priv->channel_factory);
> 
> -  self->priv->channel_factory = g_object_ref (factory);
> +  if (factory != NULL)
> +    self->priv->channel_factory = g_object_ref (factory);
> 
> You should probably not remove the type check.

The difference now is that factory given could be NULL, but I guess I could change to (factory == NULL || TP_IS_CLIENT_CHANNEL_FACTORY (factory))

*** Bug 37971 has been marked as a duplicate of this bug. ***

(In reply to comment #64)
> -      channel = tp_client_channel_factory_create_channel (
> -          self->priv->channel_factory, connection, chan_path, chan_props,
> -          &error);
> +      if (self->priv->channel_factory != NULL)
> +        channel = tp_client_channel_factory_create_channel (
> +            self->priv->channel_factory, connection, chan_path, chan_props,
> +            &error);
> +      else
> +        channel = tp_simple_client_factory_ensure_channel (
> +            tp_proxy_get_factory (self->priv->account_mgr), connection,
> +            chan_path, chan_props, &error);
> 
> This, and the equivalent for dupping features, is repeatedly duplicated.

fixed

> -  g_return_if_fail (TP_IS_CLIENT_CHANNEL_FACTORY (factory));
> 
>    tp_clear_object (&self->priv->channel_factory);
> 
> -  self->priv->channel_factory = g_object_ref (factory);
> +  if (factory != NULL)
> +    self->priv->channel_factory = g_object_ref (factory);
> 
> You should probably not remove the type check.

fixed

factories-continued2 merged into master

I've changed TpAccountManager to always guarantee that its TpAccount are prepared with all desired features set on the factory. This is now part of CORE and not an extra feature.

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=factories-continued

Here's a review of the first and last outstanding patches.

TpAccountManager: prepare its TpAccounts before exposing them
=============================================================

-tp_account_manager_ensure_account (TpAccountManager *manager,
+tp_account_manager_ensure_account (TpAccountManager *self,

It's *MUCH* more tedious to review code if you will insist on randomly renaming variables in an already-enormous patch which makes substantial changes to the inner workings of functions. Please don't do this. I definitely prefer 'self' being used consistently, but not in the same patch as fundamental changes.

This actively discourages me from reviewing this code, and I don't think I'm alone in my reaction.

Even disregarding this, the commitdiff is completely illegible. It must have been possible to make this change in smaller chunks, rather than in a single 400-line patch.

 tp_account_manager_get_valid_accounts (TpAccountManager *manager)
 {
-  TpAccountManagerPrivate *priv;
-  GList *ret;
-
   g_return_val_if_fail (TP_IS_ACCOUNT_MANAGER (manager), NULL);
 
-  priv = manager->priv;
-
-  ret = g_hash_table_get_values (priv->accounts);
-
-  return ret;
+  return g_hash_table_get_values (manager->priv->accounts);
 }

This is also *completely* unrelated to the rest of the patch, and has *no functional effect*!


Anyway, on with the review
--------------------------

+ * All #TpAccount<!-- -->s are guaranteed to have all desired features prepared
+ * (at least %TP_ACCOUNT_FEATURE_CORE).
+ * See tp_simple_client_factory_add_account_features() to define desired
+ * features.

“The returned #TpAccount<!-- -->s are guaranteed to have %TP_ACCOUNT_FEATURE_CORE prepared, along with all features previously passed to tp_simple_client_factory_add_account_features().”?

Ditto the other places where you use this wording.


Legacy accounts
---------------

This should have been a separate patch. It's probably a prerequisite for the other changes, I agree, but it could have been made independently, first.

+tp_account_manager_ensure_account (TpAccountManager *self,
     const gchar *path)
 {
-  TpAccountManagerPrivate *priv;

Why? A bunch of functions seem to randomly lose their 'priv' locals even though they use the private structure.


+  if (self->priv->legacy_accounts == NULL)
+    self->priv->legacy_accounts = g_hash_table_new_full (
+        g_str_hash, g_str_equal, g_free, g_object_unref);

Why fuss about lazily initializing a hash table?

+static void
+legacy_account_invalidated_cb (TpProxy *account,
+    guint domain,
+    gint code,
+    gchar *message,
+    gpointer user_data)
+{
+  TpAccountManager *self = user_data;
+
+  g_hash_table_remove (self->priv->legacy_accounts,
+      tp_proxy_get_object_path (account));
 }

What happens if I do this:

  TpAccountManager *am = tp_account_manager_new_with_factory (...);
  tp_account_manager_ensure_account (am, "some valid account path");
  g_object_unref (am);

I *bet* that when the TpAM eventually gets disposed, it'll crash, because this code will run:

+  tp_clear_pointer (&priv->legacy_accounts, g_hash_table_unref);

tp_clear_pointer first sets priv->legacy_accounts to NULL, and then unrefs the hash table, which will unref the account, which will make it emit ::invalidated (bug 22119), which will call legacy_account_invalidated_cb, which will crash because self->priv->legacy_account == NULL.

(And no, using tp_g_signal_connect_object() does not save you because weak refs are notified in GObject's dispose, which runs after _tp_account_manager_dispose() which is where the hash table is destroyed.)

You might get saved by tp_proxy_prepare_async() keeping the legacy account alive; so change my above example to unref am only after that's finished.

The corresponding callback for normal accounts does not have this problem because:

  /* We only want to deal with accounts being removed here. */
  if (domain != TP_DBUS_ERRORS || code != TP_DBUS_ERROR_OBJECT_REMOVED)
    return;

TpSimpleClientFactory: add code example of typical app's main()...
==================================================================

+ * A typical application using its own factory would look like this:
+ * |[
+ * int main()
+ * {
+ *   TpSimpleClientFactory *factory;
+ *   TpAccountManager *manager;
+ *
+ *   factory = my_factory_new ();

A typical application will not use its own factory; it will use #TpAutomaticClientFactory. If an application does use its own factory class, it will presumably be a subclass of #TpAutomaticClientFactory.

This example won't compile: main needs arguments.

It also will crash because it doesn't call g_type_init().

• Deprecate tp_account_manager_ensure_account()

Speaking of which: why is tp_account_manager_ensure_account() not just a wrapper around the equivalent call to tp_simple_client_factory_ensure_account()? I guess it's down to the fact that the former doesn't return a ref, whereas the latter does? Annoying if so—but does tp_account_manager_ensure_account() need to fire off a call to tp_proxy_prepare_async()?

(Oh! for an autorelease pool.)

+  /* Lazy init self->priv->account */

English nitpick: “lazily init[ialize]”.

• TpBaseClient: prepare TpAccount, TpConnection and TpChannels with factory desired features

This patch is ~1300 lines of changes and ends with

+#if 0
+
+  FIXME: Disabled for now because it is racy. If channel path is already removed
+  from the bus when AddDispatchOperation implementation starts, it will return a
+  dbus error.
+

I'm sorry, I can't bring myself to trust a patch which “refactor[s] completely the way TpBaseClient prepares its proxies” whose only addition to the test suite is to comment out part of a test case. ;) I haven't reviewed this patch, or “TpBaseClient: add constructors with factory instead of AM”

• Deprecate tp_base_client_add_*_features()

  * @account: a #TpAccount with %TP_ACCOUNT_FEATURE_CORE, and any other
- *  features added via tp_base_client_add_account_features(), prepared if
+ *  features added via tp_base_client_add_account_features(), or
+ *  tp_simple_client_factory_add_account_features(), prepared if
  *  possible

The comma after tp_base_client_add_account_features() breaks the sentence. Ditto all the other places this wording is used.

While we're deprecating things, can you use G_GNUC_DEPRECATED_FOR()—presumably via another _TP_GNUC_DEPRECATED_FOR() wrapper—instead?

• Add tp_account_manager_set_default()

+ * Note that @manager must use the default dbus-daemon as returned by
+ * tp_dbus_daemon_dup()

#TpDBusDaemon

+  g_return_if_fail (_tp_dbus_daemon_is_the_shared_one (
+      tp_proxy_get_dbus_daemon (manager)));
+  g_return_if_fail (starter_account_manager_proxy == NULL);

I think it might be kinder to people who eventually get this wrong to do

if (!_tp_dbus_daemon_is_the_shared_one (_tp_proxy_get_dbus_daemon (manager)))
  {
    CRITICAL ("'manager' must use the TpDBusDaemon returned by tp_dbus_daemon_dup()");
    g_return_if_reached ();
  }

and similarly "may only be called once per process".

+ * This function can only be called before the first usage of
+ * tp_account_manager_dup(). It is useful for applications using a custom
+ * #TpSimpleClientFactory and want the default Account Manager to use it. See
+ * tp_account_manager_new_with_factory().

“This function may only be called before the first call to tp_account_manager_dup(), and may not be called more than once. Applications which use a custom #TpSimpleClientFactory and want the default #TpAccountManager to use that factory should call this after calling tp_account_manager_new_with_factory().” or something?

• TpAccountChannelRequest: do not create a TpAccountManager for tmp handler

Looks fine.

• TpAccountManager is the "toplevel" object, do not create it through a factory

Looks fine.

commits are now much better splitted, sorry for the inconvenience.

I've fixed issues from comment #70.

for the commit "TpBaseClient: prepare TpAccount, TpConnection and TpChannels with factory desired features" I've removed the huge refactor and sticked the what's strictly necessary for factories work. We can remove the big code duplication later.

Still need to fix comment #71

(In reply to comment #71)
> • Deprecate tp_account_manager_ensure_account()
> 
> Speaking of which: why is tp_account_manager_ensure_account() not just a
> wrapper around the equivalent call to
> tp_simple_client_factory_ensure_account()? I guess it's down to the fact that
> the former doesn't return a ref, whereas the latter does? Annoying if so—but
> does tp_account_manager_ensure_account() need to fire off a call to
> tp_proxy_prepare_async()?
> 
> (Oh! for an autorelease pool.)

Yes, legacy_accounts table is just there to keep compatibility, because previous API does not return a ref and was keeping one for the user... sadness...

> • TpBaseClient: prepare TpAccount, TpConnection and TpChannels with factory
> desired features

reduced that patch to keep only what's needed, the refactor to reduce code duplication can be done later separately. sorry for the massive patch, it's now gone.

All issues from comment #71 fixed now.

Ready for next review round :)

I like the idea of tp_account_manager_set_default(), it makes things much easier as app won't have to care about the AM and can continue using tp_account_manager_dup() as it already does.

But I'm wondering if the tp_simple_*_new_with_am() still makes that much sense now. In most cases, users will just use the one returned by _dup().
Maybe we shouldn't deprecate tp_simple_new() after all.

tp_simple_new() take a TpDBusDaemon so you would have to do tp_dbus_daemon_dup() anyway, with the extra risk of user not using the default one which would force TpBaseClient to create its own new TpAccountManager (for compatibility, but probably we could break that assumption tbh).


The idea here is for process just handling channels, they would not create a TpAccountManager at all, they just need a factory, so only the account of channels it handle will be created. tp_simple_new_with_am() is just an helper around tp_simple_new_with_factory(tp_proxy_get_factory(am)); tpqt4 does the same tbh.

+   * don't even know if they are valid. For compatiliby we can't return a ref,

compatibility.

 static void
+legacy_account_invalidated_cb (TpProxy *account,
+    guint domain,
+    gint code,
+    gchar *message,
+    gpointer user_data)
+{
+  TpAccountManager *self = user_data;
+
+  /* We only want to deal with accounts being removed here. */
+  if (domain != TP_DBUS_ERRORS || code != TP_DBUS_ERROR_OBJECT_REMOVED)
+    return;
+
+  g_hash_table_remove (self->priv->legacy_accounts,
+      tp_proxy_get_object_path (account));
+}

This is still wrong. I think this will mean that, if a legacy account becomes invalid and then valid again, subsequent calls to tp_account_manager_ensure_account() will return an invalidated TpAccount object, and there's no way to stop it doing that. You do need to cope with legacy accounts becoming invalid and take them out of the dictionary; the issue is not crashing when TpAM disposes. Personally, I think I would make the dispose function iterate the hash table, removing all the callbacks, and only then destroy the hash table. (I wasn't pointing out the similar check for non-legacy accounts to say “just put this into this callback too”. :))

> TpAccountChannelRequest: do not create a TpAccountManager for tmp handler
> 
> We can now create TpSimpleHandle with account's factory. This avoid
> introspection of other accounts if app does not need them.

TpSimpleHandle*R*.


+ * Notice the call to tp_account_manager_set_default() at the beginning of your
+ * main() to ensure that any library/plugin using telepathy as well will share
+ * your #TpAccountManager using tp_account_manager_dup().

“The call to tp_account_manager_set_default() near the beginning of main() will ensure that any libraries or plugins which also use Telepathy (and call tp_account_manager_dup()) will share your #TpAccountManager.”

Thanks for all the reviews. Fixed latest comments and merged branch.

bug fixed \o/