GNOME Bugzilla – Bug 637188
Add hooks to integrate with the shell
Last modified: 2011-02-18 16:33:41 UTC
I've been slowly working on the proposed gnome 3 designs for logout and shutdown dialogs mentioned: See http://live.gnome.org/GnomeShell/Design/Whiteboards/SessionLogout and http://live.gnome.org/GnomeShell/Design/Whiteboards/SystemStopRestart) The idea is that we install a small extension that the shell loads at startup and we interface with that extension over a private dbus api. Note this patch depends on the patches from bug 636976 and bug 637187 (and a few other already commited patches)
Created attachment 176371 [details] [review] shell: add shell specific logout/shutdown dialog It installs an extension that gnome-shell loads at startup, and then gnome-session and the shell cooridinate over D-Bus.
One open question is... right now the gnome 3 dialog and fallback dialogs behave differently. The new design is such that the inhibitors dialog and confirmation dialog are merged into one. The existing behavior treats them as two dialogs. Maybe it would make sense to have the fallback dialog mimic the newer design?
Another open question is what to do about "Restart" last time i talked to the designers they didn't seem keen with two separate dialogs as originally mocked up.
Comment on attachment 176371 [details] [review] shell: add shell specific logout/shutdown dialog so there's some resistance in bug 637187 to doing this as an extension. I'll post a revised patch that only has the C side of half of this, and we'll role the actual dialog straight into gnome-shell.
Created attachment 177780 [details] [review] shell: add support for shell logout/shutdown dialog This commit changes gnome-session to use the shell for presenting the logout and shutdown dialogs. Coordination happens over the session bus. This update drops the extension bits. Note, discussion is still happening in bug 637187 and bug 636976
Created attachment 178562 [details] [review] shell: add support for shell logout/shutdown dialog This commit changes gnome-session to use the shell for presenting the logout and shutdown dialogs. Coordination happens over the session bus.
The patches in bug 637187 have landed, so we should get this in soon. Attachment 178562 [details] is updated for master. At some point we should move gnome-session to gdbus and off gconf. I don't think we should block on that before moving this in though.
Created attachment 179204 [details] [review] shell: add support for shell logout/shutdown dialog This commit changes gnome-session to use the shell for presenting the logout and shutdown dialogs. Coordination happens over the session bus.
Ray: looking a bit at this, I wonder: is there no need for the shell to know if the list of inhibitors change?
Let me reopen, because I have a few concerns with the patch. I'll do a review to explain them.
Review of attachment 179204 [details] [review]: The one thing that is a bit confusing, and I'm not sure we can fix it other than by clearly documenting it, is that the two code paths for fallback and shell do some things in a different order. It might make sense to move show_shell_end_session_dialog() & friends earlier in the code, around end_session_or_show_shell_dialog() to help clarify this. ::: gnome-session/gsm-manager.c @@ +1142,3 @@ + end_phase (manager); + return; + } I don't think this part should be moved. If there's no inhibitor, then there's no point in doing the few lines before that (that display the inhibit dialog). So this block should go at the beginning of the function. @@ +1189,3 @@ + + logout_prompt = g_settings_get_boolean (manager->priv->settings, + KEY_LOGOUT_PROMPT); Unfortunately, by doing this here instead of in user_logout(), we lost the show_confirmation variable, that was helping us deciding whether we should display the dialog or not. So we should save it somehow. @@ +1192,3 @@ + if (logout_prompt) { + show_shell_end_session_dialog (manager, type); + return; Remove the return statement. @@ +2923,3 @@ +{ + cancel_end_session (manager); + manager->priv->shell_end_session_dialog_canceled_id = 0; Err, sounds wrong. This will be done below when we disconnect, so just remove the line. @@ +2947,3 @@ +on_shell_end_session_dialog_confirmed (GsmShell *shell, + GsmManager *manager) +{ Can we add a test here like: if (manager->priv->phase >= GSM_MANAGER_PHASE_QUERY_END_SESSION) { /* Already shutting down, nothing more to do */ return; } (actually, reading the code again, it should be > and not >=, since the logout dialog of the shell isn't displayed at the same time as the logout dialog from fallback -- eek, confusing, there should be a comment about that) The reason is that I don't like to have a end_phase (like below), without being sure of which phase we're in. @@ +2948,3 @@ + GsmManager *manager) +{ + manager->priv->forceful_logout = TRUE; Please add a comment explaining that inhibitor, if existing, were presented to the user, and the user accepted to continue. @@ +2951,3 @@ + end_phase (manager); + + manager->priv->shell_end_session_dialog_confirmed_id = 0; Wrong like above :-) We should also disconnect the other signal handlers, just in case. @@ +2978,3 @@ + "end-session-dialog-open-failed", + G_CALLBACK (on_shell_end_session_dialog_canceled), + manager); I'm not exactly sure what the failed case means (it's not implemented in the gnome-shell code). Maybe it should be removed? But if we keep it, wouldn't it make sense to do something else than on_shell_end_session_dialog_canceled, like using the fallback methods? @@ +3006,3 @@ /* Global settings overides input parameter in order to disable confirmation + * dialog accordingly. For the shell, we never show the confirmation dialog, + * since the dialog is merged with the inhibit dialog. This comment is wrong: the confirmation dialog will just be displayed later on.
Review of attachment 179204 [details] [review]: ::: gnome-session/gsm-manager.c @@ +1199,3 @@ + } + + show_shell_end_session_dialog (manager, type); We also lost the forceful_logout variable from user_logout(), that would have applied here. @@ +2947,3 @@ +on_shell_end_session_dialog_confirmed (GsmShell *shell, + GsmManager *manager) +{ Hrm, I guess it should be "!=" instead of ">=", actually :-)
(In reply to comment #9) > Ray: looking a bit at this, I wonder: is there no need for the shell to know if > the list of inhibitors change? We should send out a new list of inhibitors when the list changes, but don't currently.
(In reply to comment #10) > Let me reopen, because I have a few concerns with the patch. I'll do a review > to explain them. Great, thanks.
(In reply to comment #11) > Review of attachment 179204 [details] [review]: > > The one thing that is a bit confusing, and I'm not sure we can fix it other > than by clearly documenting it, is that the two code paths for fallback and > shell do some things in a different order. Right, I mentioned this in comment 2. I think the "best" solution is to make fallback and gnome-shell both behave the gnome-shell way. That seemed more invasive so I punted, though. > It might make sense to move show_shell_end_session_dialog() & friends earlier > in the code, around end_session_or_show_shell_dialog() to help clarify this. Yea, I can group it all together. > > ::: gnome-session/gsm-manager.c > @@ +1142,3 @@ > + end_phase (manager); > + return; > + } > > I don't think this part should be moved. If there's no inhibitor, then there's > no point in doing the few lines before that (that display the inhibit dialog). > So this block should go at the beginning of the function. Right, makes sense. > @@ +1189,3 @@ > + > + logout_prompt = g_settings_get_boolean > (manager->priv->settings, > + KEY_LOGOUT_PROMPT); > > Unfortunately, by doing this here instead of in user_logout(), we lost the > show_confirmation variable, that was helping us deciding whether we should > display the dialog or not. So we should save it somehow. ok. > @@ +1192,3 @@ > + if (logout_prompt) { > + show_shell_end_session_dialog (manager, type); > + return; > > Remove the return statement. yea, that's a little tidier, > @@ +2923,3 @@ > +{ > + cancel_end_session (manager); > + manager->priv->shell_end_session_dialog_canceled_id = 0; > > Err, sounds wrong. This will be done below when we disconnect, so just remove > the line. gah. > @@ +2947,3 @@ > +on_shell_end_session_dialog_confirmed (GsmShell *shell, > + GsmManager *manager) > +{ > > Can we add a test here like: > > if (manager->priv->phase >= GSM_MANAGER_PHASE_QUERY_END_SESSION) { > /* Already shutting down, nothing more to do */ > return; > } > > (actually, reading the code again, it should be > and not >=, since the logout > dialog of the shell isn't displayed at the same time as the logout dialog from > fallback -- eek, confusing, there should be a comment about that) > > The reason is that I don't like to have a end_phase (like below), without being > sure of which phase we're in. okay. > > @@ +2948,3 @@ > + GsmManager *manager) > +{ > + manager->priv->forceful_logout = TRUE; > > Please add a comment explaining that inhibitor, if existing, were presented to > the user, and the user accepted to continue. > > @@ +2951,3 @@ > + end_phase (manager); > + > + manager->priv->shell_end_session_dialog_confirmed_id = 0; > > Wrong like above :-) We should also disconnect the other signal handlers, just > in case. double gah. > @@ +2978,3 @@ > + "end-session-dialog-open-failed", > + G_CALLBACK > (on_shell_end_session_dialog_canceled), > + manager); > > I'm not exactly sure what the failed case means (it's not implemented in the > gnome-shell code). > Maybe it should be removed? But if we keep it, wouldn't it > make sense to do something else than on_shell_end_session_dialog_canceled, like > using the fallback methods? It means that gnome-shell threw an exception instead of replying to the Open call. The two cases it does this are: 1) if gnome-session requested an invalid dialog type (so won't happen in practice) 2) if the display already has input grabs and so can't show the modal dialog. The first case would be a bug, so not something we should design for. I'm not sure about the second case. We definitely don't want to show the old fallback code randomly, but maybe gnome-session could retry at some point in the future when the grab gets dropped. > @@ +3006,3 @@ > /* Global settings overides input parameter in order to disable > confirmation > + * dialog accordingly. For the shell, we never show the confirmation > dialog, > + * since the dialog is merged with the inhibit dialog. > > This comment is wrong: the confirmation dialog will just be displayed later on. well it's all a word game, I guess. The confirmation dialog and inhibit dialog have been merged, and that new merged dialog is displayed at the time the inhibit dialog used to be displayed. I can clarify the comment.
Created attachment 181224 [details] [review] manager: don't show inhibitor dialog if no inhibitors I moved this to the wrong place when shuffling code around before. Spotted by vuntz http://bugzilla.gnome.org/637188
Created attachment 181225 [details] [review] manager: track full details of logout request When we get a logout request, there are two bits of information that come with the request: 1) Whether or not we should seek confirmation by default from the user before proceeding 2) Whether or not individual applications should be given an opportunity to ask us to seek confirmation from the user before proceeding. We track the latter in a boolean state variable (forceful_logout), but not the former. We already have an enumeration type (GsmManagerLogoutMode) that can be used to track both bits, though. This commit changes the boolean state variable to the enum type, so the whether-we-should-confirmation information is available deeper down in the logout process.
Created attachment 181226 [details] [review] manager: honor no confirmation logout mode for shell We were previously ignoring the bit. This commit makes us look at it and honor it. Based on feedback from vuntz
Created attachment 181227 [details] [review] manager: clean up shell signal connection code We weren't doing a very good job disconnecting the handlers. Spotted by vuntz
Created attachment 181228 [details] [review] manager: add some phase checking paranoia When the user confirms logout, make sure we didn't already start logging out for other reasons before processing the confirmation Suggestion form Vincent.
Created attachment 181229 [details] [review] shell: drop unused parameter to open function
Created attachment 181230 [details] [review] shell: update dialog as inhibitors change This commit allows the logout dialog to get updates when the list of inhibitors changes.
I've commited the above clean ups which should address most of the issues you found in your review.