[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 #6 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-11-25 05:54:07 PST ---
(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 :-)


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