After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 691285 - network: implement the new design
network: implement the new design
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Network
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
: 684826 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-01-07 11:54 UTC by Matthias Clasen
Modified: 2013-01-30 19:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
squashed patch (839.28 KB, patch)
2013-01-07 13:39 UTC, Matthias Clasen
needs-work Details | Review
network: add git.mk to subdir Makefile.ams (1.17 KB, patch)
2013-01-23 19:40 UTC, Dan Winship
accepted-commit_now Details | Review
network: add VPN support to the connection editor (38.07 KB, patch)
2013-01-23 19:41 UTC, Dan Winship
none Details | Review
network: add VPN support to the connection editor (38.07 KB, patch)
2013-01-28 18:56 UTC, Dan Winship
reviewed Details | Review

Description Matthias Clasen 2013-01-07 11:54:27 UTC
The wip/networking2 branch contains an implementation of newer network panel designs by Allan. What is there now:

- convert wifi ap list to egglistbox
- split out saved connections to a History dialog
- support for multiple wired profiles
- add a connection editor and use it for eding wifi and wired connections

Still missing:

- use the internal connection editor for vpn, mobile, vlan, etc
- never call out to nm-connection-editor
- a new add connection dialog

Caveats:

- the connection editor code is very similar to nm-connection-editor
- in particular the wireless-security code is almost a 1-1 copy (unfortunately, some minor edits to the ui files were necessary).

I'd like to get some early code review.
Comment 1 Matthias Clasen 2013-01-07 13:39:27 UTC
Created attachment 232902 [details] [review]
squashed patch
Comment 2 Bastien Nocera 2013-01-08 08:49:04 UTC
Review of attachment 232902 [details] [review]:

If the ce- code is nearly a 1-1 with the nm-connection-editor code, do you have a script to do automatic substitutions, so we can look into updating the code every now and then?

Finally, the new code needs to use GResource as all our code does now.

(Should I be reviewing the NM-based code as well?)

::: panels/network/Makefile.am
@@ +44,3 @@
 	rfkill.h
 
+libnetwork_la_LIBADD = $(PANEL_LIBS) $(NETWORK_PANEL_LIBS) $(NETWORK_MANAGER_LIBS) $(builddir)/connection-editor/libconnection-editor.la

You're not linking against the code under wireless-security/ ?

::: panels/network/net-device-ethernet.c
@@ +186,3 @@
+
+        if (ip4_address && ip6_address) {
+                add_details_row (details, i++, _("IP4 Address"), ip4_address);

IPv4?

@@ +187,3 @@
+        if (ip4_address && ip6_address) {
+                add_details_row (details, i++, _("IP4 Address"), ip4_address);
+                add_details_row (details, i++, _("IP6 Address"), ip6_address);

and IPv6?

@@ +191,3 @@
+                add_details_row (details, i++, _("IP Address"), ip4_address);
+        } else if (ip6_address) {
+                add_details_row (details, i++, _("IP Address"), ip6_address);

I would show "IPv6 address" even if there's only an IPv4 address.
When IPv6 on their own are more common, we can change that.

@@ +555,3 @@
+        } else {
+                a = nm_device_get_active_connection (nm_device);
+                if (a) {

No braces for single line blocks.

::: panels/network/net-device-simple.c
@@ +128,2 @@
                 g_string_append_printf (status, " - %s", speed);
+                g_free (speed);

Can you commit that separately please? And cherry-pick in gnome-3-6.

::: panels/network/net-device-wifi.c
@@ +1631,3 @@
+        guint sa, sb;
+
+#if 0

Remove dead code?

@@ +1711,3 @@
+        gtk_window_set_default_size (GTK_WINDOW (dialog), 600, 400);
+
+        button = gtk_button_new_with_mnemonic (_("_Close"));

Any reason not to use gtk_button_new_from_stock() with GTK_STOCK_CLOSE?

@@ +1718,3 @@
+                                  G_CALLBACK (gtk_widget_destroy), dialog);
+
+        forget = gtk_button_new_with_mnemonic (_("_Forget"));

Context here, and a translator comment.

::: panels/network/panel-common.c
@@ +660,3 @@
+panel_get_ip4_address_as_string (NMIP4Config *ip4, const gchar *what)
+{
+        return get_ipv4_config_address_as_string (ip4, what);

Where do those come from? Helpers from NM? If they're in our codebase, we could just export those instead.
Comment 3 Bastien Nocera 2013-01-08 08:52:31 UTC
You'll also need to update POTFILES.in and POTFILES.skip as appropriate.
Comment 4 Matthias Clasen 2013-01-08 15:36:56 UTC
I've mostly based this work on the mockups here: https://github.com/gnome-design-team/gnome-mockups/tree/master/system-settings/network/aday/png
Comment 6 Matthias Clasen 2013-01-09 13:46:59 UTC
(In reply to comment #2)
> Review of attachment 232902 [details] [review]:
> 
> If the ce- code is nearly a 1-1 with the nm-connection-editor code, do you have
> a script to do automatic substitutions, so we can look into updating the code
> every now and then?

No script, no. We can look at the diff between wireless-security/ in the branch and nm-c-e and figure out if a script is feasible.

To be clear: stuff in wireless-security/ is almost 1-1 copied. stuff in connection-editor/ is 'strongly inspired' by the nm-c-e code, but rewritten incrementally, so there's more differences.

 
> Finally, the new code needs to use GResource as all our code does now.

Ok, will do (probably not before the weekend, unless danw beats me to it)
 
> (Should I be reviewing the NM-based code as well?)
> 
> ::: panels/network/Makefile.am
> @@ +44,3 @@
>      rfkill.h
> 
> +libnetwork_la_LIBADD = $(PANEL_LIBS) $(NETWORK_PANEL_LIBS)
> $(NETWORK_MANAGER_LIBS) $(builddir)/connection-editor/libconnection-editor.la
> 
> You're not linking against the code under wireless-security/ ?

libconnection-editor is linked against libwireless-security. Not sure if that is kosher or not, but it works.


> ::: panels/network/net-device-ethernet.c
> @@ +186,3 @@
> +
> +        if (ip4_address && ip6_address) {
> +                add_details_row (details, i++, _("IP4 Address"), ip4_address);
> 
> IPv4?

Dunno, Allans mockups consistently left out the 'v'. Allan ?


> 
> @@ +191,3 @@
> +                add_details_row (details, i++, _("IP Address"), ip4_address);
> +        } else if (ip6_address) {
> +                add_details_row (details, i++, _("IP Address"), ip6_address);
> 
> I would show "IPv6 address" even if there's only an IPv4 address.
> When IPv6 on their own are more common, we can change that.

Your proposal is to always have separate 'IP4 Address' and 'IP6 Address' fields ? Or to only show them if they are set, but always indicate whether they are 4 or 6 ?


> ::: panels/network/net-device-simple.c
> @@ +128,2 @@
>                  g_string_append_printf (status, " - %s", speed);
> +                g_free (speed);
> 
> Can you commit that separately please? And cherry-pick in gnome-3-6.

Done
Comment 7 Bastien Nocera 2013-01-09 17:27:58 UTC
Review of attachment 232902 [details] [review]:

All the fixes pushed to wip/networking2

::: panels/network/Makefile.am
@@ +44,3 @@
 	rfkill.h
 
+libnetwork_la_LIBADD = $(PANEL_LIBS) $(NETWORK_PANEL_LIBS) $(NETWORK_MANAGER_LIBS) $(builddir)/connection-editor/libconnection-editor.la

Not a bug.

::: panels/network/net-device-ethernet.c
@@ +186,3 @@
+
+        if (ip4_address && ip6_address) {
+                add_details_row (details, i++, _("IP4 Address"), ip4_address);

Fixed.

@@ +187,3 @@
+        if (ip4_address && ip6_address) {
+                add_details_row (details, i++, _("IP4 Address"), ip4_address);
+                add_details_row (details, i++, _("IP6 Address"), ip6_address);

Fixed.

@@ +191,3 @@
+                add_details_row (details, i++, _("IP Address"), ip4_address);
+        } else if (ip6_address) {
+                add_details_row (details, i++, _("IP Address"), ip6_address);

Typos in my comment.

I think it should read IPv6 even if IPv6 is the only address. Fixed.

::: panels/network/net-device-simple.c
@@ +128,2 @@
                 g_string_append_printf (status, " - %s", speed);
+                g_free (speed);

Fixed.

::: panels/network/net-device-wifi.c
@@ +1631,3 @@
+        guint sa, sb;
+
+#if 0

Removed.

@@ +1711,3 @@
+        gtk_window_set_default_size (GTK_WINDOW (dialog), 600, 400);
+
+        button = gtk_button_new_with_mnemonic (_("_Close"));

Fixed.

@@ +1718,3 @@
+                                  G_CALLBACK (gtk_widget_destroy), dialog);
+
+        forget = gtk_button_new_with_mnemonic (_("_Forget"));

Fixed.

::: panels/network/panel-common.c
@@ +660,3 @@
+panel_get_ip4_address_as_string (NMIP4Config *ip4, const gchar *what)
+{
+        return get_ipv4_config_address_as_string (ip4, what);

Fixed.
Comment 8 Dan Winship 2013-01-21 18:14:51 UTC
VPN editing (and creation) is on wip/networking2 now too. It does not follow the "New VPN Connection" mockup in add-connection.png though; it uses the same set of pages as the other editors do.
Comment 9 Matthias Clasen 2013-01-21 21:34:44 UTC
Some initial impressions:

On the Add dialog:

- needs a frame around the listboxes

- looks like there should be some more padding above the listboxes, maybe ?

- when creating a vpn connection, the connection editor comes up with no page selected in the list - identity should be selected


On the VPN connection editor:

- should probably replace "Configure" with a gears button like we have for Wired, to be consistent

- the Details page looks a bit barren, with just the Last used item

- "Use hybrid authentication" looks misaligned - shouldn't it line up with the checkbox right above ?

- I think the Advanced dialog should probably be converted into another page - I guess the "Identification" stuff could go onto the Identity page, and the "Transport and Security" stuff could become a "Security" page.
Comment 10 Matthias Clasen 2013-01-21 21:44:16 UTC
I think we should maybe reconsider the "Details" page - it is relatively well-populated for wireless, but basically empty for all other connection types. We could just fold the information into the other pages, ie the current IP address should show up on the IP4 page, the DNS server addresses should show up there too, etc.

Actually, we should do that regardless whether we keep the Details page or not.
Comment 11 Dan Winship 2013-01-21 22:49:37 UTC
(In reply to comment #9)
> On the VPN connection editor:

> - "Use hybrid authentication" looks misaligned - shouldn't it line up with the
> checkbox right above ?
> 
> - I think the Advanced dialog should probably be converted into another page -
> I guess the "Identification" stuff could go onto the Identity page, and the
> "Transport and Security" stuff could become a "Security" page.

Yeah... 

So here's the catch; we don't actually control that UI; the VPN plugin just hands us a page, and we add it to the notebook.

Of course, open source, etc, etc, so we can extend NMVpnPluginUiInterface to do whatever we want... Or we could ignore the plugins and just hardcode UIs for whichever VPN types we consider important (and I guess fall back to the current code for any other types?).

Either way, we need designs... and there's no one-size-fits-all. I guess all VPNs have a gateway, and all of them have some concept of authenticating yourself to them, so we can have standard places to put that. And we could throw all the rest onto an "other stuff" page, or maybe multiple VPN-type-dependent "other stuff" pages, since the VPN types vary greatly in how much other stuff they have. (OpenConnect has just a handful of checkboxes in the main UI. VPNC has an Advanced dialog that most people don't need to touch. OpenVPN has an "Advanced" dialog with a 4-page notebook full of random options that many people do need to adjust, so it probably shouldn't even be called "Advanced"...)


Alternatively, we're not *completely* at the mercy of the existing plugin-provided UIs... the code currently does adjust the labels, to turn

    Foo:     [________]
    Bar baz: [________]

into

         Foo [________]
     Bar baz [________]

for consistency with everything else... so, maybe we could do some more heuristic stuff like that to improve things? (Though that won't let us integrate "Advanced" dialogs.)
Comment 12 Matthias Clasen 2013-01-22 03:34:49 UTC
ok. sounds like pretty much the same situation I was in with the wireless-security. Lets leave it like this for now, then. Medium-term, we should perhaps get the plugins to hand us two pages, instead of one page with an "advanced" button in it. Looking at nm-c-e, it would benefit from that, too.
Comment 13 Dan Winship 2013-01-23 19:40:11 UTC
ok, so it basically comes down to 2 commits on top of Bastien's existing stuff on networking2, plus 2 more commits for the add connection bug. Since Bastien has already reviewed all the older stuff, I'm only attaching the new stuff.

remaining issues:
  - There's a FIXME in there about the gray text in the VPN type selection
    list... I assume there must be code somewhere to do that in a theme-
    safe way?

  - The "Details" page is still there, with the issues Matthias describes

  - The IP/DNS config status has not been moved to the IP pages; they're
    busy-looking enough already that I didn't want to try to figure out a
    nice place to fit in the new information myself... Designers?
Comment 14 Dan Winship 2013-01-23 19:40:58 UTC
Created attachment 234248 [details] [review]
network: add git.mk to subdir Makefile.ams
Comment 15 Dan Winship 2013-01-23 19:41:02 UTC
Created attachment 234249 [details] [review]
network: add VPN support to the connection editor

Unfortunately, the VPN plugins provide their own .ui files for their
editor pages, so we can't make them look competely GNOME-3-ish. But
the code does try to fix them up a little bit by realigning the
labels.

vpn-helpers.[ch] is nearly identical to network-manager-applet's,
but eventually this code will move into libnm-gtk.
Comment 16 Matthias Clasen 2013-01-23 23:45:56 UTC
(In reply to comment #13)

> remaining issues:
>   - There's a FIXME in there about the gray text in the VPN type selection
>     list... I assume there must be code somewhere to do that in a theme-
>     safe way?

Use the 'dim-label' style class. In a ui file, that works via

<style>
  <class name='dim-label'/>
</style>


>   - The "Details" page is still there, with the issues Matthias describes
>
>   - The IP/DNS config status has not been moved to the IP pages; they're
>     busy-looking enough already that I didn't want to try to figure out a
>     nice place to fit in the new information myself... Designers?

Lets deal with these 2 issues later, need to get some design input indeed.
Comment 17 Dan Winship 2013-01-28 18:56:12 UTC
Created attachment 234643 [details] [review]
network: add VPN support to the connection editor

fix to use dim-label style
Comment 18 Bastien Nocera 2013-01-29 16:10:37 UTC
Review of attachment 234248 [details] [review]:

Sure.
Comment 19 Bastien Nocera 2013-01-29 16:15:00 UTC
Review of attachment 234643 [details] [review]:

I'm guessing that the vpn-* pages are cut'n'paste?
If so, good to go.

::: configure.ac
@@ +193,3 @@
+     NM_VPN_CONFIG_DIR=/etc/NetworkManager/VPN
+  else
+     NM_VPN_CONFIG_DIR=$nm_prefix/etc/NetworkManager/VPN

That's particularly hideous. Can you make sure to add a bugzilla reference to above this?
Comment 20 Dan Winship 2013-01-29 16:33:02 UTC
(In reply to comment #19)
> I'm guessing that the vpn-* pages are cut'n'paste?

Yes. Eventually to move to libnm-gtk.

> +     NM_VPN_CONFIG_DIR=/etc/NetworkManager/VPN
> +  else
> +     NM_VPN_CONFIG_DIR=$nm_prefix/etc/NetworkManager/VPN
> 
> That's particularly hideous. Can you make sure to add a bugzilla reference to
> above this?

it's not really a bug... if $prefix is /usr then $sysconfdir is going to be /etc, and if not it's going to be $prefix/etc.

Maybe it would be clearer as:

  if test $nm_prefix = /usr then;
      nm_sysconfdir=/etc
  else
      nm_sysconfdir=$nm_prefix/etc
  NM_VPN_CONFIG_DIR=$nm_sysconfdir/NetworkManager/VPN

?
Comment 21 Bastien Nocera 2013-01-29 16:35:29 UTC
(In reply to comment #20)
> it's not really a bug... if $prefix is /usr then $sysconfdir is going to be
> /etc, and if not it's going to be $prefix/etc.
> 
> Maybe it would be clearer as:
> 
>   if test $nm_prefix = /usr then;
>       nm_sysconfdir=/etc
>   else
>       nm_sysconfdir=$nm_prefix/etc
>   NM_VPN_CONFIG_DIR=$nm_sysconfdir/NetworkManager/VPN

I think the location should be exported by NetworkManager if it can't be trivially appended to one of the existing variables.
NM_VPN_CONFIG_DIR=`pkg-config --variable etc....`
Comment 22 Dan Winship 2013-01-30 18:06:36 UTC
Pushed after squashing all the various bugfixes
Comment 23 Bastien Nocera 2013-01-30 19:18:48 UTC
*** Bug 684826 has been marked as a duplicate of this bug. ***