[Bug 25243] [0.8, 0.9] don't advertise FT ContactCapability unless we have a FT client

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=25243





--- Comment #3 from David Laban <david.laban at collabora.co.uk>  2009-11-24 08:37:18 PST ---
I'm not on the approved reviewers list, and I've not really dug very deep into
the underlying code, so this is only a pre-review.

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=79c625e8ac7108a9c429c1cc6a6845d383e847df

General comments first: modifying the test and the code in the same commit: Is
that how you normally do it? I've not looked very hard at the test, but if you 
can cherry-pick it and get it to fail in the way you expect, that should be
enough, right?

Lots of FIXMEs in the C code meant that I was trying to squash the 3 commits in
my head, so I might have missed something. Is there a simple way to squash
commits together from the web interface?



http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=88f1c1dbd0b589c060576c494301be0dd61f95ce
Looks good.


http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=8468fa6df2080fc83bd85c1af9ed9e27b7bd5284

-        return GINT_TO_POINTER (TRUE);
+        return manager;   /* any non-NULL pointer would do */

Is this for consistency? If so, you should probably do something about:
 775   g_hash_table_insert (presence->per_channel_manager_caps,
 776       manager, GINT_TO_POINTER (TRUE));
To be honest, I really don't like that change. I agree with returning NULL if
you're going to check for NULL, but just picking an arbitrary pointer seems a
bit ugly. Even NULL + 1 seems less ugly to me, because it's at least guaranteed
to be the same across different runs, and will error immediately if you try to
free it. It seems a little odd to have gabble_private_tubes_factory_parse_caps
return a pointer that needs to be freed, and gabble_ft_manager_parse_caps
return a pointer that's secretly a bool.

Also:
 712       if (NULL == var)
I remember you picking me up on something like this. It's that way around for
== to stop anyone putting = in by accident, and the other way around for !=
because that looks less funky, right?

-  /* FIXME: can't be done! We'd need to turn our channel-specific caps into a
-   * real pointer. However, the only call to update_capabilities happens to
-   * work, because GPOINTER_TO_INT (FALSE) happens to be NULL. */
+  /* We don't need to do anything. If @out is NULL, we won't be called, and
+   * @in will be copied instead; if @out is non-NULL, it means FT is
supported,
+   * so it doesn't matter what @in was. */

Any chance of an assert? I wouldn't want someone else to come along and turn
you into a liar.


http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/09-conditional-ft
Unsurprisingly, this is a lot cleaner. Probably should have read this one first
or something.

One thing that caught my eye was "GValue monster = {0, };". Is this a reference
to "monster in a box" or what?


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