Bug 11291

Created attachment 10336 [details] [review]
xep 0092 support, report version of gabble

Patch looks good, but perhaps the code would be more readable if it used our lm_message_build utility function.

Niggles:

+  if (!lm_connection_send (self->lmconn, result, NULL))
+  {
+      DEBUG ("sending disco response failed");
+  }

Brackets should be indented two more spaces.

I would prefer it if there were blank lines around compound statements.

Ok, i will submit a new patch with styles fixes and the lm_message_build function.

Shouldn't it be possible for the client to set that value? Like that we can say the client is "Empathy" instead of Gabble...

At least clients should be able to get the value for a given contact handle.

I think  we need an API in the spec like:

GetClient(au: handles) -> a{s: ClientName, s: ClientVersion}
  This takes an array of contact handles and returns for each the name and version that contact is using. SelfHandle is accepted.

SetClient(s: ClientName, s: ClientVersion) -> void
  This changes our client name/version to override the default value setted by CM.

signal:
ClientChanged(a{uss})
  Emitted when the ClientName/ClientVersion changes. The value is an array of struct containing: Contact Handle, client name, client version. This is emitted when the the value changed for self contact too.

Those can be added to the org.freedesktop.Telepathy.Connection interface.

What is the purpose of this feature? Is there something you can do if you know the software version that you can't do otherwise, or is it just present for "advertising" purposes?

In Telepathy I don't know whether it makes sense to report the actual client version (e.g. Empathy) - if you're doing some sort of version-specific bug workaround, for instance, it seems more likely that it'd be a Gabble bug. Empathy doesn't implement any XMPP feature.

Also, it's possible that more than one client is sharing a connection (that's a lot of the point of the Telepathy architecture), in which case the Gabble version is the only meaningful version number we can provide. For instance, if you're doing collaborative audio editing with Jokosher, I don't think the Jokosher version is useful to put in the jabber:iq:version.

Created attachment 10339 [details] [review]
new version of previus patch

Here is the new version. Indeed, using lm_message_build is cleaner.

well my proposal was only to show the information, not something really useful. It's just "fun" to see which client a contact is running. Even if you can have many clients, I think we'll alway have a main client, like empathy, and others little clients for just a few features, like jokosher.

Anyway, that's not really a big problem, maybe it can be part of the ContactInfo-ng interface ?

I added your patch in our review system for easier review and merge.
http://projects.collabora.co.uk/~monkey/telepathy-gabble-bug-11291/

please check lm_message_node_get_attribute (iq, "from")
!= NULL


According latest coding style, when you write a function call on multi lines,
args have to be 4 spaces alligned.


Except that, patch looks good to me.

Seems I forgot to update this patch :/

Sorry for the delay. Here is a new patch against latest darcs snapshot.

Created attachment 14057 [details] [review]
Third attempt, this time, it should be good ( I hope )

Could you store the "from" field in a variable so we won't get it 2 times from the stanza?

For extra bonus point, a small test of this feature would be great. :)

I've ported this branch to Wocky and applied against current Gabble master - I don't have time to write a test though, unfortunately. Branch is here: http://git.collabora.co.uk/?p=user/robot101/telepathy-gabble.git;a=shortlog;h=refs/heads/version

(In reply to comment #13)
> I've ported this branch to Wocky and applied against current Gabble master - I
> don't have time to write a test though, unfortunately. Branch is here:
> http://git.collabora.co.uk/?p=user/robot101/telepathy-gabble.git;a=shortlog;h=refs/heads/version

Rob: Looks good; I've merged it (and added a simple test). Thanks for resurrecting this branch. It'll be in 0.11.7.

Michael: thank you for submitting the initial report and implementation(s), and sorry it's taken so long for your contribution to be included.