[Bug 25493] GTalk-compatible file transfers are not implemented

[bugzilla-daemon at freedesktop.org]

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





--- Comment #9 from Youness Alaoui <youness.alaoui at collabora.co.uk>  2010-03-19 12:43:55 PST ---
(In reply to comment #5)
> I don't have a good overview of this branch and am currently reviewing
> patch-by-patch: I'll have a look at the overall branch (i.e. collapsed patch)
> later.
> 
> Reviewed up to, but not including,
> <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=a74ef1df3e8b686ad4a959e8e62a6a6070d68a53>:
> 
Thanks for reviewing!

> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=0451d5847fe2b
> > fix typo bytestrean/bytestream
> 
> This could usefully be on a trivia branch that's merged ahead of the feature
> work.
If you want I could cherry pick it, but I don't think it's really necessary
atm, it's not like it's a bugfix or anything...

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=4afc4f205175b8:
> > +  GabbleJingleShare *trans = GABBLE_JINGLE_SHARE (object);
> 
> trans is an odd name for a thing that isn't a transport, and in any case I
> think you mean "self" :-P
Yeah, copy paste errors probably :) I checked, and it seems it was already
fixed in a later commit.

> 
> > +  /* This property is here only because jingle-session sets the media-type
> > +     when constructing the object.. */
> > +  g_object_class_install_property (object_class, PROP_MEDIA_TYPE,
> > +      g_param_spec_uint ("media-type", "media type",
> 
> I thought the whole concept of media types was part of Jingle RTP, rather than
> the layer of Jingle at which file transfers appear? If I'm right, then file
> transfers should have media type JINGLE_MEDIA_TYPE_NONE?

Well, the way I see it is that you have a 'media type file' for file transfers
(because, that's what it is, a ICE/UDP stream, in which you send a file).
But the reason this is there is explained in the comment. The jingle-session
creates the object with MEDIA_TYPE=MEDIA_TYPE_NONE, then it changes it
depending on the type of content in the jingle-session. There's just no
avoiding it with the way the jingle code works, at least, not without
refactoring it or adding special use cases, which I didn't want to do.
Simple solution, with an explanation comment seemed safer.

> 
> > +  //GabbleJingleShare *self = GABBLE_JINGLE_SHARE (content);
> > +  //GabbleJingleSharePrivate *priv = self->priv;
> > +  //JingleDialect dialect = gabble_jingle_session_get_dialect (content->session);
> 
> No commented-out cruft in commits, please (also, comments should be /* */
> rather than // in C code).
Yeah, those were temporary, they got removed/fixed later. But yeah, I'm also
surprised I didn't see them when I did the 'git add -p'.

> 
> > +const gchar *gabble_jingle_share_parse (GabbleJingleShare *sess,
> > +    LmMessage *message, GError **error);
> 
> Doesn't actually seem to exist?
You're right. Removed.

> 
> > -  PROP_BYTESTREAM,
> 
> I don't like the removal of the bytestream property from the FT channel in
> favour of bolting one on afterwards: I think I'd prefer it if the bytestream
> and the Jingle session were both properties, with one or neither supplied
> during construction.
Humm, either way is fine I think, it's just a matter of choice. But I agree
that having them both as properties would probably be nicer. I'll fix that.

> 
> > +  if (self->priv->bytestream || self->priv->jingle)
> 
> (...->bytestream != NULL || ...->jingle != NULL), please, and shouldn't this be
> a g_return_if_fail?
> 
> > +  if (resource)
> 
> != NULL
ok, sorry, I didn't know about this convention, I usually put the == NULL, but
not the != NULL. I'll fix it.
About g_return_if_fail, I simply forgot it exists.

> 
> > +  g_object_set (jingle, "dialect", JINGLE_DIALECT_GTALK4, NULL);
> 
> We always put one property/value pair per line in g_object_set/g_object_new:
> 
> g_object_set (jingle,
>     "dialect", J_D_G,
>     NULL);

Didn't know it was a convention. Now fixed. Thanks.

> 
> > +  g_assert (self->priv->bytestream == NULL && self->priv->jingle == NULL);
> 
> g_assert (a && b) is typically better written as two assertions: you get a
> better failure message that way. Also, shouldn't this be a g_return_if_fail?
Very much true. Thanks, fixed.

> 
> > +      self->priv->handle);
> > +  if (presence == NULL)
> 
> Blank lines around blocks (if/while/etc.), please
> 
> > +      if (error != NULL)
> > +        g_set_error (error, TP_ERRORS, TP_ERROR_OFFLINE,
> > +            "can't find contact's presence");
> 
> No need for the guard, g_set_error copes with NULL as a first argument (that's
> the whole point of using it instead of "*error = g_error_new ()")
> 

Don't know, that was copy/pasted code. Anyways, I won't "fix" this because I
prefer to have the guard than not have it.

> > +  if (self->priv->bytestream) {
> 
> That's not how we indent.

Yes, they were all fixed later on in another commit, I greped for ')' and '{'
and there are none (from my code anyways).

> 
> > +      size = gabble_jingle_share_get_filesize (c);
> 
> I'd prefer file_size or just size.
That method disappeared after the refactor..

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=2de68bb15afc8d1
> > Remove checking for dialect in google transport
> 
> What does this have to do with anything? The long commit message should explain
> why this change is desirable.
Oh, it's simple, it checks to see if the dialect is 'google' in the 'google
transport'.. it was not necessary and made no sense because the
google-transport is already only used when we are in google dialect.
Sorry, I thought it would be obvious.

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=aef01b07e68f6
> 
> This patch seems to remove the special handling of the components named
> video_rtp, video_rtcp, rtp and rtcp. Is that special handling duplicated
> elsewhere (in which case explain in the long commit message), or have you just
> introduced a regression?
> 
> (Reading on, it seems that you did introduce a regression, which you fixed in
> the following patch. Please don't do that, where possible - aim for each patch
> to be valid in isolation.)

I wanted to make small patches, not necessary all fully working, that's why I
separated these two changes into two separate commits, to make it easier to
read/review/understand. I usually don't aim for each patch to be 'valid' but
rather to be 'isolated set of related changes'.

> 
> > +  GHashTable *channels; /* Google transport 'channels', not TP channels */
> 
> Please comment the contents and their ownership:
> 
> /* Google transport 'channels', not Tp channels
>  * g_strdup'd component name => GINT_TO_POINTER (component ID) */
> 
> ... and why isn't it called components or component_ids, which would remove the
> confusion with Telepathy channels?
ok, I'll comment my hash table/list contents.
And it's 'channels' because that's the jingle language. They are called
'channels'. Yes, it clashes with telepathy's naming, but they are still a
'jingle channel', and I don't think it's wise to start renaming them to
something else, otherwise it will add confusion and will make it harder later
on. And they are not 'components', it is just how I decided to store the
information.. since jingle doesn't hold the 'channel name', it only used
component ids (1 for rtp, 2 for rtcp), I decided to assign each jingle channel
a unique id (irrelevant to the protocol) and use that as component_id.
But technically, it's a jingle channel, where locally I assign to it a unique
id that I use as a component_id, but it's not one since each 'channel' (each
'component id') will have its own NiceAgent, it won't be an additional
component on the same ICE Agent.
it's a bit complicated, and maybe I should document it somewhere to make it
clearer... 
If you're still confused about this, let me know (I know I'm bad at explaining
stuff)
> 
> > +      if (g_hash_table_lookup_extended (priv->channels, name,
> > +              NULL, NULL) == FALSE)
> 
> Boolean results are the one case where we don't do an explicit comparison:
> please use if (!g_h_l_e (...))
I'd have to disagree, I always *hate* it when I see if (!bool) because it makes
it so much harder to read/understand the code.. the '!' is so subtle, you don't
always spot it, and I just find it hard...
Anyways, I don't like this convention, but if it's telepathy convention, I got
no choice, so I've fixed this for you.

> 
> > +  GList **all_candidates = NULL;
> 
> At the very least, this deserves a comment. By inspection, it appears to be an
> array (of length @components) of GList of JingleCandidate. A GPtrArray, or
> indeed a GHashTable from component number to GList, might well be a clearer way
> to do this.

Yep, that part was ugly, iirc, I asked sjoerd to review that function so we can
discuss a better method of doing it, and it got refactored later on.

> 
> > +  guint idx;
> 
> Please avoid uncommon abbreviations. I'd just call it "i", tbh.

'idx' is usually for 'index' and it's a common abbreviation. it's also used in
the media-channel code..

> 
> > +      for (idx = 0; idx < components;  idx++) {
> 
> That's not how we indent. I'm going to stop bothering to mention occurrences of
> this now; consider them all to have been noted :-P
Yep, I know, needed a little getting used to, but like I said before, they were
all fixed already in later commits.

> 
> > +      GList **cands = NULL;
> 
> I think this would be clearer if it just used i, like so:
> 
> >    for (i = 0; i < components; i++)
> >      {
> >        JingleCandidate *c2 = all_candidates[i]->data;
> >
> >        if (c->component == c2->component)
> >          break;
> >      }
> >
> >      if (i >= components)
> >        {
> >          /* not found */
> >          all_candidates = g_renew (GList *, all_candidates, ++components);
> >          i = components - 1;
> >        }
> >
> >      all_candidates[i] = g_list_prepend (all_candidates[i], c);
> 
> Indeed, I remain unconvinced an extending array of GList is the right data
> structure here. (This does get somewhat better in a later patch, when you
> change it into a GList of GList.)
> 
> In fact, you seem to be mapping in an elaborate way between component names and
> lists of candidates for the component, going via integer component numbers.
> Accordingly, wouldn't it be easier to just have a map from component name to
> list of candidates?

Yes, it does get better later like I explained above. I will ignore the current
comments since they seem out of place with the code as it is now, so if you can
review the function in its current state and maybe provide an insight on how to
do it better.

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=9bbc9fb9562a
> 
> Commit message is far too long; we like linebreaks.
Yep, learned my lesson already :)

> 
> > +      while (g_hash_table_iter_next (&iter, NULL, (gpointer) &c))
> 
> This violates strict aliasing. Declare c with type gpointer; its real type will
> be obvious from context.
Humm, are you sure there will be strict aliasing errors with 'gpointer'? I'm
not getting any warning at compilation. Also, if I declare it as gpointer, then
I'd still need to cast it into GabbleJingleContent in order to call
gabble_jingle_content_parse_info

> 
> >  jingle_transport_google_set_component_name (
> > -    GabbleJingleTransportGoogle *transport, gchar *name, gint component_id)
> > +    GabbleJingleTransportGoogle *transport,
> > +    const gchar *name, gint component_id)
> 
> What's this doing here? It should have been a separate patch "fix
> const-correctness of jingle_transport_google_set_component_name", which in turn
> should have been squashed into the commit that introduced that function.
I'm trying to make small commits all the time, but at some point, there's a
limit, I don't think this is an issue to have it slip in an unrelated commit ;p

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=c66e8cb4b2fb
> > +static gchar *
> > +generate_temp_url (void)
> 
> Couldn't you just use gabble_generate_id()?
didn't know it exists... but after looking at it, it seems to generate in a
format or "%lx.%lx/%lx/%lx" or uuid (I don't know the format of uuid), but
since I'm building a path here, I'm not sure it's a great idea to have '/' in
there.. i'd prefer just using the simple code I have.

> 
> > +  GList *manifest;
> 
> Should have a comment to say what it's a list of, and who owns the items
it was refactored, it's better now.

> 
> > +            m->image_width = atoi (width);
> 
> atoi is locale-sensitive, so that's not what you want. g_ascii_strtoull would
> be more appropriate.
oups, probably some copy/paste from libjingle. Fixed, thanks!

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=9806c8a5bebc
> > properly report the direction as 'initiator' for the file transfers
> 
> Hard-coding special treatment in GabbleJingleContent doesn't seem right.
> Shouldn't this be some sort of virtual method?
it doesn't seem right indeed, but that's the best I could do. That whole jingle
code has 'if media type == video' and 'if media type == audio' all over the
place, and there are many thing that are very audio/video specific, so I just
pitched in use cases for non audio/video jingle.
Do you suggest I change this before merging? in that case, how do you suggest
to fix it ?

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=caa2d524faa97
> > +gen_manifest (GabbleJingleShare *self)
> 
> ensure_manifest(), perhaps?
yep, fixed.

> 
> > +      GabbleJingleShareManifest *m = g_slice_new0 (GabbleJingleShareManifest);
> > +      m->name = g_strdup (self->priv->filename);
> 
> Blank line between declarations and statements, please
fixed them all

> 
> > -  _gabble_jingle_content_set_media_ready (GABBLE_JINGLE_CONTENT (obj));
> > +  _gabble_jingle_content_set_media_ready (content);
> 
> This has nothing to do with the purpose of this commit, and should have been
> squashed into the commit that introduced this line in the first place
you are too verbose.. :p

> 
> > +GList *gabble_jingle_share_get_manifest (GabbleJingleShare *self)
> 
> Newline after GList *, please
fixed them all.
> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=87453d951a7c
> > do not create a new manifest if we have no filename
> 
> Obvious candidate for squashing into previous commit
Nope, they were separated on purpose.. since the previous commit just moved
that chunk of code into its own function, if I had squashed this too (or git
added it) then this patch/fix would have been lost in the change.. I don't like
reviewing a commit where code is moved from one place to another, and you have
to manually do the diff to see if it was moved or 'moved and modified'.

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=fba2d93160f9d
> > +          NULL, NULL) == TRUE)
> 
> No explicit comparison for booleans, please - comparing against TRUE is
> dangerous, because ((gboolean) 42) is a true value which is neither TRUE nor
> FALSE
as stated above, I disagree, and they are all fixed to suit your taste now.
But as an FYI, casting '42' as a gboolean is bad, I would rather do : 
gboolean b = x > 0? TRUE: FALSE;
And I got into the habit of checking for == TRUE/FALSE because we were forced
to do that in my previous job, because on some systems (can't remember if it
was AIX or MPE/x) TRUE is 0xFFFFFFFF, everything else is FALSE...
(which is also why we do '#define TRUE (1 == 1)', and not '#define TRUE 1')

> 
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=354bc1deca339
> > -    gabble_marshal_VOID__STRING_UINT,
> > +    gabble_marshal_VOID__STRING_INT,
> 
> This part isn't related to the rest of the commit, and would have been better
> squashed into the commit that introduced this signal
Nope, because at that time UINT made sense and now, in this commit in which I
actually call g_signal_emit, it made sense to make it a INT.

> 
> > +              priv->last_channel_component_id + 1) == FALSE)
> 
> Same complaint as previously
same response as previously :)
> 
> > +new_channel (GabbleJingleContent *c, const gchar *name)
> 
> If these constructs are isomorphic to Jingle components, I suggest calling them
> components, even if Google call them channels
as explained above, they are not the same as a component, it's just a 'hack'
that I transform the 'name' into a 'int' and I use the existing (and previously
unused in our case) component_id variable to store it. I could also just create
a 'gchar *channel' variable instead in the JingleCandidate..
Anyways, this should be discussed over IRC if you don't agree.

> 
> > +static guint
> > +new_channel (GabbleJingleContent *c, const gchar *name)
> 
> Is the component ID signed or unsigned? Please make your mind up. Likewise on
> gabble_jingle_content_create_channel.
Humm.. I was hesitant at the start, then I had made up my mind as a guint, I
just checked it's unsigned pretty much everyone, but I missed some spots. Fixed
now.

> 
> > -      DEBUG ("changing senders from %s to %s", produce_senders (priv->senders), senders);
> > +      DEBUG ("changing senders from %s to %s", produce_senders (priv->senders),
> > +          senders);
> 
> Secretly not actually relevant to this patch?
> 
not ninja enough :(


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