Bug 30461 - Review Conn.I.Resources
Summary: Review Conn.I.Resources
Status: ASSIGNED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Jonny Lamb
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/jo...
Whiteboard: review-, more test coverage please
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-09-29 05:46 UTC by Jonny Lamb
Modified: 2010-11-17 01:36 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonny Lamb 2010-09-29 05:46:37 UTC
Simon already made a few comments:

Regarding Gabble:

Regression tests are good, and should have caught this:

> +  if (name == g_quark_from_static_string ("MailNotificationFlags"))
> +    g_value_set_uint (value, GABBLE_RESOURCES_HUMAN_READABILITY_MAYBE);
                                              ^^^^^^^^^^^^^^^^^^^^^

> +static const gchar *
> +presence_id_to_status (GabblePresenceId id,
> +    TpConnectionPresenceType *type)

Surely this *must* be duplicating something in conn-presence.c? (It's entirely
possible that the information exists in conn-presence.c but needs some
refactoring, though.)

(Be particularly careful about Senko's new plugin-defined presences, which can
be visible on the self-handle (if the self-resource is exposed) and didn't
exist when you wrote this branch...)

> +      val = tp_g_value_slice_new_boxed (
> +          GABBLE_HASH_TYPE_RESOURCE_INFORMATION_MAP, resources);
[...]
> +      g_hash_table_unref (resources);

Just take_boxed to avoid a copy?
Comment 1 Jonny Lamb 2010-10-14 01:45:08 UTC
Okay, I rebased my branch, updated it, added client type support, and fixed your comments. CHECK IT OUT in the same place as before.
Comment 2 Simon McVittie 2010-10-14 07:44:57 UTC
Regression tests are still good, and would have probably caught this:

> +          value = tp_g_value_slice_new_boxed (
> +              dbus_g_type_get_collection ("GPtrArray", G_TYPE_STRING), array);

(a dbus-glib 'as' is a GStrv, not a GPtrArray<string>).
Comment 3 Jonny Lamb 2010-10-25 08:56:04 UTC
(In reply to comment #2)
> Regression tests are still good

Okay, added one.

> (a dbus-glib 'as' is a GStrv, not a GPtrArray<string>).

16:40 < jonnylamb> smcv: so 'as' in dbus-glib is a gchar**, not a 
                   GPtrArray<gchar*>? hm, so how does it work currently?!
16:42 < smcv> jonnylamb: it = ?
16:43 < smcv> jonnylamb: the GType for 'as' in places where there's no better 
              context is a GStrv
16:43 < smcv> jonnylamb: dbus-glib might support GPtrArray<string> as an 
              alternative (de)serialization if you explicitly ask for it
16:43 < jonnylamb> smcv: client types in gabble master and my resources 
                   branch. both do the same thing which is give a 
                   GPtrArray<gchar*> when an 'as' is expected.
16:43 < jonnylamb> smcv: *and conn.i.resources in my resource branch
16:45 < smcv> jonnylamb: if there is somewhere where you can explicitly say 
              dbus_g_type_get_collection ("GPtrArray", G_TYPE_STRING), and you 
              do, then dbus-glib might well support that
16:45 < jonnylamb> ah yes that's what I'm doing

So, like, it works fine as a GPtrArray.
Comment 4 Simon McVittie 2010-11-01 06:15:33 UTC
(In reply to comment #0)
> (Be particularly careful about Senko's new plugin-defined presences, which can
> be visible on the self-handle (if the self-resource is exposed) and didn't
> exist when you wrote this branch...)

Have you tested this with a resource having a plugin-defined presence? Does it work?

(presence/plugins.py illustrates how to test with those.)

Other than that, this looks good.
Comment 5 Jonny Lamb 2010-11-17 01:36:55 UTC
(In reply to comment #4)
> Have you tested this with a resource having a plugin-defined presence? Does it
> work?
> 
> (presence/plugins.py illustrates how to test with those.)

I don't understand -- aren't plugin-defined presences for self presence? This resources interface is for remote contacts. Plugin-defined presences don't provide more presences that remote contacts send in their presence stanza.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.