Bug 29079

Implemented gabble support (branch in bug url), targeting telepathy-glib 0.11.11, based on code in haze & tp glib examples.

alloc_params, free_params, GabbleParams, and the JABBER_PARAM enum can just go away, afaics.

There seems to be a lot of pre-Protocol cruft left in connection-manager.c, notably gabble_connection_manager_get_protocols. I'd prefer this to migrate to protocol.c (yes, I know this leaves connection-manager.c more or less empty).

write-mgr-file.c doesn't write a complete .manager file. This will break Protocol support in clients, because TpConnectionManager and TpProtocol prefer to read the .manager file, so they will never pick up Gabble's vCard field (for instance).

To fix this, in write-mgr-file.c, mgr_file_contents() should take an array (or GList) of TpBaseProtocol, and main() should use gabble_jabber_protocol_new() and pass that in. This would enable the .manager file to include all the new information: English name, vCard field, etc. (see the spec for details of how they're encoded).

It'd also avoid using TpCMProtocolSpec.

> +  MAP("server", "connect-server"),
> +  SAME("resource"),

Whitespace nitpicking: "MAP (", "SAME (" (the coding style checker doesn't enforce this for all-caps names, because you have to use MAP( when *defining* the macro)

> +      GValue *val = g_hash_table_lookup (params, params2props[i].tp_param);
> +      g_debug ("Param: %s, prop: %s, Val: %p",
> +        params2props[i].tp_param, params2props[i].conn_prop, val);

DEBUG () or nothing, please.

> +  return gabble_normalize_contact (NULL, contact,
> +    GUINT_TO_POINTER (GABBLE_JID_GLOBAL), error);

gabble_normalize_contact should gain a comment saying that @repo == NULL is explicitly allowed.

> +  g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
> +      "'account' parameter not given");

You can g_assert_not_reached () here, since 'account' is mandatory and TpBaseProtocol enforces completeness. Please also test it (see below).

> +      *icon_name = g_strdup_printf ("im-%s", tp_base_protocol_get_name (self));
> +      *vcard_field = g_strdup_printf ("x-%s", tp_base_protocol_get_name (self));

I think I'd prefer these as hard-coded strings, for clarity. Haze has to use a dynamic string, but Gabble doesn't.

> +  protocol = g_object_new (GABBLE_TYPE_JABBER_PROTOCOL,
> +      "name", "jabber",
> +      NULL);

I'd like this encapsulated as TpBaseProtocol *gabble_jabber_protocol_new (void).

> +    assert len(protocols) == 1 and 'jabber' in protocols

assertEquals(set(['jabber']), set(protocols.keys()))?

> +    assert protocol_names == ['jabber']

assertEquals(['jabber'], protocol_names), and the same for all other "==" assertions - note that it's assertEquals(expected, actual).

> +    contact = 'foo@example.com/Telepathy'
> +    normalized = unwrap(proto_iface.NormalizeContact(contact))
> +    assert contact == (normalized + '/Telepathy')

I'd prefer a normalization that touches the contact ID too:

assertEquals('foo@mit.edu', proto_iface.NormalizeContact('foo@MIT.EDU/Telepathy'))

> +    test_params = { 'account': 'test@localhost' }
> +    acc_name = unwrap(proto_iface.IdentifyAccount(test_params))

(This only works because 'password' is no longer mandatory.)

Please also check that IdentifyAccount({}) raises an appropriate error.

Thanks for the review, I've updated the branch accordingly.

(In reply to comment #2)
> alloc_params, free_params, GabbleParams, and the JABBER_PARAM enum can just go
> away, afaics.

Right.

> There seems to be a lot of pre-Protocol cruft left in connection-manager.c,
> notably gabble_connection_manager_get_protocols. I'd prefer this to migrate to
> protocol.c (yes, I know this leaves connection-manager.c more or less empty).

Right, I did that, it's mostly just moving the parameters array and removing leftover cruft.

> write-mgr-file.c doesn't write a complete .manager file. This will break
> Protocol support in clients, because TpConnectionManager and TpProtocol prefer
> to read the .manager file, so they will never pick up Gabble's vCard field (for
> instance).
> 
> To fix this, in write-mgr-file.c, mgr_file_contents() should take an array (or
> GList) of TpBaseProtocol, and main() should use gabble_jabber_protocol_new()
> and pass that in. This would enable the .manager file to include all the new
> information: English name, vCard field, etc. (see the spec for details of how
> they're encoded).

Did so.

While writing RCC properties, I manually handled serializing GType/DBus type mappings, as I couldn't find utilities to do so in dbus-glib or tp-glib. The mapping definitely covers gabble's needs (and probably the other CMs'), but isn't complete, I basically just assumed which types are most probably needed.

Also, in tp-glib example .manager file, a section name for the class is shown, but there's no set algorithm how to generate the section names. So I generate one based on channel type and target handle type (since those two parameters are always required), and a serial number to avoid possible name clashes. The idea is to have the section name be at least somewhat human-readable...

> > +  MAP("server", "connect-server"),
> > +  SAME("resource"),
> 
> Whitespace nitpicking: "MAP (", "SAME (" (the coding style checker doesn't
> enforce this for all-caps names, because you have to use MAP( when *defining*
> the macro)

Thanks, fixed.

> > +      GValue *val = g_hash_table_lookup (params, params2props[i].tp_param);
> > +      g_debug ("Param: %s, prop: %s, Val: %p",
> > +        params2props[i].tp_param, params2props[i].conn_prop, val);
> 
> DEBUG () or nothing, please.

Leftover from when I was getting the code to work properly, removed.

> 
> > +  return gabble_normalize_contact (NULL, contact,
> > +    GUINT_TO_POINTER (GABBLE_JID_GLOBAL), error);
> 
> gabble_normalize_contact should gain a comment saying that @repo == NULL is
> explicitly allowed.

Added comment for gabble_normalize_contact which says so.

> You can g_assert_not_reached () here, since 'account' is mandatory and
> TpBaseProtocol enforces completeness. Please also test it (see below).

Changed accordingly.

> 
> > +      *icon_name = g_strdup_printf ("im-%s", tp_base_protocol_get_name (self));
> > +      *vcard_field = g_strdup_printf ("x-%s", tp_base_protocol_get_name (self));
> 
> I think I'd prefer these as hard-coded strings, for clarity. Haze has to use a
> dynamic string, but Gabble doesn't.

Makes sense, did so.

> > +  protocol = g_object_new (GABBLE_TYPE_JABBER_PROTOCOL,
> > +      "name", "jabber",
> > +      NULL);
> 
> I'd like this encapsulated as TpBaseProtocol *gabble_jabber_protocol_new
> (void).

Added the method.


> 
> > +    assert len(protocols) == 1 and 'jabber' in protocols
> 
> assertEquals(set(['jabber']), set(protocols.keys()))?

Fixed asserts throughout the test.


> I'd prefer a normalization that touches the contact ID too:
> 
> assertEquals('foo@mit.edu',
> proto_iface.NormalizeContact('foo@MIT.EDU/Telepathy'))

Thanks, using that exact example.

> 
> > +    test_params = { 'account': 'test@localhost' }
> > +    acc_name = unwrap(proto_iface.IdentifyAccount(test_params))
> 
> (This only works because 'password' is no longer mandatory.)
> 
> Please also check that IdentifyAccount({}) raises an appropriate error.

Noted in a comment that the test assumes only "account" is mandatory, and also added the check that correct error is returned if the mandatory argument is missing.

Also, since Protocol has been undrafted in current tp-glib master (which this branch of gabble requires), updated the constant used in the test to reflect so.

In the test:
> +    # (Only) 'account' is mandatory for IdentifyAccount()
> +    try:
> +        proto_iface.IdentifyAccount({})
> +    except dbus.DBusException, e:
> +        assertEquals(cs.INVALID_ARGUMENT, e.get_dbus_name())
> +    else:
> +        raise AssertionError("IdentifyAccount({}) should've returned error but didn't")

call_async(q, proto_iface, 'IdentifyAccount', {})
q.expect('dbus-error', method='IdentifyAccount', name=cs.INVALID_ARGUMENT)

> +  protocol = (TpBaseProtocol *) gabble_jabber_protocol_new ();

You could make g_j_p_n return a TpBaseProtocol* to avoid the cast?

In Protocol:
> +  static GOnce init = G_ONCE_INIT;
> +
> +  g_once (&init, _init_parameters, NULL);

I find g_once_init_enter() and g_once_init_leave() typically lead to more obvious code. They're available since GLib 2.14.

In write-mgr-file:
> +    if (val && *val) \

if (tp_str_empty (val)), since telepathy-glib 0.11.1

(Previously, the coding style would have been: if (val != NULL && *val != '\0'))

I think it would be OK for write-mgr-file to assert if any of the immutable properties aren't there. After all, we know that they're there :-)

> +      g_hash_table_foreach (ctx.fixed, write_rcc_property, &ctx);

Why not use GHashTableIter? It usually produces much more comprehensible code, in my experience.

> +          gchar *kf_val = g_strdup_printf ("%llu", g_value_get_uint64 (val));

The format string should be: "%" G_GUINT64_FORMAT

Same for G_TYPE_INT64.

Also add a FIXME comment: when we depend on GLib 2.26, we can use g_key_file_set_[u]int64, which I added in Gnome bug 614864.

Also please depend on telepathy-glib 0.11.11 in configure.ac, now that it's been released with TpBaseProtocol support :-)

Thx for the review, updated my branch based on your feedback.

Protocols! Yay! Ship it.

(In reply to comment #7)
> Protocols! Yay! Ship it.

Thx, merged to git master.