Now that we have ContactList and ContactGroups we should add ContactBlocking. http://telepathy.freedesktop.org/spec/Connection_Interface_Contact_Blocking.html
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.