GNOME Bugzilla – Bug 633361
Add methods for OBEX to applet library
Last modified: 2010-10-29 18:26:24 UTC
As discussed on bug 618312
Created attachment 173402 [details] [review] Add methods for OBEX in applet library Add "browse_address" and "send_to_address" methods to the applet library, that operate on devices and open the relevant UI.
Review of attachment 173402 [details] [review]: A couple of small problems, but going in the right direction. ::: .gitignore @@ +44,3 @@ +po/insert-header.sin +po/quot.sed +po/remove-potcdate.sin Separate patch for the .gitignore please. ::: applet/bluetooth-applet.c @@ +115,3 @@ + GFile *file = G_FILE (object); + + if (g_file_mount_enclosing_volume_finish (file, result, NULL)) {; We need a way to get feedback to the user, this completely drops the error message. @@ +138,3 @@ + g_return_if_fail (address != NULL); + + uri = g_strdup_printf ("obex://%s/", address); That should be obex://[%s]/ I believe. @@ +164,3 @@ + g_return_if_fail (BLUETOOTH_IS_APPLET (self)); + + command = g_strdup_printf ("bluetooth-sendto --device=%s --name=%s", address, alias); That's probably missing some shell escaping.
Created attachment 173435 [details] [review] Add methods for OBEX in applet library Add "browse_address" and "send_to_address" methods to the applet library, that operate on devices and open the relevant UI.
Created attachment 173436 [details] [review] Update .gitignore
Review of attachment 173435 [details] [review]: ::: applet/bluetooth-applet.c @@ +121,3 @@ + } + + if (error) { You need to ignore the "already mounted" error. This same code is already in the main.c @@ +123,3 @@ + if (error) { + /* There is not much we can do beyond logging, we cannot show + dialogs to the user (we're inside a window manager) */ That's not true though. See Ray's work on system dialogues. You should probably be using a GIO style async function here, so that the feedback would be made available to the caller. @@ +177,3 @@ + quoted_address = g_shell_quote (address); + quoted_alias = g_shell_quote (alias); + command = g_strdup_printf ("bluetooth-sendto --device=%s --name=%s", address, alias); You still print with address and alias :)
Review of attachment 173436 [details] [review]: Looks good!
(In reply to comment #5) > Review of attachment 173435 [details] [review]: > > ::: applet/bluetooth-applet.c > @@ +121,3 @@ > + } > + > + if (error) { > > You need to ignore the "already mounted" error. This same code is already in > the main.c Ok > > @@ +123,3 @@ > + if (error) { > + /* There is not much we can do beyond logging, we cannot show > + dialogs to the user (we're inside a window manager) */ > > That's not true though. See Ray's work on system dialogues. Except we don't want a system modal dialog for this, do we? System dialogues are preemptive and exclusive, they are "handle with care". (Actually, even a standard modeless GtkMessageDialog would be wrong from an usab ility point of view). > You should probably be using a GIO style async function here, so that the > feedback would be made available to the caller. Ok > > @@ +177,3 @@ > + quoted_address = g_shell_quote (address); > + quoted_alias = g_shell_quote (alias); > + command = g_strdup_printf ("bluetooth-sendto --device=%s --name=%s", > address, alias); > > You still print with address and alias :) D'oh!
(In reply to comment #7) > (In reply to comment #5) > > That's not true though. See Ray's work on system dialogues. > > Except we don't want a system modal dialog for this, do we? > System dialogues are preemptive and exclusive, they are "handle with care". > (Actually, even a standard modeless GtkMessageDialog would be wrong from an > usab ility point of view). Agreed there. But some sort of feedback will need to be presented to the user anyway. I'd expect a similar feedback to that when trying to mount places from the overview mode. In any case, the legacy applet will need to show this feedback. How it's shown in the shell is another point of discussion (note the "how", not the "if").
Created attachment 173491 [details] [review] Complete applet library. Add "browse_address" and "send_to_address" methods to the applet library, that operate on devices and open the relevant UI, and add some useful enums to introspection, so that we don't need to load GnomeBluetooth.
Review of attachment 173491 [details] [review]: Separate the Makefile-lib.am changes into another patch, and it looks good to commit.
Created attachment 173493 [details] [review] Complete applet library. Add "browse_address" and "send_to_address" methods to the applet library, that operate on devices and open the relevant UI, and add some useful enums to introspection, so that we don't need to load GnomeBluetooth. Also, install the library in $prefix/lib/gnome-bluetooth, as gnome-bluetooth needs to be installed before gnome-shell, not the other way round.
Review of attachment 173493 [details] [review]: Looks good!
(In reply to comment #10) > Review of attachment 173491 [details] [review]: > > Separate the Makefile-lib.am changes into another patch, and it looks good to > commit. (In reply to comment #13) > Review of attachment 173493 [details] [review]: > > Looks good! ? Good as it stands, or two separate commits?
I would prefer one commit for the enums addition, and one for the rest, but you can commit as is if you want.