GNOME Bugzilla – Bug 706612
Update the end session dialog / power off dialog to meet the new designs
Last modified: 2013-08-26 14:47:32 UTC
See the designs at: https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/power-off-dialog.png These don't meet them exactly, but they're a good first start.
Created attachment 252796 [details] [review] loginManager: Remove login manager versions of PowerOff/Reboot These don't go through gnome-session, so they don't properly update its state machine. We should use these in the future when we want to use logind user sessions, but for now, they're just a trap.
Created attachment 252797 [details] [review] endSessionDialog: Don't error out if gnome-session hands us a dead inhibitor Sometimes gnome-session hands us a bad object path for JIT inhibitors it creates for XSMP clients. While this is a bug in gnome-session, we shouldn't show an empty-looking dialog here.
Created attachment 252798 [details] [review] endSessionDialog: Fix a warning If _timerId is undefined/null, as it is by default, we will take this path, and fail when trying to remove a source ID for undefined.
Created attachment 252799 [details] [review] endSessionDialog: Implement new designs for the end session dialog
Created attachment 252800 [details] [review] endSessionDialog: Remove the interactivity of the end session dialog This was always sort of a hidden feature, and with the new designs it's going to get unclear about what's clickable, and what's not.
Created attachment 252801 [details] [review] endSessionDialog: List other users and sessions in with the inhibitors Instead of in a separate dialog. This does not meet the designs right now, but it's a good first start.
Created attachment 252802 [details] [review] endSessionDialog: Convert to the standard _sync pattern ... for starting and stopping the timer. This helps clean up the state transitions in the code when caring about multiple things.
Created attachment 252803 [details] [review] system: Glow the power off icon when there are updates available
Created attachment 252805 [details] [review] system: Use a dialog for selecting the Power Off option
Review of attachment 252796 [details] [review]: Does it mean we show a confirmation dialog when you reboot from GDM?
Review of attachment 252797 [details] [review]: ::: js/ui/endSessionDialog.js @@ +124,3 @@ + } catch(e) { + // XXX -- sometimes JIT inhibitors generated by gnome-session + // get removed too soon. Don't fail in this case. We should probably log something here - just to help people debugging gnome-session.
Review of attachment 252798 [details] [review]: Oh sad, does it mean we need to check all places we're doing != 0?
After discussion in IRC, we discovered that was an unfortunate misunderstanding of the designs, so Jasper is going to rewrite the unreviewed patches.
(In reply to comment #10) > Does it mean we show a confirmation dialog when you reboot from GDM? We do what gnome-session tells us. If it tell us to open a confirmation dialog when we call OpenAsync, then we do. (In reply to comment #11) > We should probably log something here - just to help people debugging > gnome-session. OK. (In reply to comment #12) > Oh sad, does it mean we need to check all places we're doing != 0? Well, most places set things in the constructor to = 0 on startup, but yeah, we should probably double-check.
Attachment 252796 [details] pushed as ce76824 - loginManager: Remove login manager versions of PowerOff/Reboot Attachment 252797 [details] pushed as c44caa5 - endSessionDialog: Don't error out if gnome-session hands us a dead inhibitor Attachment 252798 [details] pushed as 77dc587 - endSessionDialog: Fix a warning
Among other things, (gnome-shell:12770): Gjs-WARNING **: JS ERROR: TypeError: this._userManager is undefined EndSessionDialog<._loadSessions/<@/opt/gnome/share/gnome-shell/js/ui/endSessionDialog.js:414 LoginManagerSystemd<.listSessions/<@/opt/gnome/share/gnome-shell/js/misc/loginManager.js:164 asyncCallback@/opt/gnome/share/gjs-1.0/overrides/Gio.js:91
As well as (gnome-shell:12770): Gjs-WARNING **: JS ERROR: ReferenceError: event is not defined PowerOffDialog<._onRestartClicked@/opt/gnome/share/gnome-shell/js/ui/status/system.js:94 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213
(gnome-shell:12770): Gjs-WARNING **: JS ERROR: ReferenceError: Util is not defined PowerOffDialog<._onInstallUpdatesClicked@/opt/gnome/share/gnome-shell/js/ui/status/system.js:88 wrapper@/opt/gnome/share/gjs-1.0/lang.js:213 Wow!
Review of attachment 252803 [details] [review]: More on the list of bugs, you need to check at startup if the prepared updates file is there, and set the glow accordingly.
Created attachment 252882 [details] [review] endSessionDialog: Remove the interactivity of the end session dialog This was always sort of a hidden feature, and with the new designs it's going to get unclear about what's clickable, and what's not.
Created attachment 252883 [details] [review] endSessionDialog: List other users and sessions in with the inhibitors Instead of in a separate dialog. This does not meet the designs right now, but it's a good first start.
Created attachment 252884 [details] [review] endSessionDialog: Convert to the standard _sync pattern ... for starting and stopping the timer. This helps clean up the state transitions in the code when caring about multiple things.
Created attachment 252885 [details] [review] system: Add an alt-switcher to re-enable Suspend in the system menu
Review of attachment 252882 [details] [review]: Ok
Review of attachment 252883 [details] [review]: In what way this does not meet the designs? ::: js/ui/endSessionDialog.js @@ +175,3 @@ + style_class: 'end-session-dialog-app-list-item-description' }); + textLayout.add(descriptionLabel, { expand: true, x_fill: true }); + } Doesn't this create items of different height if the reason is present or not?
Review of attachment 252884 [details] [review]: ::: js/ui/endSessionDialog.js @@ +317,2 @@ return; + } This doesn't make sense. We know when the dialog is closed, it's only inside cancel() or _confirm(). Those places should call _stopTimer() directly. The other uses of _sync are fine, otoh.
Review of attachment 252885 [details] [review]: ::: js/ui/status/system.js @@ +38,3 @@ + this._alternate.connect('notify::visible', Lang.bind(this, this._sync)); + + this._capturedEventId = global.stage.connect('captured-event', Lang.bind(this, this._onCapturedEvent)); You probably want to disconnect this on destroy? @@ +55,3 @@ + + if (childToShow != this._currentChild) { + this._currentChild = childToShow; Do you really need _currentChild?
(In reply to comment #25) > In what way this does not meet the designs? The designs call for two separate sections -- one for app inhibitors, and one for other sessions. This puts them all in one section. > Doesn't this create items of different height if the reason is present or not? Sure, but that was the case anyway if we word wrapped the reason. The icon should be the normalizer of height. (In reply to comment #26) > This doesn't make sense. > We know when the dialog is closed, it's only inside cancel() or _confirm(). > Those places should call _stopTimer() directly. Sort of the point of _sync is to make the state only change in one place, but I can go either way on this one since we change the state externally through .cancel / ._confirm as well.
Created attachment 252896 [details] [review] endSessionDialog: Convert to the standard _sync pattern ... for starting and stopping the timer. This helps clean up the state transitions in the code when caring about multiple things.
Created attachment 252897 [details] [review] system: Add an alt-switcher to re-enable Suspend in the system menu
Review of attachment 252896 [details] [review]: Ok
Review of attachment 252897 [details] [review]: Ok
Comment on attachment 252882 [details] [review] endSessionDialog: Remove the interactivity of the end session dialog Attachment 252882 [details] pushed as e4d46ae - endSessionDialog: Remove the interactivity of the end session dialog Gonna push this one for now since it's sort of a hidden feature.
Created attachment 252925 [details] [review] endSessionDialog: List other users and sessions in with the inhibitors Instead of in a separate dialog. This does not meet the designs right now, but it's a good first start.
Created attachment 252926 [details] [review] endSessionDialog: Convert to the standard _sync pattern ... for starting and stopping the timer. This helps clean up the state transitions in the code when caring about multiple things.
Created attachment 252927 [details] [review] endSessionDialog: Don't stop the timer when we have inhibitors
Created attachment 252928 [details] [review] endSessionDialog: Split into two sections
Created attachment 252929 [details] [review] system: Add a way to suspend from the system menu When we implemented the new designs, we lost the ability to suspend from the system menu. Re-enable this ability by re-adding the hidden "Alt" shortcut item.
Review of attachment 252929 [details] [review]: (reattached for order, still ACN)
I've just tested, and it looks great. Two small visual tweaks that would improve it: 1. When there are both inhibitor applications and inhibitor user accounts, increase the padding above each section. 2. Use a grey colour for the inhibitor headings (probably the same grey as the dialog title).
Created attachment 253093 [details] [review] add a variant of the Restart dialog for offline updates Here is an untested patch that adds a variant of the Restart dialog for when an offline update is pending.
Created attachment 253114 [details] [review] Add a variant of the Restart dialog for offline updates Detect when an offline update is pending, and show a more suitable message in the Restart dialog.
Review of attachment 253114 [details] [review]: Feel free to commit with or without change. ::: js/ui/endSessionDialog.js @@ +114,3 @@ + }, + confirmButtons: [{ signal: 'ConfirmedReboot', + label: C_("button", "Restart & Install") }], "Restart & Update" maybe?
I was following this: https://dl.dropboxusercontent.com/u/5031519/system-settings/end-session-dialogs.png
Review of attachment 252925 [details] [review]: Ok
Review of attachment 252926 [details] [review]: Yes
Review of attachment 252927 [details] [review]: Ok
Review of attachment 252928 [details] [review]: Why did you remove the class?
Review of attachment 253114 [details] [review]: No. This doesn't make sense. We need to trigger an offline update with PackageKit through calling an executable, and users should be able to normally restart without forcing updates to be installed. This needs to have new APIs in gnome-session that gnome-software can call.
(In reply to comment #49) > Review of attachment 253114 [details] [review]: > > No. This doesn't make sense. We need to trigger an offline update with > PackageKit through calling an executable, and users should be able to normally > restart without forcing updates to be installed. This needs to have new APIs in > gnome-session that gnome-software can call. You weren't there in IRC for the discussion with halfline? The new API is just gnome-session telling gnome-software if reboot was confirmed or not, all PackageKit handling will happen in gnome-software.
Attachment 252925 [details] pushed as 2e65c85 - endSessionDialog: List other users and sessions in with the inhibitors Attachment 252926 [details] pushed as aaaf25d - endSessionDialog: Convert to the standard _sync pattern Attachment 252927 [details] pushed as a779e2a - endSessionDialog: Don't stop the timer when we have inhibitors Attachment 252928 [details] pushed as dd8fd09 - endSessionDialog: Split into two sections Attachment 252929 [details] pushed as ef2345e - system: Add a way to suspend from the system menu
(In reply to comment #49) > Review of attachment 253114 [details] [review]: > > No. This doesn't make sense. We need to trigger an offline update with > PackageKit through calling an executable, and users should be able to normally > restart without forcing updates to be installed. This needs to have new APIs in > gnome-session that gnome-software can call. Thats not how it works, I'm afraid. gnome-session is not involved in software updates at all, and won't be. Once offline updates have been triggered, the next reboot _will_ go ahead and install them. But this is usually part of a single workflow, anyway. You click 'Restart & Install' in gnome-software, and then gnome-software triggers the offline update (which puts /software-update in place) and then calls Reboot. gnome-shell is not making any policy decision here, it simply displays a suitable UI under the circumstances.
Review of attachment 253114 [details] [review]: Setting back to acn, as Jasper conceded on irc.
Attachment 253114 [details] pushed as a0fa993 - Add a variant of the Restart dialog for offline updates