Bug 41801

http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=contact-blocking-41801

+    g_return_if_fail (_tp_contacts_to_handles (self, n_contacts, contacts, \
+        &handles)); \

You can't do this: g_return_if_fail can be compiled away to nothing.

Why does the first patch define a macro (contact_blocking_generic_async) which it doesn't actually use?

What does "contact-list-clt.c" mean?

I haven't really reviewed this.

  Once TP_CONNECTION_FEATURE_CONTACT_BLOCKING has been prepared we can easily
  prepare TP_CONNECTION_FEATURE_CONTACT_BLOCKING on all contacts as we have all
  the information needed. Extra contact feature for free!

This commit message is wrong. One of those should be TP_CONTACT_FEATURE_…

+ * @n_contacts: the number of contacts in @contacts (must be at least 1)

but the code doesn't enforce that.


+ * @report_abusive: if %TRUE, in addition to blocking, report these contacts as
+ * abusive to the server administrators, if supported, see
+ * #TpConnection:can-report-abusive

Better wording would be “If %TRUE, report these contacts as abusive to the server administrators as well as blocking them. See #TpConnection:can-report-abusive to discover whether reporting abuse is supported. If #TpConnection:can-report-abusive is %FALSE, this parameter will be ignored.”


+ * Expands to a call to a function that returns a #GQuark representing the
+ * "contact-blocking" feature.

Why is "contact-blocking" in quotes with a hyphen? The GQuark isn't for that string.

+ * When this feature is prepared, the #TpConnection:can-report-abusive and
+ * #TpConnection:blocked-contacts properties of the Connection have been
+ * retrieved. The #TpContact objects from #TpConnection:blocked-contacts
+ * have been prepared with the desired features.

“When this feature is prepared, #TpConnection:blocked-contacts will contain an up-to-date list of #TpContact<!-- -->s the user has blocked, and #TpConnection:can-report-abusive will indicate whether abusive contacts can be reported to the server administrator.”

I think you can put the stuff about contact features into the #TpConnection:blocked-contacts documentation.

+      goto out;
+    }
+
+  self->priv->contact_blocking_capabilities = tp_asv_get_uint32 (properties,
+      "ContactBlockingCapabilities", &valid);
+  if (!valid)
+    {
+      DEBUG ("Connection %s doesn't have ContactBlockingCapabilities property",
+          tp_proxy_get_object_path (self));
+    }
+
+out:

I don't really see that the goto buys us anything here.

+   * When calling tp_connection_block_contacts_async(), the contacts may be
+   * reporting as abusive to the server administrators by setting
+   * report_abusive to %TRUE.

“If this property is %TRUE, contacts may be reported as abusive to the server administrators by setting report_abusive to %TRUE when calling tp_connection_block_contacts_async().”

+  contacts_arr = g_ptr_array_new_with_free_func (g_object_unref);
+
+  g_hash_table_iter_init (&iter, contacts);
+  while (g_hash_table_iter_next (&iter, &key, &value))
+    {
+      TpHandle handle = GPOINTER_TO_UINT (key);
+      const gchar *id = value;
+      TpContact *contact;
+
+      contact = tp_connection_dup_contact_if_possible (self, handle, id);
+      if (contact == NULL)
+        {
+          DEBUG ("Failed to create contact %s (%d)", id, handle);
+          continue;
+        }
+
+      g_ptr_array_add (contacts_arr, contact);
+    }

There must be code elsewhere to turn an a{us} into an array of TpContacts. And presumably also to prepare 'em all with the set of features from grabbed from a factory for 'self'.

I think process_queued_blocked_changed leaks 'contacts'.


+  /* We are not supposed to add items to this queue while the blocked contacts
+   * have been fetched. */
+  g_assert_cmpuint (self->priv->blocked_changed_queue->length, ==, 0);

s/while/until/.

+void _tp_connection_blocked_changed_queue_free (GQueue *queue)

Coding style: add a newline.

+/**
+ * tp_contact_block_async:
+ * @self: a #TpContact
+ * @report_abusive: if %TRUE, in addition to blocking, report these contacts as
+ * abusive to the server administrators, if supported, see
+ * #TpConnection:can-report-abusive

You copy-pasted this, so apply the same wording change as above, but also make it singular.

+ * Convenience wrapper for tp_connection_block_contacts_async()
+ * on a single contact.

I would rather this said something like “Block communications with a contact, optionally reporting the contact as abusive to the server administrators. To block more than one contact at once, see tp_connection_block_contacts_async().”

+/**
+ * tp_contact_is_blocked:
+ * @self: a #TpContact
+ *
+ * <!-- -->
+
+ * Returns: the value of #TpContact:is-rblocked.

Typo.

+      /* Finish to prepare TP_CONNECTION_FEATURE_CONTACT_BLOCKING, we can
+       * prepare TP_CONTACT_FEATURE_CONTACT_BLOCKING on all contacts as we
+       * have now the list of blocked contacts. */

s/Finish to prepare/Finish preparing/; s/,/;/.


+  /* Preparing TP_CONNECTION_FEATURE_CONTACT_BLOCKING gives us
+   * TP_CONTACT_FEATURE_CONTACT_BLOCKING for free. Test that this work with
+   * and and new TpContact. */

s/work/works/; "with and and new"?

(In reply to comment #2)
> +    g_return_if_fail (_tp_contacts_to_handles (self, n_contacts, contacts, \
> +        &handles)); \
> 
> You can't do this: g_return_if_fail can be compiled away to nothing.

Oh good point. Fixed; I'll fix existing code as well.

> Why does the first patch define a macro (contact_blocking_generic_async) which
> it doesn't actually use?

removed.

> What does "contact-list-clt.c" mean?

client. We already have contact-list.c testing TpBaseConnection and I prefered
to creater a new file testing the client bits as it was already pretty big.
I can rename to contact-list-client if you prefer.

(In reply to comment #3)
>   Once TP_CONNECTION_FEATURE_CONTACT_BLOCKING has been prepared we can easily
>   prepare TP_CONNECTION_FEATURE_CONTACT_BLOCKING on all contacts as we have all
>   the information needed. Extra contact feature for free!
> 
> This commit message is wrong. One of those should be TP_CONTACT_FEATURE_…

fixed.

> + * @n_contacts: the number of contacts in @contacts (must be at least 1)
> 
> but the code doesn't enforce that.

_tp_contacts_to_handles() now does.

> + * @report_abusive: if %TRUE, in addition to blocking, report these contacts
> as
> + * abusive to the server administrators, if supported, see
> + * #TpConnection:can-report-abusive
> 
> Better wording would be “If %TRUE, report these contacts as abusive to the
> server administrators as well as blocking them. See
> #TpConnection:can-report-abusive to discover whether reporting abuse is
> supported. If #TpConnection:can-report-abusive is %FALSE, this parameter will
> be ignored.”

changed; thanks.
I changed in tp_contact_block_async() as well.

> + * Expands to a call to a function that returns a #GQuark representing the
> + * "contact-blocking" feature.
> 
> Why is "contact-blocking" in quotes with a hyphen? The GQuark isn't for that
> string.

That's how we document all the features.

> + * When this feature is prepared, the #TpConnection:can-report-abusive and
> + * #TpConnection:blocked-contacts properties of the Connection have been
> + * retrieved. The #TpContact objects from #TpConnection:blocked-contacts
> + * have been prepared with the desired features.
> 
> “When this feature is prepared, #TpConnection:blocked-contacts will contain an
> up-to-date list of #TpContact<!-- -->s the user has blocked, and
> #TpConnection:can-report-abusive will indicate whether abusive contacts can be
> reported to the server administrator.”

changed; thanks.

> I think you can put the stuff about contact features into the
> #TpConnection:blocked-contacts documentation.

moved.

> +      goto out;
> +    }
> +
> +  self->priv->contact_blocking_capabilities = tp_asv_get_uint32 (properties,
> +      "ContactBlockingCapabilities", &valid);
> +  if (!valid)
> +    {
> +      DEBUG ("Connection %s doesn't have ContactBlockingCapabilities
> property",
> +          tp_proxy_get_object_path (self));
> +    }
> +
> +out:
> 
> I don't really see that the goto buys us anything here.

removed.

> +   * When calling tp_connection_block_contacts_async(), the contacts may be
> +   * reporting as abusive to the server administrators by setting
> +   * report_abusive to %TRUE.
> 
> “If this property is %TRUE, contacts may be reported as abusive to the server
> administrators by setting report_abusive to %TRUE when calling
> tp_connection_block_contacts_async().”

thanks; changed (yeah I suck at writing doc).

> +  contacts_arr = g_ptr_array_new_with_free_func (g_object_unref);
> +
> +  g_hash_table_iter_init (&iter, contacts);
> +  while (g_hash_table_iter_next (&iter, &key, &value))
> +    {
> +      TpHandle handle = GPOINTER_TO_UINT (key);
> +      const gchar *id = value;
> +      TpContact *contact;
> +
> +      contact = tp_connection_dup_contact_if_possible (self, handle, id);
> +      if (contact == NULL)
> +        {
> +          DEBUG ("Failed to create contact %s (%d)", id, handle);
> +          continue;
> +        }
> +
> +      g_ptr_array_add (contacts_arr, contact);
> +    }
> 
> There must be code elsewhere to turn an a{us} into an array of TpContacts. And
> presumably also to prepare 'em all with the set of features from grabbed from a
> factory for 'self'.

There is, but it's pretty dependent of the way the queue is managed. One
option could have been to re-use the same prepare queue as the contact list
but it would have make things much more complicated. I discussed this with
Xavier and he agreed that using another queue was fine.

> I think process_queued_blocked_changed leaks 'contacts'.

Good catch; fixed.

> +  /* We are not supposed to add items to this queue while the blocked contacts
> +   * have been fetched. */
> +  g_assert_cmpuint (self->priv->blocked_changed_queue->length, ==, 0);
> 
> s/while/until/.

of course. Fixed.

> +void _tp_connection_blocked_changed_queue_free (GQueue *queue)
> 
> Coding style: add a newline.

added.

> +/**
> + * tp_contact_block_async:
> + * @self: a #TpContact
> + * @report_abusive: if %TRUE, in addition to blocking, report these contacts
> as
> + * abusive to the server administrators, if supported, see
> + * #TpConnection:can-report-abusive
> 
> You copy-pasted this, so apply the same wording change as above, but also make
> it singular.

already done. :)

> + * Convenience wrapper for tp_connection_block_contacts_async()
> + * on a single contact.
> 
> I would rather this said something like “Block communications with a contact,
> optionally reporting the contact as abusive to the server administrators. To
> block more than one contact at once, see tp_connection_block_contacts_async().”

changed.

> +/**
> + * tp_contact_is_blocked:
> + * @self: a #TpContact
> + *
> + * <!-- -->
> +
> + * Returns: the value of #TpContact:is-rblocked.
> 
> Typo.

fixed.

> +      /* Finish to prepare TP_CONNECTION_FEATURE_CONTACT_BLOCKING, we can
> +       * prepare TP_CONTACT_FEATURE_CONTACT_BLOCKING on all contacts as we
> +       * have now the list of blocked contacts. */
> 
> s/Finish to prepare/Finish preparing/; s/,/;/.

fixed.

> +  /* Preparing TP_CONNECTION_FEATURE_CONTACT_BLOCKING gives us
> +   * TP_CONTACT_FEATURE_CONTACT_BLOCKING for free. Test that this work with
> +   * and and new TpContact. */
> 
> s/work/works/; "with and and new"?

changed to "Test that this works with existing and newly created TpContact."


Thanks for the review!

(In reply to comment #5)
> (In reply to comment #3)
> > + * @n_contacts: the number of contacts in @contacts (must be at least 1)
> > 
> > but the code doesn't enforce that.
> 
> _tp_contacts_to_handles() now does.

It would be kinder to application developers to (also) g_return_if_fail() directly from the functions they actually call, so that the criticals in their terminal refer to something they do rather than the internals of tp-glib.

> > Why is "contact-blocking" in quotes with a hyphen? The GQuark isn't for that
> > string.
> 
> That's how we document all the features.

Okay, but I don't think it's very useful documentation.

> > +      goto out;
> > +    }
> > +
> > +  self->priv->contact_blocking_capabilities = tp_asv_get_uint32 (properties,
> > +      "ContactBlockingCapabilities", &valid);
> > +  if (!valid)
> > +    {
> > +      DEBUG ("Connection %s doesn't have ContactBlockingCapabilities
> > property",
> > +          tp_proxy_get_object_path (self));
> > +    }
> > +
> > +out:
> > 
> > I don't really see that the goto buys us anything here.
> 
> removed.

I should have been clearer: I didn't mean “just blindly remove the goto”, I meant “use an else block”. Without the goto, it crashes because 'properties' is NULL.

> > There must be code elsewhere to turn an a{us} into an array of TpContacts. And
> > presumably also to prepare 'em all with the set of features from grabbed from a
> > factory for 'self'.
> 
> There is, but it's pretty dependent of the way the queue is managed. One
> option could have been to re-use the same prepare queue as the contact list
> but it would have make things much more complicated. I discussed this with
> Xavier and he agreed that using another queue was fine.

Okay! This makes sense given later patches.

> > +void _tp_connection_blocked_changed_queue_free (GQueue *queue)
> > 
> > Coding style: add a newline.
> 
> added.

Sorry, I should have been clearer: I didn't mean in the header. I meant in the .c file, between 'void' and '_tp_connection_blocked_changed_queue_free'.

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > + * @n_contacts: the number of contacts in @contacts (must be at least 1)
> > > 
> > > but the code doesn't enforce that.
> > 
> > _tp_contacts_to_handles() now does.
> 
> It would be kinder to application developers to (also) g_return_if_fail()
> directly from the functions they actually call, so that the criticals in their
> terminal refer to something they do rather than the internals of tp-glib.

done.

> > > +      goto out;
> > > +    }
> > > +
> > > +  self->priv->contact_blocking_capabilities = tp_asv_get_uint32 (properties,
> > > +      "ContactBlockingCapabilities", &valid);
> > > +  if (!valid)
> > > +    {
> > > +      DEBUG ("Connection %s doesn't have ContactBlockingCapabilities
> > > property",
> > > +          tp_proxy_get_object_path (self));
> > > +    }
> > > +
> > > +out:
> > > 
> > > I don't really see that the goto buys us anything here.
> > 
> > removed.
> 
> I should have been clearer: I didn't mean “just blindly remove the goto”, I
> meant “use an else block”. Without the goto, it crashes because 'properties' is
> NULL.

Oh right it's not NULL-safe. fixed (squashed).

> > > +void _tp_connection_blocked_changed_queue_free (GQueue *queue)
> > > 
> > > Coding style: add a newline.
> > 
> > added.
> 
> Sorry, I should have been clearer: I didn't mean in the header. I meant in the
> .c file, between 'void' and '_tp_connection_blocked_changed_queue_free'.

Of course; changed (squashed).

For some reason
http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=contact-blocking-41801
hates me and doesn't display the latest version of the branch?!

ship it!

Merged to master; will be in 0.17.0.