GNOME Bugzilla – Bug 664124
Clean up updating of network menu
Last modified: 2011-12-16 07:58:42 UTC
Current code for updating network menu when AP change is messy, to do only the minimum work needed to keep everything updated at all times. This has two problems: 1) We keep the UI updated even when it's not needed 2) We have different code paths in different situations, which leads to situations like bug 661793 The following patch is an attempt to fix this issue, tackling from the opposite side. I think it should not worsen performance much, but some testing in crowded areas would help.
Created attachment 201456 [details] [review] Network Menu: update the UI only when really needed By using Main.queueDeferredWork, we can ensure that most of the menu contents (in particular, the heaviest parts like the list of wifi networks) are not updated immediately as we receive signals from NetworkManager. Instead, the menu is rebuilt some time later, or as soon as the user opens the menu. This means that it is no longer needed to optimize for the access-point-added case, replacing a lot of buggy code with a safer call to _queueCreateSection, which in turn should ensure that the more menu, if existing, is always at the end and that at most 5 networks are visible outside it.
Review of attachment 201456 [details] [review]: ::: js/ui/status/network.js @@ +1544,3 @@ let apObj = this._networks[j]; + if (apObj == this._activeNetwork) { + activeOffset--; This looks like an unrelated bug fix?
(In reply to comment #2) > Review of attachment 201456 [details] [review]: > > ::: js/ui/status/network.js > @@ +1544,3 @@ > let apObj = this._networks[j]; > + if (apObj == this._activeNetwork) { > + activeOffset--; > > This looks like an unrelated bug fix? Only partially: the main purpose of this bug is getting 5, and always 5, networks in the menu, plus a More... This fix is needed for that (otherwise, you always get 4 of them)
I'd like it in a separate patch. And what's wrong with: if (this.section.numMenuItems > NUM_VISIBLE_NETWORKS)
(In reply to comment #4) > I'd like it in a separate patch. And what's wrong with: > > if (this.section.numMenuItems > NUM_VISIBLE_NETWORKS) What do you mean by that? (or where would that apply?)
Created attachment 201708 [details] [review] Network Menu: update the UI only when really needed By using Main.queueDeferredWork, we can ensure that most of the menu contents (in particular, the heaviest parts like the list of wifi networks) are not updated immediately as we receive signals from NetworkManager. Instead, the menu is rebuilt some time later, or as soon as the user opens the menu. This means that it is no longer needed to optimize for the access-point-added case, replacing a lot of buggy code with a safer call to _queueCreateSection, which in turn should ensure that the more menu, if existing, is always at the end and that at most 5 networks are visible outside it.
Created attachment 201709 [details] [review] NetworkMenu: fix number of visible networks When placing networks in _createSection, we were taking in consideration that _activeNetwork is always first, by adding 1, but then kept this offset also for networks following it (normally, all of them, since _activeNetwork is also the most recently used), that instead should not be affected by the movement. This resulted in the menu showing 4 networks + More... instead of 5.
(In reply to comment #5) > (In reply to comment #4) > > I'd like it in a separate patch. And what's wrong with: > > > > if (this.section.numMenuItems > NUM_VISIBLE_NETWORKS) > > What do you mean by that? (or where would that apply?) I mean that instead of this activeOffset nonsense and checking the indexes, we just check the amount of menu items. that have been inserted so far.
Any update on this ? I'd love to see the network menu work better in 3.4...
(In reply to comment #8) > (In reply to comment #5) > > (In reply to comment #4) > > > I'd like it in a separate patch. And what's wrong with: > > > > > > if (this.section.numMenuItems > NUM_VISIBLE_NETWORKS) > > > > What do you mean by that? (or where would that apply?) > > I mean that instead of this activeOffset nonsense and checking the indexes, we > just check the amount of menu items. that have been inserted so far. While that would be cleaner, it would become O(N^2) to build the menu (which in turn becomes O(N^3) if the menu is rebuilt each time a new AP is found). Given all the performance problems around that code, I'd avoid that, if possible.
Untrue. JavaScript's Array.prototype.length is a constant property that's updated on array mutation, not a runtime function.
(In reply to comment #11) > Untrue. JavaScript's Array.prototype.length is a constant property that's > updated on array mutation, not a runtime function. But PopupMenu.numMenuItems is an accessor property that call _getMenuItems(), which is O(N), both in gjs binding code (to convert a GList to an Array) and in JS (to filter stuff that's not a PopupBaseMenuItem).
Review of attachment 201709 [details] [review]: OK, sure. I still think we'd need to clean the network code up more in general, later.
Comment on attachment 201709 [details] [review] NetworkMenu: fix number of visible networks Attachment 201709 [details] pushed as aad9179 - NetworkMenu: fix number of visible networks
*** Bug 665838 has been marked as a duplicate of this bug. ***
Review of attachment 201708 [details] [review]: Yes.
Attachment 201708 [details] pushed as f3cb9d0 - Network Menu: update the UI only when really needed