Bug 19683

Summary:	Refactor Capabilities/ContactCapabilities to make sense
Product:	Telepathy	Reporter:	Guillaume Desmottes <guillaume.desmottes>
Component:	gabble	Assignee:	Telepathy bugs list <telepathy-bugs>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium
Version:	unspecified
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/caps-sanity
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	22934, 23492

Description Guillaume Desmottes 2009-01-22 04:03:41 UTC

Lot of components access, modify, create and destroy the GabblePresence->per_channel_manager_caps hash table which is not very OO friendly.

We should refactor code to add accessors and do that properly.

Comment 1 Simon McVittie 2009-08-24 13:29:20 UTC

Will and I have refactored Capabilities extensively. Review would be appreciated.

Comment 2 Alban Crequy 2009-09-04 11:30:13 UTC

(In reply to comment #1)
> Will and I have refactored Capabilities extensively. Review would be
> appreciated.

I have only a few comments after such a big branch :-)


src/ft-channel.c
+      /* FIXME: should we check for SI, bytestreams and/or IBB too? */
--> Reply to the question or open a bug for that.

src/capabilities.c
gsize feature_handles_refcount = 0;
--> What isn't it static?

src/presence.c
  if (priv->cap_set == NULL)
    { 
      tmp = gabble_capability_set_dump (priv->cap_set, "  ");
--> do you mean '!=' instead of '==' ?  (2 occurences of this pattern in this file)

Comment 3 Simon McVittie 2009-09-07 08:08:03 UTC

(In reply to comment #2)
> src/ft-channel.c
> +      /* FIXME: should we check for SI, bytestreams and/or IBB too? */
> --> Reply to the question or open a bug for that.

I'll open a bug, unless Guillaume can insta-answer it...

> src/capabilities.c
> gsize feature_handles_refcount = 0;
> --> What isn't it static?

A very good question. It should be.

> src/presence.c
>   if (priv->cap_set == NULL)
>     { 
>       tmp = gabble_capability_set_dump (priv->cap_set, "  ");
> --> do you mean '!=' instead of '==' ?  (2 occurences of this pattern in this
> file)

Yes, you're right.

Comment 4 Simon McVittie 2009-09-07 08:24:58 UTC

(In reply to comment #3)
> (In reply to comment #2)
> > src/ft-channel.c
> > +      /* FIXME: should we check for SI, bytestreams and/or IBB too? */
> > --> Reply to the question or open a bug for that.
> 
> I'll open a bug, unless Guillaume can insta-answer it...

Bug #23777.

> > src/capabilities.c
> > gsize feature_handles_refcount = 0;
> > --> What isn't it static?
> A very good question. It should be.

Fixed in the branch; make check is re-running now.

(en_FR check: "Why" should have been "What" in your review comment :-)

> > src/presence.c
> [...]
> Yes, you're right.

Fixed in the branch; make check is re-running now.

Comment 5 Alban Crequy 2009-09-07 10:52:00 UTC

The branch is ok for me

Comment 6 Simon McVittie 2009-09-08 05:30:02 UTC

Merged to git master, will be in 0.9.0.

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.