We also need:
* Suspend
* Restart...
* Shutdown...
Instead of the current:
* Poweroff...


BTW, wouldn't it look nicer if all labels were right-aligned at the same level? The session statuses have an extra indent in the mockup because of the radio buttons. What happens in GTK+ menus is that labels are aligned, and normal menus have blank space before them (or icons).

(In reply to comment #0)
>  * Use new symbolic status icons

We are requesting symbolic icons, they just don't exist in the theme yet. (user-available, user-busy, user-invisible, user-idle)

(In reply to comment #1)
> BTW, wouldn't it look nicer if all labels were right-aligned at the same level?
> The session statuses have an extra indent in the mockup because of the radio
> buttons. What happens in GTK+ menus is that labels are aligned, and normal
> menus have blank space before them (or icons).

And in fact, this is how the mockups show the language menu (http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/language-menu.png). Which is correct? Or are they supposed to be aligned in the language menu and unaligned in the user menu?

I think having the labels aligned and the radio occupying the left padding is a better solution than what the mockup currently shows. It was an oversight and I will fix up the mockup.

symbolic icon theme master includes the new user status icons btw.

Created attachment 172860 [details] [review]
PopupImageMenuItem: always show icon, on the right, not the left

In the new mockups, the user menu icons are on the right, not the
left.

Also, get rid of the idea of optional icons; the design doesn't have
icons on those items, and there probably aren't going to be symbolic
versions of some of those icons anyway. So if the caller specifies
PopupImageMenuItem, then always show an icon, and just use regular
PopupMenuItems for the items that don't have icons in the current
design.

Created attachment 172861 [details] [review]
statusMenu: fix menu items to match latest mockups

Rename a few, and add Suspend and Restart, although currently they do
the same thing as Shut Down (ie, they bring up a dialog that lets you
do any of those three). This will be fixed later when we have the
in-shell modal dialogs for these features.

Created attachment 172862 [details] [review]
popupMenu: make menu layout more table-like

When there are menu items with right-aligned items, all the
right-aligned items should appear to the right of all the left-aligned
items.

Clutter doesn't have an equivalent of GtkSizeGroup, so hack something up
using ShellGenericContainer and some javascript.

Created attachment 172863 [details] [review]
statusMenu: show a radio button-style dot next to the presence status

Adding a "PopupMenuRadioButtonItem" wouldn't work well, because we'll
need radio-button indicators on multiple different styles of menu
item. Also, the current design draws the indicator in the menu item's
padding, so it's sort of special anyway. So just add support at the
BaseMenuItem level.

Also, redo the menu/menuitem padding so that all the horizontal
padding is in the menu item, or else the indicator dot will show up in
the wrong spot.

Review of attachment 172860 [details] [review]:

Looks good

Review of attachment 172861 [details] [review]:

So many choices...

Created attachment 173183 [details] [review]
boxpointer: keep a margin between the box and the screen edge

per the mockups

Review of attachment 172862 [details] [review]:

Few comments

::: js/ui/popupMenu.js
@@ +121,3 @@
+    // adds an actor to the menu item; @column defaults to the next
+    // open column, @span defaults to 1. If @span is -1, the actor
+    // will span the width of the menu item.

Might be good to add here "The code assumes children don't overlap" or something

@@ +192,3 @@
+        for (let i = 0; i < this._children.length; i++) {
+            let child = this._children[i];
+            let [min, natural] = child.actor.get_preferred_height(forWidth);

Using forWidth here doesn't make any sense. If you actually care about height-for-width, you should use the widths that getPreferredWidth(-1) would use. Otherwise, you probably can just use -1.

@@ +210,3 @@
+                else {
+                    for (let j = 0; j < child.span; j++)
+                        childBox.x2 = x + this._columnWidths[col++];

Broken logic

@@ +544,3 @@
         this.actor = this._boxPointer.actor;
         this.actor.style_class = 'popup-menu-boxpointer';
+        this._boxWrapper = new Shell.GenericContainer();

This member variable isn't actually used anywhere outside this function - could be argued either way if it's good style or not to have the variable - no strong opinions, just wanted to point it out in case it was an unintentional oversight.

@@ +567,3 @@
+                let itemColumnWidths = items[i]._delegate.getColumnWidths();
+                for (let j = 0; j < itemColumnWidths.length; j++) {
+                    if (!columnWidths[j] || itemColumnWidths[j] > columnWidths[j])

really don't like using ! here on something that either could be numeric 0 or undefined. Should not have to think about multiple branches of the definition of true in JS to figure out whether it works :-) I'd write '== null' or '=== undefined'.

@@ +791,3 @@
         this.label = new St.Label({ text: text });
+        this.actor.add_actor(this.label);
+        this.actor.add_actor(new St.Label({ text: '>' }));

Either there is some magic going on here that needs to be commented, or this won't work? Since these aren't in this._children?

Review of attachment 172863 [details] [review]:

Just some minor comments

::: js/ui/popupMenu.js
@@ +151,3 @@
     },
 
+    showDot: function(show) {

showDot(false) to hide the dot is weird to me, parameters shouldn't reverse the meaning of the method verb

 setShowDot(showDot) or showDot/hideDot

@@ +179,3 @@
+            color.blue / 255,
+            color.alpha / 255);
+        cr.arc(width / 2, height / 2, width / 3, 0, 2 * Math.PI);

Hmm, wonder if it would be better to round width/3 to be integral for width even, half-integral for width odd. Not sure. (It's certainly right for sufficiently large dots where the edges become obviously linear, but may not make a difference for normal dots.) Not worth spending time on unless the dots are obviously fuzzy, I think.

@@ +243,3 @@
+            dotBox.y1 = Math.round(box.y1 + (height - dotWidth) / 2);
+            dotBox.y2 = dotBox.y1 + dotWidth;
+            this._dot.allocate(dotBox, flags);

Pretty sure I made it work to do:

 this._dot.allocate({ x1: ...  ,
                      x2:     .
                    }, flags);

Not sure if that's clearer than 'new Clutter.ActorBox())' anyways

Review of attachment 173183 [details] [review]:

Not sure I completely agree with the visual idea and breaks Fitts-law for the menu, but the patch looks good

(In reply to comment #12)
> +        this._boxWrapper = new Shell.GenericContainer();
> 
> This member variable isn't actually used anywhere outside this function - could
> be argued either way if it's good style or not to have the variable - no strong
> opinions, just wanted to point it out in case it was an unintentional
> oversight.

I hadn't noticed that it wasn't used outside the function, but it feels weird to me to have it as just a local variable but be connecting to its signals. So I left it as is.

> +                    if (!columnWidths[j] || itemColumnWidths[j] >
> columnWidths[j])
> 
> really don't like using ! here on something that either could be numeric 0 or
> undefined.

changed to "j >= columnWidths.length"

> Either there is some magic going on here that needs to be commented

nope, I just flaked out while writing that, and then didn't notice when testing because we don't actually have any submenus yet...

(In reply to comment #13)
> Not worth spending time on unless the dots
> are obviously fuzzy, I think.

to my eyes, they aren't, although i have a 145 dpi screen... we can revisit if needed

>  this._dot.allocate({ x1: ...  ,
>                       x2:     .
>                     }, flags);
> 
> Not sure if that's clearer than 'new Clutter.ActorBox())' anyways

maybe in simple cases. Here I'm setting dotBox.x2 to "dotBox.x1 + ..." though, so I think it works better with the struct

Created attachment 173221 [details] [review]
popupMenu: make menu layout more table-like

When there are menu items with right-aligned items, all the
right-aligned items should appear to the right of all the left-aligned
items.

Clutter doesn't have an equivalent of GtkSizeGroup, so hack something up
using ShellGenericContainer and some javascript.

Created attachment 173222 [details] [review]
statusMenu: show a radio button-style dot next to the presence status

Adding a "PopupMenuRadioButtonItem" wouldn't work well, because we'll
need radio-button indicators on multiple different styles of menu
item. Also, the current design draws the indicator in the menu item's
padding, so it's sort of special anyway. So just add support at the
BaseMenuItem level.

Also, redo the menu/menuitem padding so that all the horizontal
padding is in the menu item, or else the indicator dot will show up in
the wrong spot.

Review of attachment 173221 [details] [review]:

Looks good

Review of attachment 173222 [details] [review]:

Looks good

Attachment 172860 [details] pushed as 86efdc9 - PopupImageMenuItem: always show icon, on the right, not the left
Attachment 172861 [details] pushed as fa75211 - statusMenu: fix menu items to match latest mockups
Attachment 173183 [details] pushed as 2b3c31a - boxpointer: keep a margin between the box and the screen edge
Attachment 173221 [details] pushed as 5661946 - popupMenu: make menu layout more table-like
Attachment 173222 [details] pushed as 9d94da8 - statusMenu: show a radio button-style dot next to the presence status