Merge lp:~donadigo/switchboard-plug-networking/page-cleanup into lp:~elementary-pantheon/switchboard-plug-networking/trunk

[Adam Bieńkowski]

Page cleanup:
* Added init () method
* Renamed wifi states to: Enabled and Disabled
* Moved methods from DevicePage to Page class to make them more unified.

Page cleanup. Moved some methods from DevicePage to Page class to make them more unified. Renamed wifi states to: Enabled and Disabled.

* Fixed: control_switch does not switch when device state changed not from plug.

* Switch now reflects wireless state instead of the device state.

[Danielle Foré (danrabbit)]

Enabling and disabling doesn't seem to work as expected.

Disabling always crashes the plug (I would consider this a regression since in trunk the switch does nothing. Nothing is probably better than a crash).

Enabling doesn't change the status in the sidebar (it should be "Disconnected" to match the status shown in the plug). However since the sidebar status doesn't seem to work correctly in trunk either, I don't think this is a blocker or necessarily needs to be fixed in this branch. Maybe in another branch the sidebar status should be changed so that it reads the same as the status in the plug. Its information seems to be accurate and reflect various network states.

The switch moves to the "disabled" state when merely disconnected. It sounds like you introduced this change intentionally, but it doesn't make design sense. There is already a disconnect button. This switch should reflect whether the device is on and capable of connecting to a network, not whether it is connected or not.

review: Needs Fixing (ux)

[lp:~donadigo/switchboard-plug-networking/page-cleanup]

113. By Adam Bieńkowski on 2015-07-24

Clean code

114. By Adam Bieńkowski on 2015-07-24

Remove unneded debug line

115. By Adam Bieńkowski on 2015-07-24

Fixed: control switch goes crazy because of unrelated updating it in update () method

[Adam Bieńkowski (donadigo)]

Daniel, can you test this merge: https://code.launchpad.net/~donadigo/switchboard-plug-networking/page-use-client-singals/+merge/265846 ? This is new version of the old use-client-page. It solves not changing state in wifi item.

[lp:~donadigo/switchboard-plug-networking/page-cleanup]

116. By Adam Bieńkowski on 2015-07-24

Scan for duplicates removed for now

117. By Adam Bieńkowski on 2015-07-24

Fixed: disabling control switch causes switchboard to crash in remove_access_point method

[Danielle Foré (danrabbit)]

Looks like the issues I encountered previously were fixed here with the exception of the sidebar stuff, which as I said probably can/should be addressed in another branch.

review: Approve

=== modified file 'src/Plug.vala'
--- src/Plug.vala	2015-07-18 16:56:50 +0000
+++ src/Plug.vala	2015-07-24 17:43:15 +0000
@@ -120,10 +120,6 @@
             content.add_named (wifi_page, string_id);
 
             switch_wifi_status (wifi_page);       
-            wifi_page.switch_callback.connect (() => {
-                client.wireless_set_enabled (wifi_page.control_switch.get_active ());
-                switch_wifi_status (wifi_page);
-            });
 
             device_list.wifi.activate.connect (() => {
                 if (content.get_visible_child_name () != string_id)

=== modified file 'src/Widgets/Device/DeviceItem.vala'
--- src/Widgets/Device/DeviceItem.vala	2015-07-18 19:31:20 +0000
+++ src/Widgets/Device/DeviceItem.vala	2015-07-24 17:43:15 +0000
@@ -149,11 +149,11 @@
                         status_image.icon_name = "user-available";
                         break;
                     case "wifi-enabled":
-                        row_description.label = Utils.state_to_string (NM.DeviceState.ACTIVATED);
+                        row_description.label = _("Enabled");
                         status_image.icon_name = "user-available";
                         break;
                     case "wifi-disabled":
-                        row_description.label = Utils.state_to_string (NM.DeviceState.DISCONNECTED);
+                        row_description.label = _("Disabled");
                         status_image.icon_name = "user-busy";
                         break;
                }

=== modified file 'src/Widgets/Device/DevicePage.vala'
--- src/Widgets/Device/DevicePage.vala	2015-07-18 16:56:50 +0000
+++ src/Widgets/Device/DevicePage.vala	2015-07-24 17:43:15 +0000
@@ -23,35 +23,22 @@
 namespace Network.Widgets {
     public class DevicePage : Page {
         public DeviceItem owner;
-        public InfoBox info_box;
 
         public DevicePage.from_owner (DeviceItem? _owner) {
             this.owner = _owner;
-            this.device = owner.get_item_device ();
             this.icon_name = owner.get_item_icon_name ();
             this.title = Utils.type_to_string (device.get_device_type ());
-            this.margin_end = 12;
-
             info_box = new info_box.from_owner (owner);
-            info_box.margin_end = this.INFO_BOX_MARGIN;
-            info_box.info_changed.connect (update);
+            this.init (owner.get_item_device (), info_box);
 
             var details_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 0);
             details_box.pack_start (Utils.get_advanced_button_from_device (device), false, false, 0);           
 
-            update ();
+            update (info_box);
 
             this.add (info_box);
             this.pack_end (details_box, false, false, 0);
             this.show_all ();
         }
-
-        private void update () {
-            string sent_bytes, received_bytes;
-            this.get_activity_information (device.get_iface (), out sent_bytes, out received_bytes);
-            info_box.update_activity (sent_bytes, received_bytes);
-
-            control_switch.active = (device.get_state () == NM.DeviceState.ACTIVATED);
-        }
     }
 }

=== modified file 'src/Widgets/Page.vala'
--- src/Widgets/Page.vala	2015-07-18 16:11:54 +0000
+++ src/Widgets/Page.vala	2015-07-24 17:43:15 +0000
@@ -1,11 +1,9 @@
 
-namespace Network {
+namespace Network.Widgets {
     public class Page : Gtk.Box {
-        public const int INFO_BOX_MARGIN = 16;
-
         public NM.Device device;
+        public InfoBox info_box;
         public Gtk.Switch control_switch;
-        public signal void switch_callback ();
         public signal void show_error ();
 
         private string _icon_name;    
@@ -40,6 +38,15 @@
             this.orientation = Gtk.Orientation.VERTICAL;
             this.margin = 12;
             this.spacing = 24;
+        }
+
+        public void init (NM.Device _device, Widgets.InfoBox _info_box) {
+            this.device = _device;
+            this.info_box = _info_box;
+            info_box.margin_end = 16;
+            info_box.info_changed.connect (() => {
+                update (info_box);
+            });            
 
             device_img = new Gtk.Image.from_icon_name (_icon_name, Gtk.IconSize.DIALOG);
             device_img.pixel_size = 48;
@@ -48,7 +55,13 @@
             device_label.get_style_context ().add_class ("h2");
 
             control_switch = new Gtk.Switch ();
-            control_switch.activate.connect (control_switch_activated);
+            if (device.get_device_type () == NM.DeviceType.WIFI) {
+                control_switch.active = (client.wireless_get_enabled ());
+            } else {
+                control_switch.active = (device.get_state () == NM.DeviceState.ACTIVATED);
+            }
+                        
+            control_switch.button_press_event.connect (control_switch_activated);
 
             control_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 12);
             control_box.pack_start (device_img, false, false, 0);
@@ -56,7 +69,13 @@
             control_box.pack_end (control_switch, false, false, 0);       
 
             this.add (control_box);
-            this.show_all (); 
+            this.show_all ();             
+        }
+
+        public void update (Widgets.InfoBox info_box) {
+            string sent_bytes, received_bytes;
+            this.get_activity_information (device.get_iface (), out sent_bytes, out received_bytes);
+            info_box.update_activity (sent_bytes, received_bytes);
         }
 
         public void add_switch_title (string title) {
@@ -65,7 +84,12 @@
             control_box.pack_end (label, false, false, 0);
         }
 
-        private void control_switch_activated () {
+        private bool control_switch_activated () {
+            if (device.get_device_type () == NM.DeviceType.WIFI) {
+                client.wireless_set_enabled (!client.wireless_get_enabled ());
+                return false;
+            }
+
             if (device.get_state () == NM.DeviceState.ACTIVATED) {
                 device.disconnect (null);
             } else {
@@ -79,7 +103,7 @@
                 }
             }
 
-            this.switch_callback ();            
+            return false;
         }
 
         public void get_activity_information (string iface, out string sent_bytes, out string received_bytes) {

=== modified file 'src/Widgets/WiFi/WiFiPage.vala'
--- src/Widgets/WiFi/WiFiPage.vala	2015-07-23 20:51:21 +0000
+++ src/Widgets/WiFi/WiFiPage.vala	2015-07-24 17:43:15 +0000
@@ -23,7 +23,6 @@
 namespace Network.Widgets {
     public class WiFiPage : Page {
         public new NM.DeviceWifi device;
-        private InfoBox info_box;
         private Gtk.ListBox wifi_list;
         private List<WiFiEntry> entries;
         private const string[] BLACKLISTED = { "Free Public WiFi" };
@@ -37,6 +36,8 @@
             this.device = wifidevice;
             this.icon_name = "network-wireless";
             this.title = _("Wi-Fi Network");
+            info_box = new InfoBox.from_device (device);
+            this.init (wifidevice, info_box);
 
             entries = new List<WiFiEntry> ();
 
@@ -51,10 +52,6 @@
             scrolled.vexpand = true;
             scrolled.shadow_type = Gtk.ShadowType.OUT;
 
-            info_box = new info_box.from_device (device);
-            info_box.margin_end = this.INFO_BOX_MARGIN;
-            info_box.info_changed.connect (update);
-
             var disconnect_btn = new Gtk.Button.with_label (_("Disconnect"));
             disconnect_btn.sensitive = (device.get_state () == NM.DeviceState.ACTIVATED);
             disconnect_btn.get_style_context ().add_class ("destructive-action");
@@ -90,7 +87,7 @@
             device.access_point_added.connect (add_access_point);
             device.access_point_removed.connect (remove_access_point);
 
-            update ();
+            update (info_box);
 
             this.add_switch_title (_("Wireless:"));
             this.add (scrolled);
@@ -99,14 +96,6 @@
             this.show_all ();   
         }
 
-        private void update () {
-            string sent_bytes, received_bytes;
-            this.get_activity_information (device.get_iface (), out sent_bytes, out received_bytes);
-            info_box.update_activity (sent_bytes, received_bytes);
-
-            control_switch.active = (client.wireless_get_enabled () && device.get_state () == NM.DeviceState.ACTIVATED);
-        }
-
         private void on_row_activated (Gtk.ListBoxRow row) {
             if (device != null) {  
                 /* Do not activate connection if it is already activated */
@@ -154,8 +143,10 @@
 
                 /* Check if we are successfully connected to the requested point */
                 if (device.get_active_access_point () == (row as WiFiEntry).ap) {
-                    foreach (var entry in entries)
+                    entries.@foreach ((entry) => {
                         entry.set_status_point (false, false);
+                    });
+                    
                     (row as WiFiEntry).set_status_point (true, false);
                 }
             }
@@ -190,23 +181,9 @@
                 insert_on_top = true;
             });
 
-            scan_for_duplicates.begin ();
             wifi_list.show_all ();
         }
         
-        private async void scan_for_duplicates () {
-            var entries_dup = entries.copy ();
-            entries.@foreach ((entry) => {
-                var ssid = entry.ap.get_ssid ();
-
-                entries_dup.@foreach ((entry_dup) => {
-                    if (entry_dup.ap.get_ssid () == ssid) {
-                        this.remove_access_point (entry_dup.ap);
-                    }
-                });
-            });
-        }
-
         private void add_access_point (Object ap) {
             var row = new WiFiEntry.from_access_point (ap as NM.AccessPoint);
             if (!(row.ssid in BLACKLISTED) && row.ap.get_ssid () != null) {
@@ -219,21 +196,18 @@
                 entries.append (row as WiFiEntry);
             }
 
-            if ((ap as NM.AccessPoint) == device.get_active_access_point ())
+            if ((ap as NM.AccessPoint) == device.get_active_access_point ()) {
                 row.set_status_point (true, false);
+            }
         }
         
         private void remove_access_point (Object ap_removed) {
-            var new_entries = new List<WiFiEntry> ();
-            foreach (var entry in entries) {
-                if ((entry as WiFiEntry).ap == ap_removed) {
+            entries.@foreach ((entry) => {
+                if (((WiFiEntry) entry).ap == ap_removed) {
+                    entries.remove (entry);
                     entry.destroy ();
-                } else {
-                    new_entries.append (entry);
                 }
-            }
-            
-            entries = new_entries.copy ();
+            });          
         }
 
         private int sort_func (Gtk.ListBoxRow r1, Gtk.ListBoxRow r2) {