[Bug 29079] use TpBaseProtocol in Gabble

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Senko Rasic <senko at senko.net> 2010-07-22 04:43:38 PDT ---
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 at mit.edu',
> proto_iface.NormalizeContact('foo at MIT.EDU/Telepathy'))

Thanks, using that exact example.

> 
> > +    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.

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.

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