Network Manager GUI doesn't report success or failure

[gnu]

Half the frustration of using wireless on the OLPC is that you can never tell what's really going on, without going to a text console and killing off NetworkManager and doing it all manually. This is because the NM GUI provides zero status! It doesn't tell you what access point it is trying to connect to; it doesn't tell you whether that succeeded or failed, and for what reason if there's a failure.

It doesn't tell you anything at all about wired Ethernet networks, either -- not even whether it sees one, nor whether it's currently working or not.

Even the 12-year-old reviewing the OLPC for Ed Felten noticed the flakiness of the network connection: "When the so-so connection allows you to get on, the internet is one of the best features of the whole computer." http://www.freedom-to-tinker.com/?p=1187

It should report status, all the time, accurately. Which network connection(s) it's using now, which ones it's trying to bring up, what stage of the bringup they are in, and what the current status of the bringup is.

[cjb]

It should report status, all the time, accurately. Which network connection(s) it's using now, which ones it's trying to bring up, what stage of the bringup they are in, and what the current status of the bringup is.

NetworkManager passes through five-ish states on the way to bringing up the connection; a concrete suggestion, then, is to have five small blocks underneath the AP pyramid that light up as each stage is passed.

[HoboPrimate]

To follow up on cjb, what about having it this way:

The AP pyramid icon blinks when it is trying to establish a connection (like activities do), and in its rollover menu shows a progress bar (like with the battery) that fills up along those stages. Underneath the progress bar short information is given to the current stage (Preparing Device, Configuring device, Asking for IP, Confirming IP, Connection Established). So it look like:

ESSID
######000000000000
Asking for IP

When the connection is established, the AP pyramid icon stops blinking. At this stage, the rollover menu could optionally say Connection established (but that would be redundant, as the Pyramid is now visible).

When the connection failed, perhaps the Pyramid (or circle for meshed) should show that in some way (with an error badge or differentiation in color) and say at what stage it failed in the rollover menu:

ESSID
############000000
Failed when Confirming IP

(These informational pieces of text I took from what Network Manager in my laptop tells me).

This failed connection could remain visible in its disabled state, with options in its rollover menu like Re-connect, and Remove.

I think this should be assigned to interface design.

[HoboPrimate]

I just changed Component to interface-design! Shoot me if I shouldn't have done so (but is the proper component, I think).

[crschmidt]

With this type of UI, the primary use for #2864 seems much less urgent - the primary reason I wanted it was to get some information about what was going on with the wireless. (I do strongly agree that it's neccesary though!)

[Eben]

I think that the ideas proposed by HoboPrimate above are right in line with the rest of the UI. They don't clutter up an already busy screen or force information on those that won't understand it, but they provide all of the necessary info for those that actually want to dig a bit deeper into the complexities of the network.

I also think that applying the triangle shaped "alert" badge to a failed connection is a nice idea, since negative feedback can be important.

When a connection is established, the rollover palette should have the option to "Stop connecting." When a connection is successfully established, I'd rather have a signal strength bar that has a bit more fidelity than the icon itself, and a list of actions that can be performed on the connection ("Disconnect" likely being the only one). When a connection fails, as he mentioned, the options to "Re-connect" and "Remove" make sense, with the latter literally removing the icon from the view until a later time when it is found again by a new scan.

I can attach some mockups for these palettes when I get a chance.

[Eben]

Dan, are you going to handle the implementation of these palettes? It seems there's enough info here to inform the basic implementation, so I'll assign it to you in hopes that we can move forward on this. If you get stuck and need a visual spec, let me know. Otherwise, the general palette spec in #2006 should be a good start.

[HoboPrimate]

From #2985 ,the reporter wants to have information from network-manager on:

"RF Channel of the mesh or AP to which we are connected or trying to connect to"

[kimquirk]

Please see #2985 as well for ideas on what info is needed.

[dcbw]

Please test with latest builds; most of these issues have been fixed in post-580 or so. We should open more specific bugs for exact features to change in the current implementation; there are a few different things here that should have their own bugs.

[gnu]

This doesn't appear to be fixed in the non-Update.1 non-release. E.g. hovering over an access point circle in the Home screen only shows its SSID and the radio channel -- not its IP address. When it's trying to get an IP address, it doesn't show what stage it's at (e.g. "Asked for 1.2.3.4, no response yet").

It still also shows zero status for wired Ethernet connections.

[Eben]

This is kind of a mess of frustrations and ideas right now. I'll attempt to dilute:

1. We need adequate feedback when connecting. The icons (in the Neighborhod, and in the Frame) should pulse to indicate connection is in progress. If we have more detailed info, this can be shown in the palette. (I'd need more specific info about what info we have to spec this better.)
2. When a connection is established, we should show a notification to indicate this, and pulsing should stop.
3. When a connection fails, we should show a notification to indicate this, and pusling should stop. Also, the icon should be badges with a notification badge, and the palette should offer "Retry" and "Cancel" actions.
4. We should add some additional status info to the device palette, such as the the IP address. (I'm open to suggestions on what we should actually be exposing)

Martin, these items all relates to your work on AP/Mesh devices, so I'm assigning to you. We need the devices for 8.2.0, but we could certainly live without all of the additional goodies noted above. Let us know how much of this you've already considered/covered, and what you think you can get done. I'll let you triage this ticket accordingly.

[mtd]

Replying to Eben:

1. We need adequate feedback when connecting. The icons (in the Neighborhood, and in the Frame) should pulse to indicate connection is in progress. If we have more detailed info, this can be shown in the palette. (I'd need more specific info about what info we have to spec this better.)

I'm not too familiar with the reason it doesn't pulse when in the Frame (possibly it's just that it doesn't get added to the Frame before it's connected), but as it pulses in the Neighborhood I can't see why this'd be hard. As to what additional info is available, offhand I don't know but we can see.

2. When a connection is established, we should show a notification to indicate this, and pulsing should stop.

Sure - sounds like Sugar Notifications are easy to do from what you've told me on IRC, so that shouldn't be a big deal.

3. When a connection fails, we should show a notification to indicate this, and pulsing should stop. Also, the icon should be badges with a notification badge, and the palette should offer "Retry" and "Cancel" actions.

Sure. At a minimum we might be able to usefully define failure as "the AP/nm device state went straight from ACTIVATING to INACTIVE", which I think is the only non-obvious (to me) code path here.

4. We should add some additional status info to the device palette, such as the the IP address. (I'm open to suggestions on what we should actually be exposing)

Easy, I'd think.

Martin, these items all relates to your work on AP/Mesh devices, so I'm assigning to you.

Sure. To ensure nobody feels restricted from working on this, I'll note I haven't really touched the AP code itself in the course of dealing with #6933 (mesh devices), so the AP-related changes could be done independently.

I'll let you triage this ticket accordingly.

I don't know what I can get done before the 0.81.3 freeze. Disappointingly (I know it's a small amount of code, but it's a good amount of knowledge to do it right, and I've already got other bugs so I don't want to over-promise) I'd say expect nothing for 8.2.0 :( (of course then you can only be surprised).

I've taken a stab at triage based on my read of the wrap-up of http://lists.laptop.org/pipermail/devel/2008-June/015541.html , but corrections welcome.

[marco]

We didn't make a call about using the rel- prefix or not yet. But all the sugar bugs has been triaged without it, so I guess better to stay consistent.

[mtd]

I've decided to break this work out from #6995, since it's a bit simpler than #6995 and it will make the #6995 patch simpler if I can get this one out of the way first.

I'd like anyone interested to consider testing my patch on their joyride-stream XO by doing:

cd /home/olpc
git-clone git://dev.laptop.org/users/mdengler/sugar xo-sugar-2866
cd /usr/share/sugar/shell
for dir in hardware model view ; do sudo mv $dir ${dir}.orig ; sudo ln -s /home/olpc/xo-sugar-2866/src/${dir} ; done                                              

You should get results like this:

http://dev.laptop.org/~mdengler/2866/2866_screenshot_10.png

Open issues:

1) notifications show up as a black square. There must be some flaw in my notification code: http://dev.laptop.org/git?p=users/mdengler/sugar;a=blob;f=src/view/devices/network/notifications.py;h=20f74754778176e20728a055ca129aa00f2b57c5;hb=a73a2a91b40f48214bd62324d7e203a79cebf53d#l50 2) When one tries to join a mesh, the frame's mesh icon & palette are empty (mouse over the "empty" space just to the left of the speaker icon after clicking on a mesh icon in the Mesh/Neighborhood view to see what I mean).

They should be easy but if I never get anyone testing this it'll keep taking forever.

[mtd]

I've attached a patch for #2866 and #3993. W.r.t. Eben's requirements in comment 13 ( http://dev.laptop.org/ticket/2866#comment:13 ), this patch does everything except for the notifications (point number four).

The git-web commit can be seen at: http://dev.laptop.org/git?p=users/mdengler/sugar;a=commitdiff;h=e7d04695195bad11f3aff36d3674bae20a9e21d9

It'd be nice if this could get in to 0.82.1, but of course we'll see how people like it.

[marco]

Note that 0.82.1 is going to be rescheduled to Friday. Still we have a full week to try and get it in.

[mtd]

Excellent. I hope I can satisfy everyone by then. I've been running these patches for a while, and got some testing from a few volunteers on sugar@ two weeks back. So hopefully there aren't any huge issues lurking.

[HoboPrimate]

Don't want to be pushing others to do work :/ but the info could still be presented a little better. Answering Eben's question ( http://dev.laptop.org/ticket/2866#comment:13 ) about what information can be obtained from Network Manager about the connecting process, the nm-applet in my Gnome desktop tells of these 5 steps:

Preparing Device, Configuring device, Asking for IP, Confirming IP, Connection Established

If all, or some of these steps are happen in Sugar when connecting, then it would be good to have this information in the network icon secondary palette, especially when something goes wrong and the user can then see at which step it failed.

Adding an acompanying progress bar (of 5 steps) ( http://dev.laptop.org/ticket/2866#comment:2 ) at the top of the secondary palette would be the cherry on top of this cake.

[mtd]

Replying to HoboPrimate:

Don't want to be pushing others to do work :/ but the info could still be presented a little better.

Well it's not really being presented at all (keep in mind I haven't told eben the steps available, and they're likely to change a bit in 0.7). Thanks for the suggestions - it's easy to do something nice with them (see below).

If all, or some of [NM activation steps] are happen in Sugar when connecting

Indeed, I'm seeing NM "activation stages" easily:

grep wireless-model ~/.sugar/default/logs/shell.log
1219615816.969442 DEBUG root: wireless-model: None activation stage now 1
1219615817.027726 DEBUG root: wireless-model: None activation stage now 2
1219615821.281653 DEBUG root: wireless-model: cdc2 activation stage now 4
1219615821.288158 DEBUG root: wireless-model: cdc2 activation stage now 4
1219615826.206515 DEBUG root: wireless-model: cdc2 activation stage now 5
1219615826.217288 DEBUG root: wireless-model: cdc2 activation stage now 5
1219615826.233362 DEBUG root: wireless-model: cdc2 activation stage now 6
1219615826.240385 DEBUG root: wireless-model: cdc2 activation stage now 6
1219615827.270801 DEBUG root: wireless-model: cdc2 activation stage now 7
1219615827.376867 DEBUG root: wireless-model: cdc2 activation stage now 7

(the None/cdc2 text is what nmclient.py tells me the ssid is)

it would be good to have this information in the network icon secondary palette, especially when something goes wrong and the user can then see at which step it failed.

I agree, though strings are an issue right now, I think. So far I've been quite careful to avoid changing any, and the only one that is new - "IP address" - is 1) new - I seem to remember that's better than a change; 2) quite integral; and 3) arguable unnecessary to translate.

Adding an acompanying progress bar (of 5 steps) ( http://dev.laptop.org/ticket/2866#comment:2 ) at the top of the secondary palette would be the cherry on top of this cake.

This would be easy, but we already have a progress bar for strength; suggestions welcome for how to differentiate the two (do you really mean in the secondary text? That's...hmm...interesting...if it were possible and somehow "smaller", it'd fit quite nicely nuzzled below the "Connecting..." secondary text, perhaps).

[mtd]

I've added three patches, designed to be applied in order on top of v0.82.0, corresponding to:

0001: Adds "IP address" to the mesh & wireless palettes, with associated changes to their model classes. (fixes #2866)

0002: Fix some inconsistency in the icon states by cleaning up the code. (fixes #3993 and #6944)

0003: Makes both frame icons pulse. (addresses one of Eben's requirements from comment 13 above)

I have pared them down quite a bit from my earlier patch, because I didn't want to distract from the essence. I still would like to apply a tweaked version of my earlier patch to Sugar HEAD/trunk, because a) it better prepares for #6995's patch (which has been deferred to 9.1); and b) it cleans up some minor hairiness.

[tomeu]

0001--2866-add-IP-address-to-wireless-ap-palettes.patch

'ip-changed':               (gobject.SIGNAL_RUN_FIRST, 
                             gobject.TYPE_NONE, ([])), 

Any reason why this isn't a property like what you have done in model/devices/network/mesh.py?

        def _padded(child, xalign=0, yalign=0.5): 

I'm not sure if we have any benefit from having funcs local to methods, as we should strive to write focused classes with few methods (a dozen max?). Thus perhaps in sake of keeping things simple we should refrain from using local funcs and just use regular private methods? This is just my opinion, not sure if it's shared by anyone else here.

ip_address_text = "%s: %s" % (_("IP address"), ip_address) 

Is this correct for all languages? Don't exist languages that would use a symbol different to ':' or have different conventions about spaces, word positions, etc? I would have done it like this:

ip_address_text = _("IP address: %s") % ip_address

so that translators have more choice about how to translate it.

And I prefer to use ' instead of " because find it's more readable, but again it's my personal preference.

model.connect('notify::ip-address', self._ip_address_changed_cb) 

Just in case, we make signal handlers really private:

self.__ip_address_changed_cb

Although the code you are modifying is from before we adopted this convention.

self._chan_label.props.xalign = 0.0 

I know you are following the local convention in that code, but I really dislike using abbreviations in identifiers.

            ip_address_text = "%s: %s" % (_("IP address"), ip_address) 

The same string appears two times and thus will be offered for translation two times. Perhaps we can add a constant to view/devices/network/init.py?

0003--2866-wireless-mesh-frame-icons-pulse-while-connecti.patch

self._icon.props.base_color = pulse_color   # only temporarily 

What this comment mean?

[mtd]

Replying to tomeu:

0001--2866-add-IP-address-to-wireless-ap-palettes.patch {{{ 'ip-changed': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE, ([])), }}} Any reason why this isn't a property like what you have done in model/devices/network/mesh.py?

I think you're looking at nmclient.py.

{{{ def _padded(child, xalign=0, yalign=0.5): }}} I'm not sure if we have any benefit from having funcs local to methods, as we should strive to write focused classes with few methods (a dozen max?). Thus perhaps in sake of keeping things simple we should refrain from using local funcs and just use regular private methods? This is just my opinion, not sure if it's shared by anyone else here.

Yeah I can't really justify adding that function on the class, but it seems worse to duplicate the code since it's more than a handful of lines than the little bit of decrease in simplicity.

{{{ ip_address_text = "%s: %s" % (_("IP address"), ip_address) }}} Is this correct for all languages? Don't exist languages that would use a symbol different to ':' or have different conventions about spaces, word positions, etc? I would have done it like this: {{{ ip_address_text = _("IP address: %s") % ip_address }}} so that translators have more choice about how to translate it.

Thanks -- I've fixed that.

And I prefer to use ' instead of " because find it's more readable, but again it's my personal preference.

I found plenty of pre-existing "s so I left them in but I've noted your feedback and will take it into account in the future.

[signal/event callbacks should use ] Although the code you are modifying is from before we adopted this convention.

Yeah, so as discussed on IRC I've left it consistent but undesirable for now for this 0.82.0 patch, but my larger (but more invasive) patch for master has this change (and changes all event/signal cb methods so they are consistently like that).

[self._chan_label] I know you are following the local convention in that code, but I really dislike using abbreviations in identifiers.

Again, left as-is on 0.82.0 for consistency. Will update my bigger master patch to take this into account.

{{{ ip_address_text = "%s: %s" % (_("IP address"), ip_address) }}} The same string appears two times and thus will be offered for translation two times. Perhaps we can add a constant to view/devices/network/init.py?

Thanks -- I hadn't thought of that. I've fixed the issue so we only offer the string for translation once, but as discussed on IRC, to avoid changing more files than already touched, for 0.82.0, I've put the translatable string in wireless.py. I will update my larger, master patch exatcly as you requested.

0003--2866-wireless-mesh-frame-icons-pulse-while-connecti.patch {{{ self._icon.props.base_color = pulse_color # only temporarily }}} What this comment mean?

This is there because it may seem odd to initialize that variable to that value, but it will get set to an appropriate value in _update_state(). Because the appropriate value is not known without applying the logic in _update_state(), because it's possible (though improbable; I'd consider it a type of race) for the base_color property to be tested/use between this code and _update_state(), and because until this property is set any testing/using of it will raise an Exception, I initialize the property to a known value so that Exception can't really happen. If you suggest a clearer comment (I didn't want to make it so long that I would have to spend a whole line on it), I will definitely incorporate that into my larger, master patch.

As discussed on IRC I think I've handled all your comments, so I'm setting r+ and pushing to sucrose-0.82 branch. I will update my larger patch and ask for a review/push to master separately.

Thanks for the review.

[mtd]

I've pushed to sucrose-0.82.

|TestCase|

Associate with an access point / mesh. Expose frame and hover over the frame icon representing the AP/mesh. When the secondary palette appears, verify the correct IP address is shown there.

|TestCase|

Change to the neighborhood view. Expose the frame via the frame key. Click on an access point / mesh icon. Observe the resulting frame icon - it should pulse as the Neighborhood icon is. When the connection is established, confirm the frame icon stops pulsing and becomes "solid".

[HoboPrimate]

Replying to mtd:

This would be easy, but we already have a progress bar for strength; suggestions welcome for how to differentiate the two (do you really mean in the secondary text? That's...hmm...interesting...if it were possible and somehow "smaller", it'd fit quite nicely nuzzled below the "Connecting..." secondary text, perhaps).

The progress bar for strength should be a different one than regular progress bars. Looking at the hig, I found "Level Indicators" at http://sugarlabs.org/go/UITeam/Hig/Sugar (just like the battery and network indicators on cell phones). This would differentiate it from the smooth and temporary connecting progress bar.

[mtd]

Replying to HoboPrimate:

Replying to mtd:

This would be easy, but we already have a progress bar for strength; suggestions welcome for how to differentiate the two (do you really mean in the secondary text? That's...hmm...interesting...if it were possible and somehow "smaller", it'd fit quite nicely nuzzled below the "Connecting..." secondary text, perhaps).

The progress bar for strength should be a different one than regular progress bars. Looking at the hig, I found "Level Indicators" at http://sugarlabs.org/go/UITeam/Hig/Sugar (just like the battery and network indicators on cell phones). This would differentiate it from the smooth and temporary connecting progress bar.

Shall we take this discussion to #6995 now, perhaps?

Check out http://dev.laptop.org/~mdengler/6995/6995_screenshot_50.png on your way; I don't know how to get the exact look and feel of the level indicators you mention but the stock gtk ProgressBar with DISCRETE style comes close.

[marco]

mtd tested this in joyride.

[cscott]

Exact package & version, please, and tag as dist-olpc3-testing. Thanks.

[marco]

The fix landed in sugar-0.82.1-1 but we need the following package in 8.2:

sugar-0.82.2-1.fc9

I tagged it.

[cscott]

Committed to stable repo: http://mock.laptop.org/gitweb/gitweb.cgi?p=repos;a=commitdiff;h=e0489cd6dec4b910445f489632adb44ba136c1c1

Should be in build 758 and following; please test.

[mtd]

I tested #2866 and #3993 on 8.2-759 and they seem to behave as desired and without any introducing any bugs.

Moving to finalize as marcopg requested.