[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=38142

--- Comment #41 from Xavier Claessens <xclaesse at gmail.com> 2011-07-14 07:33:46 PDT ---
(In reply to comment #22)
> Review of attachment 49071 [details]:
> 
> ::: 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

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.