GNOME Bugzilla – Bug 701954
More cleanups and fixes for the network menu
Last modified: 2013-06-14 18:21:46 UTC
It seems that every time I work on it, I manage to remove another 100 lines of code.
Created attachment 246426 [details] [review] network: Rename a variable for consistency We tend to use the name "a" in this method. This matches consistency with the rest of the code.
Created attachment 246427 [details] [review] network: Remove setActiveConnection/clearActiveConnection This can be more easily achieved by listening for changes to the device's active-connection property. VPN will still need support to track active connections, as it does not have an associated device. But as VPN can track multiple active connections, the names "set" and "clear" don't quite fit. Rename them to the more-standard "add" and "remove".
Created attachment 246428 [details] [review] network: Fix a bad signal name We try to disconnect from firmwareChangedId, not firmwareMissingId.
Created attachment 246429 [details] [review] network: Move indicator icon selection to individual devices This cuts down on the number of cross-connected "public" API between the devices, hopefully allowing us to reduce it further.
Created attachment 246430 [details] [review] network: Always show the VPN indicator when connected to VPN The new designs ask for an indicator per connected device. For now, limit ourselves to two icons, but we'll soon change this.
Review of attachment 246426 [details] [review]: Heh... consistency works the other way too, and "a" is quite small for ActiveConnection...
Review of attachment 246427 [details] [review]: Ok
Review of attachment 246426 [details] [review]: We tended to use "a" everywhere else, but I can switch it around if you want. I don't really like "active" though.
Review of attachment 246428 [details] [review]: The signal is named firmware-missing, not firmware-changed, so the one to correct is the disconnect.
Review of attachment 246429 [details] [review]: Do we need the mc parameter? Devices already have the .active_connection property Also, I don't see a getIndicatorIcon() in the vpn section.
Review of attachment 246430 [details] [review]: Bug 687853 had something similar to the final design (one indicator per device kind), if you want. Also, I think with this patch it's possible to have two vpn icons, if for example vpn is the default route.
(In reply to comment #11) > Review of attachment 246428 [details] [review]: > > The signal is named firmware-missing, not firmware-changed, so the one to > correct is the disconnect. Well, for notify::carrier we have carrierChangedId, so technically it would need to be firmwareMissingChangedId, but I figured that firmwareChangedId would be close enough. (In reply to comment #12) > Review of attachment 246429 [details] [review]: > Also, I don't see a getIndicatorIcon() in the vpn section. It was easier to simply keep the VPN indicator separate for now (it's the one special case as it doesn't have an associated device) but I suppose I can make a code cleanup...
(In reply to comment #14) > (In reply to comment #11) > > Review of attachment 246428 [details] [review] [details]: > > > > The signal is named firmware-missing, not firmware-changed, so the one to > > correct is the disconnect. > > Well, for notify::carrier we have carrierChangedId, so technically it would > need to be firmwareMissingChangedId, but I figured that firmwareChangedId would > be close enough. > > (In reply to comment #12) > > Review of attachment 246429 [details] [review] [details]: > > Also, I don't see a getIndicatorIcon() in the vpn section. > > It was easier to simply keep the VPN indicator separate for now (it's the one > special case as it doesn't have an associated device) but I suppose I can make > a code cleanup... (In reply to comment #14) > (In reply to comment #11) > > Review of attachment 246428 [details] [review] [details]: > > > > The signal is named firmware-missing, not firmware-changed, so the one to > > correct is the disconnect. > > Well, for notify::carrier we have carrierChangedId, so technically it would > need to be firmwareMissingChangedId, but I figured that firmwareChangedId would > be close enough. Bah... Ok with firmwareChangedId then. > (In reply to comment #12) > > Review of attachment 246429 [details] [review] [details]: > > Also, I don't see a getIndicatorIcon() in the vpn section. > > It was easier to simply keep the VPN indicator separate for now (it's the one > special case as it doesn't have an associated device) but I suppose I can make > a code cleanup... No, the problem is you can get the vpn to be a mainConnection, in which case you unconditonally call getIndicatorIcon() on it and that crashes.
(In reply to comment #10) > Review of attachment 246426 [details] [review]: > > We tended to use "a" everywhere else, but I can switch it around if you want. I > don't really like "active" though. Do we really? I think "a" comes from older code, newer uses "active", but ok, it's clear from the context anyway.
Created attachment 246445 [details] [review] network: Move indicator icon selection to individual devices This cuts down on the number of cross-connected "public" API between the devices, hopefully allowing us to reduce it further.
Created attachment 246446 [details] [review] network: Always show the VPN indicator when connected to VPN The new designs ask for an indicator per connected device. For now, limit ourselves to two icons, but we'll soon change this.
Created attachment 246447 [details] [review] network: Move the VPN indicator to getIndicatorIcon as well This removes the need to track the VPN connection in the main applet.
Created attachment 246448 [details] [review] network: Remove overflow of VPN configurations It's unlikely that somebody has more than five VPN connections configured, and the code is
Created attachment 246449 [details] [review] network: Rewrite VPN section to be independent of NMConnectionBased
Created attachment 246451 [details] [review] network: Merge NMConnectionBased back into NMDevice
Comment on attachment 246426 [details] [review] network: Rename a variable for consistency Attachment 246426 [details] pushed as 4cd832c - network: Rename a variable for consistency Let's push this one for now.
Review of attachment 246445 [details] [review]: I don't see any change in this, compared to the previous one.
Besides the "mc" removal, and using this.device.active_connection instead, you mean?
Attachment 246427 [details] pushed as 2cbee05 - network: Remove setActiveConnection/clearActiveConnection Attachment 246428 [details] pushed as c6fe6eb - network: Fix a bad signal name
Created attachment 246831 [details] [review] network: Remove dead code No class in here has this.carrier as a property. Presumably, this was meant to be this.device.carrier, but since this code is going to be rewritten soon anyway, might as well just junk the never-working code for now.
Created attachment 246832 [details] [review] network: "Remove" support for dial-up modems NetworkManager has never supported dial-up modems, which are the only case we have a modem device without any of these capabilities.
Created attachment 246833 [details] [review] network: Move indicator icon selection to individual devices This cuts down on the number of cross-connected "public" API between the devices, hopefully allowing us to reduce it further.
Created attachment 246834 [details] [review] network: Always show the VPN indicator when connected to VPN The new designs ask for an indicator per connected device. For now, limit ourselves to two icons, but we'll soon change this.
Created attachment 246835 [details] [review] network: Move the VPN indicator to getIndicatorIcon as well This removes the need to track the VPN connection in the main applet.
Created attachment 246836 [details] [review] network: Remove overflow of VPN configurations It's unlikely that somebody has more than five VPN connections configured, and the code is
Created attachment 246837 [details] [review] network: Rewrite VPN section to be independent of NMConnectionBased
Created attachment 246838 [details] [review] network: Merge NMConnectionBased back into NMDevice
Created attachment 246839 [details] [review] network: Make the device field private With getIndicatorIcon() replacing the main use of the .device field, we can make this a private API.
Created attachment 246840 [details] [review] network: Simplify connections to the firmware signal The status item will go away soon, so make sure the one-time fire is given its own function. At the same time, only connect to the signal when the situation actually matters.
Created attachment 246841 [details] [review] network: Remove support for virtual networking This isn't in the new design, and no good can come from this. Just allow people to use the control center to configure virtual networking.
Review of attachment 246831 [details] [review]: So you're positive we're never seeing wired in the menu again heh?
Review of attachment 246832 [details] [review]: Yes.
Review of attachment 246833 [details] [review]: Ok
Review of attachment 246834 [details] [review]: Ok
Review of attachment 246835 [details] [review]: Please squash this with the previous patch on getIndicatorIcon()
Review of attachment 246836 [details] [review]: Ok
Review of attachment 246837 [details] [review]: Oh, very nice cleanup!
Review of attachment 246838 [details] [review]: Yes
Review of attachment 246839 [details] [review]: Is this really needed?
Review of attachment 246839 [details] [review]: It was to make sure that the NMClient doesn't access wrapper.device for any device. It did when there was the _updateIcon mess beforehand, and I want to make sure that that doesn't come back.
Review of attachment 246840 [details] [review]: ::: js/ui/status/network.js @@ +416,3 @@ if (this._device.firmware_missing) { + if (!this._firmwareChangedId) + this._firmwareChangedId = this._device.connect('notify::firmware-missing', Lang.bind(this, this._firmwareChanged)); Uhm... We connect to firmware-missing two lines above this...
Review of attachment 246841 [details] [review]: Definitely. I just hope nothing breaks when we see devices or connections we don't understand.
Review of attachment 246840 [details] [review]: gah, rebase issue
Created attachment 246842 [details] [review] network: Simplify connections to the firmware signal The status item will go away soon, so make sure the one-time fire is given its own function. At the same time, only connect to the signal when the situation actually matters.
(In reply to comment #51) > Review of attachment 246839 [details] [review]: > > It was to make sure that the NMClient doesn't access wrapper.device for any > device. It did when there was the _updateIcon mess beforehand, and I want to > make sure that that doesn't come back. Heh, it's you, me and maybe Dan editing this code, we can remember it's private, without the "_" in front of it. And if someone else, in the future, will need it public, it won't need a patch just to rename it.
Review of attachment 246842 [details] [review]: Ok
(In reply to comment #57) > Heh, it's you, me and maybe Dan editing this code, we can remember it's > private, without the "_" in front of it. $ git log --format="%an" status/network.js | sort | uniq Alejandro Piñeiro Aleksander Morgado Colin Walters Cosimo Cecchi Dan Williams Dan Winship Diego Escalante Urrelo Florian Müllner Giovanni Campagna Jasper St. Pierre Jeremy Bicha Owen W. Taylor Ray Strode Rui Matos Wouter Bolsterlee > And if someone else, in the future, will need it public, it won't need a patch > just to rename it. That day should never come. If we need something on the device, write an accessor method, like I did with getIndicatorIcon(). The NMClient should not poke into the wrapper's internals, ever.
Forgot to use git bz push when pushing. We agreed on IRC that we could push this.