It would be useful to have a TpContactSearch object to perform searches. I've ported the EmpathyTpContactSearch one that Danni wrote on bgo#606947 to tp-glib. It probably needs a lot of work but this is already working so I put it here. Please comment on it. It would probably be useful to make it be able to perform various searches, which right now it can't. E.g. set the search channel, and then call tp_contact_search_search() twice with different search keys.
http://git.collabora.co.uk/?p=user/pochu/telepathy-glib.git;a=shortlog;h=refs/heads/contact-search-32053
You didn't add new API to telepathy-glib-sections.txt. Please build with --enable-gtk-doc and make sure than "make check" pass. Speaking of checking, it would be good to have tests. I didn't look at the details of the code, just the API: - Maybe the API should be TpAccount centric rather than TpConnection ? Don't know - tp_contact_search_set_connection() seems wrong, I'd make the conn/account prop construct only. That would also use to get rid of the search-keys-changed signal. - Rename tp_contact_search_search() to tp_contact_search_start()? - We need API to know the supported search terms. - You don't use the More() method if needed. - It would be good to return an higher level structur for results. Maybe we could use TpContact ?
(In reply to comment #2) > You didn't add new API to telepathy-glib-sections.txt. Please build with > --enable-gtk-doc and make sure than "make check" pass. Fixed. > Speaking of checking, it would be good to have tests. > > I didn't look at the details of the code, just the API: > > - Maybe the API should be TpAccount centric rather than TpConnection ? Don't > know Neither do I... > - tp_contact_search_set_connection() seems wrong, I'd make the conn/account > prop construct only. Agreed. If you need a TpContactSearch for another connection you can create a new object. Fixed. > That would also use to get rid of the search-keys-changed signal. How would you know the search keys then? The problem that I see with adding an API as you suggest below is that we need to query the search channel, but that may not be valid at the beginning. Or the API to query it could have a callback parameter which is called when the channel has been created and we know the search keys. Is that what you propose? > - Rename tp_contact_search_search() to tp_contact_search_start()? Or tp_contact_search_do_search? I agree _search_search() sounds a bit weird :) > - We need API to know the supported search terms. See comment above > - You don't use the More() method if needed. What's this? > - It would be good to return an higher level structur for results. Maybe we > could use TpContact ? Yeah, I guess I could do that. I'd need to translate the results from ids to TpContacts. Would returning a TpContact * const *contacts (as returned by tp_connection_get_contacts_by_id()) be acceptable, or would you put somewhere else? Does anybody else think this would be useful?
(In reply to comment #2) > - You don't use the More() method if needed. I've just added a tp_contact_search_get_more_results() and a tp_contact_search_stop() methods. Should we also have a _get_status() method and a search-status-changed signal? Also wrt making TpContactSearch handle more than one search, I'm thinking of having a tp_contact_search_reset() method that will create a new channel and invoke a callback when it's ready to make a new search. Does that sound sensible?
(In reply to comment #3) > > That would also use to get rid of the search-keys-changed signal. > > How would you know the search keys then? The problem that I see with adding an > API as you suggest below is that we need to query the search channel, but that > may not be valid at the beginning. Or the API to query it could have a callback > parameter which is called when the channel has been created and we know the > search keys. Is that what you propose? Humm you're right, that's bong. What about something like that: TpContactSearch * tp_contact_search_new (TpConnection *conn, const gchar *server); with server == NULL meaning the default server. void tp_contact_search_get_search_keys_async (TpContactSearch *self, GAsyncReadyCallback callback, gpointer user_data); GHashTable * tp_contact_search_get_search_keys_finish (TpContactSearch *self, GAsyncResult *result, GError **error); This will allow us to fetch the AvailableSearchKeys property. Alternatives include: - Use a feature preparation mechanism (ala TpProxy) - GInitable maybe ? > > - It would be good to return an higher level structur for results. Maybe we > > could use TpContact ? > > Yeah, I guess I could do that. I'd need to translate the results from ids to > TpContacts. Would returning a TpContact * const *contacts (as returned by > tp_connection_get_contacts_by_id()) be acceptable, or would you put somewhere > else? Does anybody else think this would be useful? Maybe a GList of TpContact would be easier to deal with? (In reply to comment #4) > (In reply to comment #2) > > - You don't use the More() method if needed. > > I've just added a tp_contact_search_get_more_results() and a > tp_contact_search_stop() methods. Should we also have a _get_status() method > and a search-status-changed signal? I'm not sure we should expose More() as you did. What do you think of: typedef gboolean (TpContactSearchCallback *) (TpContactSearch *self, const GError *error, GList *results, gpointer user_data); void tp_contact_search_start (TpContactSearch *self, const GHashTable *keys, TpContactSearchCallback cb, gpointer user_data); The callback could return TRUE if it wants more results. And an empty list as results would mean that we the search is finished. > Also wrt making TpContactSearch handle more than one search, I'm thinking of > having a tp_contact_search_reset() method that will create a new channel and > invoke a callback when it's ready to make a new search. Does that sound > sensible? I'm not sure it's worth it. If this API is simple enough to use (which it should), recreating a new TpContactSearch object shouldn't be a problem. Smcv: input welcome, you're the API design guru here. :)
(In reply to comment #5) > (In reply to comment #3) > > > That would also use to get rid of the search-keys-changed signal. > > > > How would you know the search keys then? The problem that I see with adding an > > API as you suggest below is that we need to query the search channel, but that > > may not be valid at the beginning. Or the API to query it could have a callback > > parameter which is called when the channel has been created and we know the > > search keys. Is that what you propose? > > Humm you're right, that's bong. > > What about something like that: > > TpContactSearch * tp_contact_search_new (TpConnection *conn, > const gchar *server); > with server == NULL meaning the default server. So we can search on random servers on e.g. XMPP? sounds good. > void tp_contact_search_get_search_keys_async (TpContactSearch *self, > GAsyncReadyCallback callback, > gpointer user_data); > > GHashTable * tp_contact_search_get_search_keys_finish (TpContactSearch *self, > GAsyncResult *result, > GError **error); > > This will allow us to fetch the AvailableSearchKeys property. Alternatives > include: > - Use a feature preparation mechanism (ala TpProxy) I'm not familiar with that. > - GInitable maybe ? This seems to be synchronous, no? That wouldn't work for us. Otherwise it seems nice. The asynchronous API sounds good too. > > > - It would be good to return an higher level structur for results. Maybe we > > > could use TpContact ? > > > > Yeah, I guess I could do that. I'd need to translate the results from ids to > > TpContacts. Would returning a TpContact * const *contacts (as returned by > > tp_connection_get_contacts_by_id()) be acceptable, or would you put somewhere > > else? Does anybody else think this would be useful? > > Maybe a GList of TpContact would be easier to deal with? Yes, could be. And sounds nicer. > (In reply to comment #4) > > (In reply to comment #2) > > > - You don't use the More() method if needed. > > > > I've just added a tp_contact_search_get_more_results() and a > > tp_contact_search_stop() methods. Should we also have a _get_status() method > > and a search-status-changed signal? > > I'm not sure we should expose More() as you did. > > What do you think of: > > typedef gboolean (TpContactSearchCallback *) (TpContactSearch *self, > const GError *error, > GList *results, > gpointer user_data); > > void tp_contact_search_start (TpContactSearch *self, > const GHashTable *keys, > TpContactSearchCallback cb, > gpointer user_data); > > The callback could return TRUE if it wants more results. And an empty list as > results would mean that we the search is finished. So normally the callback will return TRUE to get more contacts, until it gets an empty GList, in which case it will return FALSE? Sounds good to me. > > Also wrt making TpContactSearch handle more than one search, I'm thinking of > > having a tp_contact_search_reset() method that will create a new channel and > > invoke a callback when it's ready to make a new search. Does that sound > > sensible? > > > I'm not sure it's worth it. If this API is simple enough to use (which it > should), recreating a new TpContactSearch object shouldn't be a problem. Not sure about this. We discussed this on IRC and having to create a new object for every search didn't sound very friendly. Though if it's easy I could leave with that... Opinions?
(In reply to comment #5) > What do you think of: > > typedef gboolean (TpContactSearchCallback *) (TpContactSearch *self, > const GError *error, > GList *results, > gpointer user_data); > > void tp_contact_search_start (TpContactSearch *self, > const GHashTable *keys, > TpContactSearchCallback cb, > gpointer user_data); I think you should use a regular GSignal, rather than a callback. That allows more than one person to connect to the results if they're so interested. I also think you want results to appear in the list as they are returned by the server. Not all servers return the search results immediately, but trickle them out as they're found. I think we should simply keep calling More() automatically until the user calls Stop() or the object is disposed.
(In reply to comment #6) > > - GInitable maybe ? > > This seems to be synchronous, no? That wouldn't work for us. Otherwise it seems > nice. cassidy means GAsyncInitable. This class should be oriented around writing a user-interface on top of it. I had initially intended for the ContactSearch object to live for the lifetime of the dialog. Perhaps making it GAsyncInitable, plus having a set_server_async/finish, both of which will retrieve new AvailableSearchTerms. It would be nice to expose 'server' as a writable property so that we may GBinding it to the view, and simply listen to the notify signal on AvailableSearchTerms.
I had a long comment explaining what the API should look like and why, but Firefox ate it, so you're going to get a string of shorter comments instead. (In reply to comment #2) > - It would be good to return an higher level structur for results. Maybe we > could use TpContact ? No, I don't think that's appropriate. Instantiating a TpContact requires an async round-trip to get the handle, and (in a recent tp-glib) locks the (handle, identifier) pair into the CM's memory for the lifetime of the Connection. There also isn't a whole lot of overlap between the information TpContact offers, and the information in a contact search result (which looks more like a mini version of ContactInfo than anything else). The extra information in a TpContact is relatively expensive to get for random contacts (notably, the avatar), so we shouldn't get it for everyone in the result set, only for contacts that the user expresses some sort of interest in (clicks on, perhaps). I think that's out of scope for this object. A high-level data structure would be good though: I think a TpContactSearchResult GObject with an identifier, a GList<TpContactInfoField> and optional convenience accessors for commonly-returned fields would be best. Before offering convenience accessors you'd need to research what real protocols actually give us - XMPP and libpurple would be good places to start.
(In reply to comment #7) > I think you should use a regular GSignal, rather than a callback. That allows > more than one person to connect to the results if they're so interested. I agree with the result but not with the reason :-) I don't think it's worth offering API for multiple users of a contact search - it's fine to have API of the form "you are expected to connect to all the signals before calling go()", IMO. However, GObject signals are better for bindability than plain callbacks, and if we encapsulate each contact search result into an object then the signal will be very easy to work with, so I think a signal TpContactSearch::contact-found(TpContactSearchResult *) would be a good way to do it. > I also think you want results to appear in the list as they are returned by the > server. Not all servers return the search results immediately, but trickle them > out as they're found. I agree. The D-Bus API is that results come in one at a time; I think trying to batch them up in the high-level binding would be counter-productive. > I think we should simply keep calling More() automatically until the user calls > Stop() or the object is disposed. I don't agree with this. If the server stops giving us results for a bit, it's probably because if left to its own devices, it would flood us with an excessive number of search hits; having use of the high-level API imply the memory/bandwidth cost of the complete result set seems unwise. Hopefully the contact the user wanted will be in the first few hits anyway...
(In reply to comment #8) > I had initially intended for the ContactSearch object to live for the lifetime > of the dialog. Perhaps making it GAsyncInitable, plus having a > set_server_async/finish, both of which will retrieve new AvailableSearchTerms. AvailableSearchTerms could (rarely) change between search tries - we mimic XMPP exactly here. Perhaps: - on initial setup, use GAsyncInitable, because there's nothing useful you can do until you've fetched the search keys - when switching the server, use an async method, because again, there's nothing useful you can do until you've fetched the new server's search keys - when repeating a search, get the search keys in the background and emit GObject::notify for a property in the unlikely case where they changed > It would be nice to expose 'server' as a writable property so that we may > GBinding it to the view, and simply listen to the notify signal on > AvailableSearchTerms. I'm not entirely convinced, because the search is unusable until we know how we can search.
Regarding construction of the search object, you could either have it require a TpConnection at construct-time (and use Conn.I.Requests directly), or have it require a TpAccount at construct-time and go via tp_account_channel_request_create_and_handle_channel_async(). I'd vaguely lean towards the latter, because that way you can go Contacts -> Search..., specify an account, and have it be put online automatically in order to do the search :-)
FTR so I don't forget 17:37 < smcv> just realised this is one of the things I'd written on the bug, but that firefox ate 17:37 < smcv> on any error, I think you want a TpError (from TpProxy, or tp_proxy_dbus_error_to_gerror if it was a string from D-Bus) 17:37 < smcv> *GError containing a TpError 17:38 < smcv> which is easiest to do as (uint, int, string) if you want to put it in a signal 17:38 < smcv> and also an accessor for extensible details like tp_connection_get_detailed_error(), which most people won't use
I've just pushed my current implementation to http://git.collabora.co.uk/?p=user/pochu/telepathy-glib.git;a=shortlog;h=refs/heads/contact-search-32053 It's lacking real testing (it builds but I haven't actually run it), updated docs, and a gboolean tp_capabilities_supports_contact_search (TpCapabilities *, gboolean *with_limit, gboolean *with_server) If you can have a look and tell me if I'm on track, and what things you don't like, etc, that would be much appreciated :)
tp_contact_search_new_finish should have (transfer full) and have G_GNUC_WARN_UNUSED_RESULT tp_contact_search_reset_async() looks a bit weird to me but I guess that's the price we have to pay if we want to be able to change the server without creating a new object. * Since: 0.13.9 Lie! use 0.13.UNRELEASED Shouldn't tp_contact_search_search() stop a previous search, if any? You should pass self as weak_object to you call_ methods. The TpAccount, server and limit should be proper GObject properties. Also, tp_contact_search_new_async() should be a tin wrapper around g_async_initable_new_async (ie, properties should be set by the GObeject property mechanism). TpAccountChannelRequest is leaked. You can unref it just after you called tp_account_channel_request_create_and_handle_channel_async() so it will be destroyed when it's done. _create_search_channel_cb: the error is leaked in the first error case. _create_search_channel_cb should complete the operation synchronously not in idle. self->priv->keys is set in 2 places; is that expected? Also, why not return self->priv->keys in tp_contact_search_reset_finish() ? No API to ask for more results? Like Smcv, I'd prefer to have an higher level object representing search result. Niptick ------- We usually use : "GCallback _padding[7];" as padding. if (error) We use if (ptr != NULL) return tp_asv_get_strv (tp_channel_borrow_immutable_properties (self->priv->channel), this line is too long (and some others too actually). tp_contact_search_init: No need to init to NULL/0, GObject does it for us. I tend to use g_hash_table_unref instead of _destroy. close_search_channel: you can use tp_clear_object()
(In reply to comment #15) > tp_contact_search_new_finish should have (transfer full) and have > G_GNUC_WARN_UNUSED_RESULT fixed > tp_contact_search_reset_async() looks a bit weird to me but I guess that's the > price we have to pay if we want to be able to change the server without > creating a new object. > > * Since: 0.13.9 > Lie! use 0.13.UNRELEASED fixed > Shouldn't tp_contact_search_search() stop a previous search, if any? > You should pass self as weak_object to you call_ methods. Not sure if that's necessary... since we require that subsequent searches call _reset first, and _reset closes the search channel (and creates a new one) > The TpAccount, server and limit should be proper GObject properties. > Also, tp_contact_search_new_async() should be a tin wrapper around > g_async_initable_new_async (ie, properties should be set by the GObeject > property > mechanism). done > TpAccountChannelRequest is leaked. You can unref it just after you called > tp_account_channel_request_create_and_handle_channel_async() so it will be > destroyed when it's done. fixed > _create_search_channel_cb: the error is leaked in the first error case. oops, fixed > _create_search_channel_cb should complete the operation synchronously not in > idle. not sure why, but done > self->priv->keys is set in 2 places; is that expected? ahem, no > Also, why not return self->priv->keys in tp_contact_search_reset_finish() ? done > No API to ask for more results? I haven't added it... is there consensus that we want this? it may make sense since there's no way to request that, though I don't see simple UIs using it > Like Smcv, I'd prefer to have an higher level object representing search > result. I haven't done that yet... I can look at it in the morning > Niptick > ------- > > We usually use : "GCallback _padding[7];" as padding. done > if (error) > We use if (ptr != NULL) I like more the other syntax :) but I'll get used to the tp style > return tp_asv_get_strv (tp_channel_borrow_immutable_properties > (self->priv->channel), > this line is too long (and some others too actually). all fixed > tp_contact_search_init: No need to init to NULL/0, GObject does it for us. ah ok > I tend to use g_hash_table_unref instead of _destroy. sounds good, changed > close_search_channel: you can use tp_clear_object() done
(In reply to comment #16) > (In reply to comment #15) > > Like Smcv, I'd prefer to have an higher level object representing search > > result. > > I haven't done that yet... I can look at it in the morning Done, I'm now returning a GList<TpContactSearchResult>. Should I register that as a new boxed type and make the signal know the type, instead of a G_TYPE_POINTER, to not leak the objects in the list?
(In reply to comment #17) > Done, I'm now returning a GList<TpContactSearchResult>. Should I register that > as a new boxed type and make the signal know the type, instead of a > G_TYPE_POINTER, to not leak the objects in the list? Since it's a signal, you don't need to register a new type, it doesn't matter. You are, however, responsible for cleaning up whatever you create after g_signal_emit() [non-event signals are synchronous]. Thus you want a g_list_foreach(g_object_unref); g_list_free(); pair. + results = g_list_append (results, search_result); Using g_list_prepend() followed by an optional g_list_reverse() will always be faster. O(2n) vs O(n^2).
(In reply to comment #18) > (In reply to comment #17) > > > Done, I'm now returning a GList<TpContactSearchResult>. Should I register that > > as a new boxed type and make the signal know the type, instead of a > > G_TYPE_POINTER, to not leak the objects in the list? > > Since it's a signal, you don't need to register a new type, it doesn't matter. > You are, however, responsible for cleaning up whatever you create after > g_signal_emit() [non-event signals are synchronous]. Oh, I thought it was async. > Thus you want a g_list_foreach(g_object_unref); g_list_free(); pair. Fixed using g_list_free_full (list, g_object_unref). > + results = g_list_append (results, search_result); > > Using g_list_prepend() followed by an optional g_list_reverse() will always be > faster. O(2n) vs O(n^2). Thanks for the tip, fixed (with O(n) since I'm not doing the _reverse()).
> +<INCLUDE>telepathy-glib/contact-search.h</INCLUDE> telepathy-glib/telepathy-glib.h seems like a better recommendation for new code. > tp_contact_search_reset_async The finish function seems to be missing from the docs? Please enable gtk-doc and do a 'make check'. > tp_contact_search_search The name does seem a bit odd. tp_contact_search_start? tp_contact_search_begin? I think the documentation should say "Connect to the #TpContactSearch::search-results-received signal before calling this function". There should be some sort of check for calling it at the wrong time, perhaps g_return_if_fail (self->priv->state == ...NOT_STARTED); > +tp_capabilities_supports_contact_search (TpCapabilities *self, If there are fixed properties other than the channel type, your channel request is going to fail, because you only provide the channel type. So, you should skip channel classes that have more than one fixed property, for forwards-compat. (I note in passing that the channel type in telepathy-spec should specify a typical requestable channel class, and the channel class used in Gabble violates "SHOULD always include [...] TargetHandleType". Oh well. We'll just have to document that in the spec...) > + if (with_limit) > + if (!tp_strdiff (allowed_properties[j], > + TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_LIMIT)) > + *with_limit = TRUE; For clarity, please use {} on all nested control structures, except optionally the "smallest": if (with_limit) { /* <= this one is mandatory */ if (!tp_strdiff (...)) { /* <= you may omit this one if you prefer */ *with_limit = TRUE; } } I prefer to have a blank line between control structures, which in this method would mean between adding a blank line between the first two "if"s, before the inner "for", and between the two outer "if"s in the inner "for". > + * @with_limit: a #gboolean location to return if the server can be > + * set on a search, or %NULL You don't need to say it's a gboolean, that's obvious from the C signature. For g-i you should add the (out) annotation. You've swapped the documentation of with_limit and with_server. You're missing documentation of the method, which would make a gtk-doc-enabled build fail: if there is literally nothing to say apart from the Returns:, make the body of the documentation be "<!-- nothing else to say -->" or something. I'd phrase this like: @with_limit: (out): if not %NULL, used to return %TRUE if the @limit parameter to tp_contact_search_new_async() and tp_contact_search_reset_async() can be nonzero @with_server: (out): if not %NULL, used to return %TRUE if the @server parameter to tp_contact_search_new_async() and tp_contact_search_reset_async() can be non-%NULL and phrase the body of the doc-comment, and the Returns:, to include both the abstract activity, and the concrete thing that will work, something like: Return whether this protocol or connection can perform contact searches. Optionally, also return whether a limited number of results can be specified, and whether alternative servers can be searched. Returns: %TRUE if #TpContactSearch can be used > + * tp_contact_search_get_search_keys: [...] > + * Makes a search for the keys specified in @keys. To receive No, that's not what this method does :-P Better documentation would mention that the keys are vCard field names in lower case, except when they're one of the special cases from telepathy-spec like "tel;cell" or "x-n-given". > + * @keys: the keys to search for Better would be: @criteria: (transfer none) (element-type utf8 utf8): a map from keys returned by tp_contact_search_get_search_keys() to values to search for > + * tp_contact_search_reset_async: [...] > + g_assert (self->priv->async_res == NULL); This crashes if the caller resets it repeatedly, or resets it while a channel is being fetched. At the very least, it should g_return_if_fail, and document that it's an error to call this function while async_init or reset_async is in progress. However, I think it'd be nicer if you could set it up so that resetting while a reset is already in progress just makes the first reset fail with G_IO_ERROR_CANCELLED, or something. > + * @results: #GHashTable with the search results From earlier discussion on this bug, I think you forgot to change the documentation, and it's actually a GList of TpContactSearchResult. It should have (element-type TelepathyGLib.ContactSearchResult) or however you're meant to spell that. > + self->priv->account = g_value_get_object (value); Take a ref, and release it in dispose, rather than assuming the account will continue to exist. "C bindings" for the properties would be nice: tp_contact_search_get_account, tp_contact_search_get_server, tp_contact_search_get_limit. In _create_search_channel_cb you should get the Server D-Bus property, and change your server property accordingly if necessary (with GObject::notify). If you ask for a channel with Server = "" on XMPP (when we've fixed Gabble to make it work), you'll get one back with Server = "users.jabber.org" or whatever. The same for Limit. _create_search_channel_cb should either do an async Get for the search channel's state, or assume that it's initially NOT_STARTED and reset it to that. I think the latter would be fine. > + G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, async_initable_iface_init)); Doesn't need a semicolon, because the expansion of G_IMPLEMENT_INTERFACE ends with "}". Some compilers fail on empty statements. > + * The class of a #TpContactSearch. In addition to @parent_class, > + * there are four pointers reserved for possible future use. "four" is 7 in this diagram. I'd recommend just not mentioning them, or saying "some pointers". You don't really need to document @parent_class either (until it grows at least one virtual method); you can just move the /*<private>*/ further up. The same applies to the TpContactSearchResult. + * SECTION:contact-search + * @title: TpContactSearch + * @short_description: proxy object for a Telepathy contact search channel + * @see_also: #TpChannel + * + * #TpContactSearch objects represent Telepathy instant messaging connections + * accessed via D-Bus. + * + * Compared with a simple proxy for method calls, they add the following + * features: + * + * <itemizedlist> + * <listitem>connection status tracking</listitem> + * <listitem>calling GetInterfaces() automatically</listitem> + * </itemizedlist> This is cut-and-paste damage. TpContactSearch doesn't inherit from TpProxy at all, so calling it a proxy object is confusing; call it "object representing an ongoing search for contacts" or something. It doesn't represent an IM connection, and doesn't call GetInterfaces; just delete all that. Useful things to mention in this intro might include the need to call reset_async() between searches, the fact that it's async-initable, and (for those who've been reading telepathy-spec) the fact that behind the scenes, it encapsulates one or more ContactSearch channels. +TpContactSearchResult* tp_contact_search_result_new (TpContactSearch *cs, + const gchar *identifier); + +void tp_contact_search_result_insert_field (TpContactSearchResult *self, + TpContactInfoField *field); I don't think these will be useful to call if you're not a TpContactSearch, so put them in a contact-search-internal.h that isn't installed, prefix them with _tp so they're not ABI, and add contact-search-internal.h to the various lists of internal headers that don't get scanned by gtk-doc and so on. We declare (functions returning) pointers as "T *p", not "T* p" (rationale: the whitespace shouldn't trick you into misinterpreting the meaning of "int* a, b", which declares a as a pointer to int, but b as just an int). Does TpContactSearchResult really need to point to the search it came from? I don't think it does - you can delete the contact-search property altogether. If it does need to, you'd need to take a ref, at which point you'd have a cyclic ref which you'd need to break by introducing explicit cleanup or weak-refs or something. > + * SECTION:contact-search > + * @title: TpContactSearchResult Um, no. This whole section also seems to be copypasta from TpConnection, and is inaccurate. TpContactSearchResult should have an accessor for the raw fields, perhaps something like /** ... * Returns: (transfer container) (element-type TpContactInfoField): ... */ GList *tp_contact_search_result_get_fields (TpCSR *self); (it seems that transfer container is conventional for GLists). tp_contact_search_result_get_identifier, tp_contact_search_result_get_field aren't documented. I'm not convinced that tp_contact_search_result_get_field is useful: if you want to keep it, you should certainly document its limitations (e.g. if the contact has a home and work phone number, it will arbitrarily pick one or the other, without telling you which). > * TpContactSearchResult:info: > + * > + * The lalala. ... > + "The maximum number of results to be returned by the server", No. :-P I don't think you actually need this as a construct property? A getter seems like enough; you could add a read-only property too, if you're into that sort of thing. "fields" seems like a better name than "info". > + g_list_free_full (self->priv->info, > + (void (*) (gpointer)) tp_contact_info_field_free); You could use (GDestroyNotify) for this cast rather than spelling out what it means.
(In reply to comment #16) > > No API to ask for more results? > > I haven't added it... is there consensus that we want this? it may make sense > since there's no way to request that, though I don't see simple UIs using it Paging through search results might well be fairly important in some protocols, although Gabble doesn't implement it.
Thanks for the thorough review! I'll be more careful next time, specially when it comes to the documentation... (In reply to comment #20) > > +tp_capabilities_supports_contact_search (TpCapabilities *self, > > If there are fixed properties other than the channel type, your channel request > is going to fail, because you only provide the channel type. So, you should > skip channel classes that have more than one fixed property, for > forwards-compat. Hmm, can you clarify this a bit? > (I note in passing that the channel type in telepathy-spec should specify a > typical requestable channel class, and the channel class used in Gabble > violates "SHOULD always include [...] TargetHandleType". Oh well. We'll just > have to document that in the spec...) Just to be sure, there's nothing for me to fix in TpCS, right? > > + * tp_contact_search_reset_async: > [...] > > + g_assert (self->priv->async_res == NULL); > > This crashes if the caller resets it repeatedly, or resets it while a channel > is being fetched. At the very least, it should g_return_if_fail, and document > that it's an error to call this function while async_init or reset_async is in > progress. > > However, I think it'd be nicer if you could set it up so that resetting while a > reset is already in progress just makes the first reset fail with > G_IO_ERROR_CANCELLED, or something. I have changed the g_assert()s to g_return_if_fail(), and documented that. I'll look at the GCancellable thing. > I'm not convinced that tp_contact_search_result_get_field is useful: if you > want to keep it, you should certainly document its limitations (e.g. if the > contact has a home and work phone number, it will arbitrarily pick one or the > other, without telling you which). Agreed, it's not very useful in its current form... I have changed it to return a TpContactInfoField, so you don't need to get the list of fields, look for the one you want, and then free the list. Everything else should be fixed. I'll look at adding some tests tomorrow too.
(In reply to comment #22) > (In reply to comment #20) > > > +tp_capabilities_supports_contact_search (TpCapabilities *self, > > > > If there are fixed properties other than the channel type, your channel request > > is going to fail, because you only provide the channel type. So, you should > > skip channel classes that have more than one fixed property, for > > forwards-compat. > > Hmm, can you clarify this a bit? Sure. A Gabble contact search requestable-channel-class currently looks like this (with suitable abbreviations): Fixed = { ChannelType: ContactSearch }, Allowed = [Server, Limit] and you make one of these requests: { ChannelType: ContactSearch, Server: "directory.example.com" } { ChannelType: ContactSearch, Limit: 42 } { ChannelType: ContactSearch, Server: "directory.example.com", Limit: 42 } All good so far. Similarly, if Server and/or Limit isn't allowed, you cope. However, imagine a CM requires you to put more in your request, so its RCC looks like this: Fixed = { ChannelType: ContactSearch, Hypothetical.SearchType: "randomly" }, Allowed = [Limit] Your TpCapabilities code would say "yes, I can work with that", but then when you came to do the request, you wouldn't match the Hypothetical.SearchType that the CM demanded, and your channel request would fail. > > (I note in passing that the channel type in telepathy-spec should specify a > > typical requestable channel class, and the channel class used in Gabble > > violates "SHOULD always include [...] TargetHandleType". Oh well. We'll just > > have to document that in the spec...) > > Just to be sure, there's nothing for me to fix in TpCS, right? Not for that paragraph.
(In reply to comment #22) > (In reply to comment #20) > > > +tp_capabilities_supports_contact_search (TpCapabilities *self, > > > > If there are fixed properties other than the channel type, your channel request > > is going to fail, because you only provide the channel type. So, you should > > skip channel classes that have more than one fixed property, for > > forwards-compat. > > Hmm, can you clarify this a bit? Thanks, fixed. > > > + * tp_contact_search_reset_async: > > [...] > > > + g_assert (self->priv->async_res == NULL); > > > > This crashes if the caller resets it repeatedly, or resets it while a channel > > is being fetched. At the very least, it should g_return_if_fail, and document > > that it's an error to call this function while async_init or reset_async is in > > progress. > > > > However, I think it'd be nicer if you could set it up so that resetting while a > > reset is already in progress just makes the first reset fail with > > G_IO_ERROR_CANCELLED, or something. > > I have changed the g_assert()s to g_return_if_fail(), and documented that. I'll > look at the GCancellable thing. Done, should be fixed now. It's the first time I look at GCancellable, so I'm not quite sure it's the proper way, but it seems to work fine in my tests... > I'll look at adding some tests tomorrow too. I've added a couple of test cases. The TpContactSearchResult one works fine, but the TpContactSearch one fails when creating a TpContactSearch object, I guess because of the /what/ever path in the TpAccount... Need to fix that and test that the other methods work as expected.
I'm quite happy with this now. I've updated the branch at http://git.collabora.co.uk/?p=user/pochu/telepathy-glib.git;a=shortlog;h=refs/heads/contact-search-32053, and squashed it at http://git.collabora.co.uk/?p=user/pochu/telepathy-glib.git;a=shortlog;h=refs/heads/tp-contact-search-32053 Please review.
/** * TpContactSearchResult: * * A proxy object for the results of a Telepathy contact It's not a proxy (in the sense of a TpProxy). Same for TpContactSearch. struct _TpContactSearchResultPrivate GList *fields; Please add a comment explaining the content of the list and if it's owned or borrowed. case PROP_IDENTIFIER: g_free (self->priv->identifier); As it's construct only, we ususuall don't free and use: g_assert (self->priv->identifier == NULL); /* construct-only */ tp_contact_search_result_get_fields Please document that the returned list should be free using g_list_free() tests/contact-search-result.c is leaky, use check-valgrind to check for leaks. * They also abstract the creation of contact search channels, but you need * to call @tp_contact_search_reset_async between searches. This isn't very clear imho. I'd focus on explaining how the object should be used; channel creation is a TP implementation detail. Use DEBUG() instead of g_print(). request = tp_asv_new ( TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH, NULL); if (self->priv->server != NULL) tp_asv_set_string (request, TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_SERVER, self->priv->server); if (self->priv->limit != 0) tp_asv_set_uint32 (request, TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_LIMIT, self->priv->limit); Add some \n between those. self->priv->account = g_object_ref (g_value_get_object (value)); Use g_value_dup_object() account and server params are contruct only so please add an assertion as explained above. * use @tp_capabilities_supports_contact_search to check if it's syntax is "use tp_capabilities_supports_contact_search() to check.." Same for other doc comments reffering to functions. g_cancellable_cancel (self->priv->cancellable); while (self->priv->async_res != NULL) g_main_context_iteration (NULL, TRUE); This is a bit weird; maybe we should request a g_cancellable_cancel_async() in glib? Do we really need to wait btw ? tp_contact_search_start(): shouldn't we check if another search is already active? + if (with_limit) if (with_limit != NULL) + if (!tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH)) use "continue" to limit nesting. tp_capabilities_supports_contact_search: you can use tp_value_array_unpack() Having test for TpContactSearch would make me an happier badger...
Thanks for the review. (In reply to comment #26) > /** > * TpContactSearchResult: > * > * A proxy object for the results of a Telepathy contact > > It's not a proxy (in the sense of a TpProxy). > Same for TpContactSearch. Fixed > struct _TpContactSearchResultPrivate > GList *fields; > Please add a comment explaining the content of the list and if it's owned or > borrowed. > > > case PROP_IDENTIFIER: > g_free (self->priv->identifier); > As it's construct only, we ususuall don't free and use: > g_assert (self->priv->identifier == NULL); /* construct-only */ Fixed > tp_contact_search_result_get_fields > Please document that the returned list should be free using g_list_free() Done > tests/contact-search-result.c > is leaky, use check-valgrind to check for leaks. Fixed > * They also abstract the creation of contact search channels, but you need > * to call @tp_contact_search_reset_async between searches. > This isn't very clear imho. I'd focus on explaining how the object should be > used; channel creation is a TP implementation detail. I've clarified this a bit. > Use DEBUG() instead of g_print(). Done > request = tp_asv_new ( > TP_PROP_CHANNEL_CHANNEL_TYPE, > G_TYPE_STRING, > TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH, > NULL); > if (self->priv->server != NULL) > tp_asv_set_string (request, > TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_SERVER, > self->priv->server); > if (self->priv->limit != 0) > tp_asv_set_uint32 (request, > TP_PROP_CHANNEL_TYPE_CONTACT_SEARCH_LIMIT, > self->priv->limit); > Add some \n between those. Done > self->priv->account = g_object_ref (g_value_get_object (value)); > Use g_value_dup_object() Done > account and server params are contruct only so please add an assertion as > explained above. Done > * use @tp_capabilities_supports_contact_search to check if it's > syntax is "use tp_capabilities_supports_contact_search() to check.." > Same for other doc comments reffering to functions. All fixed My gtk-doc foo is (slowly) getting better... :) > g_cancellable_cancel (self->priv->cancellable); > > while (self->priv->async_res != NULL) > g_main_context_iteration (NULL, TRUE); > This is a bit weird; maybe we should request a g_cancellable_cancel_async() in > glib? > Do we really need to wait btw ? As it is now, yes, since we use the same GCancellable, so we need to wait and reset it. Maybe we could have a list of GCancellables and just cancel it without waiting, or maybe we can cancel it and unref it (if gio cancells it before disposing the GCancellable), so we don't need to keep a list. The current code works though. > tp_contact_search_start(): shouldn't we check if another search is already > active? We do: g_return_if_fail (self->priv->state == TP_CHANNEL_CONTACT_SEARCH_STATE_NOT_STARTED); > + if (with_limit) > if (with_limit != NULL) > > + if (!tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH)) > use "continue" to limit nesting. I cannot use "continue" there since I have to check both with_limit and with_server, and if I continue in the first one, I won't get to check the second one. I could do if (with_limit != NULL && !tp_strdiff (...)) to avoid one level. Not sure if it's worth it... > tp_capabilities_supports_contact_search: you can use tp_value_array_unpack() Done > Having test for TpContactSearch would make me an happier badger... I tried to look at this, but apparently I need to create some classes for the DBus tests and I couldn't really understand how that stuff works.
+ * Returns: (transfer full): a new contact search channel, or %NULL It's not a TpChannel either. + * it when you're done with %g_list_free. Is that properly linked? I'd say it should be "g_list_free()". I think the long description in SECTION:contact-search should describe the whole search process (which function use to start a search, how to get the results and then how to start a new search). Actually having a simple demo app in examples/client could be useful. You add your new files to telepathy-glib/introspection.am so they'll be added to gir. (In reply to comment #27) > > g_cancellable_cancel (self->priv->cancellable); > > > > while (self->priv->async_res != NULL) > > g_main_context_iteration (NULL, TRUE); > > This is a bit weird; maybe we should request a g_cancellable_cancel_async() in > > glib? > > Do we really need to wait btw ? > > As it is now, yes, since we use the same GCancellable, so we need to wait and > reset it. Maybe we could have a list of GCancellables and just cancel it > without waiting, or maybe we can cancel it and unref it (if gio cancells it > before disposing the GCancellable), so we don't need to keep a list. The > current code works though. The cancel, unref and create a new one option sounds saner to me. Can you check the GCancellable implementation and/or test to see if that would work? > > tp_contact_search_start(): shouldn't we check if another search is already > > active? > > We do: > > g_return_if_fail (self->priv->state == > TP_CHANNEL_CONTACT_SEARCH_STATE_NOT_STARTED); You're right sorry. > > + if (with_limit) > > if (with_limit != NULL) > > > > + if (!tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH)) > > use "continue" to limit nesting. > > I cannot use "continue" there since I have to check both with_limit and > with_server, and if I continue in the first one, I won't get to check the > second one. I could do > > if (with_limit != NULL && !tp_strdiff (...)) > > to avoid one level. Not sure if it's worth it... I meant doing: if (tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH)) continue; Wouldn't that work? > > Having test for TpContactSearch would make me an happier badger... > > I tried to look at this, but apparently I need to create some classes for the > DBus tests and I couldn't really understand how that stuff works. Yeah you have to write a server side channel object to test your API. See for example tests/lib/stream-tube-chan.c tested in tests/dbus/stream-tube.c
(In reply to comment #28) > + * Returns: (transfer full): a new contact search channel, or %NULL > It's not a TpChannel either. OK, hopefully fixed for good... > + * it when you're done with %g_list_free. > Is that properly linked? I'd say it should be "g_list_free()". I've checked, and it is. I've changed it to () for consistency though. > I think the long description in SECTION:contact-search should describe the > whole search process (which function use to start a search, how to get the > results and then how to start a new search). I've added a paragraph explaining how a normal search would be done. > Actually having a simple demo app in examples/client could be useful. Can we postpone this? I'd like to get contact search in Empathy, and the UI freeze is in ~ 1 week, and it's blocking on this. > You add your new files to telepathy-glib/introspection.am so they'll be added > to gir. Done (and checked the .gir file). > (In reply to comment #27) > > > g_cancellable_cancel (self->priv->cancellable); > > > > > > while (self->priv->async_res != NULL) > > > g_main_context_iteration (NULL, TRUE); > > > This is a bit weird; maybe we should request a g_cancellable_cancel_async() in > > > glib? > > > Do we really need to wait btw ? > > > > As it is now, yes, since we use the same GCancellable, so we need to wait and > > reset it. Maybe we could have a list of GCancellables and just cancel it > > without waiting, or maybe we can cancel it and unref it (if gio cancells it > > before disposing the GCancellable), so we don't need to keep a list. The > > current code works though. > > The cancel, unref and create a new one option sounds saner to me. Can you > check the GCancellable implementation and/or test to see if that would work? I've just done it, and it works. AFAIK when you _unref an object and the ref count gets to zero, the object is disposed directly (i.e. synchronously), so this is probably the same we were doing, but nicer / less hacky. > > > tp_contact_search_start(): shouldn't we check if another search is already > > > active? > > > > We do: > > > > g_return_if_fail (self->priv->state == > > TP_CHANNEL_CONTACT_SEARCH_STATE_NOT_STARTED); > > You're right sorry. > > > > + if (with_limit) > > > if (with_limit != NULL) > > > > > > + if (!tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH)) > > > use "continue" to limit nesting. > > > > I cannot use "continue" there since I have to check both with_limit and > > with_server, and if I continue in the first one, I won't get to check the > > second one. I could do > > > > if (with_limit != NULL && !tp_strdiff (...)) > > > > to avoid one level. Not sure if it's worth it... > > I meant doing: > if (tp_strdiff (chan_type, TP_IFACE_CHANNEL_TYPE_CONTACT_SEARCH)) > continue; > > Wouldn't that work? Oh, right. Done, thanks. > > > Having test for TpContactSearch would make me an happier badger... > > > > I tried to look at this, but apparently I need to create some classes for the > > DBus tests and I couldn't really understand how that stuff works. > > Yeah you have to write a server side channel object to test your API. > See for example tests/lib/stream-tube-chan.c tested in tests/dbus/stream-tube.c Same as for the example... can we do this later? I don't really have the time right now to get into this, and I'm sure this will take me quite some time as I'm not familiar with this dbus test stuff.
+ * @self: a contact search object We usually use "A #TpContactSearch" (In reply to comment #29) > > I think the long description in SECTION:contact-search should describe the > > whole search process (which function use to start a search, how to get the > > results and then how to start a new search). > > I've added a paragraph explaining how a normal search would be done. Nice,for the sack of completeness, you may add that the signal can be fired more than once and how we can detect that the search is done. > > Actually having a simple demo app in examples/client could be useful. > > Can we postpone this? I'd like to get contact search in Empathy, and the UI > freeze is in ~ 1 week, and it's blocking on this. Sure, just opens a bug so we don't forget. > > > > Having test for TpContactSearch would make me an happier badger... > > > > > > I tried to look at this, but apparently I need to create some classes for the > > > DBus tests and I couldn't really understand how that stuff works. > > > > Yeah you have to write a server side channel object to test your API. > > See for example tests/lib/stream-tube-chan.c tested in tests/dbus/stream-tube.c > > Same as for the example... can we do this later? I don't really have the time > right now to get into this, and I'm sure this will take me quite some time as > I'm not familiar with this dbus test stuff. I'm not really happy adding completely untested new API, but I understand than we don't have much time here. If that's fine with the maintainer (smcv), then ok. I'd also like to get the API reviewed by smcv before merging as he's the tp-glib API master.
(In reply to comment #30) > + * @self: a contact search object > We usually use "A #TpContactSearch" Hmm, in comment 20, Simon said I shouldn't do "a #gboolean" for a gboolean param, because it's obvious. This looks similar to me. > (In reply to comment #29) > > > I think the long description in SECTION:contact-search should describe the > > > whole search process (which function use to start a search, how to get the > > > results and then how to start a new search). > > > > I've added a paragraph explaining how a normal search would be done. > > Nice,for the sack of completeness, you may add that the signal can be fired > more than once and how we can detect that the search is done. Done. > > > Actually having a simple demo app in examples/client could be useful. > > > > Can we postpone this? I'd like to get contact search in Empathy, and the UI > > freeze is in ~ 1 week, and it's blocking on this. > > Sure, just opens a bug so we don't forget. Will do when this is merged. > > > > > Having test for TpContactSearch would make me an happier badger... > > > > > > > > I tried to look at this, but apparently I need to create some classes for the > > > > DBus tests and I couldn't really understand how that stuff works. > > > > > > Yeah you have to write a server side channel object to test your API. > > > See for example tests/lib/stream-tube-chan.c tested in tests/dbus/stream-tube.c > > > > Same as for the example... can we do this later? I don't really have the time > > right now to get into this, and I'm sure this will take me quite some time as > > I'm not familiar with this dbus test stuff. > > I'm not really happy adding completely untested new API, but I understand than > we don't have much time here. If that's fine with the maintainer (smcv), then > ok. I guess by untested you mean "without unit tests", since it's tested (I'm using it for the Empathy contact search dialog, and it works fine). > I'd also like to get the API reviewed by smcv before merging as he's the > tp-glib API master. Sure. Thanks for all your comments!
Created attachment 42202 [details] [review] diff against master
Review of attachment 42202 [details] [review]: I had a skim over the API. It looks nice! There are some places where more documentation might be nice—specifically, TpContactSearchResult is very sparsely documented—but this isn't a merge blocker. I found a few nits, which might be nice to fix before merging, but again they're not crucial. Code-wise, I trust the judgement of others’ past review. I did notice a bug in _result_dispose(). I didn't really look all that closely at the code in general, since it's already been reviewed. ::: telepathy-glib/capabilities.c @@ +481,3 @@ + * tp_capabilities_supports_contact_search: + * @self: a #TpCapabilities object + * @with_limit: (out): if not %NULL, used to return %TRUE if the @limit I think gtk-doc will complain that this function has no @limit argument. ::: telepathy-glib/contact-search-result.c @@ +38,3 @@ + * + * #TpContactSearchResult objects represent results for + * #TpContactSearch. it representS results. @@ +85,3 @@ + gchar *name = (gchar *) n; + + return g_strcmp0 (field->field_name, name); Do you really need to cast 'n'? If so, you should cast it to (const gchar *). @@ +136,3 @@ + g_free (self->priv->identifier); + g_list_free_full (self->priv->fields, + (GDestroyNotify) tp_contact_info_field_free); This function will break if it's called more than once, as it technically can be. You should NULL out both its fields after freeing them so that it won't break. ::: telepathy-glib/contact-search.c @@ +407,3 @@ + * TpContactSearch:server: + * + * The search server. This is only supported by some protocols, this comma should be a semi-colon. @@ +428,3 @@ + * + * The maximum number of results that the server should return. + * This is only supported by some protocols, use ditto. @@ +471,3 @@ + * + * Emitted when search results are received. Note that this signal may + * be called multiple times for the same search. emitted, not called. @@ +679,3 @@ + * The keys are vCard field names in lower case, except when + * they're one of the special cases from telepathy-spec like + * "tell;cell" or "x-n-given". See the Channel.Type.ContactSearch I think this should be "tel;cell". It's been a while since I looked at this interface. You can include hyperlinks in gtk-doc. Why not make this reference a clickable link to http://telepathy.freedesktop.org/spec/Channel_Type_Contact_Search.html ? ::: tests/dbus/contact-search.c @@ +1,1 @@ +/* Tests of TpContactSearch and TpContactSearchResult tests for, not of.
(In reply to comment #33) > ::: telepathy-glib/capabilities.c > @@ +481,3 @@ > + * tp_capabilities_supports_contact_search: > + * @self: a #TpCapabilities object > + * @with_limit: (out): if not %NULL, used to return %TRUE if the @limit > > I think gtk-doc will complain that this function has no @limit argument. It doesn't, otherwise I'd have removed it :-) I've removed that and @server now. > ::: telepathy-glib/contact-search-result.c > @@ +38,3 @@ > + * > + * #TpContactSearchResult objects represent results for > + * #TpContactSearch. > > it representS results. I say "objects", so it's "objects represent", no? I could say "TpContactSearchResult represents..." if you prefer that. > > @@ +85,3 @@ > + gchar *name = (gchar *) n; > + > + return g_strcmp0 (field->field_name, name); > > Do you really need to cast 'n'? If so, you should cast it to (const gchar *). Indeed we don't need it, removed. > > @@ +136,3 @@ > + g_free (self->priv->identifier); > + g_list_free_full (self->priv->fields, > + (GDestroyNotify) tp_contact_info_field_free); > > This function will break if it's called more than once, as it technically can > be. You should NULL out both its fields after freeing them so that it won't > break. Fixed. > > ::: telepathy-glib/contact-search.c > @@ +407,3 @@ > + * TpContactSearch:server: > + * > + * The search server. This is only supported by some protocols, > > this comma should be a semi-colon. Done. > > @@ +428,3 @@ > + * > + * The maximum number of results that the server should return. > + * This is only supported by some protocols, use > > ditto. Done too. > > @@ +471,3 @@ > + * > + * Emitted when search results are received. Note that this signal may > + * be called multiple times for the same search. > > emitted, not called. Fixed. > > @@ +679,3 @@ > + * The keys are vCard field names in lower case, except when > + * they're one of the special cases from telepathy-spec like > + * "tell;cell" or "x-n-given". See the Channel.Type.ContactSearch > > I think this should be "tel;cell". It's been a while since I looked at this > interface. You're right, fixed. > You can include hyperlinks in gtk-doc. Why not make this reference a clickable > link to http://telepathy.freedesktop.org/spec/Channel_Type_Contact_Search.html > ? Ah cool, done. > ::: tests/dbus/contact-search.c > @@ +1,1 @@ > +/* Tests of TpContactSearch and TpContactSearchResult > > tests for, not of. Fixed. Any other stuff, or can we merge this? :-)
(In reply to comment #34) > > ::: telepathy-glib/contact-search-result.c > > @@ +38,3 @@ > > + * > > + * #TpContactSearchResult objects represent results for > > + * #TpContactSearch. > > > > it representS results. > > I say "objects", so it's "objects represent", no? I could say > "TpContactSearchResult represents..." if you prefer that. Erm. Yeah. It's correct as it is. I can't read, obviously. :) + if (self->priv->fields != NULL) + { + g_list_free_full (self->priv->fields, + (GDestroyNotify) tp_contact_info_field_free); + self->priv->fields = NULL; + } You don't need the NULL check. NULL is the empty GList. > Any other stuff, or can we merge this? :-) Fix the above, file a bug for improving the documentation, and then gogogo.
(In reply to comment #35) > Fix the above, file a bug for improving the documentation, and then gogogo. Yay! merged. I've opened bug #33539 for the documentation and bug #33540 for the tests.
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.