Created attachment 187776 [details] [review]
Support wimax devices

Created attachment 187777 [details]
Screenshot of wimax part of the menu

Review of attachment 187776 [details] [review]:

Generally looks a straightforward port of NMDeviceWifi (with the advantage that one nspObj always holds exactly one NSP), and therefore should work. I did not test it, though, as I have no WiMax.
Some minor comments here and there.

::: js/ui/status/network.js
@@ +223,3 @@
+        this._signalIcon = new St.Icon({ icon_name: this._getIcon(),
+                                         style_class: 'popup-menu-icon' });
+        this._icons.add_actor(this._signalIcon);

You don't need a BoxLayout if you use just one icon.
(You can even inherit from PopupImageMenuItem instead of PopupBaseMenuItem)

@@ +225,3 @@
+        this._icons.add_actor(this._signalIcon);
+
+        this._updateId = nsp.connect('notify::signal_quality', Lang.bind(this, this._updated))

I think it should be 'notify::signal-quality' here.

@@ +1003,3 @@
+
+    destroy: function() {
+        if (this._nspChangedId) {

Where is this signal id connected?

@@ +1021,3 @@
+    _nspSortFunction: function(a, b) {
+        // sort alphabetically
+        return GLib.utf8_collate(a.nsp.get_name(), b.nsp.get_name());

Shouldn't you sort known NSPs first?

@@ +1026,3 @@
+    _findNsp: function(nsp) {
+        for (let i = 0; i < this._nsps.length; i++) {
+            if (nsp.name == this._nsps[i].name)

Unless libnm-glib plays with proxies, you can check for object equality, which should be faster.

@@ +1065,3 @@
+        let pos = this._findNsp(nsp);
+        let nspObj;
+        let needsupdate = false;

Unused variable.

@@ +1175,3 @@
+                continue;
+
+            this._createNspItem(nspObj, j + activeOffset);

How are you sure that this never overflows the screen?
Is there some kind of pratical limit on the number of visible NSPs?

@@ +1248,3 @@
+                    if (nspObj.item) {
+                        if (nspObj.item instanceof PopupMenu.PopupSubMenuMenuItem) {
+                            let items = nspObj.item.menu.getMenuItems();

I think this should be _getMenuItems()

(Bonus points if you avoid using a private function)

@@ +1998,3 @@
+            section: new PopupMenu.PopupMenuSection(),
+            devices: [ ],
+            item: this._makeToggleItem('wimax', _("WiMAX Mobile broadband"))

Would this fit in the broader concept of "Mobile Broadband"? Should the user understand the difference between GSM / CDMA and WiMax?

A different concern is if it is expected to have both GSM/CDMA and WiMax cards, in which case it would show the device name, which is worse than "Mobile Broadband" and "WiMax Mobile Broadband"

@@ +2321,3 @@
+                case NMConnectionCategory.WIMAX:
+                    icon = 'network-cellular-signal-excellent';
+                    banner = _("You're now connected to WiMAX mobile broadband connection '%s'").format(active._connection._name);

Same here. Do we want to emphasize the difference between WiMax and the other kinds of WWAN?

(In reply to comment #3)
> Review of attachment 187776 [details] [review]:
> 
> Generally looks a straightforward port of NMDeviceWifi (with the advantage that
> one nspObj always holds exactly one NSP), and therefore should work. I did not
> test it, though, as I have no WiMax.
> Some minor comments here and there.

[snip]

> @@ +1175,3 @@
> +                continue;
> +
> +            this._createNspItem(nspObj, j + activeOffset);
> 
> How are you sure that this never overflows the screen?
> Is there some kind of pratical limit on the number of visible NSPs?

There generally won't be more than 4 or 5, but there could be a few more than that in the future.  Because WiMAX networks use licensed spectrum (just like Cellular) and the operators had to pay a lot of money for it, it's unlikely we'll get a ton of networks in a scan.  There are MVNOs though (which rent excess capacity to piggy-back on an operator) which are shown as separate networks  in the scan list.

> @@ +1248,3 @@
> +                    if (nspObj.item) {
> +                        if (nspObj.item instanceof
> PopupMenu.PopupSubMenuMenuItem) {
> +                            let items = nspObj.item.menu.getMenuItems();
> 
> I think this should be _getMenuItems()
> 
> (Bonus points if you avoid using a private function)
> 
> @@ +1998,3 @@
> +            section: new PopupMenu.PopupMenuSection(),
> +            devices: [ ],
> +            item: this._makeToggleItem('wimax', _("WiMAX Mobile broadband"))
> 
> Would this fit in the broader concept of "Mobile Broadband"? Should the user
> understand the difference between GSM / CDMA and WiMax?

From the user's point of view, WiMAX is more like wifi; turn it on, there's a scan (which takes a bit longer than wifi, like 30 - 60 seconds) and then you can connect.  There's no complicated stuff like registration states, emergency-only restriction, SIM pin codes.  Its much more like wifi in that regard.  But in the end, no, there's not a ton of difference to the user, and we should just call both WiMAX and LTE "4G mobile broadband" or something like that.  So whenever we show technology indicators in the shell menu, we'll tag WiMAX with a "4G" badge and that'll probably be it.

But note that people may well have both a WiMAX device and a GSM/CDMA device in their machine at the same time, and they will have separate kill switches.  So I'm  not sure how to handle that part, because the on/off for CDMA/GSM won't turn on WiMAX, and vice-versa.  If you're a user and you care about power, you probably also don't want to have *both* wimax and cdma/gsm turned on at the same time.  So that complicates things a bit from the UI standpoint.

> A different concern is if it is expected to have both GSM/CDMA and WiMax cards,
> in which case it would show the device name, which is worse than "Mobile
> Broadband" and "WiMax Mobile Broadband"

Right, and that's a much more common case than having both a CDMA and a GSM card in the system at the same time.  It won't be uncommon to have both WiMAX and another mobile broadband card.

> @@ +2321,3 @@
> +                case NMConnectionCategory.WIMAX:
> +                    icon = 'network-cellular-signal-excellent';
> +                    banner = _("You're now connected to WiMAX mobile broadband
> connection '%s'").format(active._connection._name);
> 
> Same here. Do we want to emphasize the difference between WiMax and the other
> kinds of WWAN?

Maybe.  I think we'd need some UI direction from Jon or somebody else about how to handle this...

So Jon McCann said we can leave it as WiMAX Mobile Broadband since it is different; we shouldn't try to combine it with the GSM/UMTS/CDMA/EVDO stuff.

Created attachment 206587 [details] [review]
network: add support for WiMAX devices

====

Rebased, though the only code-related change I made as a result was to
use Lang.Class(). Also, I fixed the signal disconnection to do the
GObject.Object.prototype.disconnect hack.

Unfortunately, I can't get gnome-shell to work out of jhbuild (crash
in cogl), so this is not actually tested. (Against master. I did test
it against 3.2.)

dcbw: I forget if there was anything specific you had wanted me to
look for in this patch? It seems to work fine. (I could connect to
CLEAR, at least enough to get to their "buy a service plan" pages.)
After testing out WiMaX, I was unable to get wifi working again until
rebooting, but I'm assuming that's the kernel wifi/wimax coexistence
problems we already know about.

(In reply to comment #6)
> Unfortunately, I can't get gnome-shell to work out of jhbuild (crash
> in cogl), so this is not actually tested.

Now it is, and it works in master too (subject to the same dueling-wimax-and-wifi isues that are probably Somebody Else's Problem).

Review of attachment 206587 [details] [review]:

Code in NMDeviceWimax to deal with menu updates is very similar to what was previously NMDeviceWireless, and could probably benefit from the same change, that is, using Main.queueDeferredWork and always rebuilding the UI rather than trying to understand what changed and where (although it's true we don't have a "More..." menu here).

::: js/ui/status/network.js
@@ +200,3 @@
+
+        let iconName = this._getIcon();
+        this._nsp = nsp;

Given that you're using Lang.Class, use this.parent (here and elsewhere)

@@ +215,3 @@
+    destroy: function() {
+        // see above for explanation
+

I assume _nsp is a NMWimaxNsp, right?
In that case, there is no nm_wimax_nsp_disconnect() that could conflict.
(and there is no "above" either).

Finally, before disconnecting, you should actually check _updateId (it happens that stuff is destroyed more than once, be should be not emit warnings)

@@ +939,3 @@
+
+    get connected() {
+                     };

_enabled is never set.
If the activation rules are those of Wifi, not of WWAN, then simply remove it.

@@ +1219,3 @@
+        connection.add_setting(new NetworkManager.SettingConnection({
+            uuid: connection._uuid,
+

Should it be "Auto <Name>", to be consistent with wifi?

@@ +1971,3 @@
+        };
+        this._devices.wimax.section.addMenuItem(this._devices.wimax.item);
+        this._devices.wimax.section.addMenuItem(new PopupMenu.PopupSeparatorMenuItem());

If the separator is inside the wimax section, it won't autocollapse (even though it should not be needed and it should be properly hidden anyway). Also, it breaks consistency with the other sections (and this part of code, with sections nested within sections, is pretty hairy - maybe at some point I'll kill one level of indirection and insert each device directly in the menu)

@@ +2500,3 @@
+                        this.setIcon('network-cellular-signal-' + signalToIcon(nsp.signal_quality));
+                    }));
+                    log('An active wimax connection involves no NSP?');

if (!this._activeNspUpdateId) => you need to show the icon and connect a signal.

(It was a logic bug that affected both wifi and wwan, but was fixed quite some time ago)

(In reply to comment #8)
> I assume _nsp is a NMWimaxNsp, right?
> In that case, there is no nm_wimax_nsp_disconnect() that could conflict.

ah, right, NMDeviceWimax.destroy was wrong in the original patch, but I ended up fixing some other parts that didn't need fixing.

> _enabled is never set.
> If the activation rules are those of Wifi, not of WWAN, then simply remove it.

I'm not sure. What's the difference?

(In reply to comment #9)
> (In reply to comment #8)
> > _enabled is never set.
> > If the activation rules are those of Wifi, not of WWAN, then simply remove it.
> 
> I'm not sure. What's the difference?

If you set NMClient::wireless-enabled (and I suppose NMClient::wimax-enabled), NetworkManager picks a connection for each device automatically. This means that toggling the switch does the expected thing and starts a connection. If you set NMClient::wwan-enabled but don't explicitly pick a connection, nothing happens instead.
For this reason, wireless devices (status item + content section) are hidden when wireless is disabled (there would be nothing to show anyway, because we have no scan results), whereas wwan devices are shown, but internally marked disabled (so that we don't show an useless "network unavailable")

Do we still want this?

Probably not. Wimax has basically failed as a technology, because mobile broadband got fast enough, and had a clearer upgrade path / business model.

OK, then. We can add it back if wanted.