[Bug 41801] High level API for ContactBlocking
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Tue Nov 1 11:11:50 CET 2011
https://bugs.freedesktop.org/show_bug.cgi?id=41801
--- Comment #5 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2011-11-01 03:11:50 PDT ---
(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!
--
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.
More information about the telepathy-bugs
mailing list