GNOME Bugzilla – Bug 779841
Rework Network panel's dialogs
Last modified: 2017-08-09 17:46:42 UTC
The Network panel still have a bunch of dialogs that make no usage of GtkHeaderBar. Let's start the redesign by moving those dialogs to use a header bar widget.
Created attachment 347610 [details] [review] network: Don't use GtkVBox GtkVBox is deprecated and shouldn't be used anymore, so drop the usage of this class in the Wi-Fi editor and other parts of the panel.
Created attachment 347611 [details] [review] network: Make connection editor dialog use a headerbar In order to start moving towards the new redesigned connection editor [1], it is necessary to make the dialog use a headerbar. Currently, however, it doesn't, making the connection editor dialog inconsistent with the rest of the panels, and the GNOME desktop as a whole. Fix that by exporting the buttons as action buttons, and setting the use-header-bar property to TRUE. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Created attachment 347612 [details] [review] network: Make the history dialog use a headerbar In order to start moving towards the new redesigned Network panel [1], it is necessary to make the dialogs use a headerbar. Currently, however, they don't, making the dialog sinconsistent with the rest of the panels, and the GNOME desktop as a whole. Fix that by setting the headerbar to the History dialog. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Created attachment 349183 [details] [review] network: Make the hotspot dialog use a headerbar In order to start moving towards the new redesigned Network panel [1], it is necessary to make the dialogs use a headerbar. Currently, however, they don't, making the dialog sinconsistent with the rest of the panels, and the GNOME desktop as a whole. Fix that by setting the headerbar to the History dialog. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Created attachment 349217 [details] [review] network: Update the hotspot dialog Update the hotspot dialog according to the new redesigned Network panel[1]. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Review of attachment 347610 [details] [review]: Can't justify landing this while there's still plenty of other VBox usage in this panel (and one HBox fwiw).
Review of attachment 347611 [details] [review]: looks good
Review of attachment 347612 [details] [review]: ::: panels/network/net-device-wifi.c @@ +1851,3 @@ + /* GTK_RESPONSE_HELP is needed to move the button to the start of the headerbar + * without hiding the close button. */ + gtk_dialog_add_action_widget (GTK_DIALOG (dialog), forget, GTK_RESPONSE_HELP); This is a gtk+ implementation detail that I'm afraid will bite us later. I'd prefer that the button is explicitly packed instead
Review of attachment 349217 [details] [review]: ::: panels/network/net-device-wifi.c @@ +1248,3 @@ window = gtk_widget_get_toplevel (GTK_WIDGET (button)); + str = g_string_new (_("<big><b>Turn On Wi-Fi Hotspot?</b></big>")); This needs UI review I'm afraid or, for now, you can just use the same string that we're already using in the .ui file for title @@ +1272,3 @@ g_signal_connect (dialog, "response", G_CALLBACK (start_hotspot_response_cb), device_wifi); g_signal_connect (dialog, "delete-event", Since we're now creating new dialog instances, these events handlers need to destroy them or we'll leak
Comment on attachment 347611 [details] [review] network: Make connection editor dialog use a headerbar Attachment 347611 [details] pushed as fe2a21e - network: Make connection editor dialog use a headerbar
Created attachment 352285 [details] [review] network: Make the history dialog use a headerbar Explicitly use a headerbar rather than relying on the implicit one that GTK+ provides us.
Created attachment 352291 [details] [review] network: Update the hotspot dialog Use the previous dialog title, and always destroy the dialog instances on reponse.
Created attachment 352296 [details] [review] network: Make the history dialog use a headerbar Actually, the previous patch was not matching the mockup. This one is. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Created attachment 352298 [details] [review] network: Update the hotspot dialog Actually, the previous patch wasn't following the mockup. Updated the patch to strictly match the mockups.
Review of attachment 352298 [details] [review]: looks good, thanks
Review of attachment 352296 [details] [review]: ::: panels/network/net-device-wifi.c @@ -1903,3 @@ - gtk_dialog_add_action_widget (GTK_DIALOG (dialog), button, 0); - g_signal_connect_swapped (button, "clicked", - G_CALLBACK (gtk_widget_destroy), dialog); without an equivalent to this we're leaking the dialog
Comment on attachment 352298 [details] [review] network: Update the hotspot dialog Attachment 352298 [details] pushed as 9cc065b - network: Update the hotspot dialog
Created attachment 352363 [details] [review] network: Make the history dialog use a headerbar In order to start moving towards the new redesigned Network panel [1], it is necessary to make the dialogs use a headerbar. Currently, however, they don't, making the dialog sinconsistent with the rest of the panels, and the GNOME desktop as a whole. Fix that by setting the headerbar to the History dialog, and adjusting the elements so hat they match the "Known Wi-Fi Networks" dialog in the mockup [1]. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Review of attachment 352363 [details] [review]: ++
Comment on attachment 352363 [details] [review] network: Make the history dialog use a headerbar Attachment 352363 [details] pushed as 4ac4f4a - network: Make the history dialog use a headerbar
Created attachment 352444 [details] [review] network: Rely on notebook tabs to switch connection editor pages Instead of using a sidebar, which is what the current implementation uses to switch pages, the latest mockups [1] rely on the notebook tabs. This patch then updates the connection editor dialog to use the notebook tabs as the page switcher widget. [1] https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/network/aday2/network-wires.png
Created attachment 352445 [details] [review] network: Reduce connection editor dialog width The current default width is 600px, which is a bit too much and leaves a lot of blank space. Fix that by reducing the default width to 500px.
Created attachment 352446 [details] [review] network: Align details page labels at start Instead of keeping the labels centralized, align them at the start of the dialog, as the mockup [1] proposes. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352447 [details] [review] network: Move checkboxes to Details page In the advanced connection editor dialog, currently, the options to share a connection with all users and to connect automatically are inside the Identity page. The redesigned connection editor, however, features these options in the Details page as per the mockup [1]. Fix this by moving these options to the Details page. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352448 [details] [review] network: Drop Reset page Following the latest mockups [1], the "Forget" button is now available in the Details page, and the Reset page is gone. This patch then removes the Reset page, and moves the functionality to the Details page. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352449 [details] [review] network: Adapt forget button label according to connection type Per the mockup [1], the Forget button has different labels depending on the connection type. For example, when editing a VPN connection, the Forget button reads "Remove VPN", while when editing a Wi-Fi connection reads "Forget Connection". This patch adapts the forget button label according to the connection type. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352450 [details] [review] network: Move Security page to the end Per the mockups at [1], the Security page is the last visible page. This patch also adds a small code refactoring to avoid multiple string comparisons. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352451 [details] [review] network: Use radio buttons instead of a combobox in IP pages Per the latest mockups [1], the IP pages use a set of four radio buttons to control the method, rather than a switch + a combobox, which is what the current implementation uses. This patch, then, adapts the IP pages of the connection editor dialog to use a set of radio buttons at the top of the page. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352452 [details] [review] network: Simplify DNS management in connection editor When editing the DNS servers of a given connection, a simple entry is enough to display and edit the DNS servers. The user can separate IP addresses with commas. This is exemplified by the mockup at [1]. This, however, is not the current implementation, which uses a combination of listbox rows, entries and buttons to manage that with added complexity. Fix that by using an entry to handle the DNS servers. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352453 [details] [review] network: Use a table-like widget to edit routes According to the latest mockups for the connection editor dialog [1], the IPv4 and IPv6 pages are supposed to use a table-like editor to manage the routes. This editor is not only easier to comprehend, but also improves the size of the dialog, requiring much less vertical space to present the routes. The current implementation, however, uses a vertical layout and a toolbar, which is inefficient in its usage of space. Fix that by implementing the table-like editor widget, both in IPv4 and IPv6 pages. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352454 [details] [review] network: Use a table-like widget in address editor According to the latest mockups for the connection editor dialog [1], the IPv4 and IPv6 pages are supposed to use a table-like editor to manage the addresses, in a similar fashion of what was done to the routes editor. This way of editing is not only easier to comprehend, but also improves the size of the dialog, requiring much less vertical space to present the routes. The current implementation, however, uses a vertical layout and a toolbar, which is inefficient in its usage of space. Fix that by implementing the table-like editor widget, both in IPv4 and IPv6 pages. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 352455 [details] [review] network: Improve alignment and spacing This patch improves the alignment and spacing of IPv4 and IPv6 widgets so that they look uniform and consistent.
Review of attachment 352445 [details] [review]: ok
Review of attachment 352444 [details] [review]: looks good
Review of attachment 352446 [details] [review]: ::: panels/network/connection-editor/details-page.ui @@ -18,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> - <property name="hexpand">True</property> why are these removed? @@ +70,3 @@ <property name="label">1Mb/sec</property> <property name="selectable">True</property> + <property name="hexpand">True</property> and these added? hexpand doesn't seem to have any effect here so if anything we should just remove the ones above
Review of attachment 352447 [details] [review]: ce-page-vpn.c also has an all_user_check that should probably be removed since it will show up in the Details tab already
Review of attachment 352448 [details] [review]: looks fine, please double check the button's string ::: panels/network/connection-editor/details-page.ui @@ +358,3 @@ + <child> + <object class="GtkButton" id="button_forget"> + <property name="label" translatable="yes">_Forget</property> the mockup says 'Forget Connection'
Review of attachment 352449 [details] [review]: aha, ok, disregard the previous comment
Review of attachment 352448 [details] [review]: btw, we're losing the reset functionality. did someone from the design team ask for that or was it just overlooked in the mockups? ::: panels/network/connection-editor/details-page.ui @@ +358,3 @@ + <child> + <object class="GtkButton" id="button_forget"> + <property name="label" translatable="yes">_Forget</property> the mockup says 'Forget Connection'
Review of attachment 352450 [details] [review]: looks fine
Attachment 352444 [details] pushed as b62ed2f - network: Rely on notebook tabs to switch connection editor pages Attachment 352445 [details] pushed as 6da5f2f - network: Reduce connection editor dialog width
Review of attachment 352451 [details] [review]: the ipv6 comments apply to the ipv4 file as well ::: panels/network/connection-editor/ce-page-ip6.c @@ +590,3 @@ + radios[2] = GTK_TOGGLE_BUTTON (gtk_builder_get_object (CE_PAGE (page)->builder, "radio_local")); + radios[3] = GTK_TOGGLE_BUTTON (gtk_builder_get_object (CE_PAGE (page)->builder, "radio_manual")); + radios[4] = GTK_TOGGLE_BUTTON (gtk_builder_get_object (CE_PAGE (page)->builder, "radio_disabled")); this one is already in priv->disabled @@ +595,3 @@ + g_signal_connect (radios[1], "toggled", G_CALLBACK (method_changed), page); + g_signal_connect (radios[2], "toggled", G_CALLBACK (method_changed), page); + g_signal_connect (radios[3], "toggled", G_CALLBACK (method_changed), page); could use a loop to do this @@ +599,3 @@ + switch (method) { + case IP6_METHOD_AUTO: + gtk_toggle_button_set_active (radios[0], TRUE); I'd prefer if all this array indexing used the enum values as indices to make it more readable @@ +611,3 @@ + break; + case IP6_METHOD_IGNORE: + gtk_toggle_button_set_active (radios[4], TRUE); I think _SHARED should be handled here too and you can remove the default: case
(In reply to Rui Matos from comment #35) > Review of attachment 352446 [details] [review] [review]: > > ::: panels/network/connection-editor/details-page.ui > @@ -18,3 @@ > <property name="visible">True</property> > <property name="can_focus">False</property> > - <property name="hexpand">True</property> > > why are these removed? > > @@ +70,3 @@ > <property name="label">1Mb/sec</property> > <property name="selectable">True</property> > + <property name="hexpand">True</property> > > and these added? > > hexpand doesn't seem to have any effect here so if anything we should just > remove the ones above Not really. This is a workaround to GtkGrid's behavior. Because the many labels and multiple widgets below spanning more than 1 column, the simplest solution here is to make ~all~ right labels expand horizontally and "push" the first column to use the minimum width.
(In reply to Rui Matos from comment #39) > Review of attachment 352448 [details] [review] [review]: > > btw, we're losing the reset functionality. did someone from the design team > ask for that or was it just overlooked in the mockups? This is intentional. Quoting IRC: aday: the distinction between reset and forget is a bit confusing currently - that might be one reason aday: you could also argue that the utility of the reset option is fairly low
Created attachment 353693 [details] [review] network: Move checkboxes to Details page (In reply to Rui Matos from comment #36) > Review of attachment 352447 [details] [review] [review]: > > ce-page-vpn.c also has an all_user_check that should probably be removed > since it will show up in the Details tab already Nice catch! Thanks, that's fixed now.
Created attachment 353694 [details] [review] network: Use radio buttons instead of a combobox in IP pages Thanks for the review. The code indeed looks cleaner now!
Review of attachment 352452 [details] [review]: same comments apply to the ipv6 page ::: panels/network/connection-editor/ce-page-ip4.c @@ +350,3 @@ gint i; + entry = GTK_ENTRY (gtk_builder_get_object (CE_PAGE (page)->builder, "dns_entry")); could use priv->dns_entry @@ +727,3 @@ if (g_str_equal (method, NM_SETTING_IP4_CONFIG_METHOD_AUTO) || g_str_equal (method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL)) + dns_addresses = g_strsplit (dns_text, ", ", -1); g_strsplit_set() would be more lenient for the user. it would allow multiple spaces, spaces only and comas only for separators @@ +734,3 @@ const gchar *text; + text = g_strstrip (dns_addresses[i]); if you use g_strsplit_set() above this isn't needed @@ +737,3 @@ if (!*text) { /* ignore empty rows */ + widget_unset_error (page->dns_entry); this check for empty strings could be done in the for() loop condition, i.e. add && *dns_addresses[i] also, the comment doesn't make sense since there are no rows anymore @@ +743,2 @@ if (text && !nm_utils_ipaddr_valid (AF_INET, text)) { + widget_set_error (page->dns_entry); in this case I think we should clear the dns_servers array and break; otherwise there might be a malformed address in the middle and at the end of the loop the entry will still be with the non-error style and we'd silently ignore the malformed address @@ +842,3 @@ goto out; + ignore_auto_dns = g_utf8_strlen (dns_text, -1) > 0; this should use dns_servers->len > 0 otherwise even a random string in the entry will trigger it but, more importantly, this isn't the same as what currently exists. NM by default *adds* the statically configured dns addresses to the automatic ones which are obtained from dhcp. this change means that we can only do static addresses or automatic now. perhaps we need design input here
Review of attachment 352453 [details] [review]: same comments for ipv6 ::: panels/network/connection-editor/ce-page-ip4.c @@ +148,3 @@ +static gboolean +validate_row (GtkWidget *row) a better name would be empty_row() or row_is_empty() @@ +254,2 @@ delete_button = gtk_button_new (); + gtk_widget_set_sensitive (delete_button, FALSE); doesn't seem to belong in this patch @@ +838,3 @@ g_ptr_array_add (routes, route); + + if (!l || !l->next) !l will never happen here @@ +839,3 @@ + + if (!l || !l->next) + ensure_empty_routes_row (page); do we really need this with all the connections above to each entry's ::activate signal? or perhaps this is enough and we don't need all those since this is called on every change? @@ +853,2 @@ ignore_auto_dns = g_utf8_strlen (dns_text, -1) > 0; + ignore_auto_routes = routes != NULL; same comment as for ignore_auto_dns from the dns patch
Review of attachment 352454 [details] [review]: seems good
Review of attachment 352455 [details] [review]: ok
Review of attachment 353693 [details] [review]: ok
Review of attachment 353694 [details] [review]: with that change, looks fine ::: panels/network/connection-editor/ce-page-ip4.c @@ +656,3 @@ + gtk_toggle_button_set_active (radios[RADIO_DISABLED], TRUE); + break; + default: please add IP4_METHOD_SHARED here too (effectively making it look like DISABLED in the UI) and remove the default: switch label. same in the ipv6 page
Review of attachment 353693 [details] [review]: ::: panels/network/connection-editor/ce-page-vpn.c @@ -127,3 @@ g_signal_connect_swapped (page->name, "changed", G_CALLBACK (ce_page_changed), page); - - widget = GTK_WIDGET (gtk_builder_get_object (CE_PAGE (page)->builder, this 'widget' variable isn't needed anymore and causes an unused variable compiler warning
Review of attachment 352448 [details] [review]: ok, I just wanted to be sure dropping reset was intended. fwiw, I think it's ok to drop it too
Review of attachment 352446 [details] [review]: ok then
Attachment 352446 [details] pushed as 10d4eea - network: Align details page labels at start Attachment 352448 [details] pushed as 8137036 - network: Drop Reset page Attachment 352449 [details] pushed as 3458566 - network: Adapt forget button label according to connection type Attachment 352450 [details] pushed as 18a42a0 - network: Move Security page to the end Attachment 353693 [details] pushed as ce289c3 - network: Move checkboxes to Details page Attachment 353694 [details] pushed as 8f78b27 - network: Use radio buttons instead of a combobox in IP pages
Created attachment 355365 [details] [review] network: Simplify DNS management in connection editor When editing the DNS servers of a given connection, a simple entry is enough to display and edit the DNS servers. The user can separate IP addresses with commas. This is exemplified by the mockup at [1]. This, however, is not the current implementation, which uses a combination of listbox rows, entries and buttons to manage that with added complexity. Fix that by using an entry to handle the DNS servers. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 355366 [details] [review] network: Use a table-like widget to edit routes According to the latest mockups for the connection editor dialog [1], the IPv4 and IPv6 pages are supposed to use a table-like editor to manage the routes. This editor is not only easier to comprehend, but also improves the size of the dialog, requiring much less vertical space to present the routes. The current implementation, however, uses a vertical layout and a toolbar, which is inefficient in its usage of space. Fix that by implementing the table-like editor widget, both in IPv4 and IPv6 pages. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 355367 [details] [review] network: Use a table-like widget in address editor According to the latest mockups for the connection editor dialog [1], the IPv4 and IPv6 pages are supposed to use a table-like editor to manage the addresses, in a similar fashion of what was done to the routes editor. This way of editing is not only easier to comprehend, but also improves the size of the dialog, requiring much less vertical space to present the routes. The current implementation, however, uses a vertical layout and a toolbar, which is inefficient in its usage of space. Fix that by implementing the table-like editor widget, both in IPv4 and IPv6 pages. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/network/aday2/network-wires.png
Created attachment 355368 [details] [review] network: Improve alignment and spacing This patch improves the alignment and spacing of IPv4 and IPv6 widgets so that they look uniform and consistent.
Comment on attachment 355368 [details] [review] network: Improve alignment and spacing Readded previously accepted patches.
Review of attachment 355365 [details] [review]: same for ipv6 page ::: panels/network/connection-editor/ce-page-ip4.c @@ +765,1 @@ if (dns_servers->len == 0) { hmm, we should clear the error from the entry if len == 0. right now if you input a wrong address and then delete it the entry will still be red
Review of attachment 355366 [details] [review]: still think validate_row() could be better named but no biggie. looks fine
Review of attachment 355367 [details] [review]: looks good, thanks for the patches! ::: panels/network/connection-editor/ce-page-ip4.c @@ +246,2 @@ delete_button = gtk_button_new (); gtk_widget_set_sensitive (delete_button, FALSE); this line should have been in this patch, not the previous ;-)
Attachment 355365 [details] pushed as 2e57009 - network: Simplify DNS management in connection editor Attachment 355366 [details] pushed as 08657fa - network: Use a table-like widget to edit routes Attachment 355367 [details] pushed as 1d0757d - network: Use a table-like widget in address editor Attachment 355368 [details] pushed as ae59c77 - network: Improve alignment and spacing