Created attachment 59185 [details] [review]
the patch itself 

apply with -p1 on gnomebt base dir

Created attachment 59186 [details]
screenshot 1

gnome-obex-send progress dialog

Created attachment 59187 [details]
screenshot  2

basic error dialog (remote device can not receive files)

Created attachment 59188 [details]
screenshot 3

error during transfer (remote device cancelled transfer)

Forgot to mention that this patch wont work unless libbtctl is patched too (patch available at #325016).

short changelog:

* obex/gnome-obex-send.c: show overall progress, time remaining, handle all error situations correctly
* obex/chooser.c: replace choose_bdaddr() function with choose_bd() which returns both device address and it's name
* obex/chooser.h: -
* src/chooser.gob: new get_bd() function, use GTK_WIN_POS_CENTER window position
* src/controller.gob: add **err parameter to many functions (needs patch for libbtctl at #325016 applied)
* src/gconftest.c: reflect changes in controller.gob
* src/gnomebt-controller-test.c: reflect changes in controller.gob plus several improvements
* src/gnomebt-controller.override: reflect changes in controller.gob
* ui/gnome-obex-send.glade: make dialog similar to nautilus progress dialog
* configure.in: update gtk+ dep to 2.6

Created attachment 62494 [details] [review]
fixed patch

Removed configure.in part of patch since GTK+ dep is already fixed in cvs.

> * obex/chooser.c: replace choose_bdaddr() function with choose_bd() which
> returns both device address and it's name

Why not return the GnomebtDeviceDesc, and get the bdaddr and name from that instead?

> +static gchar **filesToSend = NULL;
> +static guint currentFile = 0;
> +static struct stat fileStat;
> +static guint totalBytes = 0;
> +static guint bytesSent = 0;
> +static guint totalFiles = 0;
> +static gint64 firstTransferTime = 0;
> +static gint64 lastUpdateTime = 0;

Don't use C++-style variable names, use C-style instead "totalBytes" becomes "total_bytes". filesToSend should be a GList instead of an array.

Why did you remove "MyApp *app" everywhere in the code, and use global instead?

> +       if (*err) {
> +               g_warning("gnomebt_controller_channels_for_service() failed: %s (%s)",
> +                       (*err)->message, errno ? g_strerror(errno): "");

Please use "No Reason" instead of an empty string for error reporting.

> -       if (channel < 1) {
> -               if (is_palm_device (bdaddrstr)) {
> -                       report_error (_("Make sure \"Beam receive\" is enabled in the Power preferences, and Bluetooth is enabled."));

Why remove the Palm special-casing?

> +                               if (!g_thread_create((GThreadFunc)establish_connection, NULL, FALSE, NULL)) {
> +                                       g_warning("Failed to create thread. Continuing synchroniously.");

The chances of a thread not being created properly are slim, don't fallback to sync, simply assert. (should be "synchronously" btw)
It would also make the code much clearer in terms of possible threading problems.

> +                               handle_file_sending_error(g_strdup_printf(_("Could not send file '%s'"), fname), "");

You're leaking memory from the strdup here.

Finally, are there any good reasons to use GTK+ in the threads at all? Why not only do the actual transfer in the threads, and push the transfer details updates using GAsyncQueues instead?

I also forgot "bluetooth" should be "Bluetooth" in the user-visible strings.

Thanks for detailed comments. My arguments:

> Why not return the GnomebtDeviceDesc, and get the bdaddr and name from that
> instead?

Because in the end of choose_bd() function btchooser and btctl objects are freed. And I don't know how would I duplicate that GnomebtDeviceDesc structure (fix me if I'm wrong at this point :D)

> Don't use C++-style variable names, use C-style instead "totalBytes" becomes
> "total_bytes". filesToSend should be a GList instead of an array.

Sorry, bad habbits.. Will be fixed.

> Why did you remove "MyApp *app" everywhere in the code, and use global instead?

I don't see any reason why app should be passed to all functions.. Therefore some problems arise when using threads (how would you pass that *app to function when using g_thread_create()?)

> Please use "No Reason" instead of an empty string for error reporting.

Will be fixed.

> Why remove the Palm special-casing?

I think that current error messages are quiet informative for any kind of device..

> The chances of a thread not being created properly are slim, don't fallback
> to sync, simply assert.

Will be fixed.

> You're leaking memory from the strdup here.

Will be fixed as well.

> Why not only do the actual transfer in the threads, and push the transfer 
> details updates using GAsyncQueues instead?

Sounds convincing :D I'm just not that experienced in GTK+ world yet :) To put it another way, I didn't spot all this GAsyncQueues stuff when browsing docs. 
Finally, is it worth rewriting everything once again while current implementation fully works.

(In reply to comment #10)
> Thanks for detailed comments. My arguments:
> 
> > Why not return the GnomebtDeviceDesc, and get the bdaddr and name from that
> > instead?
> 
> Because in the end of choose_bd() function btchooser and btctl objects are
> freed. And I don't know how would I duplicate that GnomebtDeviceDesc structure
> (fix me if I'm wrong at this point :D)

device = g_new0 (GnomebtDeviceDesc, 1);
*device = *new_device;
device->name = g_strdup (olddev->name);
device->bdaddr = g_strdup (olddev->bdaddr);

> > Why did you remove "MyApp *app" everywhere in the code, and use global instead?
> 
> I don't see any reason why app should be passed to all functions.. Therefore
> some problems arise when using threads (how would you pass that *app to
> function when using g_thread_create()?)

Because that's bad habits. No global variables.

From the g_thread_create() API docs:
data : 	an argument to supply to the new thread.

> > Why remove the Palm special-casing?
> 
> I think that current error messages are quiet informative for any kind of
> device..

They're not informative enough when using Palm devices as the recipient, which was the reason why this message got added in the first place. It should be called when we couldn't find the service port from a Palm device.

> > Why not only do the actual transfer in the threads, and push the transfer 
> > details updates using GAsyncQueues instead?
> 
> Sounds convincing :D I'm just not that experienced in GTK+ world yet :) To put
> it another way, I didn't spot all this GAsyncQueues stuff when browsing docs. 
> Finally, is it worth rewriting everything once again while current
> implementation fully works.

Mainly because right now, I can't follow the code you've written and be sure there wouldn't be any deadlocks, leaks, or races. You wouldn't need a full rewrite, especially given that you only bring gnome-obex-send.c to 800 lines, not 8000...

OK. Everything is clear now. I'll try to redo the patch when I have some free time.

Obsoleted by bluetooth-send.