Merge lp:~charlesk/gnome-bluetooth/indicators into lp:~ubuntu-desktop/gnome-bluetooth/ubuntu

[Charles Kerr]

Adds a menubar visibility toggle to 01_use_app_indicator.patch (LP: #829690)

 - Modify the properties panel to add a visibility toggle as spec'ed in
   https://wiki.ubuntu.com/Bluetooth#specification
 - Add a GSettings schema for storing that visibility toggle
 - Modify bluetooth-applet to support the visibility toggle

[lp:~charlesk/gnome-bluetooth/indicators]

62. By Charles Kerr on 2012-02-01

Capitalization fixes to text in the popup menu (LP: #906042)

63. By Charles Kerr on 2012-02-01

replace the three single dots '...' by the unicode symbol U+2026 (…).

[Sebastien Bacher (seb128)]

Thank you for the work, some comment from a first review:

304 ++ const gchar * const * schemas = g_settings_list_schemas ();
305 ++ for (i=0; !schema_exists && schemas && schemas[i]; i++)
306 ++ if (!g_strcmp0 (schema, schemas[i]))
307 ++ schema_exists = TRUE;

what's the point of checking if the schemas exists when it's part of the same source and we know it's being installed? if the schemas is not there the installation is corrupted and we better let the code error out...

286 ++on_settings_visible_changed (GSettings *settings, gchar *key, CcBluetoothPanel * panel)
287 ++{
288 ++ g_return_if_fail (!g_strcmp0 (key, VISIBLE_KEY));

why do you check the key? is the signal connected on another key than visible_key? that seems not required

289 ++
290 ++ GtkToggleButton *toggle = GTK_TOGGLE_BUTTON(panel->priv->indicator_check);
291 ++ gboolean oldval = gtk_toggle_button_get_active (toggle);
292 ++ gboolean newval = g_settings_get_boolean (settings, key);
293 ++ if (oldval != newval)
294 ++ gtk_toggle_button_set_active (toggle, newval);

do we need those checks? what do you try to avoid? if the visibility changed then it's likely that the checkbox state will change, it's also not obvious that all those checks spare any work over calling set_active in any case.

review: Needs Fixing

[Sebastien Bacher (seb128)]

note that instead of connecting the callback to the gsettings value you can probably simplify the code using g_settings_bind() to bind the widget state to the key value

[lp:~charlesk/gnome-bluetooth/indicators]

64. By Charles Kerr on 2012-02-02

Improve the schema handling from seb's review feedback.

1. Since the schema is part of this package, don't test to see if it exists before we use it. If the installation is corrupt, better to error out.

2. Simplify tying the togglebutton to the GSettings boolean by using g_settings_bind().

[Charles Kerr (charlesk)]

Seb, thanks for the useful feedback. Using g_settings_bind() did in fact remove all that extra code.

Likewise, I've removed the test-for-schema code before calling g_settings_new().

Fixed r64

[Charles Kerr (charlesk)]

review: Needs Resubmitting

[Sebastien Bacher (seb128)]

Thanks, that looks better, small minor comments,questions though:

> ++ else if ((gsettings != NULL) && !g_settings_get_boolean (gsettings, GSETTINGS_VISIBLE_KEY))

the NULL checking is not needed there, g_settings_new() can't return null, it works or the install is broken it exits

> g_clear_object (&self->priv->indicator_settings);

shouldn't g_object_unref() be used rather there?

> ++++ b/applet/com.canonical.indicator.bluetooth.gschema.xml

you should use a .xml.in with _summary and _description keys and list it in POTFILES.in to make the summary and description translatable and the .xml should be generated at build time

Otherwise the changes look good and seem to work fine, the checkbox seems a bit too close from the ui frame in the capplet though and maybe it would be better with some margin but I will let design comment on that

review: Needs Fixing

[Charles Kerr (charlesk)]

Seb, thanks for the additional feedback; it's all been helpful.

>> ++ else if ((gsettings != NULL) && !g_settings_get_boolean (gsettings, GSETTINGS_VISIBLE_KEY))
> the NULL checking is not needed there, g_settings_new() can't return null, it works or the install is broken it exits

Fixed in r65

>> g_clear_object (&self->priv->indicator_settings);
> shouldn't g_object_unref() be used rather there?

g_clear_object calls g_object_unref() and NULLs the pointer. I suppose NULLing is unnecessary in _finalize() but IMO it's harmless.

>> ++++ b/applet/com.canonical.indicator.bluetooth.gschema.xml
> you should use a .xml.in with _summary and _description keys and list it in POTFILES.in

Great point; fixed in r65

> the checkbox seems a bit too close from the ui frame in the capplet though and maybe it would be better with some margin

Yeah, I think that's true. Adding a 6-pixel margin between them in r66 looks better.

[lp:~charlesk/gnome-bluetooth/indicators]

65. By Charles Kerr on 2012-02-02

More improvements generated by code review:

 - remove an unnecessary (gsettings==NULL) safeguard -- g_settings_new() can't return NULL

 - insead of bundling a schema.xml directly, use an .xml.in file marked for translation

66. By Charles Kerr on 2012-02-02

Add a 6 pixel margin between the ui frame and the visibility checkbox.

[Sebastien Bacher (seb128)]

Thanks, that looks great, I will merge it tomorrow. Bonus point if you open a bug on bugzilla.gnome.org about the string changes in case they want to take it.

Small note also, shouldn't gsettingsschema_in_files be added to EXTRA_DIST?

[lp:~charlesk/gnome-bluetooth/indicators]

67. By Charles Kerr on 2012-02-02

Add $(gsettingsschema_in_files) to EXTRA_DIST

[Charles Kerr (charlesk)]

> Bonus point if you open a bug on bugzilla.gnome.org about the string changes in case they want to take it.

https://bugzilla.gnome.org/show_bug.cgi?id=669284

> also, shouldn't gsettingsschema_in_files be added to EXTRA_DIST?

Yep. r67.

[Sebastien Bacher (seb128)]

Thanks, I'm merging that

review: Approve

=== modified file 'debian/patches/01_use_app_indicator.patch'
--- debian/patches/01_use_app_indicator.patch	2012-01-29 22:11:20 +0000
+++ debian/patches/01_use_app_indicator.patch	2012-02-02 22:03:19 +0000
@@ -1,8 +1,6 @@
 
-Index: gnome-bluetooth-3.2.1/applet/main.c
-===================================================================
---- gnome-bluetooth-3.2.1.orig/applet/main.c	2012-01-29 17:06:45.334805472 -0500
-+++ gnome-bluetooth-3.2.1/applet/main.c	2012-01-29 17:06:51.358835339 -0500
+--- a/applet/main.c
++++ b/applet/main.c
 @@ -29,6 +29,9 @@
  #include <glib/gi18n.h>
  #include <gtk/gtk.h>
@@ -13,7 +11,39 @@
  #include <bluetooth-chooser.h>
  #include <bluetooth-client.h>
  #include <bluetooth-utils.h>
-@@ -417,7 +420,11 @@
+@@ -37,6 +40,12 @@
+ #include "notify.h"
+ #include "agent.h"
+ 
++#ifdef HAVE_APP_INDICATOR
++ #define BLUETOOTH_INDICATOR_GSETTINGS_SCHEMA_ID "com.canonical.indicator.bluetooth"
++ #define GSETTINGS_VISIBLE_KEY	"visible"
++ static GSettings *gsettings = NULL;
++#endif /* HAVE_APP_INDICATOR */
++
+ static gboolean option_debug = FALSE;
+ static BluetoothApplet *applet = NULL;
+ static gboolean discover_lock = FALSE;
+@@ -391,11 +400,14 @@
+ 	else
+ 		set_icon (TRUE);
+ 
+-	if (state != BLUETOOTH_KILLSWITCH_STATE_NO_ADAPTER) {
++	if (state == BLUETOOTH_KILLSWITCH_STATE_NO_ADAPTER)
++		hide_icon ();
++#ifdef HAVE_APP_INDICATOR
++	else if (!g_settings_get_boolean (gsettings, GSETTINGS_VISIBLE_KEY))
++		hide_icon ();
++#endif
++	else
+ 		show_icon ();
+-		return;
+-	}
+-	hide_icon ();
+ }
+ 
+ static void
+@@ -417,7 +429,11 @@
  
  	discoverable = bluetooth_applet_get_discoverable (applet);
  
@@ -25,7 +55,7 @@
  	gtk_toggle_action_set_active (GTK_TOGGLE_ACTION (object), discoverable);
  
  	discover_lock = FALSE;
-@@ -854,7 +861,11 @@
+@@ -854,7 +870,11 @@
  int main(int argc, char *argv[])
  {
  	GApplication *app;
@@ -37,13 +67,16 @@
  	GtkWidget *menu;
  	GOptionContext *context;
  	GError *error = NULL;
-@@ -919,14 +930,21 @@
+@@ -919,14 +939,24 @@
  	update_discoverability ((GObject*) applet, NULL, NULL);
  	update_device_list (applet, NULL);
  
 +#ifdef HAVE_APP_INDICATOR
 +	indicator = init_notification();
 +	app_indicator_set_menu(indicator, GTK_MENU(menu));
++	gsettings = g_settings_new (BLUETOOTH_INDICATOR_GSETTINGS_SCHEMA_ID);
++	g_signal_connect (gsettings, "changed::" GSETTINGS_VISIBLE_KEY,
++	                  G_CALLBACK(update_icon_visibility), NULL);
 +#else
  	statusicon = init_notification();
 +#endif /* HAVE_APP_INDICATOR */
@@ -59,10 +92,8 @@
  
  	setup_agents(applet);
  
-Index: gnome-bluetooth-3.2.1/applet/Makefile.am
-===================================================================
---- gnome-bluetooth-3.2.1.orig/applet/Makefile.am	2012-01-29 17:06:45.322805412 -0500
-+++ gnome-bluetooth-3.2.1/applet/Makefile.am	2012-01-29 17:06:51.358835339 -0500
+--- a/applet/Makefile.am
++++ b/applet/Makefile.am
 @@ -18,6 +18,13 @@
  
  INCLUDES = -I$(top_srcdir)/lib
@@ -77,10 +108,21 @@
  man_MANS = bluetooth-applet.1
  
  ui_DATA = popup-menu.ui authorisation-dialogue.ui confirm-dialogue.ui passkey-dialogue.ui
-Index: gnome-bluetooth-3.2.1/applet/notify.c
-===================================================================
---- gnome-bluetooth-3.2.1.orig/applet/notify.c	2012-01-29 17:06:45.346805532 -0500
-+++ gnome-bluetooth-3.2.1/applet/notify.c	2012-01-29 17:06:51.358835339 -0500
+@@ -37,3 +44,12 @@
+ EXTRA_DIST += $(man_MANS) $(autostart_in_in_files) $(ui_DATA)
+ 
+ MAINTAINERCLEANFILES = Makefile.in
++
++if HAVE_APP_INDICATOR
++gsettingsschema_in_files = com.canonical.indicator.bluetooth.gschema.xml.in
++gsettings_SCHEMAS = $(gsettingsschema_in_files:.xml.in=.xml)
++@INTLTOOL_XML_NOMERGE_RULE@
++@GSETTINGS_RULES@
++EXTRA_DIST += $(gsettingsschema_in_files)
++CLEANFILES += $(gsettings_SCHEMAS)
++endif
+--- a/applet/notify.c
++++ b/applet/notify.c
 @@ -29,12 +29,19 @@
  #include <glib/gi18n.h>
  #include <gtk/gtk.h>
@@ -220,10 +262,8 @@
 +#endif /* HAVE_APP_INDICATOR */
  	}
  }
-Index: gnome-bluetooth-3.2.1/applet/notify.h
-===================================================================
---- gnome-bluetooth-3.2.1.orig/applet/notify.h	2012-01-29 17:06:45.358805587 -0500
-+++ gnome-bluetooth-3.2.1/applet/notify.h	2012-01-29 17:06:51.358835339 -0500
+--- a/applet/notify.h
++++ b/applet/notify.h
 @@ -22,7 +22,13 @@
   *
   */
@@ -238,10 +278,8 @@
  void cleanup_notification(void);
  
  gboolean notification_supports_actions(void);
-Index: gnome-bluetooth-3.2.1/applet/popup-menu.ui
-===================================================================
---- gnome-bluetooth-3.2.1.orig/applet/popup-menu.ui	2011-10-17 07:26:48.000000000 -0400
-+++ gnome-bluetooth-3.2.1/applet/popup-menu.ui	2012-01-29 17:07:16.042957738 -0500
+--- a/applet/popup-menu.ui
++++ b/applet/popup-menu.ui
 @@ -63,7 +63,7 @@
  			<object class="GtkActionGroup" id="preferences-action-group">
  				<child>
@@ -251,10 +289,8 @@
  						<property name="stock-id">gtk-preferences</property>
  						<signal name="activate" handler="settings_callback"/>
  					</object>
-Index: gnome-bluetooth-3.2.1/applet/test-agentdialog.c
-===================================================================
---- gnome-bluetooth-3.2.1.orig/applet/test-agentdialog.c	2012-01-29 17:06:45.314805367 -0500
-+++ gnome-bluetooth-3.2.1/applet/test-agentdialog.c	2012-01-29 17:06:51.358835339 -0500
+--- a/applet/test-agentdialog.c
++++ b/applet/test-agentdialog.c
 @@ -32,7 +32,11 @@
  
  int main(int argc, char *argv[])
@@ -282,10 +318,8 @@
  
  	setup_agents(applet);
  
-Index: gnome-bluetooth-3.2.1/configure.ac
-===================================================================
---- gnome-bluetooth-3.2.1.orig/configure.ac	2012-01-29 17:06:45.374805671 -0500
-+++ gnome-bluetooth-3.2.1/configure.ac	2012-01-29 17:06:51.358835339 -0500
+--- a/configure.ac
++++ b/configure.ac
 @@ -98,6 +98,31 @@
  		  gtk+-3.0 >= $GTK_REQUIRED
  		  libnotify >= $NOTIFY_REQUIRED)
@@ -325,3 +359,91 @@
 +	Application Indicators......:  ${enable_appindicator}
  "
  
+--- a/properties/bluetooth.ui
++++ b/properties/bluetooth.ui
+@@ -667,6 +667,26 @@
+             <property name="height">1</property>
+           </packing>
+         </child>
++        <child>
++          <object class="GtkCheckButton" id="menubar_visibility_toggle">
++            <property name="label" translatable="yes">_Show Bluetooth status in the menu bar</property>
++            <property name="use_action_appearance">False</property>
++            <property name="visible">True</property>
++            <property name="can_focus">True</property>
++            <property name="receives_default">False</property>
++            <property name="margin_top">6</property>
++            <property name="use_action_appearance">False</property>
++            <property name="use_underline">True</property>
++            <property name="xalign">0</property>
++            <property name="draw_indicator">True</property>
++          </object>
++          <packing>
++            <property name="left_attach">0</property>
++            <property name="top_attach">2</property>
++            <property name="width">2</property>
++            <property name="height">1</property>
++          </packing>
++        </child>
+       </object>
+     </child>
+   </object>
+--- a/properties/cc-bluetooth-panel.c
++++ b/properties/cc-bluetooth-panel.c
+@@ -55,6 +55,7 @@
+ 	BluetoothClient     *client;
+ 	BluetoothKillswitch *killswitch;
+ 	gboolean             debug;
++	GSettings           *indicator_settings;
+ };
+ 
+ static void cc_bluetooth_panel_finalize (GObject *object);
+@@ -106,6 +107,8 @@
+ 		self->priv->client = NULL;
+ 	}
+ 
++	g_clear_object (&self->priv->indicator_settings);
++
+ 	G_OBJECT_CLASS (cc_bluetooth_panel_parent_class)->finalize (object);
+ }
+ 
+@@ -731,6 +734,12 @@
+ 			  G_CALLBACK (killswitch_changed), self);
+ 	cc_bluetooth_panel_update_power (self);
+ 
++	/* Set up the menubar visibility toggle */
++	self->priv->indicator_settings = g_settings_new ("com.canonical.indicator.bluetooth");
++	g_settings_bind (self->priv->indicator_settings, "visible",
++	                 WID("menubar_visibility_toggle"), "active",
++	                 G_SETTINGS_BIND_DEFAULT);
++
+ 	gtk_widget_show_all (GTK_WIDGET (self));
+ }
+ 
+@@ -758,4 +767,3 @@
+ g_io_module_unload (GIOModule *module)
+ {
+ }
+-
+--- /dev/null
++++ b/applet/com.canonical.indicator.bluetooth.gschema.xml.in
+@@ -0,0 +1,9 @@
++<schemalist>
++  <schema id="com.canonical.indicator.bluetooth" path="/com/canonical/indicator/bluetooth/">
++    <key name="visible" type="b">
++      <default>true</default>
++      <_summary>Whether or not to show the bluetooth indicator in the menu bar.</_summary>
++      <_description>Whether or not to show the bluetooth indicator in the menu bar.</_description>
++    </key>
++  </schema>
++</schemalist>
+--- a/po/POTFILES.in
++++ b/po/POTFILES.in
+@@ -10,6 +10,7 @@
+ applet/notify.c
+ applet/agent.c
+ applet/bluetooth-applet.desktop.in.in
++applet/com.canonical.indicator.bluetooth.gschema.xml.in
+ [type: gettext/glade] applet/popup-menu.ui
+ [type: gettext/glade] applet/authorisation-dialogue.ui
+ [type: gettext/glade] applet/confirm-dialogue.ui

=== added file 'debian/patches/03_text_changes.patch'
--- debian/patches/03_text_changes.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/03_text_changes.patch	2012-02-02 22:03:19 +0000
@@ -0,0 +1,28 @@
+--- a/applet/popup-menu.ui
++++ b/applet/popup-menu.ui
+@@ -33,14 +33,14 @@
+ 				</child>
+ 				<child>
+ 					<object class="GtkAction" id="send-file">
+-						<property name="label" translatable="yes">Send files to device...</property>
++						<property name="label" translatable="yes">Send Files to Device…</property>
+ 						<property name="icon-name">document-send</property>
+ 						<signal name="activate" handler="sendto_callback"/>
+ 					</object>
+ 				</child>
+ 				<child>
+ 					<object class="GtkAction" id="browse-device">
+-						<property name="label" translatable="yes">Browse files on device...</property>
++						<property name="label" translatable="yes">Browse Files on Device…</property>
+ 						<signal name="activate" handler="browse_callback"/>
+ 					</object>
+ 				</child>
+@@ -53,7 +53,7 @@
+ 				</child>
+ 				<child>
+ 					<object class="GtkAction" id="setup-new">
+-						<property name="label" translatable="yes">Set up new device...</property>
++						<property name="label" translatable="yes">Set Up New Device…</property>
+ 						<signal name="activate" handler="wizard_callback"/>
+ 					</object>
+ 				</child>

=== modified file 'debian/patches/series'
--- debian/patches/series	2012-01-23 20:09:53 +0000
+++ debian/patches/series	2012-02-02 22:03:19 +0000
@@ -4,6 +4,7 @@
 git_update_indicator_list.patch
 01_use_app_indicator.patch
 02_gettext_macros.patch
+03_text_changes.patch
 nodisplay_autostart.patch
 onlyshowin_unity.patch
 fix_browse_devices_selector.patch