Created attachment 165202 [details] [review]
[PopupMenu] Add support for submenus

Adds class PopupSubMenuMenuItem, which is an item holding a submenu,
that is opened when the corresponding item is active, and is keyboard
and mouse navigable like its Gtk counterpart.
Patch depends on bug 622730

Created attachment 165327 [details] [review]
Same patch, some bugs corrected

The mockups sort of suggest submenus but I didn't draw any yet I think.  I wasn't sure that I wanted to have classic submenus at all but maybe have the first menu expand to include the submenu below the arrow as if it were an expander.  One of the overall goals here, again, is reducing the number of widgets and controls, and windows on the screen.

FWIW the OS X dock menus have completely "traditional" submenus, attached directly without a second arrow, etc

(In reply to comment #3)
> The mockups sort of suggest submenus but I didn't draw any yet I think.  I
> wasn't sure that I wanted to have classic submenus at all but maybe have the
> first menu expand to include the submenu below the arrow as if it were an
> expander.  One of the overall goals here, again, is reducing the number of
> widgets and controls, and windows on the screen.

I believe an hierarchical organization of items is necessary for most complex menus (like Bluetooth and network). This said, "child" items can be shown under their parent or next to them.
From a keyboard-only point of view inserting elements makes it difficult to reach the farthest.
Similarly, how do you expect such items to disappear, using a timeout? Or when an other item holding a submenu is focused? The latter could cause undesirable hover chains.

(In reply to comment #4)
> FWIW the OS X dock menus have completely "traditional" submenus, attached
> directly without a second arrow, etc

My submenus have arrows because they're mostly regular PopupMenus, but it should not be difficult to change that.

Created attachment 166063 [details] [review]
Same patch, rebased against current HEAD + attachment 166053 [details] [review]

This was last commented two months ago.
Any progress reviewing? Any design discussion or input?

Comment on attachment 166063 [details] [review]
Same patch, rebased against current HEAD + attachment 166053 [details] [review]

i was hoping there would have been design input before now... sorry.

>+        menuItem.connect('destroy', function(emitter) {
>+            emitter.disconnect(emitter._activateId);
>+            emitter.disconnect(emitter._activeChangeId);
>+            if(emitter == this._activeMenuItem)

you know "emitter" is menuItem here. just ignore the signal argument and use menuItem directly. Also, space between "if" and "(".

>+        if (activateFirst) {
>+            let items = this.getMenuItems().filter(function(item) {
>+                return item.actor.visible && item.actor.reactive;
>+            });
>+            if (items.length > 0)
>+                items[0].setActive(true);
>+        }

seems a little weird. I'd just use a for loop and break after finding and activating an activatable item.

>+        if (this._activeMenuItem)
>+            this._activeMenuItem.setActive(false);

why does this have to get moved? (no objection to it, just wondering)

>+    _init: function(text) {
>+        // HACK prevent default activation and hovering behaviour
>+        // by setting reactive to false
>+        PopupBaseMenuItem.prototype._init.call(this, false);
>+        this.actor.reactive = this.actor.track_hover = true;

this needs the newer version of that code from the slider bug

>+            let childitems = this.menu.getMenuItems().filter(function (item) {
>+                return item.actor.visible && item.actor.reactive;
>+            });

again with the too much work... and the "activate first item" code should only exist once anyway

(In reply to comment #8)
> (From update of attachment 166063 [details] [review])
> >+        if (this._activeMenuItem)
> >+            this._activeMenuItem.setActive(false);
> 
> why does this have to get moved? (no objection to it, just wondering)

Because setActive(false) on a PopupSubMenuMenuItem closes the attached menu (if it is open), but we want the child menu to close before the parent.

> >+    _init: function(text) {
> >+        // HACK prevent default activation and hovering behaviour
> >+        // by setting reactive to false
> >+        PopupBaseMenuItem.prototype._init.call(this, false);
> >+        this.actor.reactive = this.actor.track_hover = true;
> 
> this needs the newer version of that code from the slider bug

I've patched for this, but it is uglier than it sounds, as normal items are reactive-activate-defaultHover, sliders are reactive-noActivate-defaultHover, separators are nonReactive-noActivate-noDefaultHover, submenus are reactive-noActivate-noDefaultHover...

Created attachment 170336 [details] [review]
Same patch, with suggestions included

i changed PopupBaseMenuItem._init to use Params.parse() instead, which
makes the subclasses much less icky