GNOME Bugzilla – Bug 646580
Added access points cause menu oddities
Last modified: 2011-04-26 12:47:09 UTC
When an access point is added, the wireless device section is removed and then added again. When you have the menu open at that point, we get several oddities: - If the hover (and key focus) isn't on the wireless section, this causes the More... submenu to close. - If the hover (and key focus) is on the wireless section, then because of PopupMenuManager._onKeyFocusChanged(), the menu closes In an area with a lot of networks, it's actually pretty common for networks to appear and disappear on rescans - at my home, it happens every couple of scans, so it's quite possible to hit this and get confused. (Though I'm hitting it far more because I've been playing around with the menu a lot and keeping it open for extended periods of time.) One approach: - Write the code to incrementally update the menu rather than just recreating it. - If removing an item would remove the item with the key focus, move the key focus to an adjacent item first. The simpler approach would be simply to freeze the list while the menu is open - though that would dealing with the (rare) case where the user clicks on an access point that has vanished since the menu was open. Just building the menu on open and leaving it in that order would also mean that we could do sophisticated code to order the list of networks without worrying about what happens if strength/timestamp/etc. change while the menu is open. [ I don't consider this a very high priority item, it was just bugging me until I figured out what was going on. ]
Created attachment 185330 [details] [review] NetworkMenu: keep wirelesss networks in predictable order Adds a function that compares wireless networks and keeps them sorted at all times. Order is: first already configured connections, then first secure networks, then alphabtic. Also, the appearance of a new access point no longer causes the whole menu to be rebuilt (but it still linear searches for the position, I guess that could be skipped), which caused the addition of more code for tracking the active access point. This fixes both the ordering problem and the rebuilding problem. Since now items are destroyed far less often (adding and removing a connection still rebuilds the menu, but that happens only after clicking, or with the network panel open), I did not add code to move the focus around.
Comment on attachment 185330 [details] [review] NetworkMenu: keep wirelesss networks in predictable order >+ // we don't refresh the view here, setActiveConnection will hm... setActiveConnection gets called how? Is that guaranteed? >+ _networkSortFunction: function(one, two) { >+ let oneHasConnection = !!one.connections.length; let oneHasConnection = one.connections.length != 0; >+ // (false - true = -1, true - false = 1) >+ // (-1 = good order, 1 = wrong order) >+ if (oneHasConnection != twoHasConnection) >+ return twoHasConnections - falseHasConnections; Ew. First boolean operators on numbers, now mathematical operators on booleans. (Also, both variable names are wrong in the last line...) if (oneHasConnection && !twoHasConnection) return -1; else if (twoHasConnection && !oneHasConnection) return 1; >+ let oneHasSecurity = one.security != NMApSecurityType.NONE; >+ let twoHasSecurity = two.security != NMApSecurityType.NONE; you mean NMAccessPointSecurity.NONE ? >+ return one.ssidText.localeCompare(two.ssidText); owen says "I wouldn't really trust localeCompare much". Use GLib.utf8_collate() >+ if (this._networks.length == 0) { >+ // only network in the list >+ this._networks.push(apObj); >+ this._clearSection(); >+ this._createSection(); why do you still clear/create in this case? >+ for (pos = 0; >+ pos < this._networks.length && >+ this._networksSortFunction(this._networks[i], apObj) < 0; ++pos) { typo. this._networkSortFunction >+ if (this._shouldShowConnectionList()) { >+ this._createNetworkItem(apObj); >+ menuPos += (this._activeConnectionItem ? 1 : 0); >+ if (menuPos < 5) >+ this.section.addMenuItem(apObj.item, menuPos); >+ else { >+ if (!this._overflowItem) { >+ this._overflowItem = new PopupMenu.PopupSubMenuMenuItem(_("More...")); >+ } >+ this._overflowItem.menu.addMenuItem(apObj.item, menuPos - 5); >+ } >+ } it would be better to not duplicate all this logic > anyauto = connections.length == 0; >+ >+ if (anyauto) { >+ // this potentially changes the sorting order >+ forceupdate = true; >+ break; >+ } I'm not clear on what "anyauto" means. Also, if anyauto is true, it doesn't matter whether forceupdate is true or false. >+ // this ponentially changes the sorting order typo >- if (any && this._shouldShowConnectionList()) { >- // we need to show this connection Where does all of this code go? What is causing the connection to be added now? >+ if(apObj == this._activeNetwork) fix the missing space before ( while you're there
Giovanni, did you intentionally change the component to 'message-tray', or was that an error? Doesn't make much sense to me ...
Ops... I don't know how it happened... Reverting!
*** Bug 647469 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > (From update of attachment 185330 [details] [review]) > >+ // we don't refresh the view here, setActiveConnection will > > hm... setActiveConnection gets called how? Is that guaranteed? Yes. NetworkManager will notify the creation of a new ActiveConnection object, and NMApplet will associate it to the NMDevice. > >+ return one.ssidText.localeCompare(two.ssidText); > > owen says "I wouldn't really trust localeCompare much". Use GLib.utf8_collate() I wanted to avoid two extra conversions to UTF-8, but I guess localeCompare is implemented internally as g_utf8_collate (using JSAPI locale hooks), so it's probably fine. > >+ if (this._networks.length == 0) { > >+ // only network in the list > >+ this._networks.push(apObj); > >+ this._clearSection(); > >+ this._createSection(); > > why do you still clear/create in this case? Because in case the device already knew about the network, it means "destroy the only network I saw, and create its menu item again", and I don't need to duplicate code or to have special cases in the loop that finds the position. Whereas, in case the network is new, clearSection is a no-op and createSection does all the work. > > >+ if (this._shouldShowConnectionList()) { > >+ this._createNetworkItem(apObj); > >+ menuPos += (this._activeConnectionItem ? 1 : 0); > >+ if (menuPos < 5) > >+ this.section.addMenuItem(apObj.item, menuPos); > >+ else { > >+ if (!this._overflowItem) { > >+ this._overflowItem = new PopupMenu.PopupSubMenuMenuItem(_("More...")); > >+ } > >+ this._overflowItem.menu.addMenuItem(apObj.item, menuPos - 5); > >+ } > >+ } > > it would be better to not duplicate all this logic What do you propose? NMDeviceWireless._addNetworkAtPosition(apObj, position)? > > anyauto = connections.length == 0; > >+ > >+ if (anyauto) { > >+ // this potentially changes the sorting order > >+ forceupdate = true; > >+ break; > >+ } > > I'm not clear on what "anyauto" means. Also, if anyauto is true, it doesn't > matter whether forceupdate is true or false. anyauto means: some network lost its stored connection, and I need to create an item that autoconnects. > >- if (any && this._shouldShowConnectionList()) { > >- // we need to show this connection > > Where does all of this code go? What is causing the connection to be added now? There is a clearSection / createSection at the end.
Created attachment 185733 [details] [review] NetworkMenu: keep wirelesss networks in predictable order Adds a function that compares wireless networks and keeps them sorted at all times. Order is: first already configured connections, then first secure networks, then alphabtic. Also, the appearance of a new access point no longer causes the whole menu to be rebuilt (but it still linear searches for the position, I guess that could be skipped), which caused the addition of more code for tracking the active access point.
h(In reply to comment #6) > (In reply to comment #2) > > (From update of attachment 185330 [details] [review] [details]) > > >+ // we don't refresh the view here, setActiveConnection will > > > > hm... setActiveConnection gets called how? Is that guaranteed? > > Yes. NetworkManager will notify the creation of a new ActiveConnection object, > and NMApplet will associate it to the NMDevice. > > > >+ return one.ssidText.localeCompare(two.ssidText); > > > > owen says "I wouldn't really trust localeCompare much". Use GLib.utf8_collate() > > I wanted to avoid two extra conversions to UTF-8, but I guess localeCompare is > implemented internally as g_utf8_collate (using JSAPI locale hooks), so it's > probably fine. It just doesn't matter here - 20 networks might mean 50-60 comparisons. But a note if someone is trying to optimize, note that: network.ssid_key = GLib.utf8_collate_key(...to_utf8(network.ssid)) *doesn't* work properly, because utf8_collate_key() is writing UTF-8 encoded 32-bit sequences and assuming that they will by byte-by-byte compared. But if you use '<', Spidermonkey does code-point-by-code-point comparison which will produce different results. So, basically, GLib.utf8_collate_key() needs to be annotated to return a byte array, and we need a way to do byte-by-byte ordering comparison on byte arrays; for small quantities of comparison g_utf8_collate() is fine.
Comment on attachment 185733 [details] [review] NetworkMenu: keep wirelesss networks in predictable order looks basically right. I haven't tested it though. (Probably ok to commit post-freeze; any bugs will turn up eventually.)
Attachment 185733 [details] pushed as d0780d1 - NetworkMenu: keep wirelesss networks in predictable order