[Bug 32053] Add a TpContactSearch proxy object
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Wed Dec 15 17:27:17 CET 2010
https://bugs.freedesktop.org/show_bug.cgi?id=32053
Simon McVittie <simon.mcvittie at collabora.co.uk> changed:
What |Removed |Added
----------------------------------------------------------------------------
URL| |http://git.collabora.co.uk/
| |?p=user/pochu/telepathy-gli
| |b.git;a=shortlog;h=refs/hea
| |ds/contact-search-32053
Status|NEW |ASSIGNED
Status Whiteboard| |review-
AssignedTo|telepathy-bugs at lists.freede |pochu27 at gmail.com
|sktop.org |
--- Comment #20 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-12-15 08:27:17 PST ---
> +<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.
--
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