Bug 21701

Summary:	Tidy up caps parsing
Product:	Telepathy	Reporter:	Will Thompson <will>
Component:	gabble	Assignee:	Telepathy bugs list <telepathy-bugs>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	Keywords:	patch
Version:	unspecified
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/wjt/telepathy-gabble-wjt.git;a=shortlog;h=refs/heads/tidy-caps
Whiteboard:
i915 platform:		i915 features:

Description Will Thompson 2009-05-12 07:37:28 UTC

This branch refactors some of Gabble's caps handling to be more obviously correct.

 src/capabilities.c   |   43 +++++
 src/capabilities.h   |    2 +
 src/presence-cache.c |  428 ++++++++++++++++++++++++--------------------------
 3 files changed, 254 insertions(+), 219 deletions(-)

Comment 1 Guillaume Desmottes 2009-05-13 05:34:19 UTC

capabilities_parse: I guess you just copy/pasted the old function but maybe we could change it to use tp_strdiff now.

set_caps_for: early return would be function clearer

I'm not sure about b5b629fdc8b79a33927dfd9c5de8d08e826df0e2
I guess that should be discussed.

08f75b635e3d328ecf148ca3286a56a4800972d2
The logic of the new code SEEMS ok to me but I can't assert for sure that's exactly the same as the old code.

While looking at _caps_disco_cb I noticed that few lines are too long.

Comment 2 Will Thompson 2009-05-13 06:11:46 UTC

(In reply to comment #1)
> capabilities_parse: I guess you just copy/pasted the old function but maybe we
> could change it to use tp_strdiff now.

      if (0 != strcmp (child->name, "feature"))

If Loudmouth gives us an LmMessageNode with a null name, we may already have lost, surely?

          if (0 == strcmp (var, i->ns))

Both var and i->ns are known to be non-NULL.

> set_caps_for: early return would be function clearer

I'm not sure I agree that it makes it clearer but: 7d48d7e366c33b24a.

> I'm not sure about b5b629fdc8b79a33927dfd9c5de8d08e826df0e2
> I guess that should be discussed.

I agree. I added a FIXME as a reminder; this branch was meant to just be refactoring and minor bugfixing, not changing behaviour.

> 08f75b635e3d328ecf148ca3286a56a4800972d2
> The logic of the new code SEEMS ok to me but I can't assert for sure that's
> exactly the same as the old code.

All the old tests pass. :-)

> While looking at _caps_disco_cb I noticed that few lines are too long.

Fixed in 053ed1ad246a; I added a similar function in 89c7ecfe9ea8.

Comment 3 Guillaume Desmottes 2009-05-19 08:40:39 UTC

+1 from me

Comment 4 Will Thompson 2009-05-19 10:04:57 UTC

Merged, will be in Gabble 0.7.28.

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.