[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #44 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2011-07-14 07:57:13 PDT ---
(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.

-- 
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.