[Bug 39188] TpFileTransferChannel: add API to send and receive the file

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=39188

--- Comment #10 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2011-11-10 04:52:35 PST ---
(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.

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