[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #45 from Xavier Claessens <xclaesse at gmail.com> 2011-07-15 00:59:57 PDT ---
(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

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