From a implementation point of view, the only thing you need to change if you want to power-off rather than reboot is rather than calling "/usr/libexec/pk-trigger-offline-update" you just need to call "/usr/libexec/pk-trigger-offline-update power-off". Old versions of PackageKit will ignore this extra parameter so don't worry about breaking things.

See https://gitorious.org/packagekit/packagekit/commit/98d9a3d3bda2e9c1bfee0ad23443a84be71d55e2 for the PackageKit commit for this functionality.

Created attachment 267127 [details]
wireframes

Attaching slightly updated wireframes based on feedback from Jakub.

What's the default state of that checkbox i.e opt-in or opt-out? Opt-out might lead to the case where the user quickly presses enter or cick the button and end up installing updates at inconvenient times.

(In reply to comment #3)
> What's the default state of that checkbox i.e opt-in or opt-out? Opt-out might
> lead to the case where the user quickly presses enter or cick the button and
> end up installing updates at inconvenient times.

The idea is that the default behaviour would be to install the updates. I agree with you that there's a risk there, but the alternative is people perpetually ignoring their updates (and they could be critical fixes).

(In reply to comment #3)
...

Also, this will only be shown on power off - which isn't something you generally do if you are quickly wanting to stop using your computer. It's not like it would stop someone from closing their laptop to run for the bus.

(In reply to comment #4)
> (In reply to comment #3)
> > What's the default state of that checkbox i.e opt-in or opt-out? Opt-out might
> > lead to the case where the user quickly presses enter or cick the button and
> > end up installing updates at inconvenient times.
> 
> The idea is that the default behaviour would be to install the updates. I agree
> with you that there's a risk there, but the alternative is people perpetually
> ignoring their updates (and they could be critical fixes).

Why? Will we stop showing notifications?

(In reply to comment #6)
> Why? Will we stop showing notifications?

The issue is timing - the notifications usual pop up when you are busy with something else, so people often dismiss or ignore them. Offering to install the updates at shutdown time is a good way to provide a reminder at a time when people are in a position to update.

Looking at the implementation side of this, there's a number of things we need:

1. PackageKit needs to add some api to trigger not just trigger offline-update-and-restart, which is currently done by creating /system-update, but also a way to trigger offline-update-and-poweroff. That could be done in various ways, like creating a marker file in /var/cache/PackageKit. Looking at the mockups, it has to be possible to set this independently of preparing the offline update itself, and it has to be possible to set and unset it repeatedly (the user may toggle the checkbox a couple of times, just for fun).

Strawman: pk-trigger-offline-update --set-restart-mode=shutdown|reboot

2. It looks like we should prepare the offline update as soon as we notice that it is available. This will make us default to installing offline updates. Unchecking the 'Install pending updates' checkbox will have to call pk-trigger-offline-update --cancel, and checking it again will have to call pk-trigger-offline-update to prepare the update again. Slight trick here is that we don't want a polkit dialog to come up while we are already in a system modal dialog.

3. Currently the shell decides whether to show the regular power off dialog or the restart & install dialog by looking for a prepared system update. That won't work for this, we have to differentiate the two ways to trigger these dialogs somehow.

Wrt to the second point, the default policy is to allow pkexec /usr/libexec/pk-trigger-offline-updates from active sessions. We should probably check the permission, and only show the checkbox if it is allowed without challenge.

https://gitorious.org/packagekit/packagekit/commit/98d9a3d3bda2e9c1bfee0ad23443a84be71d55e2

adds the needed 'reboot mode' functionality to PackageKit

<hughsie> mclasen, i think creating the prepared-update file as soon as possible is a very good idea, just not setting the trigger

for differentiating when to show which dialog:

<mclasen> we may even be able to continue doing what we do now
<mclasen> if the update has already been triggered, show 'restart & install'
<mclasen> otherwise show 'poweroff'
<mclasen> and use the existence of a prepared update to show the checkbox or not

Allan: When clicking "Install updates" from gnome-software, I think it would be better to provide two buttons labeled, like in the power off case, "Restart & install" and "Install & Shut down". This is more convenient that ticking a check box, and it's consistent with the other dialog.

But I really like the general idea!

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

(In reply to comment #13)
> Allan: When clicking "Install updates" from gnome-software, I think it would be
> better to provide two buttons labeled, like in the power off case, "Restart &
> install" and "Install & Shut down". This is more convenient that ticking a
> check box, and it's consistent with the other dialog.

I can see what you mean. I thought about that, but was a bit nervous about having the buttons switch positions in the two dialogs - could lead to accidents.

Also, while the check box is less convenient, but that might actually be a good thing - it makes you think about what you're doing.
 
> But I really like the general idea!

Good! :)

Then I'd say keep the order of the buttons identical for both dialogs. When users click on "Install updates", they do not say whether they want the computer to restart or not (I suspect they may only click on that button when they're done with their work and thus want to shut down the machine when it's done).

Anyway, that's not a destructive operation: in the worst case, they will have to press the power button when they come back.

I think it would be a good idea to warn the user if they have low battery power - since we obviously don't want them running out of juice mid-install.

Preferably, we would prevent an update if on low battery and not plugged into the AC. However, I'm concerned that we might not be able to reliably detect low power states - hence my suggestion that we use a warning only, at least to begin with.

Latest mockups are now in git:

https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/end-session-dialogs.png

Why not warn when not on AC power? The risk (completely breaking the system) is just too large, and you cannot know in advance how long the update will take.

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

Created attachment 269619 [details] [review]
endSessionDialog: 48px icon as per mockups

Created attachment 269620 [details] [review]
endSessionDialog: Simplify CSS padding handling

... so that we add more items to messageLayout without having to
hardcode the same padding in each of the children.

Created attachment 269621 [details] [review]
endSessionDialog: Offer offline updates in the shutdown dialog

This adds a checkbox for installing software updates to the shut down
dialog. The implementation relies on an already prepared offline update
and uses PackageKit's pk-trigger-offline-update helper to trigger the
installation.

Created attachment 269622 [details] [review]
endSessionDialog: Avoid double-poweroff with offline updates

When the user selects "Power Off", we have to reboot to the offline
update process which will power the computer off once the installation
is done.

Created attachment 269623 [details] [review]
endSessionDialog: Warn when trying to install updates on battery power

Interrupting update installation can mess up the package database quite
a bit and could lead to totally destroying the distro installtion. To
avoid running out of juice during an upgrade, warn when someone tries to
install updates on battery power.

Created attachment 269624 [details] [review]
endSessionDialog: Fix the "Restart & Install" button

This makes sure the dialog correctly updates itself when choosing
"Restart & Install" in gnome-software.

While at this, make sure the & symbol in the button's label is escaped
to avoid runtime warnings, now that the label actually gets used.

Created attachment 269625 [details] [review]
endSessionDialog: Warn when trying to install updates on battery power

Interrupting update installation can mess up the package database quite
a bit and could lead to totally destroying the distro installtion. To
avoid running out of juice during an upgrade, warn when someone tries to
install updates on battery power.

Review of attachment 269621 [details] [review]:

::: js/ui/endSessionDialog.js
@@ +41,3 @@
 let _endSessionDialog = null;
 
+const trigger_offline_update = GLib.build_filenamev([Config.LIBEXECDIR, 'pk-trigger-offline-update']);

I wonder if there's a better way for figuring out where pk-trigger-offline-update is installed. When gnome-shell and PackageKit are installed to different prefixes (one in jhbuild and the other in /usr, for example) we don't get correct path like this.

Maybe packagekit could have the full path to its helper in its .pc file? Or maybe turn pk-trigger-offline-update into a dbus service instead, which would make the polkit permission handling a cleaner as well?

Review of attachment 269619 [details] [review]:

This commit message should be "Use a 48px icon as per mockups".

Review of attachment 269620 [details] [review]:

Yeah, makes sense.

Review of attachment 269621 [details] [review]:

I don't quite understand what you're trying to do here. When the checkbox is checked, you call pk-trigger-offline-update, and when then you also call it when rebooting or powering off? Do you need to do both?

::: js/ui/endSessionDialog.js
@@ +41,3 @@
 let _endSessionDialog = null;
 
+const trigger_offline_update = GLib.build_filenamev([Config.LIBEXECDIR, 'pk-trigger-offline-update']);

This should be TRIGGER_OFFLINE_UPDATE.

@@ +350,3 @@
+                subject = dialogContent.subjectWithUpdates;
+
+            /* If we're ready to install updates, prompt the user */

Use // in JS comments, as above.

Review of attachment 269622 [details] [review]:

::: js/ui/endSessionDialog.js
@@ +459,3 @@
             } else if (signal == "ConfirmedShutdown") {
+                // When doing offline update, we need to always emit the reboot
+                // signal to get to the offline update process.

// To actually trigger the offline update, we need to reboot
// to do the upgrade. When the upgrade is complete, the computer
// will shut down automatically.

Review of attachment 269624 [details] [review]:

The first part is correct.

::: js/ui/endSessionDialog.js
@@ +363,3 @@
             return;
 
+        if (this._type == 2 && this._updatesFile.query_exists(null)) {

Hm, I'm wondering if this shouldn't be in OpenAsync instead.

Review of attachment 269625 [details] [review]:

OK.

(In reply to comment #30)
> Review of attachment 269621 [details] [review]:
> 
> I don't quite understand what you're trying to do here. When the checkbox is
> checked, you call pk-trigger-offline-update, and when then you also call it
> when rebooting or powering off? Do you need to do both?

It doesn't look like it is strictly necessary for the reboot and poweroff buttons to work - just calling pk-trigger-offline-update when one of them is clicked should be enough. 

It will make a minor difference when using the cancel button. With the code as currently written, if you check the checkbox, cancel the dialog and bring it up again, the checkbox will still be checked. If you take out the trigger call on checkbox change, that won't be the case anymore.

(In reply to comment #34)
> It will make a minor difference when using the cancel button. With the code as
> currently written, if you check the checkbox, cancel the dialog and bring it up
> again, the checkbox will still be checked. If you take out the trigger call on
> checkbox change, that won't be the case anymore.

Would it make sense to store the checkbox state in gsettings, so that it remembers the last chosen state?

(In reply to comment #35)
> (In reply to comment #34)
> > It will make a minor difference when using the cancel button. With the code as
> > currently written, if you check the checkbox, cancel the dialog and bring it up
> > again, the checkbox will still be checked. If you take out the trigger call on
> > checkbox change, that won't be the case anymore.
> 
> Would it make sense to store the checkbox state in gsettings, so that it
> remembers the last chosen state?

No.

Anyway, the logic here is that _sync() tries to keep the checkbox state synced to the existance of '/system-update' file, and I think it makes sense to continue doing it that way.

That way, if some other process touches '/system-update' while the shutdown dialog is up, this automatically causes the checkbox to get updated to the correct value. And the other way around, when the user toggles the check box and if something goes wrong, the user gets immediate feedback -- the value doesn't "stick".

If some other process touches the '/system-update' file, we need to figure out what it is. That's user policy, and no automated process should touch it.

A checkbox should never suddenly be checked or unchecked. That can seem like it's "mocking" the user.

(In reply to comment #38)
> If some other process touches the '/system-update' file, we need to figure out
> what it is. That's user policy, and no automated process should touch it.
> 
> A checkbox should never suddenly be checked or unchecked. That can seem like
> it's "mocking" the user.

That other process might be the user clicking 'Install updates on next restart' in gnome-software, or running some 'queue-updates-for-later' cmdline utility. Neither of these will mock the user in the poweroff dialog. We need to check the existence of the file to get the initial state of the checkbox right, thats all.

You missed the "while the shutdown dialog is up" part. I agree that we need the initial state correct, but we don't need to track it while it's up, and I don't think the checkbox should make changes "live". If the user clicks cancel, even after checking/unchecking the checkbox, nothing should happen.

Okay, here comes an updated series, taking into account the comments. I've gone with the approach Jasper suggested and the checkbox no longer tries to make any changes live. Incidentally, this also makes the dialog's internal logic a bit easier to understand (at least for me!).

Another thing that I've changed after discussing it with hughsie, is that we're no longer building the pk-trigger-offline-update path from Config.LIBEXECDIR, but instead just hardcoding /usr/libexec/. This fixes the case where jhbuilt gnome-shell fails to find the system PackageKit's pk-trigger-offline-update and/or the path doesn't match up with the path in the polkit rules.

Created attachment 269722 [details] [review]
endSessionDialog: Use a 48px icon as per mockups

Created attachment 269723 [details] [review]
endSessionDialog: Simplify CSS padding handling

... so that we add more items to messageLayout without having to
hardcode the same padding in each of the children.

Created attachment 269724 [details] [review]
endSessionDialog: Fix the "Restart & Install" button

This moves the dialog type overriding that gnome-software uses to bring
up restartInstallDialogContent from _sync() to OpenAsync(), in order to
ensure the type is overridden early enough to show the correct buttons.

While at this, make sure the & symbol in the button's label is escaped
to avoid runtime warnings, now that the label actually gets used.

Created attachment 269725 [details] [review]
endSessionDialog: Offer offline updates in the shutdown dialog

This adds a checkbox for installing software updates to the shut down
dialog. The implementation relies on an already prepared offline update
and uses PackageKit's pk-trigger-offline-update helper to trigger the
installation.

Created attachment 269726 [details] [review]
endSessionDialog: Warn when trying to install updates on battery power

Interrupting update installation can mess up the package database quite
a bit and could lead to totally destroying the distro installtion. To
avoid running out of juice during an upgrade, warn when someone tries to
install updates on battery power.

Review of attachment 269722 [details] [review]:

OK

Review of attachment 269723 [details] [review]:

Was already a-c-n

Review of attachment 269726 [details] [review]:

Dito.

Review of attachment 269725 [details] [review]:

Looks good to me, just some suggestions (feel free to ignore)

::: js/ui/endSessionDialog.js
@@ +93,2 @@
     confirmButtons: [{ signal: 'ConfirmedReboot',
                        label:  C_("button", "Restart") },

In the mockup, the button labels change as well - it might be a good idea to at least add the strings somewhere now, to not have to ask for a string freeze later :-)

@@ +388,3 @@
+
+    _offlineUpdateTriggered: function() {
+        return this._updatesFile.query_exists(null);

Both functions are one-liners that are only used in one place - could just as well move the code inline
(preparedUpdate = this._preparedUpdateFile.query_exists(null); read just as clear as
 preparedupdate = this._preparedUpdateExists(); IMHO)

@@ +435,3 @@
+
+        // Offline update not available; just emit the signal
+        if (!this._checkBox.actor.visible) {

If you use (!this._checkBox.actor.visible || !this._checkBox.actor.checked) you can remove the if-else below

Review of attachment 269724 [details] [review]:

LGTM

(In reply to comment #50)
> Looks good to me, just some suggestions (feel free to ignore)

Cool, thanks to both you and Jasper for the reviews!


> In the mockup, the button labels change as well - it might be a good idea to at
> least add the strings somewhere now, to not have to ask for a string freeze
> later :-)

Ah hm, good idea. I went through a few different options with aday a few days ago and decided to avoid the changing button labels for now -- they were causing the dialog to grow too wide (https://people.gnome.org/~klember/end-session-updates-shutdown.webm vs https://people.gnome.org/~klember/end-session-updates-shutdown2.webm). But there's a good chance we need two extra strings in the dialog that gets invoked from gnome-software.


> Both functions are one-liners that are only used in one place - could just as
> well move the code inline
> (preparedUpdate = this._preparedUpdateFile.query_exists(null); read just as
> clear as
>  preparedupdate = this._preparedUpdateExists(); IMHO)

Done.


> If you use (!this._checkBox.actor.visible || !this._checkBox.actor.checked) you
> can remove the if-else below

No, the difference in these two cases is that if the checkbox is hidden, we just reboot, but if it's shown and unchecked, we need to first call 'pk-trigger-offline-update --cancel' before rebooting.

Created attachment 269737 [details] [review]
endSessionDialog: Offer offline updates in the shutdown dialog

This adds a checkbox for installing software updates to the shut down
dialog. The implementation relies on an already prepared offline update
and uses PackageKit's pk-trigger-offline-update helper to trigger the
installation.

Created attachment 269738 [details] [review]
endSessionDialog: Add extra strings for translation

Add two extra, currently unused strings in order to make the life easier
for translators and avoid breaking the upcoming string freezes.

Attachment 269722 [details] pushed as c176af4 - endSessionDialog: Use a 48px icon as per mockups
Attachment 269723 [details] pushed as 5bec5fb - endSessionDialog: Simplify CSS padding handling
Attachment 269724 [details] pushed as 7551e13 - endSessionDialog: Fix the "Restart & Install" button
Attachment 269726 [details] pushed as 46163a6 - endSessionDialog: Warn when trying to install updates on battery power
Attachment 269737 [details] pushed as 645ef09 - endSessionDialog: Offer offline updates in the shutdown dialog
Attachment 269738 [details] pushed as 0fa6be4 - endSessionDialog: Add extra strings for translation