Bug 25243

The fix for 0.8, plus a bit of cleanup:

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/08-conditional-ft

The corresponding change for 0.9:

http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/09-conditional-ft

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?

*** Bug 23578 has been marked as a duplicate of this bug. ***

In Bug #23578, wjt wrote:
> In the 0.8 series, we should probably add a flag on the Capabilities interface,
> so that MC in its old-school caps fallback mode will set it given a handler for
> FT channels. In the 0.9 series, we should fix it properly.

There's no need to involve the Capabilities interface, I don't think - MC 5.2 and 5.3 both implement both ContactCapabilities draft 1 and ContactCapabilities final, so Gabble can just look at the ContactCapabilities API.

This does mean we shouldn't remove draft-1 support from MC 5.3 until Gabble 0.9 is the generally recommended version for distributions, but we already knew that.

(In reply to comment #3)
> 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?

It was necessary to change the tests in this instance, because some of them
wrongly asserted that FileTransfer was in the "basic capabilities" set.

We normally try to make sure that each commit compiles and passes tests,
since that makes `git bisect` much more useful, and is generally considered
good practice anyway.

How to commit tests varies by personal preference. I usually write the
regression test for a new feature, watch it fail, implement the feature to
make it pass, then commit the feature *before* the test (to keep the
invariant that the tests pass in each commit). Will prefers to commit them
both together.

> 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?

No, but you can add a 'smcv' remote that is my repository (as documented
at http://telepathy.freedesktop.org/wiki/Git), then use something like
`git diff upstream/telepathy-gabble-0.8...smcv/08-conditional-ft | gview -`
to look at the squashed commits.

> 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));

Indeed.

> 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'd need to be (((char *) NULL) + 1) or something, because sizeof(void) is
undefined.

The fact that NULL is the special-cased thing is subtle and nasty, but this
is the stable branch, so we shouldn't rewrite unrelated code to make sense
(also, Will and I already did that for the corresponding part of the
development branch).

> 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.

I don't think there's anything wrong with that - that's why there's a callback
to free the caps object (which is NULL for the FT manager, since it doesn't
need freeing).

The representation of caps in 0.8 is somewhat mad, but the first major change
in 0.9 was that Will and I rewrote caps handling :-)

> 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?

Where did I write that? I don't see it in any of the lines I added?

The rationale for that style is exactly as you said. I personally don't like
it, and discourage it for Telepathy code, because gcc can usually warn about
accidental assignment-in-a-comparison in any case, and the version with
"variable == value" looks more like the way you'd usually say it.

If there are minor coding style issues in existing code that I'm not touching
(or only re-indenting) I won't generally change them, particularly not in
the same commit as a significant change, or on a stable branch.

> -  /* 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.

A valid point, I'll add one. However, this is the stable branch, and in this
case it's known to have rather strange capabilities code, so everyone should
be treading very carefully anyway :-)

> 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.

Yeah, this one is how I wished I'd been able to do it the first time :-)

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

Either that, or just a reference to the monstrous nature of some of our D-Bus
type signatures :-)

My only comment on the 0.8 branch is, are you sure the assertion is correct? One of the other channel managers just debugs a sad face in the update function, I think...

The 0.9 version looks fine.

The only call to gabble_caps_channel_manager_update_capabilities is from presence-cache.c:915, update_caps_helper, which calls gabble_caps_channel_manager_copy_capabilities if @out is NULL, or gabble_caps_channel_manager_update_capabilities otherwise. So yes, I'm sure. May I merge?

(In reply to comment #9)
> The only call to gabble_caps_channel_manager_update_capabilities is from
> presence-cache.c:915, update_caps_helper, which calls
> gabble_caps_channel_manager_copy_capabilities if @out is NULL, or
> gabble_caps_channel_manager_update_capabilities otherwise. So yes, I'm sure.
> May I merge?

Merge Monkey has descended from the Great Darcs Jungle In The Sky to say yes, please merge!

Fixed in git, will be in 0.8.9 and 0.9.3.