GNOME Bugzilla – Bug 704670
Redesign the network menu for the new aggregate menu
Last modified: 2013-07-28 19:57:05 UTC
Act 2 of the Aggregate Menu saga.
Created attachment 249771 [details] [review] popupMenu: Remove column widths This code is too complicated to keep, and the last straw came after the fixed width menu in the aggregate menu design. This will break some existing popup menus that rely on the fixed width, but this will soon be replaced with the aggregate menu. We'll also soon clean this up further by replacing PopupBaseMenuItem's custom layout code with an StBoxLayout.
Created attachment 249772 [details] [review] network: Fix the AP strength indicator never getting updated I intended to make a few code cleanups, but I apparently forgot to hook up _updateAccessPoint. Merge it with _activeApChanged, which is where the notify::active-access-point signal is actually hooked up to.
Created attachment 249773 [details] [review] network: Remove the firmware changed signal According to Dan Williams, if firmware is installed the device will disappear and reappear, and this is unlikely to change any time soon. Just make our lives easier by removing the tracking.
Created attachment 249774 [details] [review] network: Remove separators between device sections This is not needed in the new design. Doing this allows us to remove some complex section visibility tracking, also.
Created attachment 249775 [details] [review] network: Remove the global wireless killswitch "Airplane Mode" is able to be turned on in gnome-control-center, and it will replace the killswitch UI we have in the menu right now.
Created attachment 249776 [details] [review] network: Remove the enable networking item If networking is disabled, like in an rfkill case, the menus should be completely hidden. Additionally, with this gone, we can clean up how we handle menu item visibility.
Created attachment 249777 [details] [review] network: Make the device section headers not bold The new device section headers will be simple text strings in the new designs. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249778 [details] [review] network: Hide the signal icon in the mobile broadband section This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details. This also removes the last use of PopupImageMenuItem, so remove that as well.
Created attachment 249779 [details] [review] network: Remove overflow implementation The entire menu will go away when we port to a new Wi-Fi dialog design, but for now, to make tiny modifications to the existing design, remove the overflow submenu entirely. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249780 [details] [review] network: Remove the Wired section This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details. Note that this does have an interesting side effect of not showing network connectivity status on wired. This is intentional, and error states will still be shown in the top bar when they happen.
Created attachment 249781 [details] [review] network: Make Bluetooth devices into Modem devices We shouldn't differentiate between mobile devices depending on the type of connection technology they use.
Created attachment 249782 [details] [review] network: Don't make the toplevel keep a list of connections Instead, simply pass the settings object around. In the future, we might remove toplevel connection tracking entirely, and move that to NMConnectionSection, having that code listen for the appropriate signals itself.
Created attachment 249783 [details] [review] network: Rework the VPN rewrite to work for NMDeviceModem as well Replace NMNetworkMenuItem with NMConnectionItem, based on NMVPNConnectionItem, and replace NMDevice with NMConnectionSection, based on a combination of NMVPNSection and NMDevice. Since this rips apart NMDevice, we'll temporarily remove NMDeviceWireless. We'll add it back in a later commit, along with the new Wi-Fi dialog.
Created attachment 249784 [details] [review] network: Implement new designs for VPN/modem submenus This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249785 [details] [review] network: Remove syncDescription Replace it with setDeviceDescription, which gives device wrappers more control about how to handle description changes.
Created attachment 249786 [details] [review] network: Remove TODO We won't ever be adding WiMax support here.
Created attachment 249787 [details] [review] network: Make sure that the network menu is insensitive when in the lock screen Since the network section of the aggregate menu will be shown in the lock screen, we need to ensure that users can't tweak with network settings or anything like that.
Created attachment 249788 [details] [review] network: Introduce a new, rewritten Wi-Fi section Remove the Wi-Fi chooser from the menu and put it in a dialog instead. This frees up the submenu to simply have three items: an rfkill toggle, a button to show the dialog, and a button to show network settings. Ideally, we'd autodetect the "needs network" case by user initiation and automatically show the dialog if needed, but lower-level plumbing is neccessary, so the menu item to show the dialog is an acceptable compromise instead. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Ok, first of all, testing: - the wifi submenu shows a signal icon even if wifi is off (what is it taking it from?) - the wifi status does not distinguish off vs unavailable, so if you are unavailable (radio switch off), and click turn on, but have no network configured, you see no change except that "Turn on" becomes "Turn off", which is confusing - if on wired, the top bar icon is disconnected, which is confusing and wrong - if wifi is off, and I click select network, I get an empty dialog and wifi stays off - the wifi menu does not expand to the end of the popup (the extra spacing is put after the arrow, not between the title and the arrow)
Oh and the language menu regressed terribly, as well as submenus in remote menu.
Review of attachment 249771 [details] [review]: Right, this is regressing RemoteMenu - arrows are no longer aligned. ::: js/ui/popupMenu.js @@ +277,3 @@ availWidth = box.x2 - x; + else + availWidth = naturalWidth; Shouldn't you take the actual available size in account here?
Review of attachment 249772 [details] [review]: ::: js/ui/status/network.js @@ +825,3 @@ if (res != null) this._activeNetwork = this._networks[res.network]; } You need to fire icon-changed here too, because the active access point changed.
Review of attachment 249773 [details] [review]: Ok
Review of attachment 249774 [details] [review]: Personally, I would merge this with the remove global killswitch one, but your call.
Review of attachment 249775 [details] [review]: There was some discussion in IRC about airplane mode today, and the outcome was that we want to keep wifi visible even when airplane mode is on, so you need to make sure that turning wifi on in any way will enable the radio switch.
Review of attachment 249776 [details] [review]: Yes (was discussed in IRC already)
Review of attachment 249771 [details] [review]: Also, I noticed now, the user avatar is slightly bigger than the regular icons. How do align the labels without columns?
Review of attachment 249777 [details] [review]: Ok
Review of attachment 249778 [details] [review]: This should go together the "implement new designs for VPN and modem" patch. Also, I personally would leave ImageMenuItem in for a while, and then if it is really dead code we can think of removing it, in a separate commit with a title signaling the change.
Review of attachment 249779 [details] [review]: You're removing overflow from other menus too, so make sure to mention that in the commit message. Also, please mention that in the design, we have a fixed number of menu items.
Review of attachment 249780 [details] [review]: This has the interesting side effect of showing an error state when I'm perfectly fine on wired...
Review of attachment 249781 [details] [review]: The same effect could have been achieved by changing the category, as we definitely need to distinguish on technology in the code. In particular, as long as gnome-control-center offers no support for bluetooth devices, asking NM directly and hoping for PAN is better than opening the network panel without a dialog. Btw, NMDeviceBt is not a NMDeviceModem, so it doesn't have an UDI (so device.udi.indexOf() crashes, line 288), and it doesn't have current_capabilities (so we would not create a mobile device anyway, and we would crash later on)
Review of attachment 249782 [details] [review]: IMHO, this patch is not useful without "the future", but you use settings in the wireless dialog, so ok.
Review of attachment 249783 [details] [review]: ::: js/ui/status/network.js @@ +82,2 @@ + this.menuItem = new PopupMenu.PopupSwitchMenuItem(connection.get_id(), false); + this.menuItem.connect('toggled', Lang.bind(this, this._toggle)); Wait a minute, a NMConnectionItem, that has-a MenuItem? Uhm... @@ +169,3 @@ + + _hasConnection: function(connection) { + return this._connectionItems.has(connection.get_uuid()); If you index by string, you can maybe avoid Hash.Map and use an object directly. @@ +226,3 @@ + for (let i = 0; i < items.length; i++) { + let item = items[i]; + let icon = item.getIndicatorIcon(); I don't see this in NMConnectionIcon. Also, shouldn't it be the device responsability to handle icons? (Or, if this is a VPN thing, it should go in the vpn section)
Review of attachment 249785 [details] [review]: OK
Review of attachment 249784 [details] [review]: This whole thing is a bit messy: there can only be one "Connect" menu item per section, yet you keep one per connection. It doesn't make sense to me. ::: js/ui/status/network.js @@ +281,3 @@ this._device = device; + this._autoConnectItem = this.item.menu.addSettingsAction(_("Connect"), Lang.bind(this, this._autoConnect)); AddSettingsAction?
Review of attachment 249786 [details] [review]: Ok (I disagree with not adding wi-max, but I understand nobody is going to do it...)
Review of attachment 249787 [details] [review]: Ok
Review of attachment 249788 [details] [review]: Mostly fine. ::: js/ui/status/network.js @@ +927,3 @@ + let ap = this._device.active_access_point; + if (!ap) + return _("Off"); // XXX -- interpret actual status At least Hardware Disabled is part of the mockups here. @@ +933,3 @@ + + _getMenuIcon: function() { + return this.getIndicatorIcon() || 'network-wireless-connected-symbolic'; If getIndicatorIcon() returns '', we're not connected, so -connected-symbolic is wrong.
(In reply to comment #20) > Oh and the language menu regressed terribly, as well as submenus in remote > menu. I know about the language menu and arrow popup regressions, and they'll mostly be fixed by https://git.gnome.org/browse/gnome-shell/commit/?h=wip/aggregate-menu&id=0610fc3db04ef36b2f11500d91dcf66c244862ff I still need to figure out how to do ornaments properly. (In reply to comment #37) > Ok (I disagree with not adding wi-max, but I understand nobody is going to do > it...) We removed Bridge connections, and I think those are far more common than WiMax. The new menu is for extremely common operations, not complex configuration. (In reply to comment #39) > Review of attachment 249788 [details] [review]: > > If getIndicatorIcon() returns '', we're not connected, so -connected-symbolic > is wrong. getMenuIcon() is the icon for the device submenu item. In the mockups, the connected Wi-Fi icon is always shown, but I thought it was weird to have that when you were very low on signal. The designers thought that a good compromise is that if you have an indicator up top, we show that in the submenu, otherwise we show the generic icon. (In reply to comment #34) > Review of attachment 249783 [details] [review]: > > ::: js/ui/status/network.js > @@ +82,2 @@ > + this.menuItem = new PopupMenu.PopupSwitchMenuItem(connection.get_id(), > false); > + this.menuItem.connect('toggled', Lang.bind(this, this._toggle)); > > Wait a minute, a NMConnectionItem, that has-a MenuItem? Uhm... What's wrong with that? You know I always prefer composition over inheritance. > @@ +169,3 @@ > + > + _hasConnection: function(connection) { > + return this._connectionItems.has(connection.get_uuid()); > > If you index by string, you can maybe avoid Hash.Map and use an object > directly. Yuck. Why are we trying to avoid Hash.Map? I did it for convenience of just writing the code, but I can rewrite it to use Object. (Also, if we depend on mozjs17, that has a native Map, if we're concerned about performance) > @@ +226,3 @@ > I don't see this in NMConnectionIcon. > > Also, shouldn't it be the device responsability to handle icons? > > (Or, if this is a VPN thing, it should go in the vpn section) Yeah, now that I look it over, this is only used by VPN. The original idea was that the device would ask all its NMConnectionItems to give it an icon. The loop was a leftover before I had _activeConnectionItem. I'll just remove it, and leave it to the device. (In reply to comment #36) > Review of attachment 249784 [details] [review]: > > This whole thing is a bit messy: there can only be one "Connect" menu item per > section, yet you keep one per connection. It doesn't make sense to me. I don't understand what this means. Are you talking about switchItem vs. labelItem? There's three cases for modem (or BT when we reintroduce that): * 0 connections. In this case the "auto connect" item is shown that will spawn g-c-c with options to configure. * 1 connection. In this case, the label section is shown, and one item will be in there with options to "Connect" or "Turn Off" * 2+ connections. In this case, the switch section is shown, with switches for each connection. Yeah, we could do shuffling of the "Connect" item so that only one is created and it handles 0/1 connections all on its own, but I think the resulting code for this is fairly elegant and neatly handles cases where we only have connection A that's off, then add connection B (move to switch), activates connection B (turns switch on, sets label to "Turn Off"), then remove connection A (shows label with "Turn Off").
(In reply to comment #40) > (In reply to comment #20) > > Oh and the language menu regressed terribly, as well as submenus in remote > > menu. > > I know about the language menu and arrow popup regressions, and they'll mostly > be fixed by > > https://git.gnome.org/browse/gnome-shell/commit/?h=wip/aggregate-menu&id=0610fc3db04ef36b2f11500d91dcf66c244862ff > > I still need to figure out how to do ornaments properly. Can you postpone this patch until you have the other ready? > (In reply to comment #37) > > Ok (I disagree with not adding wi-max, but I understand nobody is going to do > > it...) > > We removed Bridge connections, and I think those are far more common than > WiMax. The new menu is for extremely common operations, not complex > configuration. This is wrong. The criteria is how often the user needs a certain feature, not ow many users. For bridge, it was removed because it's a server/enterprise configuration, and once set, it's basically fixed forever (like wired). WiMax, on the other hand, is half way between Wi-Fi and WWAN, and if you use it, you enable/disable it often. > (In reply to comment #39) > > Review of attachment 249788 [details] [review] [details]: > > > > If getIndicatorIcon() returns '', we're not connected, so -connected-symbolic > > is wrong. > > getMenuIcon() is the icon for the device submenu item. In the mockups, the > connected Wi-Fi icon is always shown, but I thought it was weird to have that > when you were very low on signal. The designers thought that a good compromise > is that if you have an indicator up top, we show that in the submenu, otherwise > we show the generic icon. This doesn't answer my point in any way. If we are not connected, using a connected icon, in any form or position, is wrong. network-wireless-connected-symbolic is a connected with weak signal icon, so it's wrong if I'm not connected at all. Arguably, the weak signal icon is also wrong when the icon name is appropriate (ad-hoc / hotspot), but that's a theme bug. > (In reply to comment #34) > > Review of attachment 249783 [details] [review] [details]: > > > > ::: js/ui/status/network.js > > @@ +82,2 @@ > > + this.menuItem = new PopupMenu.PopupSwitchMenuItem(connection.get_id(), > > false); > > + this.menuItem.connect('toggled', Lang.bind(this, this._toggle)); > > > > Wait a minute, a NMConnectionItem, that has-a MenuItem? Uhm... > > What's wrong with that? You know I always prefer composition over inheritance. Yeah, but the name is misleading. Anyway, it's a style preference I guess. > > @@ +169,3 @@ > > + > > + _hasConnection: function(connection) { > > + return this._connectionItems.has(connection.get_uuid()); > > > > If you index by string, you can maybe avoid Hash.Map and use an object > > directly. > > Yuck. Why are we trying to avoid Hash.Map? I did it for convenience of just > writing the code, but I can rewrite it to use Object. > > (Also, if we depend on mozjs17, that has a native Map, if we're concerned about > performance) No, my concern was on calling .has() and .get() over in and []. But ok either way. > [...] > > (In reply to comment #36) > > Review of attachment 249784 [details] [review] [details]: > > > > This whole thing is a bit messy: there can only be one "Connect" menu item per > > section, yet you keep one per connection. It doesn't make sense to me. > > I don't understand what this means. Are you talking about switchItem vs. > labelItem? > > There's three cases for modem (or BT when we reintroduce that): > > * 0 connections. In this case the "auto connect" item is shown that will spawn > g-c-c with options to configure. > * 1 connection. In this case, the label section is shown, and one item will be > in there with options to "Connect" or "Turn Off" > * 2+ connections. In this case, the switch section is shown, with switches for > each connection. > > Yeah, we could do shuffling of the "Connect" item so that only one is created > and it handles 0/1 connections all on its own, but I think the resulting code > for this is fairly elegant and neatly handles cases where we only have > connection A that's off, then add connection B (move to switch), activates > connection B (turns switch on, sets label to "Turn Off"), then remove > connection A (shows label with "Turn Off"). I'm sorry, but that's not elegant at all. Having one special item in the device, that is updated and shown or hidden as appropriate (the device already is the place when we make this decision) is a way better solution, and we don't to push down in NMConnectionItem details about other connections. By the way, I think ideally we would remove the autoconnect item altogether. For modems, when plugging it for the first time you would get a notification pointing you to the control center, like we handle media players and cameras. For bluetooth, the first connection is configured in the bluetooth wizard when pairing, and if you don't do it that, NM doesn't even see the device. For VPN, automatic connection makes no sense.
(In reply to comment #41) > By the way, I think ideally we would remove the autoconnect item altogether. > For modems, when plugging it for the first time you would get a notification > pointing you to the control center, like we handle media players and cameras. > For bluetooth, the first connection is configured in the bluetooth wizard when > pairing, and if you don't do it that, NM doesn't even see the device. > For VPN, automatic connection makes no sense. We currently only have an autoconnect item for modems and bluetooth. I can certainly remove the code for both, but activateAutomaticConnection was there before, so I chose to keep it. > I'm sorry, but that's not elegant at all. > Having one special item in the device, that is updated and shown or hidden as > appropriate (the device already is the place when we make this decision) is a > way better solution, and we don't to push down in NMConnectionItem details > about other connections. With the autoconnect item removed, that means we show "Connect" if there's one connection, and a switch if there's more than one connection. Having both representations of "connect to this connection" seems like it should be part of the connection item.
Created attachment 249967 [details] [review] network: Fix the AP strength indicator never getting updated I intended to make a few code cleanups, but I apparently forgot to hook up _updateAccessPoint. Merge it with _activeApChanged, which is where the notify::active-access-point signal is actually hooked up to. Here's a new stack that should fix most of the review comments. If I missed something, plesae let me know. This doesn't implement the new submenu behavior for airplane mode. That will happen as a set of followup commits afterwards.
Created attachment 249968 [details] [review] network: Fix the AP strength indicator never getting updated I intended to make a few code cleanups, but I apparently forgot to hook up _updateAccessPoint. Merge it with _activeApChanged, which is where the notify::active-access-point signal is actually hooked up to.
Created attachment 249969 [details] [review] network: Remove the firmware changed signal According to Dan Williams, if firmware is installed the device will disappear and reappear, and this is unlikely to change any time soon. Just make our lives easier by removing the tracking.
Created attachment 249970 [details] [review] network: Remove separators between device sections This is not needed in the new design. Doing this allows us to remove some complex section visibility tracking, also.
Created attachment 249971 [details] [review] network: Make the device section headers not bold The new device section headers will be simple text strings in the new designs. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249972 [details] [review] network: Remove the global wireless killswitch "Airplane Mode" is able to be turned on in gnome-control-center, and it will replace the killswitch UI we have in the menu right now.
Created attachment 249973 [details] [review] network: Remove the enable networking item If networking is disabled, like in an rfkill case, the menus should be completely hidden. Additionally, with this gone, we can clean up how we handle menu item visibility.
Created attachment 249974 [details] [review] network: Remove overflow implementation The code is complicated by requiring overflow, and in order to incrementally improve the code to match the designs, remove overflow. In the new design, we'll have a fixed number of menu items, and Wi-Fi will be done by a separate design, so we can't be too concerned with the menu not fitting on the screen. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249975 [details] [review] network: Remove the Wired section This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details. Note that this does have an interesting side effect of not showing network connectivity status on wired. This is intentional, and error states will still be shown in the top bar when they happen.
Created attachment 249976 [details] [review] network: Don't pass a list of connections to the devices Instead, just add them after they're constructed. This allows us to not have to pass the connections to each device, and prevents issues with having to enumerate the connections in the middle of construction.
Created attachment 249977 [details] [review] network: Rework the VPN rewrite to work for modem and bluetooth as well Replace NMNetworkMenuItem with NMConnectionItem, based on NMVPNConnectionItem, and replace NMDevice with NMConnectionSection and NMConnectionDevice. Since this rips apart NMDevice, and since wi-fi should not be connection-based, we'll temporarily remove NMDeviceWireless. We'll add it back in a later commit, along with the new Wi-Fi dialog.
Created attachment 249978 [details] [review] network: Implement new designs for VPN/modem submenus This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 249979 [details] [review] network: Remove syncDescription Replace it with setDeviceDescription, which gives device wrappers more control about how to handle description changes.
Created attachment 249980 [details] [review] network: Make sure that the network menu is insensitive when in the lock screen Since the network section of the aggregate menu will be shown in the lock screen, we need to ensure that users can't tweak with network settings or anything like that.
Created attachment 249981 [details] [review] network: Introduce a new, rewritten Wi-Fi section Remove the Wi-Fi chooser from the menu and put it in a dialog instead. This frees up the submenu to simply have three items: an rfkill toggle, a button to show the dialog, and a button to show network settings. Ideally, we'd autodetect the "needs network" case by user initiation and automatically show the dialog if needed, but lower-level plumbing is neccessary, so the menu item to show the dialog is an acceptable compromise instead. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Review of attachment 249968 [details] [review]: ::: js/ui/status/network.js @@ +814,3 @@ + this._activeAccessPoint.disconnect(this._strengthChangedId); + this._strengthChangedId = 0; + } Now that I look at this, you need to disconnect strengthChangedId in destroy() as well.
Review of attachment 249973 [details] [review]: The commit message is a bit misleading, because rfkills do not toggle the NetworkingEnabled property. It's more of a sysadmin tool for "panic" situations when you want to shutdown all connectivity fast. Anyway, looks good.
Review of attachment 249972 [details] [review]: In a later patch, if you want, but you should update the device status handling to say "disabled" instead of "unavailable" when soft-rfkilled (and "hardware disabled" when hard-rfkilled)
Review of attachment 249975 [details] [review]: You should mention that for this to work correctly we need once again to decouple top bar icon handling from NMDevice/NMConnectionSection/NMWhatever tracking, and that will require new NM API (including DBus API, which will be painful for jhbuild users)
Review of attachment 249976 [details] [review]: It was originally an optimization to pass the list of connections, because the addConnection path is intentionally slow (it's rarely taken), while device construction happens at every login. Also, what happened with passing down the NMRemoteSettings object? Finally, if you do this, you can have all classes chain up at the beginning of _init(), and consistently add .category to the prototype.
Review of attachment 249977 [details] [review]: ::: js/ui/status/network.js @@ +108,2 @@ else + this._client.deactivate_connection(this._activeConnection); deactivate_connection() keeps a connection queued for auto-activation. You should use ._device.disconnect() when possible. (Ok, in reality this is a minor concern, because wwan connections are not marked for auto-activation by default, but the user can choose it) @@ +251,1 @@ } No chain-up?
(In reply to comment #42) > > I'm sorry, but that's not elegant at all. > > Having one special item in the device, that is updated and shown or hidden as > > appropriate (the device already is the place when we make this decision) is a > > way better solution, and we don't to push down in NMConnectionItem details > > about other connections. > > With the autoconnect item removed, that means we show "Connect" if there's one > connection, and a switch if there's more than one connection. Having both > representations of "connect to this connection" seems like it should be part of > the connection item. I disagree. On the one hand, you have: "(de)activate this device/feature, not matter how", which is a device action; on the other hand you have "activate this connection/profile", which is a connection action. They are conceptually different and they should be handled at a different level.
(In reply to comment #64) > I disagree. On the one hand, you have: "(de)activate this device/feature, not > matter how", which is a device action; on the other hand you have "activate > this connection/profile", which is a connection action. They are conceptually > different and they should be handled at a different level. That's definitely not what the "Connect" item is about. For VPN, if we have one connection, we show a "Connect" label item that connects to the one configured VPN connection. It's not "activate the VPN, no matter how". If I click it, and then add another VPN, it will show two switches -- one on, one off, and if I remove the first VPN, it will show "Connect" again, not "Disconnect", even though I definitely "turned VPN on". It's "activate the one VPN connection I have configured". I also kept in the autoconnect item that launches g-c-c, but if we want to remove it, that's more code we can remove.
Review of attachment 249981 [details] [review]: ::: js/ui/status/network.js @@ +571,3 @@ + accessPoints.forEach(Lang.bind(this, function(ap) { + this._accessPointAdded(this._device, ap); + })); I wonder if it makes sense to show a placeholder like "No available networks" or "No networks in range" @@ +924,3 @@ + + this._sync(); + }, I wish we have multiple inheritance to share this code...
Review of attachment 249978 [details] [review]: The discussion in IRC was way to long, and it went beyond the point of making real progress. Time to say, "it works, go ahead".
(In reply to comment #62) > Review of attachment 249976 [details] [review]: > > It was originally an optimization to pass the list of connections, because the > addConnection path is intentionally slow (it's rarely taken), while device > construction happens at every login. I don't see how calling checkConnection from the constructor is any slower from calling it after. > Also, what happened with passing down the NMRemoteSettings object? Still happens in the wi-fi commit. The reason I don't can't really do it yet is because of the 'changed'/'removed' signals we add at a top level, and because of the expando properties we add everywhere. > Finally, if you do this, you can have all classes chain up at the beginning of > _init(), and consistently add .category to the prototype. Changed.
Created attachment 250213 [details] [review] network: Fix the AP strength indicator never getting updated I intended to make a few code cleanups, but I apparently forgot to hook up _updateAccessPoint. Merge it with _activeApChanged, which is where the notify::active-access-point signal is actually hooked up to.
Created attachment 250214 [details] [review] network: Remove the global wireless killswitch "Airplane Mode" is able to be turned on in gnome-control-center, and it will replace the killswitch UI we have in the menu right now.
Created attachment 250215 [details] [review] network: Remove the Wired section This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details. Note that this does have an interesting side effect of not showing network connectivity status on wired. This is intentional, and error states will still be shown in the top bar when they happen. This also means that if you're connected to both wired and wireless, even though wired is the default route, we'll first notice the wireless active connection, and we'll show that in the top bar. New NM API that will help figuring out the active connection of the default device is being implemented to stop this from happening.
Created attachment 250216 [details] [review] network: Don't pass a list of connections to the devices Instead, just add them after they're constructed. This allows us to not have to pass the connections to each device, and prevents issues with having to enumerate the connections in the middle of construction.
Created attachment 250217 [details] [review] network: Rework the VPN rewrite to work for modem and bluetooth as well Replace NMNetworkMenuItem with NMConnectionItem, based on NMVPNConnectionItem, and replace NMDevice with NMConnectionSection and NMConnectionDevice. Since this rips apart NMDevice, and since wi-fi should not be connection-based, we'll temporarily remove NMDeviceWireless. We'll add it back in a later commit, along with the new Wi-Fi dialog.
Created attachment 250218 [details] [review] network: Implement new designs for VPN/modem submenus This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Created attachment 250219 [details] [review] network: Introduce a new, rewritten Wi-Fi section Remove the Wi-Fi chooser from the menu and put it in a dialog instead. This frees up the submenu to simply have three items: an rfkill toggle, a button to show the dialog, and a button to show network settings. Ideally, we'd autodetect the "needs network" case by user initiation and automatically show the dialog if needed, but lower-level plumbing is neccessary, so the menu item to show the dialog is an acceptable compromise instead. This is a part of the new system status design, see https://wiki.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/ for design details.
Review of attachment 250213 [details] [review]: Ok
Review of attachment 250214 [details] [review]: Uh you didn't incorporate my suggestion, but ok.
Review of attachment 250215 [details] [review]: Ok
Review of attachment 250216 [details] [review]: Right, because we switched to checkConnection in the constructor a while ago, bah.
Review of attachment 250217 [details] [review]: ::: js/ui/status/network.js @@ +249,3 @@ + GObject.Object.prototype.disconnect.call(this._device, this._activeConnectionChangedId); + this._stateChangedId = 0; + } Still no chainup?
Review of attachment 250218 [details] [review]: Looks good
Review of attachment 250219 [details] [review]: Sorry, a few more questions... ::: js/ui/status/network.js @@ +514,3 @@ + this.actor.connect('clicked', Lang.bind(this, function() { + this.actor.grab_key_focus(); + this.emit('selected'); This signal is redundant, because it's already emitted when the actor gets focus one line above this. @@ +524,3 @@ + + this.actor.label_actor = this._label; + this._content.add(this._label, { x_align: St.Align.START }); Shouldn't the label be the expanding actor, instead of the icons? @@ +917,3 @@ + this._activeAccessPoint.disconnect(this._strengthChangedId); + this._strengthChangedId = 0; + } Shouldn't you destroy the item here too?
Attachment 249969 [details] pushed as f822514 - network: Remove the firmware changed signal Attachment 249970 [details] pushed as 3271e65 - network: Remove separators between device sections Attachment 249971 [details] pushed as 6295d0f - network: Make the device section headers not bold Attachment 249973 [details] pushed as e62045e - network: Remove the enable networking item Attachment 249974 [details] pushed as d6dc9af - network: Remove overflow implementation Attachment 249979 [details] pushed as 6827dac - network: Remove syncDescription Attachment 249980 [details] pushed as c8c839a - network: Make sure that the network menu is insensitive when in the lock screen Attachment 250213 [details] pushed as 8e9e583 - network: Fix the AP strength indicator never getting updated Attachment 250214 [details] pushed as bda3e53 - network: Remove the global wireless killswitch Attachment 250215 [details] pushed as fc6a0c1 - network: Remove the Wired section Attachment 250216 [details] pushed as 39bff8b - network: Don't pass a list of connections to the devices Attachment 250217 [details] pushed as 4305ebc - network: Rework the VPN rewrite to work for modem and bluetooth as well Attachment 250218 [details] pushed as 246271f - network: Implement new designs for VPN/modem submenus Attachment 250219 [details] pushed as 728c3a7 - network: Introduce a new, rewritten Wi-Fi section Pushed with suggested comments fixed.