We should have a TpFileTransferChannel (or TpFTChannel?) object implementing basically the same features we currently have in EmpathyTpFile and EmpathyFTHandler. This could be used by nautilus-sendto as I think they plan to implement FT itself at some point.
As per our discussion, here's an attempt at a first API draft: Properties and accessors ------------------------ TpFTState *tp_ft_channel_get_state (TpFTChannel *self); const gchar *tp_ft_channel_get_content_type (TpFTChannel *self); const gchar *tp_ft_channel_get_filename (TpFTChannel *self); guint64 *tp_ft_channel_get_size (TpFTChannel *self); TpFTHashType tp_ft_channel_get_content_hash_type (TpFTChannel *self); const gchar *tp_ft_channel_get_content_hash (TpFTChannel *self); const gchar *tp_ft_channel_get_description (TpFTChannel *self); gint64 *tp_ft_channel_get_date (TpFTChannel *self); guint64 *tp_ft_channel_get_transferred_bytes (TpFTChannel *self); guint64 *tp_ft_channel_get_initial_offset (TpFTChannel *self); const gchar *tp_ft_channel_get_uri (TpFTChannel *self); Methods ------- TpFTChannel tp_ft_channel_new (TpConnection *conn, const gchar *object_path, const GHashTable *immutable_properties, GError **error); void tp_ft_channel_accept_async (TpFTChannel *self, guint64 offset, GFile *file, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); void tp_ft_channel_accept_finish (TpFTChannel *self, GAsyncResult *result, GError **error); void tp_ft_channel_offer_async (TpFTChannel *self, GFile *file, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); void tp_ft_channel_offer_finish (TpFTChannel *self, GAsyncResult *result, GError **error); Signals ------- "transferred-bytes-changed" (guint64 count); "uri-defined" (const gchar uri);
(In reply to comment #1) > As per our discussion, here's an attempt at a first API draft: I think we should name this TpFileTransferChannel. > Properties and accessors > ------------------------ > > TpFTState *tp_ft_channel_get_state (TpFTChannel *self); That's TpFileTransferState. Do we really want to expose this to the user? If we succeed send/receive operation when the sending is done, can't we just use the GError of the _finish() function to tell user about failures? > TpFTHashType tp_ft_channel_get_content_hash_type (TpFTChannel *self); > const gchar *tp_ft_channel_get_content_hash (TpFTChannel *self); Humm maybe we shouldn't expose that and let tp-glib does the hashing and checking? > gint64 *tp_ft_channel_get_date (TpFTChannel *self); Maybe we could use a GDateTime instead? > guint64 *tp_ft_channel_get_initial_offset (TpFTChannel *self); Does clients care as tp-glib will do the streaming of the file? > Methods > ------- > > void tp_ft_channel_accept_async (TpFTChannel *self, > guint64 offset, > GFile *file, > GCancellable *cancellable, > GAsyncReadyCallback callback, > gpointer user_data); I'd swap the offset and file args. > void tp_ft_channel_accept_finish (TpFTChannel *self, > GAsyncResult *result, > GError **error); > > void tp_ft_channel_offer_async (TpFTChannel *self, > GFile *file, > GCancellable *cancellable, > GAsyncReadyCallback callback, > gpointer user_data); > > void tp_ft_channel_offer_finish (TpFTChannel *self, > GAsyncResult *result, > GError **error); Do we really need a GCancellable? User can just close the channel to stop sending/receiving. And once the operation has been cancelled we can't do much with the channel anyway. When are these operations succeeding? As soon as the D-Bus call returns or when the sending/receiving is done? > Signals > ------- > > "uri-defined" (const gchar uri); No need for that, we can use "notify:uri".
(In reply to comment #1) > TpFTState *tp_ft_channel_get_state (TpFTChannel *self); > const gchar *tp_ft_channel_get_content_type (TpFTChannel *self); > const gchar *tp_ft_channel_get_filename (TpFTChannel *self); > guint64 *tp_ft_channel_get_size (TpFTChannel *self); > TpFTHashType tp_ft_channel_get_content_hash_type (TpFTChannel *self); > const gchar *tp_ft_channel_get_content_hash (TpFTChannel *self); > const gchar *tp_ft_channel_get_description (TpFTChannel *self); > gint64 *tp_ft_channel_get_date (TpFTChannel *self); > guint64 *tp_ft_channel_get_transferred_bytes (TpFTChannel *self); > guint64 *tp_ft_channel_get_initial_offset (TpFTChannel *self); > const gchar *tp_ft_channel_get_uri (TpFTChannel *self); state, content-type, filename, size, description, date, transferred-bytes and uri should all be GObject properties. I'm not sure it's useful to expose initial-offset, content-hash or content-hash-type, these are all internal to the channel (the channel should deal with the initial offset, and also confirm the hash for us -- we need a mechanism to know the hash is bad though). I think we should also add a "transferred-percentage" property which is a double ranging from [0,1], this allows programmers to create a GBinding to bind the file transfer channel straight to a GtkProgressBar. > Signals > ------- > > "transferred-bytes-changed" (guint64 count); Just use notify::transferred-bytes -- this also lets users bind the transferred bytes to a GtkLabel via a transform function to pretty print it (I'd say we should provide that function, but tp-glib doesn't provide i18n, we should get something for it in GLib perhaps). > "uri-defined" (const gchar uri); Just use notify::uri -- as cassidy has already said.
(In reply to comment #2) > > gint64 *tp_ft_channel_get_date (TpFTChannel *self); > > Maybe we could use a GDateTime instead? GDateTimes require memory management (unreffing etc). Perhaps it's better to return a gint64 and let the user create a GDateTime if it's useful to her? Or we could have both accessors. > > void tp_ft_channel_accept_async (TpFTChannel *self, > > guint64 offset, > > GFile *file, > > GCancellable *cancellable, > > GAsyncReadyCallback callback, > > gpointer user_data); > > I'd swap the offset and file args. > Do we really need a GCancellable? User can just close the channel to stop > sending/receiving. And once the operation has been cancelled we can't do much > with the channel anyway. Agreed. > When are these operations succeeding? As soon as the D-Bus call returns or > when the sending/receiving is done? I was wondering the same thing. Do these return once the FT is set up (in which case you need to monitor the state) or once the transfer is complete (in which case we don't need to expose the state, we can just return whatever we want in the GError).
(In reply to comment #2) > > TpFTState *tp_ft_channel_get_state (TpFTChannel *self); > > That's TpFileTransferState. > > Do we really want to expose this to the user? If we succeed send/receive > operation when the sending is done, can't we just use the GError of the > _finish() function to tell user about failures? Actually, I've thought about this, and we do want to expose state, it's useful in the UI, so we can do strings like "Waiting for the recipient to accept the file".
(In reply to comment #3) > (In reply to comment #1) > state, content-type, filename, size, description, date, transferred-bytes and > uri should all be GObject properties. Agreed. > I'm not sure it's useful to expose initial-offset, content-hash or > content-hash-type, these are all internal to the channel (the channel should > deal with the initial offset, and also confirm the hash for us -- we need a > mechanism to know the hash is bad though). Not sure about that. Sjokkis and I already discussed hashs and the problem is for outgoing channels. You have to compute the hash *before* requesting the channel (as that's an immutable prop) so we can't do it in TpFileTransferChannel as it doesn't exist yet. Having hash being handled by TpFileTransferChannel for incoming channels but not for outgoing ones sound weird to me. So our current plan was to not handle them in TpFileTransferChannel and create later an higher FT helper object which will. This is just the current state of our brainstorming sessions, I'm ready to be convinced that's a bad idea. > I think we should also add a "transferred-percentage" property which is a > double ranging from [0,1], this allows programmers to create a GBinding to bind > the file transfer channel straight to a GtkProgressBar. That's a great idea; I love it! > > Signals > > ------- > > > > "transferred-bytes-changed" (guint64 count); > > Just use notify::transferred-bytes -- this also lets users bind the transferred > bytes to a GtkLabel via a transform function to pretty print it (I'd say we > should provide that function, but tp-glib doesn't provide i18n, we should get > something for it in GLib perhaps). Good idea. (In reply to comment #4) > (In reply to comment #2) > > > gint64 *tp_ft_channel_get_date (TpFTChannel *self); > > > > Maybe we could use a GDateTime instead? > > GDateTimes require memory management (unreffing etc). Perhaps it's better to > return a gint64 and let the user create a GDateTime if it's useful to her? Or > we could have both accessors. I don't get your point here. tp_file_transfer_channel_get_date_time (); will return a (transfer none) GDateTime object so that's pretty convenient to use. > > > void tp_ft_channel_accept_async (TpFTChannel *self, > > > guint64 offset, > > > GFile *file, > > > GCancellable *cancellable, > > > GAsyncReadyCallback callback, > > > gpointer user_data); > > > > I'd swap the offset and file args. agreed. > > When are these operations succeeding? As soon as the D-Bus call returns or > > when the sending/receiving is done? > > I was wondering the same thing. Do these return once the FT is set up (in which > case you need to monitor the state) or once the transfer is complete (in which > case we don't need to expose the state, we can just return whatever we want in > the GError). I'd be tempted for the latter, that seems easier and more convenient to use. (In reply to comment #5) > (In reply to comment #2) > > > > TpFTState *tp_ft_channel_get_state (TpFTChannel *self); > > > > That's TpFileTransferState. > > > > Do we really want to expose this to the user? If we succeed send/receive > > operation when the sending is done, can't we just use the GError of the > > _finish() function to tell user about failures? > > Actually, I've thought about this, and we do want to expose state, it's useful > in the UI, so we can do strings like "Waiting for the recipient to accept the > file". That's a good point, but I'm stil tempted to think that terminating the async operation when the transfer is complete (or failed) is a good idea.
(In reply to comment #3) > I think we should also add a "transferred-percentage" property which is a > double ranging from [0,1], this allows programmers to create a GBinding to bind > the file transfer channel straight to a GtkProgressBar. If it's not [0,100] it's not a percentage. transferred-proportion?
(In reply to comment #7) > If it's not [0,100] it's not a percentage. transferred-proportion? GTK calls the property 'fraction', so 'transferred-fraction' ?
(In reply to comment #6) > Not sure about that. Sjokkis and I already discussed hashs and the problem is > for outgoing channels. You have to compute the hash *before* requesting the > channel (as that's an immutable prop) so we can't do it in > TpFileTransferChannel as it doesn't exist yet. Having hash being handled by > TpFileTransferChannel for incoming channels but not for outgoing ones sound > weird to me. > > So our current plan was to not handle them in TpFileTransferChannel and create > later an higher FT helper object which will. That means they need to be included in the channel-request, not that they need to be exposed as properties. It's already complicated to create the channel-request, you need to look up all the properties from the GFile, so perhaps an extra utility function that creates the channel request from the GFile is required (I think you've been adding other channel-request builders to tp-glib, right?). > > GDateTimes require memory management (unreffing etc). Perhaps it's better to > > return a gint64 and let the user create a GDateTime if it's useful to her? Or > > we could have both accessors. > > I don't get your point here. tp_file_transfer_channel_get_date_time (); will > return a (transfer none) GDateTime object so that's pretty convenient to use. I suppose you could have the object own a GDateTime that gets unreffed on finalize and just return pointers to it. > > Actually, I've thought about this, and we do want to expose state, it's useful > > in the UI, so we can do strings like "Waiting for the recipient to accept the > > file". > > That's a good point, but I'm stil tempted to think that terminating the async > operation when the transfer is complete (or failed) is a good idea. Yeah. It makes it easier for a simple file transfer "did it work or not", and a file transfer observer can then additionally use the state to show extra information. ~ Another thought: the 'uri' property will be automatically set from the GFile, right? We don't need a setter for that.
Do we already have an implementation for this proposal? A WIP branch somewhere?
I need this for adding FT approver in adium themes, so here is a first step. It introspects all properties, but is missing public methods.
So it seems sjokkis actually already started a branch for this too: http://cgit.freedesktop.org/~sjokkis/telepathy-glib/log/?h=fdo%2331376 His branch is more complete, but mine has the advantage of doing the mutable/immutable properties correctly, with a feature to get the mutable properties. My branch also hook into the automatic channel factory. I suggest taking my branch as a base, then copy sjokkis' code for all methods and actions. Sorry about this duplicate work, we really should have assigned this bug and linked the branch, I though this was only API discussion with no actual code :(
I added getters to my branch as well now. What is still missing is methods to AcceptFile, ProvideFile and setURI property. Unit tests are also missing. This can be merged from sjokkis branch.
http://cgit.freedesktop.org/~sjokkis/telepathy-glib/log/?h=fdo%2331376
I've merged sjokkis's commits into my branch now, make check does not pass for some obscure reason though. Again, sorry for this misunderstanding, luckily our branches were close enough to make merging easy, so I've taken the best of both. The bug is now assigned to sjokkis and I let him continue his work based on this.
For the record, here is the merged branch: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=ft
Here are the things that my branch does differently: - Properties that are not immutable must be fetched with a GetAll call, we usually do that with a feature that must be prepared on the channel. The spec was actually missing tags to tell which properties are mutable and which aren't, so I've also fixed the spec for this. - Properties are all read-only, so no need of a set_properties Things my branch adds: - Create TpFileTransferChannel subclass in automatic factory - Give the state reason with the _get_state() getter. - "file" property that gives a GFile instead of URI string, since all operations are using GFile anyway. This is consistent with at least tp_contact_get_avatar_file() where we also decided to wrap the URI into a GFile. - Since it now has a feature to prepare for mutable properties, I've added code to prepare it in unit tests. Review for sjokkis code I've imported: - tp_file_transfer_channel_accept_file_async(): I don't think you set a value to self->priv->address_type and self->priv->access_control - tp_file_transfer_channel_offer_file_async(): I don't think you need to create self->priv->address there, it is given in provide_file_cb() if I understand correctly. Things missing: - Since I changed to spec to allow approver to define the URI where to save incoming file transfer, I don't think _accept_file_async() should take one in args. But the URI could not be defined by approver, so there are still cases where we need to give a file... I think we need a separate _set_uri_async() that will set the URI property, that's needed to tell observers as well. - Unit test does not pass, the channel gets closed when feature is being prepared... I don't know what's going on :( Note: This branch contains not directly related changes like _tp_determine_socket_address_type and _tp_determine_access_control_type that could maybe already be merged?
Actually though about something else that needs clarification: Is the ContentType property really a content type, or a MIME type? It seems to have its importance, see for example g_content_type_from_mime_type();
Since I have to focus on my SoC project in the next few weeks, I'm passing this on to Cassidy. There are some util functions, some moved from stream tube so they can be shared, and some new, that can be merged right away. Those can be merged from this branch: http://cgit.freedesktop.org/~sjokkis/telepathy-glib/log/?h=util Main work is here: http://cgit.freedesktop.org/~sjokkis/telepathy-glib/log/?h=ft-channel
(In reply to comment #19) > There are some util functions, some moved from stream tube so they can be > shared, and some new, that can be merged right away. Those can be merged from > this branch: > > > http://cgit.freedesktop.org/~sjokkis/telepathy-glib/log/?h=util I opened bug #38997 for that.
I took Morten's branch, did some cleanup and kept only the "uncontroversial" bits. It's less useful (no send/receive code yet) but can still be worth merging. Especially, it would make implementing the FT approver in the Shell easier. http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=ft-channel-31376 I'll do some squashing before merging.
+ features[FEAT_CORE].name = TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE; + features[FEAT_CORE].core = TRUE; Does that mean the feature will be prepared automatically at object construction, or just that if we call tp_proxy_prepare_async() that feature will be prepared first? Also you forgot to list it in the automatic factory's dup_features(). + /* Hidden properties */ + GHashTable *available_socket_types; For string properties it is obvious that they are borrowed from immutable_properties table because they are "const gchar *" but you should comment that this GHashTable is borrowed too, was wondering why it is not unreffed in dispose/finalize. Note that you could actually ref it (that's cheap, unlike strings). TpFileTransferChannel:content-type: This is actually a MIME type according to spec. This property name is confusing because in GLib world content-type != mime-type: "A content type is a platform specific string that defines the type of a file. On unix it is a mime type, on win32 it is an extension string like ".doc", ".txt" or a percieved string like "audio"." See for example g_content_type_from_mime_type(). + /** + * TpFileTransferChannel:size + * + * A 64-bit guint holding the size of the file to be transferred. s/A 64-bit guint/A guint64/ ? Should be a
Oops, hit commit by error, continuing... Same comment for TpFileTransferChannel:transferred-bytes Overall I find the doc minimalistic to what the value really means. The tp spec is more useful and I don't think we should rely on reading the spec for each property... Maybe copy/paste a bit of the spec into the gtk-doc would be useful. I think especially about filename property, the spec describe much better what it really means. "filename" could mean an absolute path, etc... $(srcdir)/text-channel.c $(srcdir)/text-channel.h \ + $(srcdir)/file-transfer-channel.c $(srcdir)/file-transfer-channel.h \ Indentation seems wrong Did not look at unit tests yet. C'est tout, pour le moment.
(In reply to comment #22) > + features[FEAT_CORE].name = TP_FILE_TRANSFER_CHANNEL_FEATURE_CORE; > + features[FEAT_CORE].core = TRUE; > > Does that mean the feature will be prepared automatically at object > construction, or just that if we call tp_proxy_prepare_async() that feature > will be prepared first? Both. > Also you forgot to list it in the automatic factory's > dup_features(). I added it to dup_features(). > + /* Hidden properties */ > + GHashTable *available_socket_types; > > For string properties it is obvious that they are borrowed from > immutable_properties table because they are "const gchar *" but you should > comment that this GHashTable is borrowed too, was wondering why it is not > unreffed in dispose/finalize. Note that you could actually ref it (that's > cheap, unlike strings). I added a comment. > TpFileTransferChannel:content-type: > > This is actually a MIME type according to spec. This property name is confusing > because in GLib world content-type != mime-type: "A content type is a platform > specific string that defines the type of a file. On unix it is a mime type, on > win32 it is an extension string like ".doc", ".txt" or a percieved string like > "audio"." See for example g_content_type_from_mime_type(). Good catch; I renamed it to mime-type. > + /** > + * TpFileTransferChannel:size > + * > + * A 64-bit guint holding the size of the file to be transferred. > s/A 64-bit guint/A guint64/ ? Should be a fixed. (In reply to comment #23) > Same comment for TpFileTransferChannel:transferred-bytes fixed. > Overall I find the doc minimalistic to what the value really means. The tp spec > is more useful and I don't think we should rely on reading the spec for each > property... Maybe copy/paste a bit of the spec into the gtk-doc would be > useful. I think especially about filename property, the spec describe much > better what it really means. "filename" could mean an absolute path, etc... I improved the doc. > $(srcdir)/text-channel.c $(srcdir)/text-channel.h \ > + $(srcdir)/file-transfer-channel.c $(srcdir)/file-transfer-channel.h \ > > Indentation seems wrong fixed.
Looks good now, +1
I merged a squashed version of this branch and opened bug #39188 about adding the send/receive API. Will be in 0.15.5.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.