Merge lp:~donadigo/switchboard-plug-networking/page-cleanup into lp:~elementary-pantheon/switchboard-plug-networking/trunk
- page-cleanup
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Danielle Foré |
Approved revision: | 117 |
Merged at revision: | 115 |
Proposed branch: | lp:~donadigo/switchboard-plug-networking/page-cleanup |
Merge into: | lp:~elementary-pantheon/switchboard-plug-networking/trunk |
Diff against target: |
281 lines (+48/-67) 5 files modified
src/Plug.vala (+0/-4) src/Widgets/Device/DeviceItem.vala (+2/-2) src/Widgets/Device/DevicePage.vala (+2/-15) src/Widgets/Page.vala (+32/-8) src/Widgets/WiFi/WiFiPage.vala (+12/-38) |
To merge this branch: | bzr merge lp:~donadigo/switchboard-plug-networking/page-cleanup |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Danielle Foré | Approve | ||
Review via email: mp+265743@code.launchpad.net |
Commit message
Page cleanup:
* Added init () method
* Renamed wifi states to: Enabled and Disabled
* Moved methods from DevicePage to Page class to make them more unified.
Description of the change
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.
- 113. By Adam Bieńkowski
-
Clean code
- 114. By Adam Bieńkowski
-
Remove unneded debug line
- 115. By Adam Bieńkowski
-
Fixed: control switch goes crazy because of unrelated updating it in update () method
Adam Bieńkowski (donadigo) wrote : | # |
Daniel, can you test this merge: https:/
- 116. By Adam Bieńkowski
-
Scan for duplicates removed for now
- 117. By Adam Bieńkowski
-
Fixed: disabling control switch causes switchboard to crash in remove_access_point method
Danielle Foré (danrabbit) wrote : | # |
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.
Preview Diff
1 | === modified file 'src/Plug.vala' | |||
2 | --- src/Plug.vala 2015-07-18 16:56:50 +0000 | |||
3 | +++ src/Plug.vala 2015-07-24 17:43:15 +0000 | |||
4 | @@ -120,10 +120,6 @@ | |||
5 | 120 | content.add_named (wifi_page, string_id); | 120 | content.add_named (wifi_page, string_id); |
6 | 121 | 121 | ||
7 | 122 | switch_wifi_status (wifi_page); | 122 | switch_wifi_status (wifi_page); |
8 | 123 | wifi_page.switch_callback.connect (() => { | ||
9 | 124 | client.wireless_set_enabled (wifi_page.control_switch.get_active ()); | ||
10 | 125 | switch_wifi_status (wifi_page); | ||
11 | 126 | }); | ||
12 | 127 | 123 | ||
13 | 128 | device_list.wifi.activate.connect (() => { | 124 | device_list.wifi.activate.connect (() => { |
14 | 129 | if (content.get_visible_child_name () != string_id) | 125 | if (content.get_visible_child_name () != string_id) |
15 | 130 | 126 | ||
16 | === modified file 'src/Widgets/Device/DeviceItem.vala' | |||
17 | --- src/Widgets/Device/DeviceItem.vala 2015-07-18 19:31:20 +0000 | |||
18 | +++ src/Widgets/Device/DeviceItem.vala 2015-07-24 17:43:15 +0000 | |||
19 | @@ -149,11 +149,11 @@ | |||
20 | 149 | status_image.icon_name = "user-available"; | 149 | status_image.icon_name = "user-available"; |
21 | 150 | break; | 150 | break; |
22 | 151 | case "wifi-enabled": | 151 | case "wifi-enabled": |
24 | 152 | row_description.label = Utils.state_to_string (NM.DeviceState.ACTIVATED); | 152 | row_description.label = _("Enabled"); |
25 | 153 | status_image.icon_name = "user-available"; | 153 | status_image.icon_name = "user-available"; |
26 | 154 | break; | 154 | break; |
27 | 155 | case "wifi-disabled": | 155 | case "wifi-disabled": |
29 | 156 | row_description.label = Utils.state_to_string (NM.DeviceState.DISCONNECTED); | 156 | row_description.label = _("Disabled"); |
30 | 157 | status_image.icon_name = "user-busy"; | 157 | status_image.icon_name = "user-busy"; |
31 | 158 | break; | 158 | break; |
32 | 159 | } | 159 | } |
33 | 160 | 160 | ||
34 | === modified file 'src/Widgets/Device/DevicePage.vala' | |||
35 | --- src/Widgets/Device/DevicePage.vala 2015-07-18 16:56:50 +0000 | |||
36 | +++ src/Widgets/Device/DevicePage.vala 2015-07-24 17:43:15 +0000 | |||
37 | @@ -23,35 +23,22 @@ | |||
38 | 23 | namespace Network.Widgets { | 23 | namespace Network.Widgets { |
39 | 24 | public class DevicePage : Page { | 24 | public class DevicePage : Page { |
40 | 25 | public DeviceItem owner; | 25 | public DeviceItem owner; |
41 | 26 | public InfoBox info_box; | ||
42 | 27 | 26 | ||
43 | 28 | public DevicePage.from_owner (DeviceItem? _owner) { | 27 | public DevicePage.from_owner (DeviceItem? _owner) { |
44 | 29 | this.owner = _owner; | 28 | this.owner = _owner; |
45 | 30 | this.device = owner.get_item_device (); | ||
46 | 31 | this.icon_name = owner.get_item_icon_name (); | 29 | this.icon_name = owner.get_item_icon_name (); |
47 | 32 | this.title = Utils.type_to_string (device.get_device_type ()); | 30 | this.title = Utils.type_to_string (device.get_device_type ()); |
48 | 33 | this.margin_end = 12; | ||
49 | 34 | |||
50 | 35 | info_box = new info_box.from_owner (owner); | 31 | info_box = new info_box.from_owner (owner); |
53 | 36 | info_box.margin_end = this.INFO_BOX_MARGIN; | 32 | this.init (owner.get_item_device (), info_box); |
52 | 37 | info_box.info_changed.connect (update); | ||
54 | 38 | 33 | ||
55 | 39 | var details_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 0); | 34 | var details_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 0); |
56 | 40 | details_box.pack_start (Utils.get_advanced_button_from_device (device), false, false, 0); | 35 | details_box.pack_start (Utils.get_advanced_button_from_device (device), false, false, 0); |
57 | 41 | 36 | ||
59 | 42 | update (); | 37 | update (info_box); |
60 | 43 | 38 | ||
61 | 44 | this.add (info_box); | 39 | this.add (info_box); |
62 | 45 | this.pack_end (details_box, false, false, 0); | 40 | this.pack_end (details_box, false, false, 0); |
63 | 46 | this.show_all (); | 41 | this.show_all (); |
64 | 47 | } | 42 | } |
65 | 48 | |||
66 | 49 | private void update () { | ||
67 | 50 | string sent_bytes, received_bytes; | ||
68 | 51 | this.get_activity_information (device.get_iface (), out sent_bytes, out received_bytes); | ||
69 | 52 | info_box.update_activity (sent_bytes, received_bytes); | ||
70 | 53 | |||
71 | 54 | control_switch.active = (device.get_state () == NM.DeviceState.ACTIVATED); | ||
72 | 55 | } | ||
73 | 56 | } | 43 | } |
74 | 57 | } | 44 | } |
75 | 58 | 45 | ||
76 | === modified file 'src/Widgets/Page.vala' | |||
77 | --- src/Widgets/Page.vala 2015-07-18 16:11:54 +0000 | |||
78 | +++ src/Widgets/Page.vala 2015-07-24 17:43:15 +0000 | |||
79 | @@ -1,11 +1,9 @@ | |||
80 | 1 | 1 | ||
82 | 2 | namespace Network { | 2 | namespace Network.Widgets { |
83 | 3 | public class Page : Gtk.Box { | 3 | public class Page : Gtk.Box { |
84 | 4 | public const int INFO_BOX_MARGIN = 16; | ||
85 | 5 | |||
86 | 6 | public NM.Device device; | 4 | public NM.Device device; |
87 | 5 | public InfoBox info_box; | ||
88 | 7 | public Gtk.Switch control_switch; | 6 | public Gtk.Switch control_switch; |
89 | 8 | public signal void switch_callback (); | ||
90 | 9 | public signal void show_error (); | 7 | public signal void show_error (); |
91 | 10 | 8 | ||
92 | 11 | private string _icon_name; | 9 | private string _icon_name; |
93 | @@ -40,6 +38,15 @@ | |||
94 | 40 | this.orientation = Gtk.Orientation.VERTICAL; | 38 | this.orientation = Gtk.Orientation.VERTICAL; |
95 | 41 | this.margin = 12; | 39 | this.margin = 12; |
96 | 42 | this.spacing = 24; | 40 | this.spacing = 24; |
97 | 41 | } | ||
98 | 42 | |||
99 | 43 | public void init (NM.Device _device, Widgets.InfoBox _info_box) { | ||
100 | 44 | this.device = _device; | ||
101 | 45 | this.info_box = _info_box; | ||
102 | 46 | info_box.margin_end = 16; | ||
103 | 47 | info_box.info_changed.connect (() => { | ||
104 | 48 | update (info_box); | ||
105 | 49 | }); | ||
106 | 43 | 50 | ||
107 | 44 | device_img = new Gtk.Image.from_icon_name (_icon_name, Gtk.IconSize.DIALOG); | 51 | device_img = new Gtk.Image.from_icon_name (_icon_name, Gtk.IconSize.DIALOG); |
108 | 45 | device_img.pixel_size = 48; | 52 | device_img.pixel_size = 48; |
109 | @@ -48,7 +55,13 @@ | |||
110 | 48 | device_label.get_style_context ().add_class ("h2"); | 55 | device_label.get_style_context ().add_class ("h2"); |
111 | 49 | 56 | ||
112 | 50 | control_switch = new Gtk.Switch (); | 57 | control_switch = new Gtk.Switch (); |
114 | 51 | control_switch.activate.connect (control_switch_activated); | 58 | if (device.get_device_type () == NM.DeviceType.WIFI) { |
115 | 59 | control_switch.active = (client.wireless_get_enabled ()); | ||
116 | 60 | } else { | ||
117 | 61 | control_switch.active = (device.get_state () == NM.DeviceState.ACTIVATED); | ||
118 | 62 | } | ||
119 | 63 | |||
120 | 64 | control_switch.button_press_event.connect (control_switch_activated); | ||
121 | 52 | 65 | ||
122 | 53 | control_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 12); | 66 | control_box = new Gtk.Box (Gtk.Orientation.HORIZONTAL, 12); |
123 | 54 | control_box.pack_start (device_img, false, false, 0); | 67 | control_box.pack_start (device_img, false, false, 0); |
124 | @@ -56,7 +69,13 @@ | |||
125 | 56 | control_box.pack_end (control_switch, false, false, 0); | 69 | control_box.pack_end (control_switch, false, false, 0); |
126 | 57 | 70 | ||
127 | 58 | this.add (control_box); | 71 | this.add (control_box); |
129 | 59 | this.show_all (); | 72 | this.show_all (); |
130 | 73 | } | ||
131 | 74 | |||
132 | 75 | public void update (Widgets.InfoBox info_box) { | ||
133 | 76 | string sent_bytes, received_bytes; | ||
134 | 77 | this.get_activity_information (device.get_iface (), out sent_bytes, out received_bytes); | ||
135 | 78 | info_box.update_activity (sent_bytes, received_bytes); | ||
136 | 60 | } | 79 | } |
137 | 61 | 80 | ||
138 | 62 | public void add_switch_title (string title) { | 81 | public void add_switch_title (string title) { |
139 | @@ -65,7 +84,12 @@ | |||
140 | 65 | control_box.pack_end (label, false, false, 0); | 84 | control_box.pack_end (label, false, false, 0); |
141 | 66 | } | 85 | } |
142 | 67 | 86 | ||
144 | 68 | private void control_switch_activated () { | 87 | private bool control_switch_activated () { |
145 | 88 | if (device.get_device_type () == NM.DeviceType.WIFI) { | ||
146 | 89 | client.wireless_set_enabled (!client.wireless_get_enabled ()); | ||
147 | 90 | return false; | ||
148 | 91 | } | ||
149 | 92 | |||
150 | 69 | if (device.get_state () == NM.DeviceState.ACTIVATED) { | 93 | if (device.get_state () == NM.DeviceState.ACTIVATED) { |
151 | 70 | device.disconnect (null); | 94 | device.disconnect (null); |
152 | 71 | } else { | 95 | } else { |
153 | @@ -79,7 +103,7 @@ | |||
154 | 79 | } | 103 | } |
155 | 80 | } | 104 | } |
156 | 81 | 105 | ||
158 | 82 | this.switch_callback (); | 106 | return false; |
159 | 83 | } | 107 | } |
160 | 84 | 108 | ||
161 | 85 | public void get_activity_information (string iface, out string sent_bytes, out string received_bytes) { | 109 | public void get_activity_information (string iface, out string sent_bytes, out string received_bytes) { |
162 | 86 | 110 | ||
163 | === modified file 'src/Widgets/WiFi/WiFiPage.vala' | |||
164 | --- src/Widgets/WiFi/WiFiPage.vala 2015-07-23 20:51:21 +0000 | |||
165 | +++ src/Widgets/WiFi/WiFiPage.vala 2015-07-24 17:43:15 +0000 | |||
166 | @@ -23,7 +23,6 @@ | |||
167 | 23 | namespace Network.Widgets { | 23 | namespace Network.Widgets { |
168 | 24 | public class WiFiPage : Page { | 24 | public class WiFiPage : Page { |
169 | 25 | public new NM.DeviceWifi device; | 25 | public new NM.DeviceWifi device; |
170 | 26 | private InfoBox info_box; | ||
171 | 27 | private Gtk.ListBox wifi_list; | 26 | private Gtk.ListBox wifi_list; |
172 | 28 | private List<WiFiEntry> entries; | 27 | private List<WiFiEntry> entries; |
173 | 29 | private const string[] BLACKLISTED = { "Free Public WiFi" }; | 28 | private const string[] BLACKLISTED = { "Free Public WiFi" }; |
174 | @@ -37,6 +36,8 @@ | |||
175 | 37 | this.device = wifidevice; | 36 | this.device = wifidevice; |
176 | 38 | this.icon_name = "network-wireless"; | 37 | this.icon_name = "network-wireless"; |
177 | 39 | this.title = _("Wi-Fi Network"); | 38 | this.title = _("Wi-Fi Network"); |
178 | 39 | info_box = new InfoBox.from_device (device); | ||
179 | 40 | this.init (wifidevice, info_box); | ||
180 | 40 | 41 | ||
181 | 41 | entries = new List<WiFiEntry> (); | 42 | entries = new List<WiFiEntry> (); |
182 | 42 | 43 | ||
183 | @@ -51,10 +52,6 @@ | |||
184 | 51 | scrolled.vexpand = true; | 52 | scrolled.vexpand = true; |
185 | 52 | scrolled.shadow_type = Gtk.ShadowType.OUT; | 53 | scrolled.shadow_type = Gtk.ShadowType.OUT; |
186 | 53 | 54 | ||
187 | 54 | info_box = new info_box.from_device (device); | ||
188 | 55 | info_box.margin_end = this.INFO_BOX_MARGIN; | ||
189 | 56 | info_box.info_changed.connect (update); | ||
190 | 57 | |||
191 | 58 | var disconnect_btn = new Gtk.Button.with_label (_("Disconnect")); | 55 | var disconnect_btn = new Gtk.Button.with_label (_("Disconnect")); |
192 | 59 | disconnect_btn.sensitive = (device.get_state () == NM.DeviceState.ACTIVATED); | 56 | disconnect_btn.sensitive = (device.get_state () == NM.DeviceState.ACTIVATED); |
193 | 60 | disconnect_btn.get_style_context ().add_class ("destructive-action"); | 57 | disconnect_btn.get_style_context ().add_class ("destructive-action"); |
194 | @@ -90,7 +87,7 @@ | |||
195 | 90 | device.access_point_added.connect (add_access_point); | 87 | device.access_point_added.connect (add_access_point); |
196 | 91 | device.access_point_removed.connect (remove_access_point); | 88 | device.access_point_removed.connect (remove_access_point); |
197 | 92 | 89 | ||
199 | 93 | update (); | 90 | update (info_box); |
200 | 94 | 91 | ||
201 | 95 | this.add_switch_title (_("Wireless:")); | 92 | this.add_switch_title (_("Wireless:")); |
202 | 96 | this.add (scrolled); | 93 | this.add (scrolled); |
203 | @@ -99,14 +96,6 @@ | |||
204 | 99 | this.show_all (); | 96 | this.show_all (); |
205 | 100 | } | 97 | } |
206 | 101 | 98 | ||
207 | 102 | private void update () { | ||
208 | 103 | string sent_bytes, received_bytes; | ||
209 | 104 | this.get_activity_information (device.get_iface (), out sent_bytes, out received_bytes); | ||
210 | 105 | info_box.update_activity (sent_bytes, received_bytes); | ||
211 | 106 | |||
212 | 107 | control_switch.active = (client.wireless_get_enabled () && device.get_state () == NM.DeviceState.ACTIVATED); | ||
213 | 108 | } | ||
214 | 109 | |||
215 | 110 | private void on_row_activated (Gtk.ListBoxRow row) { | 99 | private void on_row_activated (Gtk.ListBoxRow row) { |
216 | 111 | if (device != null) { | 100 | if (device != null) { |
217 | 112 | /* Do not activate connection if it is already activated */ | 101 | /* Do not activate connection if it is already activated */ |
218 | @@ -154,8 +143,10 @@ | |||
219 | 154 | 143 | ||
220 | 155 | /* Check if we are successfully connected to the requested point */ | 144 | /* Check if we are successfully connected to the requested point */ |
221 | 156 | if (device.get_active_access_point () == (row as WiFiEntry).ap) { | 145 | if (device.get_active_access_point () == (row as WiFiEntry).ap) { |
223 | 157 | foreach (var entry in entries) | 146 | entries.@foreach ((entry) => { |
224 | 158 | entry.set_status_point (false, false); | 147 | entry.set_status_point (false, false); |
225 | 148 | }); | ||
226 | 149 | |||
227 | 159 | (row as WiFiEntry).set_status_point (true, false); | 150 | (row as WiFiEntry).set_status_point (true, false); |
228 | 160 | } | 151 | } |
229 | 161 | } | 152 | } |
230 | @@ -190,23 +181,9 @@ | |||
231 | 190 | insert_on_top = true; | 181 | insert_on_top = true; |
232 | 191 | }); | 182 | }); |
233 | 192 | 183 | ||
234 | 193 | scan_for_duplicates.begin (); | ||
235 | 194 | wifi_list.show_all (); | 184 | wifi_list.show_all (); |
236 | 195 | } | 185 | } |
237 | 196 | 186 | ||
238 | 197 | private async void scan_for_duplicates () { | ||
239 | 198 | var entries_dup = entries.copy (); | ||
240 | 199 | entries.@foreach ((entry) => { | ||
241 | 200 | var ssid = entry.ap.get_ssid (); | ||
242 | 201 | |||
243 | 202 | entries_dup.@foreach ((entry_dup) => { | ||
244 | 203 | if (entry_dup.ap.get_ssid () == ssid) { | ||
245 | 204 | this.remove_access_point (entry_dup.ap); | ||
246 | 205 | } | ||
247 | 206 | }); | ||
248 | 207 | }); | ||
249 | 208 | } | ||
250 | 209 | |||
251 | 210 | private void add_access_point (Object ap) { | 187 | private void add_access_point (Object ap) { |
252 | 211 | var row = new WiFiEntry.from_access_point (ap as NM.AccessPoint); | 188 | var row = new WiFiEntry.from_access_point (ap as NM.AccessPoint); |
253 | 212 | if (!(row.ssid in BLACKLISTED) && row.ap.get_ssid () != null) { | 189 | if (!(row.ssid in BLACKLISTED) && row.ap.get_ssid () != null) { |
254 | @@ -219,21 +196,18 @@ | |||
255 | 219 | entries.append (row as WiFiEntry); | 196 | entries.append (row as WiFiEntry); |
256 | 220 | } | 197 | } |
257 | 221 | 198 | ||
259 | 222 | if ((ap as NM.AccessPoint) == device.get_active_access_point ()) | 199 | if ((ap as NM.AccessPoint) == device.get_active_access_point ()) { |
260 | 223 | row.set_status_point (true, false); | 200 | row.set_status_point (true, false); |
261 | 201 | } | ||
262 | 224 | } | 202 | } |
263 | 225 | 203 | ||
264 | 226 | private void remove_access_point (Object ap_removed) { | 204 | private void remove_access_point (Object ap_removed) { |
268 | 227 | var new_entries = new List<WiFiEntry> (); | 205 | entries.@foreach ((entry) => { |
269 | 228 | foreach (var entry in entries) { | 206 | if (((WiFiEntry) entry).ap == ap_removed) { |
270 | 229 | if ((entry as WiFiEntry).ap == ap_removed) { | 207 | entries.remove (entry); |
271 | 230 | entry.destroy (); | 208 | entry.destroy (); |
272 | 231 | } else { | ||
273 | 232 | new_entries.append (entry); | ||
274 | 233 | } | 209 | } |
278 | 234 | } | 210 | }); |
276 | 235 | |||
277 | 236 | entries = new_entries.copy (); | ||
279 | 237 | } | 211 | } |
280 | 238 | 212 | ||
281 | 239 | private int sort_func (Gtk.ListBoxRow r1, Gtk.ListBoxRow r2) { | 213 | private int sort_func (Gtk.ListBoxRow r1, Gtk.ListBoxRow r2) { |
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.