Comments after a first (very) fast review:

• Does the nautilus-sendto plugin still work with your branch? If it doesn't, fix it
• remove all trailing spaces

empathy-ui-utils

• empathy_receive_file_with_file_chooser: is ui-utils really the right place for such function?
• file_manager_receive_file_response_cb: you unref the handler but it's not clear when it has been reffed. Add a comment?

empathy-utils.h
• Shouldn't we define these errors in tp-file.h ?

empathy-event-manager.c
• event_manager_approve_channel_cb: the EmpathyTpContactFactory is not unreffed

empathy-ft-manager
• ft_manager_format_progress_bytes_and_percentage, ft_manager_format_error_message  and ft_manager_format_contact_info should return a gchar *

empathy-tp-file
• remove tabs
• ft_operation_close_clean: no need of the else as you early return
•   if (priv->op_callback)   we usually use (if ptr != NULL)
• file_read_async_cb: last line is too long
• As you rewrote most of this file, it would be good to fix it to use tp-style, see http://telepathy.freedesktop.org/wiki/Style  Basically you just have to change function definitions and declarations

empathy-ft-handler
• wrap > 80 lines
• need API documentation
• use tp-style
• ft_handler_populate_outgoing_request and hash_job_done: you can use new tp_gvalue_slice_new_* helper functions
• use g_slice to allocate hash_data->buffer
• channel_get_all_properties_cb: the contact factory is not unreffed

empathy-ft-factory
• same comments about long lines, API doc and tp-style

I fixed all the other comments you raised, except:

> • Does the nautilus-sendto plugin still work with your branch? If it doesn't,
> fix it

You're right and I am working on it. Using the new machinery from outside Empathy has exposed another bug in the empathy code, so I will need some more debug to get it work.

The nautilus-sendto ported branch can be found here [1]. Note that it's a bit strange that we need an EmpathyDispatcher object sitting around to actually use the FT from outside Empathy...I am currently talking with Sjoerd to come up with a nicer solution, but in the meanwhile, this works just fine.

[1] http://git.collabora.co.uk/?p=user/cosimoc/nautilus-sendto.git;a=shortlog;h=refs/heads/ft_rework

What do you mean that you need an EmpathyDispatcher around? I thought I fixed the refcounting bug that the dispatcher had so you could just call empathy_dispatcher_send_file and it'll ref and unref itself appropriately?

I don't mean to review your nst branch here, but if there is a failure then it just quits. Is that because the Empathy UI will give a nice error message?

(get_selected_contact should just use empathy_contact_selector_dup_contact but I suppose that's an unrelated issue.)

I forgot to finish off that last comment before sending it.

If you need to ref the dispatcher then perhaps the ft factory needs to ref the dispatcher. I suppose if you're talking to Sjoerd about it then I'll let you two get at it.

Also, regarding your ft_rework branch itself, could you gtk-doc it please?

(In reply to comment #6)
> Also, regarding your ft_rework branch itself, could you gtk-doc it please?

Done, I wrote the documentation for the new objects.

Please update or rebase your branch on top of master and be sure that "make
check" works; it should perform some coding style checks.

Ok, I fixed the warnings brought up by the script.

(In reply to comment #3)
> The nautilus-sendto ported branch can be found here [1]. Note that it's a bit
> strange that we need an EmpathyDispatcher object sitting around to actually use
> the FT from outside Empathy...I am currently talking with Sjoerd to come up
> with a nicer solution, but in the meanwhile, this works just fine.

I added an empathy_dispatcher_find_channel_class_async() to EmpathyDispatcher that fixes this, and updated my nautilus-sendto branch to get rid of the dispatcher object, now it works as expected.

76813a57ea207aa12ec694ba5a1c415a4f29f5ec
Why did you add :
+      <xi:include href="xml/api-index-deprecated.xml"/>
+      <xi:include href="xml/api-index-full.xml"/>
Should probably have be done in a separated commit

I'm not convinced by making the "use checksum" optionnal. That seems a technical detail that shouldn't be exposed to users to me.
Furthermore most users won't activate it (as it's disabled by default) and those we do will be annoyed by it only the day when they'll try to send a big file (in which case they'll probably cancel it and retry with checksum disabled).

What are the cons to always enable it? I guess main reason is the possible slowdown if the file is big and/or on a remote location, right?

What would you think about this approach:
- Alice sends a (big) file to Bob
- Alice's FT manager displays "calculating checksum" (with a percentage displayed if possible)
- If Alice doesn't want to wait, she hits a "skip this step" button and file is transfered without using the checksum
- When Bob receives the file (we assume the checksum has been sent here), his FT manager displays "Checking file" (with a percentage too).
- Bob can also skips this step if he wants to.
- Once the checksum has been checked the FT manager displays that the FT has been completed (or an error if the checksum is wrong).

Comments and suggestions welcome.

I like Guillaume's idea of a "skip this step button". Maybe we should make some horrible icon or emblem appear when the user skips it though, to dissuade him or her from skipping it?

So, you click "skip" and it says something like "sending but recipient cannot confirm same file" (or something like that) under the transfer. I'm not sure whether the recipient should be bothered with details of why he can't confirm using a checksum though.

And if you skip checking on receiving have some similar emblem to be like "wowzer cannot confirm".

* Don't use slices for buffers of 4096 bytes
* when freeing misc data structures, no need to set all their fields to NULL 
  when freeing the structure afterwards.
* no need to check if data is NULL when calling g_free
* no need to check data is NULL before calling g_list_free
* +find_channel_class_idle_cb -> empathy_dispatcher_find_channel_class return 
  NULL is valid, not an indication things aren't ready
* use if (p == NULL) instead of if (p)
* when disposing the dispatcher, cancel all the timeouts
* FtHandler -> properites of the transport aren't GObject properites ?*
* TpFile -> Use constructed not constructor
* Hashing the incoming file should ideally be done while receiving, not 
  afterwards (but this can be improved in a seperate branch, not a blocker)
* Option for hashing is unnecessary
* No eta when hashing (not a blocker, different branch/bug)
* When pressing stop in the UI it indicates the FT as failing (broken pipe)?

There is no reason to disable or skip the checksumming. The time checksumming takes is not significant enough to clutter the UI with buttons the user doesn't understand the consequences of anyway

I fixed all the issues you raised in your review;
I also added a function to EmpathyDispatcher which allows us to enable/disable checksum according to the CM capability (so that we don't do useless hashing if I can't request a channel with ContentHashType set).
This made the removal of the hashing option nicer, as we are now smart about when doing the hashing.

I will fix the other two points after merging, in separate branches.

By the way, I don't get the broken pipe error (neither with Salut nor with Gabble) when stopping the FT while it runs.

This is now merged in git master, closing.