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

[bugzilla-daemon at freedesktop.org]

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





--- Comment #6 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-03-19 09:01:02 PST ---
After some IRC discussion I've started reviewing the flattened version of
shared-ft. I don't think implementing the whole lot at once is necessarily the
best way to have done it, but it's happened now.

This is not a full review, but some of the changes I'm suggesting may be
disruptive (naming/layering improvements), so I thought I should put them on
the bug sooner rather than later.

Naming
======

Easy to do but somewhat important to make the code understandable:

I don't like the naming of the "GtalkFtManager" - the name misleads me into
thinking it's either a ChannelManager, or something that manages GtalkFT
objects,
neither of which it actually is (and as a trivial side point, I don't like the
miscapitalization of GTalk). Perhaps it could mirror the D-Bus API, and hence
be
called a GTalkFileCollection? I think that'd make a lot of sense, and avoid the
confusing code where a variable called gtalk_ft holds a GtalkFtManager.

I'm going to call this a file collection from now on, until/unless someone
suggests a better name :-)

We'd conventionally call this GabbleGTalkFileCollection by way of namespacing,
although I can see that that name is getting a bit unwieldy, so I can
understand why you omitted the prefix.

As noted in comment #5, it's very confusing that the <channel>s from GTalk's
protocol are called "channels" in the code, given that this is Telepathy code
where a channel means something different. Could they be called components,
perhaps? They seem to correspond 1:1 to Jingle components, AFAICS?

I was very surprised to see structs called "GabbleChannel" and "JingleChannel"
within the file collection. GabbleChannel seems to be a thin wrapper around a
channel object, which is likely to be obsoleted by the structural changes I
suggest below. If it isn't, I think it should just be replaced by a pointer to
the channel object, and its boolean attributes should either become methods on
the channel object, or be incorporated into the methods that are currently
conditional on them.

JingleChannel probably deserves to be a real object with methods and stuff: its
name is also far too generic, and I'd call it [Gabble]GTalkFileComponent or
something.

Structural/design
=================

> -  PROP_BYTESTREAM,

See earlier patch-by-patch review - I think this should remain a (nullable)
property, and the newly renamed file collection should be another. With the
layering changes I'm about to suggest, they could both be construct-only, since
any subsequent change from NULL to non-NULL during the offer process would be
done internally.

> +      gtalk_ft_manager_terminate (self->priv->gtalk_ft, self);
> +      /* the terminate could synchronously unref it and set it to NULL */
> +      if (self->priv->gtalk_ft != NULL)
> +        {
> +          g_object_unref (self->priv->gtalk_ft);
> +          self->priv->gtalk_ft = NULL;
> +        }

This is rather terrifying. Isn't there a better way we could do this
relationship?

For a modular design (good layering), there should be a concept of objects
being in an order: objects are allowed to know about lower layers, but not
about
higher layers. I think we need to decide between these two stacks:

* GabbleFtManager > GTalkFileCollection > GabbleFileTransferChannel
* GabbleFtManager > GabbleFileTransferChannel > GTalkFileCollection

I initially thought the first one made more sense, but after looking at the
code a bit more, I think you're right to have the channel ref the file
collection (which means you're half way to having the second relationship).
This has the nice side-effect that the GTalkFileCollection could probably
end up in Wocky one day.

As far as I can tell, the calls between the objects are:

ChannelManager calling into file collection:

* new_from_session() (when receiving)
* get_channels() (when receiving)

Channel calling into file collection:

* new() then initiate() (when offering)
* block_reading()
* accept()
* send_data()
* completed()
* terminate()

File collection calling into channel:

* new() (in new_from_session()) [1]
* set_gtalk_ft() during add_channel() [2]
* set_gtalk_ft_state() [3]
* gtalk_ft_write_blocked() [4]
* gtalk_ft_data_received() [5]

My suggestion for the file collection's API would go something like this:

When created to receive files, all its components should be blocked. Most of
the logic of new_from_session() should move into the GabbleFtManager, so the
GabbleFtManager would take care of creating a Telepathy channel for each
component, and handing the file collection and component number to the
channel as a construct-time property (much like the current handling of
bytestreams). This eliminates [1] and [2].

The channel would unblock the relevant component on the file collection,
and off we go: subsequently, the channel would "drive" the file collection, a
lot like it now does with bytestreams. [3], [4] and [5] would have to become
signals or something: perhaps they could even have a boolean return value, so
the file collection can block itself automatically if nobody's reading yet?

Another good way to handle this might be to take the JingleChannel struct
from the file collection, and either merge its functionality into
GabbleJingleShare (if possible), or promote it to an object in its own
right, analogous to the bytestream objects we already have. [3], [4] and [5]
would then be signals emitted from this object, rather than from the
file collection?

Not obvious
===========

These might be explained better in the commit log, I haven't checked yet
(I haven't been reading commit messages during this whole-branch review).

> -  if (self->priv->state != TP_FILE_TRANSFER_STATE_COMPLETED)
> +  if (self->priv->state != TP_FILE_TRANSFER_STATE_COMPLETED &&
> +      self->priv->state != TP_FILE_TRANSFER_STATE_CANCELLED)

I suspect that the guard against cancelling an already-cancelled FT is good,
but is this a fix for a pre-existing bug, or what?

> -      if (self->priv->transport)
> +      if (self->priv->transport &&
> +          (self->priv->bytestream || self->priv->gtalk_ft))

Why did this logic change? Is it possible for channel_open() to be called
with neither a bytestream nor a GTalk FT?

> +gabble_file_transfer_channel_set_bytestream (GabbleFileTransferChannel *self,
...
>    if (bytestream == NULL)
> -    return;
> +    return FALSE;

Not your fault (pre-existing code), but why isn't this a g_return_if_fail?
If it's deliberate that a NULL bytestream is silently ignored, I'd at least
expect a comment above the function.

> +  if (self->priv->bytestream || self->priv->gtalk_ft)
> +    return FALSE;

Similarly, why would it not be a programming error to assign more than one?
Exactly the same applies to ..._set_gtalk_ft.

> -  self->priv->remote_accepted = TRUE;

Why is it OK that bytestream_negotiate_cb() no longer does this?

Minor stuff
===========

> +PKG_CHECK_MODULES(NICE, nice >= 0.0.10.1)

Should be 0.0.11 - we don't want to depend on any unreleased module

> -close_bytestrean_and_transport (GabbleFileTransferChannel *self)
> +close_session_and_transport (GabbleFileTransferChannel *self)

gabble_file_transfer_channel_close_transports(), perhaps?

For future reference
====================

(Things that don't need fixing here, but should be considered in future)

> +<node name="/Channel_Type_FileTransfer_Future"

This should be /Channel_Type_File_Transfer_Future really, but it doesn't matter
much since this interface is temporary. In the Telepathy code generation stuff,
the node name is (ab)used to name auto-generated identifiers - this mistake is
why you have GABBLE_IFACE_CHANNEL_TYPE_FILETRANSFER_FUTURE rather than
GABBLE_IFACE_CHANNEL_TYPE_FILE_TRANSFER_FUTURE, for instance.

Please also replace tabs with an appropriate number of spaces (setting your
editor to highlight tabs may help). Again, it doesn't matter much here, since
we'll fix it when incorporating this functionality in the spec.

> +static void transferred_chunk (GabbleFileTransferChannel *self, guint64 count);

I generally prefer to avoid forward declarations and just put the static method
above the first call to it. (It's hard to do that for GInterface initialization
functions, since they need to be called in the G_DEFINE_TYPE_WITH_CODE, yet
reference method implementations, but it's easy to do for most helper
functions.)

I also somewhat prefer private functions to be properly namespaced
(gabble_file_transfer_channel_foo(), or a somewhat abbreviated version like
ft_channel_foo()) even though they're static - this results in a bit more
typing, but better debug messages (the DEBUG macro automatically incorporates
the function name).

If the function is conceptually a method on a named private data structure
(like your JingleChannel and GabbleChannel structs, for instance), it's nice to
name it accordingly (pending_stun_server_free() in jingle-factory.c is a simple
example).


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