GNOME Bugzilla – Bug 622730
Add support for dynamic menus
Last modified: 2010-07-20 13:36:34 UTC
Created attachment 164620 [details] [review] Proposed patch Current PopupMenuManager can only handle addition of menus, and only in the same order as they're navigated. Instead we should support adding in any order and we should support removal as well. Use cases vary from system status area (bluetooth comes and goes, as well as UPS and sometimes battery too) to user managed sidebars, extensions showing application lists and finally keyboard navigation for app well icons.
Comment on attachment 164620 [details] [review] Proposed patch >+ let menudata = { menu: menu, >+ openStateChangeId: menu.connect('open-state-changed', Lang.bind(this, this._onMenuOpenState)), >+ activateId: menu.connect('activate', Lang.bind(this, this._onMenuActivated)), >+ enterId: 0, >+ buttonPressId: 0 >+ }; The indentation there is odd. Each of the following attributes should line up with "menu:". If you want to avoid going too far off the right, just move the openStateChangeId and activateId initializations to separate lines, like the enterId and buttonPressId initializations. >+ if(position == undefined) need a space before the "(", there and in lots of other places >- this._activeMenu.sourceActor.contains(src)); >+ (this._activeMenu.sourceActor && this._activeMenu.sourceActor.contains(src))); what is this for? it seems like an unrelated change. If so, it should go in a separate patch >+ _findMenu: function(item) { >+ let pos = -1; >+ for(let i = 0;i < this._menus.length;++i) { space before "(" and after ";". also, use "i++", not "++i" >+ if(item == menudata.menu) { there's an extra space between "item" and "==" there >+ pos = i; >+ break; >+ } >+ } >+ return pos; don't need this... just "return i" from inside the if, and "return -1" at the end. >- let next = findNextInCycle(this._menus, this._activeMenu, direction); >+ let pos = this._findMenu(this._activeMenu); >+ let next; >+ if(pos+direction < 0) >+ next = this._menus[(this._menus.length - 1)].menu; >+ else >+ next = this._menus[(pos + direction) % (this._menus.length)].menu; why would you want keynav to work in any order except the visual order?
(In reply to comment #1) > >- this._activeMenu.sourceActor.contains(src)); > >+ (this._activeMenu.sourceActor && this._activeMenu.sourceActor.contains(src))); > > what is this for? it seems like an unrelated change. If so, it should go in a > separate patch addMenu allows for a menu without a sourceActor, in which case this._activeMenu.sourceActor.contains fails with TypeError > >- let next = findNextInCycle(this._menus, this._activeMenu, direction); > >+ let pos = this._findMenu(this._activeMenu); > >+ let next; > >+ if(pos+direction < 0) > >+ next = this._menus[(this._menus.length - 1)].menu; > >+ else > >+ next = this._menus[(pos + direction) % (this._menus.length)].menu; > > why would you want keynav to work in any order except the visual order? What do you mean with that? The code just replaced findNextInCycle, taking into account that this._menus[] is not the menu itself (and will never compare equal to this._activeMenu) - instead you neet this._menus[].menu
Created attachment 166053 [details] [review] Same patch, style corrected
Comment on attachment 166053 [details] [review] Same patch, style corrected >in arbitrary order (in particular to reintroduce those which where removed). typo ("where" instead of "were") >+ let menudata = { menu: menu, >+ openStateChangeId: menu.connect('open-state-changed', Lang.bind(this, this._onMenuOpenState)), indentation is still wrong here >- this._activeMenu.sourceActor.contains(src)); >+ (this._activeMenu.sourceActor && this._activeMenu.sourceActor.contains(src))); as I said before, this is a separate fix (it's fixing a pre-existing bug), so it (along with the _eventIsOnAnyMenuSource part) should be in a separate patch. (One way to do this in git is to do "git reset HEAD^", then "git add -p js/ui/popupMenu.js", answering "y" for the two diff chunks that you want, and "n" for all of the others, and then "git commit" to commit just those chunks. Then do "git commit -a" to recommit the remaining portion.) >+ if (pos+direction < 0) spaces around "+". But actually, you should be able to just replace the whole if with: next = this._menus[mod(pos + direction, this._menus.length).menu; (where mod(), defined elsewhere in popupMenu.js, does the right thing when wrapping around either end.)
Created attachment 166180 [details] [review] First patch (undefined sourceActor fix)
Created attachment 166181 [details] [review] Second patch, hoping it is good
pushed (after removing the whitespace added on the blank lines around _findMenu).