Created attachment 355348 [details] [review]
network: Replace the notebook with a stack

The Network panel uses a GtkNotebook internally to manage
the different setup pages of the network devices. While it
does the job, we now have a modern widget for that: GtkStack.

With GtkStack, managing the pages becomes a lot easier and
we gain almost for free the nice transition between pages,
besides of course being a widget that consumes slightly less
resources.

Besides all these gains, using a GtkStack will allow us to
implement the new Wi-Fi panel in a more cohesive manner,
sharing large portions of code and avoiding copy pasta.

This commit then turns the GtkNotebook into a GtkStack, and
renames and adapts the code to reflect that. Fortunately,
the code got actually simpler with the move.

Created attachment 355349 [details] [review]
network: Don't manage Wi-Fi devices

The Wi-Fi devices are going to managed with the
to-be-introduced Wi-Fi panel, and don't need to
be available in the Networks panel anymore.

This patch then blacklists Wi-Fi devices and don't
let the Networks panel manage them.

Created attachment 355350 [details] [review]
network: Cleanup code

Now that we're splitting the Network panel into the
Wi-Fi, we can make a few cleanups.

Created attachment 355351 [details] [review]
network: Rework NetDeviceWifi interface

The UI definitions of the Wi-Fi devices currently contain many
widgets in the stack, such as the tower icon, the enable/disable
switch and the status.

In the new Wi-Fi panel, all those widgets will clutter the
interface and break the entire UI.

Fix that by splitting those widgets in two different containers:

  1. The header_box container, with the menu button and the
     enable/disable switch.
  2. The center_box widget, with the title and status labels,
     which will be consumed by the Wi-Fi panel to be the center
     widget of the headerbar.

This commit also introduces two getters that expose those two
containers. With that, another load of code could be simplified.

Created attachment 355352 [details] [review]
wifi: Introduce Wi-Fi panel

The glory moment has come. The new Wi-Fi panel is finally
introduced using a different code style from the rest of
the Network panel, since Control Center itself is written
using the GTK+ C code style.

The Wi-Fi panel uses modern GTK+ features like template
classes and new widgets. The files are stored together
with the Network panel so that we can reuse the abstraction
layer that the Network panel has to manage devices.

Created attachment 355353 [details] [review]
network: Add Wi-Fi widgets using device product as title

When there are multiple Wi-Fi devices, we must show a stack
switcher in the header if the Wi-Fi panel with the name of
the device.

The problem is that, currently, NetDeviceWifi does not add
its widgets to the main stack setting a stack title, and so
the stack switcher is empty.

Fix that by always adding the widgets to the stack using the
device product name as title.

Created attachment 355360 [details] [review]
wifi: Introduce Wi-Fi panel

The glory moment has come. The new Wi-Fi panel is finally
introduced using a different code style from the rest of
the Network panel, since Control Center itself is written
using the GTK+ C code style.

The Wi-Fi panel uses modern GTK+ features like template
classes and new widgets. The files are stored together
with the Network panel so that we can reuse the abstraction
layer that the Network panel has to manage devices.

Created attachment 355501 [details] [review]
wifi: disable "activatable" property in the Airplane Mode row

The Airplane Mode box contains a 'GtkListBoxRow' widget which
does not provide any extra functionality if it's clicked.
So it makes sense to disable the activatable property for
avoiding possible confusions about the funcionality.

Review of attachment 355348 [details] [review]:

looks better indeed

Review of attachment 355349 [details] [review]:

just commit message nits:
... are going to *be* managed ... the *Network* panel anymore.
... devices and *doesn't* let... the *Network* panel...

Review of attachment 355350 [details] [review]:

... splitting Wi-Fi out of the Network panel... sounds better, also, I'd merge this commit with the previous but it can stay separate as well

Review of attachment 355351 [details] [review]:

lgtm

Review of attachment 355353 [details] [review]:

... in the header *of* the Wi-Fi panel...

Review of attachment 355360 [details] [review]:

what happens if NM is not running or has 0 wifi devices?

::: panels/network/cc-wifi-panel.c
@@ +123,3 @@
+
+  result = g_dbus_proxy_get_cached_property (self->rfkill_proxy, "HasAirplaneMode");
+  enabled = g_variant_get_boolean (result);

result might be NULL and must be unrefed

@@ +126,3 @@
+
+  result = g_dbus_proxy_get_cached_property (self->rfkill_proxy, "ShouldShowAirplaneMode");
+  should_show = g_variant_get_boolean (result);

idem

@@ +133,3 @@
+
+  result = g_dbus_proxy_get_cached_property (self->rfkill_proxy, "AirplaneMode");
+  enabled = g_variant_get_boolean (result);

idem

@@ +136,3 @@
+
+  result = g_dbus_proxy_get_cached_property (self->rfkill_proxy, "HardwareAirplaneMode");
+  hw_enabled = !!g_variant_get_boolean (result);

!! ??
idem

@@ +250,3 @@
+  if (error)
+    {
+      g_printerr ("Error creating rfkill proxy: %s\n", error->message);

this should handle error being G_IO_ERROR_CANCELLED (panel finalized before the proxy shows up) and not print a message in that case

@@ +321,3 @@
+  g_clear_pointer (&self->device_id_to_listbox, g_hash_table_destroy);
+
+  G_OBJECT_CLASS (cc_wifi_panel_parent_class)->finalize (object);

needs to free ->client and ->devices too

@@ +339,3 @@
+                            GParamSpec   *pspec)
+{
+  CcWifiPanel *self = CC_WIFI_PANEL (object);

this raises a variable unused compilation warning

@@ +343,3 @@
+  switch (prop_id)
+    {
+    case PROP_PARAMETERS:

do you intend to add something here?

@@ +387,3 @@
+  gtk_widget_init_template (GTK_WIDGET (self));
+
+  self->device_id_to_listbox = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);

unused hash table

::: panels/network/gnome-wifi-panel.desktop.in.in
@@ +2,3 @@
+# Translators: Add soft hyphens to your translations so that the icon view won't clip your translations. See https://bugzilla.gnome.org/show_bug.cgi?id=647087#c13 for details
+_Name=Wi-Fi
+_Comment=Control how you connect to the Wi-Fi

not a native speaker but "... to Wi-Fi networks" sounds better to me

@@ +15,3 @@
+X-GNOME-Bugzilla-Component=network
+X-GNOME-Bugzilla-Version=@VERSION@
+# Translators: those are keywords for the network control-center panel

... the *wi-fi* ...

@@ +16,3 @@
+X-GNOME-Bugzilla-Version=@VERSION@
+# Translators: those are keywords for the network control-center panel
+_Keywords=Network;Wireless;Wi-Fi;Wifi;IP;LAN;Proxy;WAN;Broadband;Modem;DNS;

I'd leave at least Proxy, WAN and Modem out

Review of attachment 355501 [details] [review]:

nit: there are a few typos in the commit message

also, it would be nice if this was a done as a review since this is new code

Review of attachment 355360 [details] [review]:

should we remove the airplane switch from the network panel?

::: panels/network/wifi.ui
@@ +139,3 @@
+                    </child>
+                    <child>
+                      <object class="GtkSpinner" id="spinner">

unused?

(In reply to Rui Matos from comment #14)
> Review of attachment 355360 [details] [review] [review]:
> 
> what happens if NM is not running or has 0 wifi devices?

First case is legit, and I'll fix the code.

Second case will be addressed later, and not in the panel. The idea is to make panels dynamic, so that the Wi-Fi panel won't be visible when you don't have wi-fi adapters.

(In reply to Rui Matos from comment #16)
> ::: panels/network/wifi.ui
> @@ +139,3 @@
> +                    </child>
> +                    <child>
> +                      <object class="GtkSpinner" id="spinner">
> 
> unused?

disregard this comment, I see where it's used

A UI design comment regarding the height of the wifi panel overall. By default, I see the airplane mode switch plus 3 wifi networks only which doesn't seem ideal. We should find a way to show more wifi networks by default and perhaps the airplane mode row/switch doesn't need to be in such a central/top position as it's not used so often?

(In reply to Georges Basile Stavracas Neto from comment #17)
> (In reply to Rui Matos from comment #14)
> > Review of attachment 355360 [details] [review] [review] [review]:
> > 
> > what happens if NM is not running or has 0 wifi devices?
> 
> First case is legit, and I'll fix the code.
> 
> Second case will be addressed later, and not in the panel. The idea is to
> make panels dynamic, so that the Wi-Fi panel won't be visible when you don't
> have wi-fi adapters.


This is a interesting thing to discuss about. Consider the following use case or scenario:
It the user plugs a Wi-Fi dongle with the CC window already open, the Wi-Fi option will be shown instantanelly in the menu? or in other case, maybe like Bluetooth panel, show an empty state with the tipical "Unabailable blah blah" message.

Attachment 355348 [details] pushed as 158591a - network: Replace the notebook with a stack
Attachment 355349 [details] pushed as 3317e88 - network: Don't manage Wi-Fi devices
Attachment 355351 [details] pushed as cf62c0a - network: Rework NetDeviceWifi interface
Attachment 355353 [details] pushed as b12a56d - network: Add Wi-Fi widgets using device product as title

Created attachment 355828 [details] [review]
wifi: Introduce Wi-Fi panel

- Addressed all the code issues
 - Added "NetworkManager not running" and "No Wi-Fi Adapters" pages
 - Added capacity to handle command-line arguments
 - Merged changes from attachment 355501 [details] [review] (non-activatable Airplane Mode listbox row)

Review of attachment 355828 [details] [review]:

almost there :-)

::: panels/network/cc-wifi-panel.c
@@ +204,3 @@
+      title_widget = net_device_wifi_get_title_widget (NET_DEVICE_WIFI (net_device));
+
+      gtk_stack_add_named (self->center_stack, title_widget, "single");

if we add device A, then B and then B gets removed, won't we try to add the same widget with the same name to this stack again?

i.e. below on the else branch we should always remove the "single" named widget from the stack

@@ +235,3 @@
+	self->arg_operation = OPERATION_NULL;
+	g_clear_pointer (&self->arg_device, g_free);
+	g_clear_pointer (&self->arg_access_point, g_free);

inconsistent indentation and, please, no tabs!

@@ +334,3 @@
+		  return TRUE;
+	  }
+}

indentation is all over the place in this function, seems to have tabs

@@ +397,3 @@
+  self->rfkill_proxy = g_dbus_proxy_new_for_bus_finish (res, &error);
+
+  if (error && !g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))

if there's an error and it's CANCELLED we still need to free the error variable

Created attachment 355856 [details] [review]
wifi: Introduce Wi-Fi panel

- Remove tabs from the code (sorry!)
 - Fix header stack management

Review of attachment 355856 [details] [review]:

with that addressed, looks fine, thanks

::: panels/network/cc-wifi-panel.c
@@ +230,3 @@
+      if (single_page_widget)
+        {
+          g_object_ref (single_page_widget);

I believe this isn't needed (and is a leak without a corresponding unref) since the reference is kept in the NetDeviceWifi's GtkBuilder instance

(In reply to Rui Matos from comment #25)
> Review of attachment 355856 [details] [review] [review]:
> 
> with that addressed, looks fine, thanks
> 
> ::: panels/network/cc-wifi-panel.c
> @@ +230,3 @@
> +      if (single_page_widget)
> +        {
> +          g_object_ref (single_page_widget);
> 
> I believe this isn't needed (and is a leak without a corresponding unref)
> since the reference is kept in the NetDeviceWifi's GtkBuilder instance

I unfortunately don't have hardware to really test it, so for safety reasons, I'll wrap the gtk_container_remove() with the ref()/unref() standard.

Thanks for the reviews. I'm in beer/coffee/juice debt :)

Attachment 355856 [details] pushed as 016efdf - wifi: Introduce Wi-Fi panel