Bug 30461

Summary:	Review Conn.I.Resources
Product:	Telepathy	Reporter:	Jonny Lamb <jonny.lamb>
Component:	gabble	Assignee:	Jonny Lamb <jonny.lamb>
Status:	RESOLVED MOVED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	git master
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/jonny/telepathy-gabble.git;a=shortlog;h=refs/heads/resources
Whiteboard:	review-, more test coverage please
i915 platform:		i915 features:

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.

Comment 6 GitLab Migration User 2019-12-03 19:49:03 UTC

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-gabble/issues/101.

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.