[Bug 29079] use TpBaseProtocol in Gabble

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-07-21 04:26:39 PDT ---
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 at 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 at mit.edu',
proto_iface.NormalizeContact('foo at MIT.EDU/Telepathy'))

> +    test_params = { 'account': 'test at 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.

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