Implement a reader for http://telepathy.freedesktop.org/wiki/service-profile-v1

Submitted by Vivek Dasmohapatra

Assigned to Telepathy bugs list

Link to original bug (#32179)

Description

Provide API to give a user access to the new-style service profiles.

Version: git master

added 5. Tp-Glib bugzilla labels

Will Thompson @wjt said:

The new stuff you've added to Makefile.am in http://git.collabora.co.uk/?p=user/vivek/telepathy-glib;a=commitdiff;h=3b8ef822779a2ede7c602890f9cbe2529531931b#patch1 is very visibly misaligned, due to using tabs where the rest of the file uses spaces. Also I don't see why HANDLE_LEAK_DEBUG_CFLAGS was added: maybe this was mistakenly re-added during a rebase? Handles are always "leaked" these days, so HANDLE_LEAK_DEBUG_CFLAGS is obsolete.

Oh hey, it's removed in the following patch. Could you rebase this spurious change out of existence? I find skim-reading the patch series in gitweb before submitting it for review is a good way to catch these kinds of things ahead of time.

• • Since: WHEN

Use "0.13.UNRELEASED". The release scripts grep for "UNRELEASED" and make distcheck fail if it's found.

• GHashTable params; / TpConnectionManagerParam */

Is that the key or the value?

• /* holding state during parsing */
• ParseState state;
• gboolean interested;
• struct {
• GString *content;
• TpConnectionManagerParam *param;
• Presence *presence;
• xmlParserCtxtPtr context;
• } current;

Can't the parser state be a separate struct that has a reference to the TpServiceProfile? It might make the separation between temporary state and the actual profile object clearer.

If you --enable-gtk-doc, make check fails because there's no entry in telepathy-glib.sections for TpServiceProfile and its symbols.

There's a tonne of unnecessary and irregular linebreaks between functions in service-profile.h. Some are separated by one line, others by two, and for all the trivial get-me-a-string accessors I think it'd be better with no space in between, to be honest.

Their names should contain is or get. eg. tp_service_profile_valid() and tp_service_profile_get_id(). Really they should all be GObject properties too.

guint tp_service_profile_get_n_forbidden_channel_classes (
    TpServiceProfile *profile);

guint tp_service_profile_get_n_forbidden_channel_class_props (
    TpServiceProfile *profile,
    guint index);

gboolean tp_service_profile_get_forbidden_channel_class_property (
    TpServiceProfile *profile,
    guint fcc,
    guint prop,
    const gchar const **name,
    const gchar const **signature,
    const GValue const **value);

I think I would represent channel classes as GHashTables as used by tp_asv_new() and friends. Then you could just have GList *tp_service_profile_get_forbidden_channel_classes (TpServiceProfile *);. The internal list-of-FCCProperty representation could also be replaced with this.

• @id: the ID of the [resence to fetch

This should be a p ^

GList *tp_service_profile_list_presence_ids (TpServiceProfile *self);

GList of gchar * is unconventional; a const gchar * const * would be more consistent with the rest of Telepathy. And I do wonder whether a list/array of some TpServicePresenceSpec struct would be better, or maybe just returning a ref of the internal hash?

if (value == NULL || g_str_equal (value, "0") || g_str_equal (value, "false")) { *flag = FALSE; return TRUE; }

if (g_str_equal (value, "1") || g_str_equal (value, "true")) return *flag = TRUE;

The latter is clever but not readable. It should be symmetrical with the first branch.

    if (1) {

      FCCProperty *p = g_new0 (FCCProperty, 1);

    ((GList *) data->forbidden_channel_classes)->data =
      g_list_prepend (((GList *) data->forbidden_channel_classes)->data, p);

    fprintf (stderr, "allocated fcc property %p\n", p);
    }

I don't think you meant to leave this code in in this form? It's unreadable and has if (1) and fprintf and ... The unreadable bit would go away if FCCs were represented as a{sv}s, I think.

There are a load of

DEBUG ();

which should either be removed or have useful information added to the message. This should also be removed from _get_property and _set_property:

DEBUG ("%u", prop_id);

_end_element_ns() would be easier to read if it were not 160 lines long, and were better split up into sub-functions and a switch on the state.

This isn't a full review, I'll do some more on the actual code later.