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

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=32179

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review-

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2010-12-14 09:40:56 PST ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.