http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=ft-send-receive-39188 contains Mortem's code which need to be finished/cleaned.

I think the receiving code should compute the hasing itself while receiving the file. If not, we have to re-read the whole file once the transfer is completed which is a bit stupid (Empathy does that atm).

Downside is we won't be able to use g_output_stream_splice_async() any more as it doesn't give us a way to get the data being transfered.

Here is the branch cleaned up.

It doesn't do any hashing at all. I spoke to Guillaume about this and it really needs to be another utility class unfortunately. This only really implements {Offer,Provide}File.

I'm lost in a sea of green pluses, but here's a few initial thoughts:

+ * To send a file to a contact, one should create a File Transfer
+ * channel with the appropriate D-Bus properties set by specifying
+ * their values in the channel creation method call. The file transfer
+ * invitation will be sent to the remote contact when the channel is
+ * created.

It would be much more helpful to give a code example here.

You could make _TpFileTransferChannelPrivate.access_control_param a GValue rather than a GValue *; but no big deal.

+static void
+operation_failed (TpFileTransferChannel *self,
+    const GError *error)
+{
+  g_simple_async_result_set_from_error (self->priv->result, error);

It comes to my attention that we do now depend on a new enough GLib that we could use _take_error where appropriate. (Almost every place that calls operation_failed calls g_error_free right afterwards.)

+static void
+client_socket_connected (TpFileTransferChannel *self)
+{
+  GSocketConnection *conn;
+  GError *error = NULL;
+
+  conn = g_socket_connection_factory_create_connection (
+      self->priv->client_socket);
+  if (conn == NULL)
+    {
+      DEBUG ("Failed to create client connection: %s", error->message);
+      operation_failed (self, error);
+      return;
+    }

but here, error will always be NULL on the error path and we'll crash.

+static void
+invalidated_cb (TpFileTransferChannel *self,
+    guint domain,
+    gint code,
+    gchar *message,
+    gpointer user_data)
+{
+  /* stop splicing */
+  g_cancellable_cancel (self->priv->cancellable);
+}
+

I don't think this is safe. TpProxy's _dispose() method, which is called after TpFileTransferChannel's, emits invalidated if the proxy wasn't already invalidated. But by that time, self->priv->cancellable will have been freed by TpFTChannel's _dispose() implementation.

+static void
+operation_failed (TpFileTransferChannel *self,
+    const GError *error)
+{
+  g_simple_async_result_set_from_error (self->priv->result, error);
+
+  g_simple_async_result_complete (self->priv->result);
+  tp_clear_object (&self->priv->result);
+}

The stray newline here offends me.

 static void
+tp_file_transfer_channel_uri_defined_cb (TpChannel *proxy,
+    const gchar *uri,
+    gpointer user_data,
+    GObject *weak_object)
+{
+  TpFileTransferChannel *self = (TpFileTransferChannel *) proxy;
+
+  self->priv->file = g_file_new_for_uri (uri);
+  g_object_notify (G_OBJECT (self), "file");
+}

If the CM erroneously emits this signal when URI was already defined, this leaks.

(Sorry this keeps coming in in dribs and drabs; I'll say as much when it's finished.)

(In reply to comment #3)
> It doesn't do any hashing at all. I spoke to Guillaume about this and it really
> needs to be another utility class unfortunately. This only really implements
> {Offer,Provide}File.

I'd like to have a rough idea of how this API will look like, just to be sure that yours will work with it.
Especially, we have to be sure that we provide any info/state changes to feed to Empathy's file transfer dialog.

Also, like I said in Comment 2, hashing of the incoming file should be done by the FT chan, but that shouldn't be a problem to add internal API to pass the hashing to the higher level object I guess.

+  self->priv->service_name = tp_asv_get_string (properties,
+      TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA_SERVICE_NAME);
+  if (self->priv->service_name == NULL)
+    {
+      DEBUG ("Channel %s doesn't have Chan.I.FileTransfer.Metadata.ServiceName "
+          "in its immutable properties", tp_proxy_get_object_path (self));
+    }

Does this really warrant being DEBUG'd, particularly if the channel doesn't even claim to implement FT.Metadata? Ditto Metadata.Metadata below.

+    * TpFileTransferChannel:file
+    *
+    * For incoming, this property may be set to the location where the file
+    * will be saved once the transfer starts. The feature
+    * %TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE must already be prepared for this
+    * property to have a meaningful value, and to receive change notification.
+    * Once the initial value is set, this can no be changed.

no be changed!

+    * For outgoing, this property may be set to the location of the file being
+    * sent. The feature %TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE does not have
+    * to be prepared and there is no change notification.

Both paragraphs here are ambiguous. “may be set”, but it's a read-only property.  How does one set it? Who sets it? etc.


   /**
+   * TpFileTransferChannel:state
+   *
+   * A TpFileTransferState holding the state of the file transfer.
+   *
+   * Since 0.15.UNRELEASED
+   */

Are you missing “The %TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE feature has to be prepared for this property to be meaningful and kept up to date.” here?

+  /**
+   * TpFileTransferChannel:service-name:
+   *
+   * A string representing the service name that will be used over
+   * this file transfer channel or %NULL.

This doesn't really help someone reading the documentation understand what this property is for.

+   * Additional information about the file transfer set by the channel
+   * initiator or %NULL.

You need a comma, and maybe “, or %NULL if the initiator did not provide any additional information”. Would it be kinder to make it an empty hash table in that case?


+  else if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_PENDING))
+    {
+      /* The connection is pending */
+      GSource *source;
+
+      source = g_socket_create_source (self->priv->client_socket, G_IO_OUT,
+          NULL);
+
+      g_source_attach (source, g_main_context_get_thread_default ());
+      g_source_set_callback (source, (GSourceFunc) client_socket_cb, self,
+          NULL);

I assume this was taken verbatim from Empathy and works? :)

+/**
+ * tp_file_transfer_channel_accept_file_finish:
+ * @self: a #TpFileTransferChannel
+ * @result: a #GAsyncResult
+ * @error: a #GError to fill
+ *
+ * Finishes a file transfer accept operation.

“Finishes a call to tp_file_transfer_channel_accept_file_async()”?

+gboolean
+tp_file_transfer_channel_accept_file_finish (TpFileTransferChannel *self,
+    GAsyncResult *result,
+    GError **error)
+{
+  DEBUG ("enter");

rly.

ditto provide_file_finish.

(In reply to comment #4)
> I'm lost in a sea of green pluses, but here's a few initial thoughts:
> 
> + * To send a file to a contact, one should create a File Transfer
> + * channel with the appropriate D-Bus properties set by specifying
> + * their values in the channel creation method call. The file transfer
> + * invitation will be sent to the remote contact when the channel is
> + * created.
> 
> It would be much more helpful to give a code example here.

le done.

> +static void
> +operation_failed (TpFileTransferChannel *self,
> +    const GError *error)
> +{
> +  g_simple_async_result_set_from_error (self->priv->result, error);
> 
> It comes to my attention that we do now depend on a new enough GLib that we
> could use _take_error where appropriate. (Almost every place that calls
> operation_failed calls g_error_free right afterwards.)

OK, done.

> +static void
> +client_socket_connected (TpFileTransferChannel *self)
> +{
> +  GSocketConnection *conn;
> +  GError *error = NULL;
> +
> +  conn = g_socket_connection_factory_create_connection (
> +      self->priv->client_socket);
> +  if (conn == NULL)
> +    {
> +      DEBUG ("Failed to create client connection: %s", error->message);
> +      operation_failed (self, error);
> +      return;
> +    }
> 
> but here, error will always be NULL on the error path and we'll crash.

Good point.

(In reply to comment #5)
> I don't think this is safe. TpProxy's _dispose() method, which is called after
> TpFileTransferChannel's, emits invalidated if the proxy wasn't already
> invalidated. But by that time, self->priv->cancellable will have been freed by
> TpFTChannel's _dispose() implementation.

Okay, I'll add a check for cancellable being NULL. I want the splicing to stop soon after the channel closes. This is the best way, right?

(In reply to comment #6)
> The stray newline here offends me.

Get out more.

> If the CM erroneously emits this signal when URI was already defined, this
> leaks.

Good point, fixed.

(In reply to comment #8)
> +  self->priv->service_name = tp_asv_get_string (properties,
> +      TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA_SERVICE_NAME);
> +  if (self->priv->service_name == NULL)
> +    {
> +      DEBUG ("Channel %s doesn't have Chan.I.FileTransfer.Metadata.ServiceName
> "
> +          "in its immutable properties", tp_proxy_get_object_path (self));
> +    }
> 
> Does this really warrant being DEBUG'd, particularly if the channel doesn't
> even claim to implement FT.Metadata? Ditto Metadata.Metadata below.

I guess not; removed.

> +    * TpFileTransferChannel:file
> +    *
> +    * For incoming, this property may be set to the location where the file
> +    * will be saved once the transfer starts. The feature
> +    * %TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE must already be prepared for this
> +    * property to have a meaningful value, and to receive change notification.
> +    * Once the initial value is set, this can no be changed.
> 
> no be changed!

I guess I didn't check these strings myself enough. Fixed.

> +    * For outgoing, this property may be set to the location of the file being
> +    * sent. The feature %TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE does not have
> +    * to be prepared and there is no change notification.
> 
> Both paragraphs here are ambiguous. “may be set”, but it's a read-only
> property.  How does one set it? Who sets it? etc.

I updated these docs, what do you think now?

>    /**
> +   * TpFileTransferChannel:state
> +   *
> +   * A TpFileTransferState holding the state of the file transfer.
> +   *
> +   * Since 0.15.UNRELEASED
> +   */
> 
> Are you missing “The %TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE feature has to be
> prepared for this property to be meaningful and kept up to date.” here?

Yes. Not anymore.

> +  /**
> +   * TpFileTransferChannel:service-name:
> +   *
> +   * A string representing the service name that will be used over
> +   * this file transfer channel or %NULL.
> 
> This doesn't really help someone reading the documentation understand what this
> property is for.

Okay updated, and with an example, better?

> +   * Additional information about the file transfer set by the channel
> +   * initiator or %NULL.
> 
> You need a comma, and maybe “, or %NULL if the initiator did not provide any
> additional information”. Would it be kinder to make it an empty hash table in
> that case?

Yes, done.

> +  else if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_PENDING))
> +    {
> +      /* The connection is pending */
> +      GSource *source;
> +
> +      source = g_socket_create_source (self->priv->client_socket, G_IO_OUT,
> +          NULL);
> +
> +      g_source_attach (source, g_main_context_get_thread_default ());
> +      g_source_set_callback (source, (GSourceFunc) client_socket_cb, self,
> +          NULL);
> 
> I assume this was taken verbatim from Empathy and works? :)

Morten actually took it from TpStreamTubeChannel.

> +/**
> + * tp_file_transfer_channel_accept_file_finish:
> + * @self: a #TpFileTransferChannel
> + * @result: a #GAsyncResult
> + * @error: a #GError to fill
> + *
> + * Finishes a file transfer accept operation.
> 
> “Finishes a call to tp_file_transfer_channel_accept_file_async()”?

Okay, and provide_file_finish too.

> +gboolean
> +tp_file_transfer_channel_accept_file_finish (TpFileTransferChannel *self,
> +    GAsyncResult *result,
> +    GError **error)
> +{
> +  DEBUG ("enter");
> 
> rly.
> 
> ditto provide_file_finish.

Removed.

(In reply to comment #7)
> (In reply to comment #3)
> > It doesn't do any hashing at all. I spoke to Guillaume about this and it really
> > needs to be another utility class unfortunately. This only really implements
> > {Offer,Provide}File.
>
> I'd like to have a rough idea of how this API will look like, just to be sure
> that yours will work with it.
> Especially, we have to be sure that we provide any info/state changes to feed
> to Empathy's file transfer dialog.

Let's say it's called TpFTHelper for now. Perhaps something like this:

TpFTHelper * tp_ft_helper_new_outgoing (
    TpAccount *);

void tp_ft_helper_send_file_async (
    TpFTHelper *,
    GFile *source,
    GHashTable *asv,
    GAsyncReadyCallback,
    gpointer);
gboolean tp_ft_helper_send_file_finish (
    TpFTHelper *,
    GAsyncResult *,
    GError **);

GObject properties:

 guint64: transferred-bytes (including change notification)
 TpFTHelperState: state (including change notification)

send_file should do this:
 1. Hash the GFile;
 2. request the channel with additional properties given in the asv
    argument (like Metadata.ServiceName, Metadata.Metadata);
 3. call provide_file at the right time;
 4. call the callback once the transfer is complete.

Empathy's file transfer window only needs to know a few things:

 1. When the file gets accepted (this is currently not done by a state
    change but when the transferred bytes property "changes" to zero).
 2. When hashing has started and when hashing has stopped.
 2. When bytes are sent. notify::transferred-bytes is fine for this.
 3. When the transfer is complete. notify::state is fine for this too.

This feels a lot like EmpathyFTHandler.

(I have actually already ported Empathy to use the new TpFTChannel
methods; see https://bugzilla.gnome.org/show_bug.cgi?id=663682)

TpFTHelper * tp_ft_helper_new_incoming (
    TpFileTransferChannel *);

void tp_ft_helper_receive_file_async (
    TpFTHelper *,
    GFile *destination,
    GAsyncReadyCallback, gpointer);
gboolean tp_ft_helper_receive_file_finish (
    TpFTHelper *,
    GAsyncResult *,
    GError **);

receive_file should do this:
 * Call accept_file on the TpFTChannel;
 * hash the file either on the way in (ideally), or hash the file
   after the transfer is complete (less cool);
 * call the callback when the transfer is complete.

The same properties and change notification can work for incoming too.

The _new_* functions could take the GFile instead of waiting and
giving it to send/receive?

What do you think?

> Also, like I said in Comment 2, hashing of the incoming file should be done by
> the FT chan, but that shouldn't be a problem to add internal API to pass the
> hashing to the higher level object I guess.

Yeah I don't think this'll be a big problem.

+ * <informalexample><programlisting>GHashTable *request = tp_asv_new (

Do you know about the |[ ... ]| shortcut for <informalexample><programlisting> ... <//>?

+ * Once a #TpFileTransferChannel is created as a proxy to the channel
+ * on D-Bus, the "notify::state" GObject signal should be monitored
+ * and when at %TP_FILE_TRANSFER_STATE_ACCEPTED
  * tp_file_transfer_channel_provide_file_async() should be called.

I don't think this flows very well. “The "notify::state" GObject signals on the resulting channel should be monitored; when the channel moves to state  %TP_FILE_TRANSFER_STATE_ACCEPTED, tp_file_transfer_channel_provide_file_async() should be called.” maybe?

+   * <informalexample><programlisting>tp_base_client_take_handler_filter (handler, tp_asv_new (
+   *               TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING, TP_IFACE_CHANNEL_TYPE_FILE_TRANSFER,
+   *               TP_PROP_CHANNEL_TARGET_HANDLE_TYPE, G_TYPE_UINT, TP_HANDLE_TYPE_CONTACT,
+   *                TP_PROP_CHANNEL_REQUESTED, G_TYPE_BOOLEAN, FALSE,
+   *               TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA_SERVICE_NAME, G_TYPE_STRING, "service.name",
+   *               NULL));
+   * </programlisting></informalexample>

REQUESTED has an extra space of indentation.

I forgot to say before: I think the documentation for TpFileTransferChannel:metadata should say what type its keys and values have, like the getter does.

I've just noticed that lots of the gtk-doc comments for the various new properties don't have colons at the end of their names:

  /**
   * TpFileTransferChannel:date

I wonder if they work regardless?

(In reply to comment #10)
> (In reply to comment #8)
> > Both paragraphs here are ambiguous. “may be set”, but it's a read-only
> > property.  How does one set it? Who sets it? etc.
> 
> I updated these docs, what do you think now?

Clearer!

> > +  /**
> > +   * TpFileTransferChannel:service-name:
> > +   *
> > +   * A string representing the service name that will be used over
> > +   * this file transfer channel or %NULL.
> > 
> > This doesn't really help someone reading the documentation understand what this
> > property is for.
> 
> Okay updated, and with an example, better?

Yup.

(In reply to comment #12)
> + * <informalexample><programlisting>GHashTable *request = tp_asv_new (
> 
> Do you know about the |[ ... ]| shortcut for <informalexample><programlisting>
> ... <//>?

No? I tried replacing them and it didn't work and can't find an example of them in use in tp-glib already?

> + * Once a #TpFileTransferChannel is created as a proxy to the channel
> + * on D-Bus, the "notify::state" GObject signal should be monitored
> + * and when at %TP_FILE_TRANSFER_STATE_ACCEPTED
>   * tp_file_transfer_channel_provide_file_async() should be called.
> 
> I don't think this flows very well. “The "notify::state" GObject signals on the
> resulting channel should be monitored; when the channel moves to state 
> %TP_FILE_TRANSFER_STATE_ACCEPTED, tp_file_transfer_channel_provide_file_async()
> should be called.” maybe?

OK.

> +   * <informalexample><programlisting>tp_base_client_take_handler_filter
> (handler, tp_asv_new (
> +   *               TP_PROP_CHANNEL_CHANNEL_TYPE, G_TYPE_STRING,
> TP_IFACE_CHANNEL_TYPE_FILE_TRANSFER,
> +   *               TP_PROP_CHANNEL_TARGET_HANDLE_TYPE, G_TYPE_UINT,
> TP_HANDLE_TYPE_CONTACT,
> +   *                TP_PROP_CHANNEL_REQUESTED, G_TYPE_BOOLEAN, FALSE,
> +   *              
> TP_PROP_CHANNEL_INTERFACE_FILE_TRANSFER_METADATA_SERVICE_NAME, G_TYPE_STRING,
> "service.name",
> +   *               NULL));
> +   * </programlisting></informalexample>
> 
> REQUESTED has an extra space of indentation.

Fixed.

> I forgot to say before: I think the documentation for
> TpFileTransferChannel:metadata should say what type its keys and values have,
> like the getter does.

Is the gtk-doc type string not enough?

      "metadata"                 GHashTable_gchararray+GStrv_*  : Read

> I've just noticed that lots of the gtk-doc comments for the various new
> properties don't have colons at the end of their names:

Yeah I saw that and forgot to add another commit adding them, but also didn't think it was such a big deal as the original properties don't have them either.

>   /**
>    * TpFileTransferChannel:date
> 
> I wonder if they work regardless?

Yeaaahhh they do. I'll fix it anyway.

(In reply to comment #11)
> Let's say it's called TpFTHelper for now. Perhaps something like this:

Looks good to me, but it should be called TpFileTransferHelper to stay coherent with the channel class name.

Open a new bug for this?

(In reply to comment #14)
> (In reply to comment #11)
> > Let's say it's called TpFTHelper for now. Perhaps something like this:
> 
> Looks good to me, but it should be called TpFileTransferHelper to stay coherent
> with the channel class name.

Heh, yes I was only using ft here for its length.

> Open a new bug for this?

I opened bug #42909.

(In reply to comment #13)
> (In reply to comment #12)
> > + * <informalexample><programlisting>GHashTable *request = tp_asv_new (
> > 
> > Do you know about the |[ ... ]| shortcut for <informalexample><programlisting>
> > ... <//>?
> 
> No? I tried replacing them and it didn't work and can't find an example of them
> in use in tp-glib already?

http://cgit.freedesktop.org/telepathy/telepathy-glib/tree/telepathy-glib/account-manager.c#n1012 is the first of 26 examples found by `git grep '|\[' telepathy-glib`. :)

> > I forgot to say before: I think the documentation for
> > TpFileTransferChannel:metadata should say what type its keys and values have,
> > like the getter does.
> 
> Is the gtk-doc type string not enough?
> 
>       "metadata"                 GHashTable_gchararray+GStrv_*  : Read

you gee ell why you don't got no alibi.

Maybe there could be an example of how to provide metadata when requesting a channel?

(In reply to comment #16)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > + * <informalexample><programlisting>GHashTable *request = tp_asv_new (
> > > 
> > > Do you know about the |[ ... ]| shortcut for <informalexample><programlisting>
> > > ... <//>?
> > 
> > No? I tried replacing them and it didn't work and can't find an example of them
> > in use in tp-glib already?
> 
> http://cgit.freedesktop.org/telepathy/telepathy-glib/tree/telepathy-glib/account-manager.c#n1012
> is the first of 26 examples found by `git grep '|\[' telepathy-glib`. :)

Ohhhhhhhh, I read |[ ... ]| as [[ ... ]]. I did this.

> > > I forgot to say before: I think the documentation for
> > > TpFileTransferChannel:metadata should say what type its keys and values have,
> > > like the getter does.
> > 
> > Is the gtk-doc type string not enough?
> > 
> >       "metadata"                 GHashTable_gchararray+GStrv_*  : Read
> 
> you gee ell why you don't got no alibi.
> 
> Maybe there could be an example of how to provide metadata when requesting a
> channel?

Okay. Done.

OMG MERGED LOL

There is a use-case not covered by this:

For incoming file transfers, the approver could display a "save as" button and set the URI property, then give to handler. In that case handler does not have to give a GFile in accept_file_async() and we need an API for the approver to set the URI but not start the transfer.

I suggest to add something like this in the beginning of accept_file_async():

  if (file != NULL && self->priv->file != NULL)
    g_return_if_fail (g_file_equal (file, self->priv->file));
  if (file == NULL)
    file = self->priv->file;

And file_replace_async_cb() should be modified to not set URI if we are using the URI already set.

We probably also need a convenient _set_uri_async() method that approver can call.

So IMO the merged code is already a good step forward, but I'm reopening to keep track of this last use-case to handle.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/68.