Merge lp:~ted/indicator-network/blank-items into lp:indicator-network/13.10

[Ted Gould]

Ensure APs with SSIDs we don't want to show aren't shown

This change reshuffles the AP handling a little bit in that we're looking at each item that we're creating, and possibily rejecting the menu item getting created at all. But, because there is some amount of uncertainty here, we need to track each and ensure that if the SSID updates to something valid, we can then show it. So this branch makes sure to set up notification of that change, and then rejects anything we might see as suspect.

To use the NM Utils library we required a small wrapper there as the NM VAPI isn't quite clear on what it wants and I didn't want to force it. Making our own wrapper function made things a lot cleaner, even though it makes the diff much larger.

[lp:~ted/indicator-network/blank-items]

303. By Ted Gould on 2013-10-08

Make sure to grab the header for prototypes

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:303
http://jenkins.qa.ubuntu.com/job/indicator-network-ci/70/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-amd64-ci/60
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-armhf-ci/60/console

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/indicator-network-ci/70/rebuild

review: Needs Fixing (continuous-integration)

[Ted Gould (ted)]

Jenkins hickup.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:303
http://jenkins.qa.ubuntu.com/job/indicator-network-ci/71/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-amd64-ci/61
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-armhf-ci/61
        deb: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-armhf-ci/61/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/indicator-network-ci/71/rebuild

review: Approve (continuous-integration)

[Sebastien Bacher (seb128)]

Thanks, that seems to fix the issue, it just hit a segfaults though:

#0 util_wrapper_is_empty_ssid (bytes=0x0)
    at /home/seb128/boulot/package/download/ted/build-area/indicator-network-0.5.1+13.10.20131005/network/util-wrapper.c:26
#1 0x08053bbb in network_device_wifi_menu_insert_ap_item (
    self=self@entry=0x8bbe370, ap=ap@entry=0x8bd72c8)
    at /home/seb128/boulot/package/download/ted/build-area/indicator-network-0.5.1+13.10.20131005/obj-i686-linux-gnu/network/device-wifi.c:774
#2 0x080542b7 in network_device_wifi_menu_access_point_added_cb (
    self=0x8bbe370, device=<optimized out>, user_data=<optimized out>)
    at /home/seb128/boulot/package/download/ted/build-area/indicator-network-0.5.1+13.10.20131005/obj-i686-linux-gnu/network/device-wifi.c:599
#3 0xb75c558e in g_cclosure_marshal_VOID__OBJECT (closure=0x8c0bd58,
    return_value=0x0, n_param_values=2, param_values=0xbf887d50,
    invocation_hint=0xbf887cfc, marshal_data=0x0)
    at /build/buildd/glib2.0-2.38.0/./gobject/gmarshal.c:1272
#4 0xb75c28ae in g_closure_invoke (closure=0x8c0bd58,
    return_value=return_value@entry=0x0, n_param_values=2,
    param_values=param_values@entry=0xbf887d50,
    invocation_hint=invocation_hint@entry=0xbf887cfc)
    at /build/buildd/glib2.0-2.38.0/./gobject/gclosure.c:777

review: Needs Fixing

[lp:~ted/indicator-network/blank-items]

304. By Ted Gould on 2013-10-08

Check for the byte array being NULL

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:304
http://jenkins.qa.ubuntu.com/job/indicator-network-ci/72/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-amd64-ci/62
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-armhf-ci/62
        deb: http://jenkins.qa.ubuntu.com/job/indicator-network-saucy-armhf-ci/62/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/indicator-network-ci/72/rebuild

review: Approve (continuous-integration)

[Sebastien Bacher (seb128)]

The new revision works fine on my system which used to have the issue, thanks! The code looks fine as well, but I just had easy look at it so it would be good to get another code review but getting that in still

review: Approve

[Sebastien Bacher (seb128)]

(but getting that in -> before getting that in)

[Charles Kerr (charlesk)]

Code level looks OK.

review: Approve

=== modified file 'network/CMakeLists.txt'
--- network/CMakeLists.txt	2013-09-06 16:19:21 +0000
+++ network/CMakeLists.txt	2013-10-08 15:00:14 +0000
@@ -16,6 +16,7 @@
   CUSTOM_VAPIS
     "${CMAKE_CURRENT_SOURCE_DIR}/action-muxer.vapi"
     "${CMAKE_CURRENT_SOURCE_DIR}/url-dispatcher.vapi"
+    "${CMAKE_CURRENT_SOURCE_DIR}/util-wrapper.vapi"
 )
 
 vala_add(libnetwork
@@ -103,6 +104,7 @@
   ${LIBNETWORK_VALA_SOURCES}
   ${LIBNETWORK_VALA_C}
   action-muxer.c
+  util-wrapper.c
 )
 
 add_library(
@@ -130,6 +132,7 @@
   CUSTOM_VAPIS
     "${CMAKE_CURRENT_SOURCE_DIR}/config.vapi"
     "${CMAKE_CURRENT_SOURCE_DIR}/action-muxer.vapi"
+    "${CMAKE_CURRENT_SOURCE_DIR}/util-wrapper.vapi"
   OPTIONS
     --vapidir=.
 )

=== modified file 'network/device-wifi.vala'
--- network/device-wifi.vala	2013-09-30 18:49:50 +0000
+++ network/device-wifi.vala	2013-10-08 15:00:14 +0000
@@ -32,6 +32,7 @@
 		private  NM.Client   client;
 		private  string      action_prefix;
 		private  bool        show_settings;
+		private  HashTable<string, NM.AccessPoint> aps = new HashTable<string, NM.AccessPoint>(str_hash, str_equal);
 
 		public WifiMenu (NM.Client client, DeviceWifi device, Menu global_menu, SimpleActionGroup actions, string action_prefix, bool show_settings)
 		{
@@ -61,12 +62,17 @@
 
 			for (uint i = 0; i<aps.length; i++)
 			{
-				insert_ap (aps.get (i));
+				access_point_added_cb (device, aps.get (i));
 			}
 		}
 
 		~WifiMenu ()
 		{
+			/* Make sure to clean up! */
+			foreach (var ap in aps.get_values()) {
+				access_point_removed_cb(this.device, ap);
+			}
+
 			device.access_point_added.disconnect   (access_point_added_cb);
 			device.access_point_removed.disconnect (access_point_removed_cb);
 			device.notify.disconnect               (active_access_point_changed);
@@ -89,14 +95,36 @@
 			item.set_attribute ("action", "s",  activate_action_id);
 		}
 
-		private void access_point_added_cb (NM.DeviceWifi device, GLib.Object ap)
-		{
-			insert_ap ((AccessPoint)ap);
-		}
-
-		private void access_point_removed_cb (NM.DeviceWifi device, GLib.Object ap)
-		{
-			remove_ap ((AccessPoint)ap);
+		private void access_point_ssid_cb (GLib.Object obj, GLib.ParamSpec pspec)
+		{
+			AccessPoint ap = (AccessPoint)obj;
+			remove_ap_item(ap);
+			insert_ap_item(ap);
+		}
+
+		private void access_point_added_cb (NM.DeviceWifi device, GLib.Object user_data)
+		{
+			AccessPoint ap = (AccessPoint)user_data;
+			string ap_path = ap.get_path();
+
+			if (aps.lookup(ap_path) == null) {
+				aps.insert(ap_path, ap);
+				insert_ap_item (ap);
+
+				ap.notify["ssid"].connect(access_point_ssid_cb);
+			}
+		}
+
+		private void access_point_removed_cb (NM.DeviceWifi device, GLib.Object user_data)
+		{
+			AccessPoint ap = (AccessPoint)user_data;
+			string ap_path = ap.get_path();
+
+			if (aps.lookup(ap_path) == null) {
+				ap.notify["ssid"].disconnect(access_point_ssid_cb);
+				aps.remove(ap_path);
+				remove_ap_item (ap);
+			}
 		}
 
 		private void active_access_point_changed (GLib.Object obj, ParamSpec pspec)
@@ -126,15 +154,23 @@
 		 * - Previously used APs are ordered by signal strength
 		 * - Unused APs are ordered by signal strength
 		 */
-		private void insert_ap (AccessPoint ap)
+		private void insert_ap_item (AccessPoint ap)
 		{
 			var rs = new NM.RemoteSettings (null);
 			SList <NM.Connection>? dev_conns = null;
 			bool has_connection = false;
+			string label;
 
 			if (ap == null)
 				return;
 
+			if (UtilWrapper.is_empty_ssid(ap.get_ssid ()))
+				return;
+
+			label = Utils.ssid_to_utf8(ap.get_ssid ());
+			if (label == null || label[0] == '\0')
+				return;
+
 			//If it is the active access point it always goes first
 			if (ap == device.active_access_point)
 			{
@@ -155,7 +191,7 @@
 
 
 			//Remove duplicate SSID
-			for (int i = 1; i < apsmenu.get_n_items(); i++)
+			for (int i = 0; i < apsmenu.get_n_items(); i++)
 			{
 				string path;
 
@@ -182,8 +218,7 @@
 			//Find the right spot for the AP
 			var item = new MenuItem (null, null);
 			bind_ap_item (ap, item);
-			/* We need to start at 1 and end at -1 to avoid the toggle and the settings */
-			for (int i = 1; i < apsmenu.get_n_items() - 1; i++)
+			for (int i = 0; i < apsmenu.get_n_items(); i++)
 			{
 				string path;
 
@@ -245,7 +280,7 @@
 			return ap_conns.length () > 0;
 		}
 
-		private void remove_ap (AccessPoint ap)
+		private void remove_ap_item (AccessPoint ap)
 		{
 			for (int i = 1; i < apsmenu.get_n_items(); i++)
 			{

=== added file 'network/util-wrapper.c'
--- network/util-wrapper.c	1970-01-01 00:00:00 +0000
+++ network/util-wrapper.c	2013-10-08 15:00:14 +0000
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Ted Gould <[e-mail]>
+ */
+
+#include <glib.h>
+#include <NetworkManager/nm-utils.h>
+
+gboolean
+util_wrapper_is_empty_ssid (GByteArray * bytes)
+{
+	if (bytes == NULL)
+		return TRUE;
+
+	return nm_utils_is_empty_ssid(bytes->data, bytes->len);
+}

=== added file 'network/util-wrapper.h'
--- network/util-wrapper.h	1970-01-01 00:00:00 +0000
+++ network/util-wrapper.h	2013-10-08 15:00:14 +0000
@@ -0,0 +1,20 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Ted Gould <[e-mail]>
+ */
+
+gboolean util_wrapper_is_empty_ssid (GByteArray * bytes);

=== added file 'network/util-wrapper.vapi'
--- network/util-wrapper.vapi	1970-01-01 00:00:00 +0000
+++ network/util-wrapper.vapi	2013-10-08 15:00:14 +0000
@@ -0,0 +1,5 @@
+[CCode (cprefix = "UtilWrapper", lower_case_cprefix = "util_wrapper_")]
+namespace UtilWrapper {
+	[CCode (cheader_filename = "util-wrapper.h")]
+	public bool is_empty_ssid (GLib.ByteArray ssid);
+}