[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #65 from Xavier Claessens <xclaesse at gmail.com> 2011-07-26 02:24:09 PDT ---
(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

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