[Bug 25493] GTalk-compatible file transfers are not implemented
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Thu Mar 18 15:34:52 CET 2010
http://bugs.freedesktop.org/show_bug.cgi?id=25493
--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-03-18 07:34:51 PST ---
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>:
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.
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
> + /* 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?
> + //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).
> +const gchar *gabble_jingle_share_parse (GabbleJingleShare *sess,
> + LmMessage *message, GError **error);
Doesn't actually seem to exist?
> - 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.
> + 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
> + 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);
> + 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?
> + 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 ()")
> + if (self->priv->bytestream) {
That's not how we indent.
> + size = gabble_jingle_share_get_filesize (c);
I'd prefer file_size or just size.
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.
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.)
> + 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?
> + 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 (...))
> + 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.
> + guint idx;
Please avoid uncommon abbreviations. I'd just call it "i", tbh.
> + 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
> + 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?
http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=9bbc9fb9562a
Commit message is far too long; we like linebreaks.
> + 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.
> 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.
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()?
> + GList *manifest;
Should have a comment to say what it's a list of, and who owns the items
> + m->image_width = atoi (width);
atoi is locale-sensitive, so that's not what you want. g_ascii_strtoull would
be more appropriate.
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?
http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=caa2d524faa97
> +gen_manifest (GabbleJingleShare *self)
ensure_manifest(), perhaps?
> + GabbleJingleShareManifest *m = g_slice_new0 (GabbleJingleShareManifest);
> + m->name = g_strdup (self->priv->filename);
Blank line between declarations and statements, please
> - _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
> +GList *gabble_jingle_share_get_manifest (GabbleJingleShare *self)
Newline after GList *, please
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
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
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
> + priv->last_channel_component_id + 1) == FALSE)
Same complaint 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
> +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.
> - 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?
--
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
More information about the telepathy-bugs
mailing list