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

[bugzilla-daemon at freedesktop.org]

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





--- Comment #22 from Youness Alaoui <youness.alaoui at collabora.co.uk>  2010-03-25 09:22:11 PST ---
(In reply to comment #20)
> Strict aliasing
> ===============
> 
> <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=05d39c74f81e3a7>
> +  union {
> +    gpointer ptr;
> +    GTalkFileCollection *self;
> +  };
> 
> While very nice to use, this feature (anonymous unions) is unfortunately a gcc
> extension. Please be portable by giving the union a name (union {...} self;)
> and using data->self.ptr, data->self.self (perhaps rename the
> GTalkFileCollection member to fc, so it's data->self.fc).
Ah ok, I didn't know it wasn't standard, I'll fix it.

> 
> To be honest I'd be inclined to just make it:
> 
>     /* really a GTalkFileCollection, but, strict aliasing */
>     gpointer self;
No, I don't really like doing that, i'll explain why below..

> 
> and use data->self where a gpointer would be acceptable (probably most places -
> remember that you can pass a gpointer to a function expecting a typed pointer,
> or assign a gpointer to a typed pointer variable), or
> 
>     GTalkFileCollection *self = data->self;
> 
>     blah_blah (self->priv->bees);
> 
> in places where that won't work.
> 
> Similarly, in the next patch:
> 
> > +        while (g_hash_table_iter_next (&iter, NULL, &value))
> >            {
> > +            c = value;
> >              gabble_jingle_content_parse_transport_info (c, node, error);
> 
> you can just pass @value to g_j_c_p_t_i, and delete @c altogether?
No I didn't want to use value directly because I don't like using a gpointer
like that. I prefer to have something with its type and use that rather than
use an auto-cast-ed gpointer.
Also, 'c' is used elsewhere in this function, so I can't remove it anyways, so
I might as well use it.

> 
> For the future
> ==============
> 
> +  /* the weakref to the channel here is held through the GList *channels */
> 
> A more concise wording (which also works for strong refs) would be "borrowed
> from @channels".
> 
> +  /* GList of weakreffed GabbleFileTransferChannel */
> +  /* GHashTable of GabbleFileTransferChannel => GINT_TO_POINTER (gboolean) */
> 
> You don't need to repeat the container type, which is obvious from the
> declaration - only the types/ownerships of the contents, which aren't.
> 
I just find it easier to wrote "GHashTable of" rather than "a hash table of"
and without the "a list of" or "a hash table of", I feel like the comment is
'empty' :p

(In reply to comment #21)
> (In reply to comment #19)
> > except that I'd like all the instances of gtalk_fc, GTALK_FC and (if any)
> > GTalkFc to be gtalk_file_collection, etc.
> 
> To clarify that: I'd prefer properties and other public API to have the full
> name, and I'd prefer instance members (self->priv->gtalk_file_collection) to
> have the full name to be consistent with the properties they implement, but
> local variables can be gtalk_fc or fc or collection or something.
ok, I see.. I've fixed it now and used gtalk_file_collection everywhere.
I prefered gtalk_fc because I hate long var names because of the 80char limit
and I'm forced to split into two lines every method call using the variable...


> 
> The review-fixes branch looks good up to
> http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=131531e300923
> (your last commit at the time of writing), apart from the things I mentioned.
> The documentation you added really helps - thanks!
> 
Thanks!


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