Right, this needs some gnome-session D-Bus API work; we want to let gnome-session take care of initiating the suspend, because it's clever about a few things (eg, making sure to lock the screen before suspending so there isn't a race condition when you resume). But currently there's only API for "pop up a dialog with all 4 options".

*** Bug 641555 has been marked as a duplicate of this bug. ***

well now the api means "pop up a dialog with only shutdown" so this doesn't even sort of work.  We could switch to using upower but that has the problems mentioned in bug 575880 comment 6.

So for now, I'm going to just going to take it out.

I'm pretty sure I remember seeing some conversations go by between the designers where this item was destined for the chopping block anyway.  Regardless, we can add it back when we have a way to make it work.

(In reply to comment #3)
> well now the api means "pop up a dialog with only shutdown" so this doesn't
> even sort of work.  We could switch to using upower but that has the problems
> mentioned in bug 575880 comment 6.
> 
> So for now, I'm going to just going to take it out.
> 
> I'm pretty sure I remember seeing some conversations go by between the
> designers where this item was destined for the chopping block anyway. 
> Regardless, we can add it back when we have a way to make it work.

We've had suspend buttons in gnome-panel for ages. How hard can this be?

This would leave us with no way to suspend a non-laptop.

Created attachment 180128 [details] [review]
statusMenu: temporarily drop Suspend menu item

It doesn't currently work, so hide it for now.

It's not clear it's going to stay around long term,
anyway. If it doesn't we can delete the code, then.
Otherwise, we can add the code back when we have
something that works.

(In reply to comment #5)
> Created an attachment (id=180128) [details] [review]
> statusMenu: temporarily drop Suspend menu item
> 
> It doesn't currently work, so hide it for now.
> 
> It's not clear it's going to stay around long term,
> anyway. If it doesn't we can delete the code, then.
> Otherwise, we can add the code back when we have
> something that works.

I am not really sure this is the right approach as this just adds pressure to either fix it before the UI freeze or leaving 3.0 without a way to suspend desktop systems.

(In reply to comment #4)
> (In reply to comment #3)
> > well now the api means "pop up a dialog with only shutdown" so this doesn't
> > even sort of work.  We could switch to using upower but that has the problems
> > mentioned in bug 575880 comment 6.
> > 
> > So for now, I'm going to just going to take it out.
> > 
> > I'm pretty sure I remember seeing some conversations go by between the
> > designers where this item was destined for the chopping block anyway. 
> > Regardless, we can add it back when we have a way to make it work.
> 
> We've had suspend buttons in gnome-panel for ages. How hard can this be?
Ages ago we had a suspend button in the panel. it talked to gnome-power-manager, but I don't think gnome-power-manager exports an api for doing suspends now (with the expectation that you'll use the upower api).

For a long time though we've dropped the separate suspend item in the panel and instead had a unified dialog (presented by gnome-session).  that dialog is no longer unified in the shell case.

> This would leave us with no way to suspend a non-laptop.
true.

What we could probably do is make the shell use the upower client api directly and then fix it do the policy things that gnome-session is doing right now.

Created attachment 180137 [details] [review]
statusMenu: add back suspend item

This commit adds back the suspend item removed
by commit 59191bc4b7899eb4c8aa1dba866a82dd77ef7907

So attachment 180137 [details] [review] makes gnome-shell use the upower api for the sleep item.

There are still two missing pieces:

1) fixing the upower client library to do screen locking, etc, that gnome-power-manager used to do for us many moons ago

2) making sure a suspend item is something actually wanted

an alternative to 1) is to do the same stuff that gnome-power-manager used to do directly ourselves.  This is the route that gnome-session used to take.

Review of attachment 180137 [details] [review]:

Codewise looks fine, whether we want it or not is not up to me to decide but it makes as otherwise
there is no way to suspend desktops other then resorting to the cmdline.

::: js/ui/statusMenu.js
@@ +89,3 @@
 
+    _updateSuspend: function() {
+        if (this._upClient.get_can_suspend ())

Remove the space after suspend.

Review of attachment 180137 [details] [review]:

Oh one more thing:

::: js/ui/statusMenu.js
@@ +152,3 @@
         this.menu.addMenuItem(item);
 
+        item = new PopupMenu.PopupMenuItem(_("Suspend..."));

As this no longer opens a dialog remove the "..."

Can we please decide what to do here? Just not having a way to suspend is a regression ... if we can't come up with something else we should just add it back and replace it once we have something else (maybe post 3.0).

What I would like to do is to suspend by default on power button tap and show power options on a longer press.  And retain the hard power off on extended hold.

Created attachment 181388 [details] [review]
Implemented suspend function in statusMenu.js

Users can now suspend successfully from the top right status menu.

I admit, that this is rather hacky, but just another way of doing it very simply in contrast to fully importing and implementing UPower like halfline did. 

If we were to implement if fully, why not implement fully for others instead of using Util.spawn?  :(

Created attachment 181389 [details] [review]
using Util.spawn

I admit that this is rather hacky, compared to what halfline did by fully implementing the API calls...

But if we were to implement it fully, why not we do the same for others instead of using Util.spawn? :(

Comment on attachment 181389 [details] [review]
using Util.spawn

sorry for the double post. git bz hanged for a long time so i decided to attach this manually... but it already did pasted something.

(In reply to comment #16)
> Created an attachment (id=181389) [details] [review]
> using Util.spawn
> 
> I admit that this is rather hacky, compared to what halfline did by fully
> implementing the API calls...
> 
> But if we were to implement it fully, why not we do the same for others instead
> of using Util.spawn? :(

The problem isn't the implementation but the design, Jon's proposal cannot be done because we do not get such events by the hardware (and we cannot fix this unless we start building our own hardware).

This resulted into a long IRC conversation (sorry don't have the logs) where it was proposed to just suspend by default and only shutdown when using a modifier key. This was under the assumption that laptops can stay suspended "for weeks" and so for most users there is no need to ever shutdown.

My measurements to verify that claim come to the conclusion that this is not true (at least for some hardware), and having a shorted battery life on hardware that otherwise lasts ~6 hours (the laptop I tested with drains half the battery when being suspended for a day) isn't a great user experience either.

So I am against a design that either
1) requires hardware that do not exists 
or
2) makes unverified assumptions about hardware that does not seem to be true

As the UI freeze is tomorrow I'd suggest just adding the suspend item back and think about something that works (without ignoring reality) for 3.2.

Review of attachment 181388 [details] [review]:

I fail to see the point here ... we shouldn't randomly spawn external utilities when we can just either
use the upower library or at least issue the dbus call directly.

Jon posted updated designs here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

Owen's asked me to look into implementing them, so stay tuned.

(In reply to comment #20)
> Jon posted updated designs here:
> 
> https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart
> 
> Owen's asked me to look into implementing them, so stay tuned.

Which basically ignores everything I said in comment 18 ...

(In reply to comment #21)
> (In reply to comment #20)
> > Jon posted updated designs here:
> > 
> > https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart
> > 
> > Owen's asked me to look into implementing them, so stay tuned.
> 
> Which basically ignores everything I said in comment 18 ...

Yes, it's a calculated intent to ignore it. The set of users that:

 - Have hardware that isn't properly handled by our software
   (suspended power draw simply shouldn't be that high)
 - Are aware of it
 - Take the effort to decide between Suspend and Shutdown 
   based on their future intensions

Is not large enough to make it worth making the user interface harder to navigate for everyone. Savy users who know they want to shutdow will be able to find a way to suspend with reasonable convenience.

hey,

I understand you aren't completely happy with the design, but it's what we have right now.  You know as well as anyone, if we should ultimately do something else, then we need to change the design first.  And the best avenue for doing that is by directly talking 1-on-1 with the designer.

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > Jon posted updated designs here:
> > > 
> > > https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart
> > > 
> > > Owen's asked me to look into implementing them, so stay tuned.
> > 
> > Which basically ignores everything I said in comment 18 ...
> 
> Yes, it's a calculated intent to ignore it. The set of users that:
> 
>  - Have hardware that isn't properly handled by our software
>    (suspended power draw simply shouldn't be that high)
>  - Are aware of it
>  - Take the effort to decide between Suspend and Shutdown 
>    based on their future intensions
> 
> Is not large enough to make it worth making the user interface harder to
> navigate for everyone.

1) We don't have any numbers on that just vague assumptions
2) Err ... aware of what? Yes I am aware that this is the case for my particular hardware .. But I am not just talking about myself here otherwise I could just write an extension or even patch the code locally and be done with it.

(In reply to comment #23)
> hey,
> 
> I understand you aren't completely happy with the design, but it's what we have
> right now.  You know as well as anyone, if we should ultimately do something
> else, then we need to change the design first.  And the best avenue for doing
> that is by directly talking 1-on-1 with the designer.

I tried but it is pointless if we are designing for hardware that might exist in 5 years and ignore what our users have today.

But anyway I said what I think about this, you are free to ignore it .. I am not going to start a useless flamewar here so I will just leave it at that and shut up.

"Yes, it's a calculated intent to ignore it. The set of users that:

 - Have hardware that isn't properly handled by our software
   (suspended power draw simply shouldn't be that high)
 - Are aware of it
 - Take the effort to decide between Suspend and Shutdown 
   based on their future intensions

Is not large enough"

this behaviour doesn't seem to be limited to laptops in any way. My understanding from the kernel developers is that suspend is badly broken on a lot of desktop hardware and will not give good results at all, so it may not be a very good idea to make it the default behaviour and hide power off.

(though as it happens, my desktop suspends quite well - I only lose the network adapter on a resume one in every five tries. What do I do when I lose the network adapter on a resume? I reboot, of course. *sigh*)

(In reply to comment #25)
> "Yes, it's a calculated intent to ignore it. The set of users that:
> 
>  - Have hardware that isn't properly handled by our software
>    (suspended power draw simply shouldn't be that high)
>  - Are aware of it
>  - Take the effort to decide between Suspend and Shutdown 
>    based on their future intensions
> 
> Is not large enough"
> 
> this behaviour doesn't seem to be limited to laptops in any way. My
> understanding from the kernel developers is that suspend is badly broken on a
> lot of desktop hardware and will not give good results at all, so it may not be
> a very good idea to make it the default behaviour and hide power off.

Matthew Garrett, who is about as clued in about suspend and resume as anybody, was comfortable with a default option to suspend desktops - at least for desktops where the system thinks that it can suspend them at all

Note that there are only two options here that make sense:

 - Encourage users to suspend by making it the easy option to select
 - Not offer suspend at all

We cannot offer a suspend option that breaks your network every fifth time and count on users to figure that out and remember not to pick the Suspend option.

I'll throw up a rough cut now for some of suspend/shutdown parts of the mockups shortly.

We should probably do the restart bits on bug 641375 instead of here, since it's a more relevant bug report for that part.

Created attachment 181505 [details] [review]
statusMenu: Add back suspend item

This commit adds back the suspend item removed
by commit 59191bc4b7899eb4c8aa1dba866a82dd77ef7907

Created attachment 181506 [details] [review]
build: Add upower as dependency in moduleset

Now that we use it to suspend, we need to pull it
in during the build process.

Created attachment 181507 [details] [review]
statusMenu: s/Shut Down.../Power Off.../

This updates the text to match the latest mockups
here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

Created attachment 181508 [details] [review]
statusMenu: Only show one of suspend or poweroff, not both

The designs here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

only show either "Suspend" or "Power off..." never both.

Right now, that means if your system supports suspend you'll get a
suspend item, and if it doesn't, you'll get a power off item.

Created attachment 181509 [details] [review]
statusMenu: Show Power Off when Alt key is down

The designs here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

specify there should be a way to gain access to the "Power Off..." item
when holding a modifier.

This commit makes it appear dynamically when the user hits the alt key.

If we do end up showing the "Power Off..." item from a user initiated
action (versus by default because suspend is unsupported), then we
style it bold.

Things in the air:

- There's still the open question from comment 9 and comment 10 on who should do screen locking (upower or us)

- Aside from that, another change that may make sense is to move the alt handling code to the generic menu instead of as a special case in the statusMenu code.

- There's a sort of odd behavior where alt doesn't work until you mouse into the menu.

I redid it to use a new PopupAlternatingMenuItem class instead of trying to handle all the menu toggling inside statusMenu.js.

Since it's a different approach the patches are less incremental.

Will post shortly.

Created attachment 181538 [details] [review]
build: Add upower as dependency in moduleset

Now that we use it to suspend, we need to pull it
in during the build process.

Created attachment 181539 [details] [review]
popupMenu: add alternate menu item

This is special menu item that can alternate
between two choices when you hit the alt key.

It will be useful for getting a hybrid
suspend/power off menu item.

Created attachment 181540 [details] [review]
statusMenu: change how we stop the system

This updates the way we stop the system to
somewhat match the designs here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

We suspend by default unless suspend is unavailable, and offer shutdown
as a choice by hold down the alt key.

I just noticed that I need to either squash attachment 181538 [details] [review] with attachment 181540 [details] [review] or swap their order for the commit message of attachment 181538 [details] [review] to make sense.

I don't really understand the point of using the Alt key to show the power off item. This will be useful for advanced (whatever it means) users that will be clever enough to find out this keybinding, but all people that won't think "hmm, I need to read the help because there must be a way of forcing power off" are just screwed.

Would it be so costly in terms of UI clarity to always show both Poweroff and Suspend in the last section of the user menu? By putting Suspend at the end, we clearly make it the natural choice, and if it works well (as the design assumes), people will use it. Or let's make Poweroff... show a shutdown dialog providing Shutdown/Reboot/Hibernate: that way, only Suspend is a direct action, which makes it even more clear it's what users should do. (If we want to make the menu shorter, Log Out and Switch User could be merged with less harm.)

On the general change, FWIW, I think it's really dumb to suspend your computer when you know you won't use it for the whole night and most of the day after, which is the common case for home desktops. Users know better than the software in this case. In suspend modes, computers do use power, and environmentalists usually say devices in standby mode use the power of a whole nuclear plant in France - I don't think GNOME should support this trend.

I totally agree with Milan Bouchet-Valat. I only suspend if i am going away less than 1/2 hours. Why waste energy? The boot time is short this thays... 

Like Milan said, why not but a dynamic button before suspend ? Or use an inline submenu to show more options? The "My account" menu item can be removed (is pretty useless) to not increase the menu height too much. For 3.2, we can add the ability for change the language without having to logout and than remove the logout and switch user options if the machine only have one account to decrease the menu height even more. But for now, please show a discoverable option to power off.

Personally, this was the worst design decision of the shell (which i like).

Hey guys,

Everyone has things they like and don't like about the shell.  I'm sure we all agree that we can't all agree on every detail.

Anyway, shell development has very consciously been driven from designs.  And those designs, have very consciously being created and/or vetted by designers.   

McCann is the designer behind these specific designs, so if you have issues talk with him about it.  Note, I"m not saying if you talk to him that you're guaranteed to ultimately end up with exactly what you want. There are tons of people all with different opinions and there is only one shell.  That means there are going to be disagreements behind every decision.  Also, I don't think voting on details like this works either. There needs to be one person or a small group of people making the tough calls to ensure the final result is coherent and cohesive (but talk to him!).

Shutdown is still available from the login screen, FWIW.

Review of attachment 181538 [details] [review]:

::: tools/build/gnome-shell.modules
@@ +280,3 @@
   </tarball>
 
+  <tarball autogenargs="--enable-introspection=no" id="upower" version="0.9.8">

The whole point of building this rather than depending on the system version is to get introspection, right?

I'd sort of like to switch the gnome-shell moduleset over to all git, no tarballs, but not much point in doing that piecemeal, so OK this way.

Review of attachment 181539 [details] [review]:

I think that it has to reliable or it's really confusing. As implemented this way, it's not reliable at all... e.g., if you press alt at the wrong time it doesn't work. My suggestion for technique

  - connect to notify::mapped
  - when mapped, install a capturing stage keypress handler, and check the current state of MOD1 key using global.get_pointer()
  - if you get an Alt-L or Alt-R keypress or release, recheck the current state of the MOD1 key using global.get_pointer()
  - when unmapped disconnect

If we had a bunch of alternating menu items, we might want to have some sort of "ModifierTracker" object that you can turn on and connect to signals on to avoid duplicate calls to get_pointer(), but at the current time, that seems unlikely

I don't like hard-coding the Alt/MOD1 correspondance, but I can't think of a better way to do it without getting into XKB hell.

::: js/ui/popupMenu.js
@@ +364,3 @@
+        if (!this.isDefault) {
+            if (!this.actor.has_style_pseudo_class('alternate'))
+                this.actor.add_style_pseudo_class('alternate');

You don't need the check - add_style_pseudo_class already avoids duplicates

@@ +368,3 @@
+        } else {
+            if (this.actor.has_style_pseudo_class('alternate'))
+                this.actor.remove_style_pseudo_class('alternate');

And remove_style_pseudo_class is a no-op if the class isn't there

@@ +373,3 @@
+    },
+
+    alternate: function() {

it doesn't make sense to me to have a public API function to change this - I think it should be tied tightly to the state of the modifier key

@@ +397,3 @@
+    },
+
+    swapDefault: function(shouldSwap) {

swapDefault/forceDefault makes my head hurt. Why don't we just have:

  setText
  setAlternateText

and if you setAlternateText to null, the alternating menu item doesn't alternate.

Hi,

(In reply to comment #44)
> Review of attachment 181539 [details] [review]:
> 
> I think that it has to reliable or it's really confusing. As implemented this
> way, it's not reliable at all... e.g., if you press alt at the wrong time it
> doesn't work.
Well it's not really at the wrong time, it's at the wrong position.  The mouse has to go into the menu before it works. Totally worth fixing though.

> My suggestion for technique
> 
>   - connect to notify::mapped
>   - when mapped, install a capturing stage keypress handler, and check the
> current state of MOD1 key using global.get_pointer()
>   - if you get an Alt-L or Alt-R keypress or release, recheck the current state
> of the MOD1 key using global.get_pointer()
>   - when unmapped disconnect
Sounds good, I'll look into it.

> I don't like hard-coding the Alt/MOD1 correspondance, but I can't think of a
> better way to do it without getting into XKB hell.
I'd be a lot more concerned if it was a different modifier than alt.  Alt and Mod1 are so much the same that gdk doesn't even provide a GDK_ALT_MASK like it does for shift, control, super, hyper, etc...

> @@ +373,3 @@
> +    },
> +
> +    alternate: function() {
> 
> it doesn't make sense to me to have a public API function to change this - I
> think it should be tied tightly to the state of the modifier key
Okay.

> @@ +397,3 @@
> +    },
> +
> +    swapDefault: function(shouldSwap) {
> 
> swapDefault/forceDefault makes my head hurt. Why don't we just have:
> 
>   setText
>   setAlternateText
> 
> and if you setAlternateText to null, the alternating menu item doesn't
> alternate.
The problem with that technique is then we have to track which one is in the front at activate time.  Obviously, doing a string comparison on the label would be suboptimal.  I guess I could just check if the alternate text is null and and know that way.

Maybe it would be nicer from an api point of few to pass an id in with label. or some such

(In reply to comment #45)

 
> > My suggestion for technique
> > 
> >   - connect to notify::mapped
> >   - when mapped, install a capturing stage keypress handler, and check the
> > current state of MOD1 key using global.get_pointer()
> >   - if you get an Alt-L or Alt-R keypress or release, recheck the current state
> > of the MOD1 key using global.get_pointer()
> >   - when unmapped disconnect
> Sounds good, I'll look into it.
> 
> > I don't like hard-coding the Alt/MOD1 correspondance, but I can't think of a
> > better way to do it without getting into XKB hell.
> I'd be a lot more concerned if it was a different modifier than alt.  Alt and
> Mod1 are so much the same that gdk doesn't even provide a GDK_ALT_MASK like it
> does for shift, control, super, hyper, etc...

Well, sort of - shift/control are there because they are standard in the X protocol. MOD1 was the messy case - on old Sun keyboards you had (as I recall):

 [Shift][ Alt ][ Meta ][             Space            ] ....

So meta was the primary modifier there and alt was on PC keyboards - and you wanted to use the primary modifier not Alt. So we intentionally *didn't* devirtualize and figure out what was Alt and what was Meta, but just bound actions to MOD1.

The virtual mods for Super/Hyper were added later. But nowdays MOD1 mapped to Meta really isn't a factor or something we need to worry about.

> > 
> >   setText
> >   setAlternateText
> > 
> > and if you setAlternateText to null, the alternating menu item doesn't
> > alternate.
> The problem with that technique is then we have to track which one is in the
> front at activate time.  Obviously, doing a string comparison on the label
> would be suboptimal.  I guess I could just check if the alternate text is null
> and and know that way.

I'd suggest:

 if (!this._haveSuspend || this._suspendItem.isAlternate))
   doPowerOffStuff()

Review of attachment 181540 [details] [review]:

Will need some changes for the menu item changes. Other than that, noticed:

::: js/ui/statusMenu.js
@@ +165,3 @@
         this.menu.addMenuItem(item);
+        this._suspendOrPowerOffItem = item;
+        this._updateSuspendOrPowerOff();

Slightly prefer to see this moved after the signal connection - I expect to always see 'connect signal and then do the initial update' pairs and not have them split.

@@ +205,3 @@
+            this._upClient.suspend_sync(null);
+        } else {
+            Util.spawn(['gnome-session-save', '--shutdown-dialog']);

Pretty sure we wanted a Restart button in that dialog. Is that hard?

OK, so in design terms - this is certainly a hard question, and there are legitimate reasons for people to do all sorts of things - suspend, power off, log out, even hibernate. But we know from experience that having *all* of them as options doesn't work.

I'm not all that swayed by the environmental argument. Power consumption of a suspended laptop (or desktop) isn't 0, but it's pretty small. Without measuring a bunch of laptops, it's hard to be sure, but if you search around, you'll find numbers around 1 watt for a suspended laptop. So depending on the country, you'd be talking 2-5 cents of electricity a week. There seem to be a lot more promising ways to save 2-5 cents worth of electicity than managing when your laptop is suspended or not. To improve the power consumption of the Linux universe, much more useful is to make sure suspend works and suspend on idle even when on AC.

More concerning to me is trying to milk battery life from a laptop when travelling ... if a laptop really is using 1W of power when suspended (again, I'd like to have real numbers across a range of laptops) that might mean losing half your power in a day on a laptop with a smaller battery. But at the end, I don't think asking users to have a mental model of how much power their laptop uses when suspended vs. shut off is reasonable. It's not solving the  problem to give the user options that enable them to craft a correct solution - solving the situation means taking it out of the set of things the user needs to worry about - let them concentrate on catching their plane. So, really, the only actual solution I see is going towards hybrid suspend.

There's two things we have to balance here - clearly we need something that work for current hardware, for current drivers, for our current user base. But we also have to concentrate on designing something that is genuinely excellent. Which often means being a bit aspirational in what we expect from the hardware and the drivers. Not worrying excessively about all the corner cases and old hardware and broken hardware and users with specialized needs.

It's not like powering off is made impossible - you can log out to power off. That should be easily discoverable. And the modifier key is there for advanced users. So I'm pretty comfortable with this approach. And if it doesn't work, we can always add Power Off... back.

Created attachment 181612 [details] [review]
popupMenu: add alternate menu item

This is special menu item that can alternate
between two choices when you hit the alt key.

It will be useful for getting a hybrid
suspend/power off menu item.

Created attachment 181613 [details] [review]
statusMenu: change how we stop the system

This updates the way we stop the system to
somewhat match the designs here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

We suspend by default unless suspend is unavailable, and offer shutdown
as a choice by hold down the alt key.

(I posted comment 49 and comment 50 before seeing comment 47 and comment 48)

Hi,
> ::: js/ui/statusMenu.js
> @@ +165,3 @@
>          this.menu.addMenuItem(item);
> +        this._suspendOrPowerOffItem = item;
> +        this._updateSuspendOrPowerOff();
> 
> Slightly prefer to see this moved after the signal connection - I expect to
> always see 'connect signal and then do the initial update' pairs and not have
> them split.
okay,

> @@ +205,3 @@
> +            this._upClient.suspend_sync(null);
> +        } else {
> +            Util.spawn(['gnome-session-save', '--shutdown-dialog']);
> 
> Pretty sure we wanted a Restart button in that dialog. Is that hard?
We can fudge it pretty easily with a patch to endSessionDialog but the design doesn't have one for that case.  

It only has one for the multiple choice dialog shown when you hit the power button.

(In reply to comment #52)
> > @@ +205,3 @@
> > +            this._upClient.suspend_sync(null);
> > +        } else {
> > +            Util.spawn(['gnome-session-save', '--shutdown-dialog']);
> > 
> > Pretty sure we wanted a Restart button in that dialog. Is that hard?
> We can fudge it pretty easily with a patch to endSessionDialog but the design
> doesn't have one for that case.  
> 
> It only has one for the multiple choice dialog shown when you hit the power
> button.

Ah, OK, it was unclear in the early versions of the designs I saw and I thought mccann told me that it was going to be the same for both.

Review of attachment 181612 [details] [review]:

::: data/theme/gnome-shell.css
@@ +142,3 @@
 
+.popup-alternating-menu-item {
+}

I wouldn't add this empty class - style application is O(n) in the number of the rules, and even if n is already huge, worth not making it bigger.

::: js/ui/popupMenu.js
@@ +347,3 @@
+
+        this.actor.connect('parent-set', Lang.bind(this, this._onParentSet));
+        this._notifyMappedId = this.actor.connect('notify::mapped', Lang.bind(this, this._onMapped));

You don't use this and it seems like it would always stay around unti lthe actor was destroyed so no reason to disconnect

@@ +356,3 @@
+            this._alternateIfNeeded();
+        } else {
+            if (this._capturedEventId > 0) {

prefer != - the only ID that is guaranteed not to be used in glib terms is 0. In C, yes, it's a guint , but 0 or non-zero is what you should be thinking.

@@ +368,3 @@
+        if ((mods & Clutter.ModifierType.MOD1_MASK) != 0) {
+            if (this.isDefault)
+                this._alternate();

this clearly shows that this code should be:

 this._setIsDefault((mods & Clutter.ModifierType.MOD1_MASK) == 0);

then have the short-circuit inside setIsDefault()

(actually, I prefer the reversed isAlternate to isDefault because then we don't have two terminology items 'default' and 'alternate' appearing, instead of just one, but up to you on that)

@@ +378,3 @@
+        if (event.type() != Clutter.EventType.KEY_PRESS &&
+            event.type() != Clutter.EventType.KEY_RELEASE)
+            return;

slightly prefer return false; for event functions rather than counting on undef is false.

@@ +386,3 @@
+    },
+
+    _onParentSet: function(actor, oldParent) {

leftover

@@ +403,3 @@
+    },
+
+    canAlternate: function() {

This strike me as the model asking the view about something that is a state of the model - did it have a reason to set alternate text? But OK if you rename to 'hasAlternateText'.

@@ +430,3 @@
+
+        this._updateLabel();
+        this.emit('alternated');

leave this out until we need it

Looking into this more, we would need a patch to gnome-session as well to add restart anyway, so I don't think it could make this release.

(In reply to comment #54)
> Review of attachment 181612 [details] [review]:
> 
> ::: data/theme/gnome-shell.css
> @@ +142,3 @@
> 
> +.popup-alternating-menu-item {
> +}
> 
> I wouldn't add this empty class - style application is O(n) in the number of
> the rules, and even if n is already huge, worth not making it bigger.
Okay, I was mimicing some of the other items, but i'll drop it.

> ::: js/ui/popupMenu.js
> @@ +347,3 @@
> +
> +        this.actor.connect('parent-set', Lang.bind(this, this._onParentSet));
> +        this._notifyMappedId = this.actor.connect('notify::mapped',
> Lang.bind(this, this._onMapped));
> 
> You don't use this and it seems like it would always stay around unti lthe
> actor was destroyed so no reason to disconnect
ah yea.

> @@ +356,3 @@
> +            this._alternateIfNeeded();
> +        } else {
> +            if (this._capturedEventId > 0) {
> 
> prefer != - the only ID that is guaranteed not to be used in glib terms is 0.
> In C, yes, it's a guint , but 0 or non-zero is what you should be thinking.
Okay (i actually usually use != 0 as well, but this was a copy-and-paste)

> @@ +368,3 @@
> +        if ((mods & Clutter.ModifierType.MOD1_MASK) != 0) {
> +            if (this.isDefault)
> +                this._alternate();
> 
> this clearly shows that this code should be:
> 
>  this._setIsDefault((mods & Clutter.ModifierType.MOD1_MASK) == 0);
> 
> then have the short-circuit inside setIsDefault()
Sure, I can change it.  I'm not sure I agree it's obviously better your way, though.

> (actually, I prefer the reversed isAlternate to isDefault because then we don't
> have two terminology items 'default' and 'alternate' appearing, instead of just
> one, but up to you on that)
I actually started with isAlternate but changed it since in a way they're both alternates, and I had a (then public) function named alternate() that alternated between the alternates.

> 
> @@ +378,3 @@
> +        if (event.type() != Clutter.EventType.KEY_PRESS &&
> +            event.type() != Clutter.EventType.KEY_RELEASE)
> +            return;
> 
> slightly prefer return false; for event functions rather than counting on undef
> is false.
ugh, yea me too.

Created attachment 181624 [details] [review]
popupMenu: add alternate menu item

This is special menu item that can alternate
between two choices when you hit the alt key.

It will be useful for getting a hybrid
suspend/power off menu item.

Created attachment 181625 [details] [review]
statusMenu: change how we stop the system

This updates the way we stop the system to
somewhat match the designs here:

https://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart

We suspend by default unless suspend is unavailable, and offer shutdown
as a choice by hold down the alt key.

I decided to use the name "state" to match some of the other menu items in the code and then switch to using an enumeration value instead of boolean.

Review of attachment 181624 [details] [review]:

Hopefully just one more iteration

::: js/ui/popupMenu.js
@@ +369,3 @@
+    _setState: function(state) {
+        if (this.state != state)
+            this._alternate();

I wouldn't have written it with the split between _setState() and _alternate() [I generally have a distaste for functions that toggle, so I wouldn't go to that length to introduce one] but if you are most comfortable writing it this way, OK.

@@ +434,3 @@
+            this._nonDefaultActivateId = this.connect('activate', Lang.bind(this,function() {
+                if (this.state == PopupAlternatingMenuItemState.ALTERNATIVE)
+                    this._alternate();

It's an alternative until the user releases the modifier. Dont' see why we need this complexity.

@@ +444,3 @@
+        this._alternateText = alternateText;
+
+        this._updateLabel();

You need some sort of if (!this._canAlternate) { this._setState(DEFAULT); } here - dont 'think we should ever report an impossible state. [I'm not too concerned though if canAlternate becomes true and the alt key is already held down - it's OK to stick with the DEFAULT state in that case]

Review of attachment 181625 [details] [review]:

Looks fine

Created attachment 181654 [details] [review]
popupMenu: add alternate menu item

This is special menu item that can alternate
between two choices when you hit the alt key.

It will be useful for getting a hybrid
suspend/power off menu item.

hang on, going to do one more update after another read through

Created attachment 181656 [details] [review]
popupMenu: add alternate menu item

This is special menu item that can alternate
between two choices when you hit the alt key.

It will be useful for getting a hybrid
suspend/power off menu item.

Review of attachment 181656 [details] [review]:

One bug, good to commit with that fixed.

::: js/ui/popupMenu.js
@@ +424,3 @@
+
+        if (!this._canAlternate())
+            this._setState(PopupAlternatingMenuItemState.DEFAULT);

This doesn't update the label if state didn't change - just do the this._updateLabel() unconditionally - the pseudo-class and text changes will be short-circuited inside the C code, so it doesn't hurt to do them twice..

(committed with that change)

Reopening as I think the reasoning of this is wrong. I hope that this is the right place to discuss the design decision.

From an environmental point of view it would be horrible if we suspended by default especially for desktop PCs. This consumes considerable amount of extra power compared to a switched off system which should consume less than 1 Watt for newer ATX standard compliant devices.

Besides there are quite some areas where you really need to switch off devices (airport luggage for example) and even if you can switch off easily by pressing the Power button people are not really used to it. I think the visual clutter of the "Shut Down" button is rather low compared to the disadvantages of loosing it.

Not the right place.

>> Not the right place.

As much as I love some of the directions gnome-shell is taking, the much I hate the epic fail in communication, giant last minute changes and the total absence of real world testing.

If bugzilla is not the right place for discussion, please point out a better place...

Visibly, not on the mailing list either. 

This decision leads to waste of energy.

just for those following along, bug 643357 is dealing with the screen locking issue mentioned in comment 9 and comment 10.