[Bug 38142] proxy factories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #63 from Will Thompson <will.thompson at collabora.co.uk> 2011-07-25 08:53:58 PDT ---
+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.

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