GNOME Bugzilla – Bug 645485
Poor integration between Shell and gedit 'do you really want to log out' dialogs
Last modified: 2011-03-26 00:14:12 UTC
Both gedit and GNOME Shell have 'do you really want to do that, Dave' dialogs that pop up when you try and log out or shut down with unsaved documents. However, the two don't behave very well together. When you try and shut down, you see the gedit dialog pop up with the options 'log out without saving', 'cancel logout', and 'save' - but you can't do any of these actions because you can't interact with it. The GNOME Shell dialog is one of those which prevents you interacting with anything behind it. The Shell dialog presents fewer choices than the gedit one; you can only 'Log Out' (or 'Shut Down') or 'Cancel'. So you can see these other choices, but can't actually do them. If you select 'Cancel' from the Shell dialog, the gedit dialog doesn't go away, which doesn't really make much sense either. There ought to be much better integration between apps and the Shell when it comes to this kind of behaviour.
Ray - how is this supposed to work? It really was quite bad last time I tried.
The way it's suppose to work is, you look at the list of waiting apps (and their messages) and decide if any of them require attention. 1a) If none of them are important you click logout and it just powers through. 1b) If any given app requires attention, you can click that one in the list and it jumps you to it. Once on the app, you should be able to cancel logout, save, or bring back up the logout dialog and do the same process with the next waiting app. 1c) Alternatively, you should be able to cancel from the log out dialog and everything should go back to before you initiated the logout. Things have clearly regressed, though. Two things I've noticed trying just now: 1) If you click "Cancel Logout" from the app, it brings back up the logout dialog instead of canceling log out. 2) If you click "Cancel" in the logout dialog, the dialog from the app doesn't go away. I'll look into those two things. It might also be good to make it somehow clearer that the items in the list are clickable (maybe some sort of prelight?) .
"It might also be good to make it somehow clearer that the items in the list are clickable (maybe some sort of prelight?)" definitely - I didn't twig that at all.
okay so there are 3 independent issues here, each with fixes in their own components. Let's split them out into separate bug reports. We'll keep this one for problem 1) in comment 2, since I already moved this bug around once, and then I'll file separate reports for gnome-shell dialog app list prelighting, and gedit's exit confirmation dialog not going away when logout is canceled (problem 2) in comment 2)
Created attachment 184033 [details] [review] shell: Allow apps to cancel log out If an application attempts to inhibit logout, we inform the user about it and give the user a chance to interact with the app. If then, the user clicks "Cancel Logout" or whatever, we need to cancel the log out request.. This commit fixes a bug where immediately after canceling the log out request, we would reshow the dialog (because the inhibitor list was changed)
the other two bugs are bug 645491 for prelight and bug 645492 for lingering close confirmation dialog
Review of attachment 184033 [details] [review]: I'd actually just change things in gsm-shell.c: - set to TRUE in on_end_session_dialog_canceled() - set to FALSE in gsm_shell_open_end_session_dialog() There's no reason gsm-manager.c needs to know about this internal detail. Also, looking at the code, I think we're leaking the inhibitors in gsm-shell.c: in gsm_shell_open_end_session_dialog(), we should unref the old inhibitors and disconnect the signal handlers (unless the new inhibitors point to the same memory as the old one, in which case we shouldn't do anything -- yes, it will happen when called from on_need_end_session_dialog_update()).
(In reply to comment #7) > Review of attachment 184033 [details] [review]: > > I'd actually just change things in gsm-shell.c: > - set to TRUE in on_end_session_dialog_canceled() > - set to FALSE in gsm_shell_open_end_session_dialog() That actually won't work without shell changes, since on_end_session_dialog_canceled is only called when the user hits the cancel button in the shell dialog, not when the user hits the cancel button in the gedit dialog. We could potentially make the shell fire that signal when the user clicks the cancel button in gedit, and that would probably fix this bug even without attachment 184033 [details] [review]. I'll have a quick look and see how hard it is to do it that way. > Also, looking at the code, I think we're leaking the inhibitors in gsm-shell.c: > in gsm_shell_open_end_session_dialog(), we should unref the old inhibitors and > disconnect the signal handlers (unless the new inhibitors point to the same > memory as the old one, in which case we shouldn't do anything -- yes, it will > happen when called from on_need_end_session_dialog_update()). yup, indeed.
Created attachment 184078 [details] [review] shell: don't leak inhibitor list on updates gsm_shell_open_end_session_dialog takes a reference to a passed in inhibitor store. This function is explicitly called by the manager whenever a log out should happen, but also gets implicitly called whenever the inhibitor store is changed. This commit makes sure we don't leak a reference to the store during implicit calls to gsm_shell_open_end_session_dialog.
so as per comment 8, I looked into doing this on the shell side of the fence. The fix is a little different than I thought. on_end_session_dialog_canceled isn't involved, since the dialog is dismissed long before the user clicks cancel. The problem is that the session manager doesn't know that the dialog has been dismissed because the shell never tells it. Since it thinks the dialog is still open, it tries to do updates.
Created attachment 184097 [details] [review] endSessionDialog: emit Closed signal when dialog disappears The session manager needs to keep its internal state for the dialog in sync with ours.
Created attachment 184098 [details] [review] shell: listen for 'Closed' signal Right now we track when the shell end session dialog is closed by tracking when the user clicks cancel or (e.g.) logout. The problem is, there are other cases besides the user hitting a button on the dialog that leads to the dialog getting closed. This commit tracks the closed state of the dialog via the closed signal instead of the button siganls.
Review of attachment 184098 [details] [review]: Thanks, that looks cleaner, indeed. It feels to me that we don't need has_open_dialog anymore. Is this right? I'd prefer to move code around (see comment below) before committing. Since it's not fun, I can do that myself and push afterwards, if you prefer. ::: gnome-session/gsm-shell.c @@ +76,3 @@ END_SESSION_DIALOG_CANCELED, END_SESSION_DIALOG_CONFIRMED, + END_SESSION_DIALOG_CLOSED, I'd move everywhere CLOSED before CANCELED and CONFIRMED, since it occurs before.
Comment on attachment 184078 [details] [review] shell: don't leak inhibitor list on updates I've pushed this, but tweaked.
(In reply to comment #13) > Review of attachment 184098 [details] [review]: > > Thanks, that looks cleaner, indeed. It feels to me that we don't need > has_open_dialog anymore. Is this right? Well, if we don't have has_open_dialog then we'll only know "dialog is open with stale entries" (which is implied by shell->priv->update_idle_id != 0) and not the more general "dialog is open". I think we could probably get away with dropping it for now, since the only place that checks it only runs when there are stale entries. > I'd prefer to move code around (see comment below) before committing. Since > it's not fun, I can do that myself and push afterwards, if you prefer. > > ::: gnome-session/gsm-shell.c > @@ +76,3 @@ > END_SESSION_DIALOG_CANCELED, > END_SESSION_DIALOG_CONFIRMED, > + END_SESSION_DIALOG_CLOSED, > > I'd move everywhere CLOSED before CANCELED and CONFIRMED, since it occurs > before. Sure, i can do that.
Created attachment 184105 [details] [review] shell: listen for 'Closed' signal Right now we track when the shell end session dialog is closed by tracking when the user clicks cancel or (e.g.) logout. The problem is, there are other cases besides the user hitting a button on the dialog that leads to the dialog getting closed. This commit tracks the closed state of the dialog via the closed signal instead of the button siganls.
Created attachment 184106 [details] [review] shell: drop has_open_dialog state variable We don't actually need it, since the only time we care if the dialog is open, if when it also has stale inhibitors. That case is already implied by shell->priv->update_idle_id != 0.
Review of attachment 184097 [details] [review]: Looks good, assuming that the gnome-session part of this works as expected.
Comment on attachment 184097 [details] [review] endSessionDialog: emit Closed signal when dialog disappears Attachment 184097 [details] pushed as 85c0074 - endSessionDialog: emit Closed signal when dialog disappears
Review of attachment 184105 [details] [review]: Thanks a lot, please commit with the changes indicated below. ::: gnome-session/gsm-shell.c @@ +487,3 @@ + g_signal_handlers_disconnect_by_func (shell->priv->inhibitors, + G_CALLBACK (queue_end_session_dialog_update), + shell); Ah, you might want to change gsm_shell_open_end_session_dialog() back a bit, since I tweaked your other patch; you'll need to move the calls to g_signal_connect_swapped() outside of the if. (sorry, my fault here since I changed things while you weren't looking) ::: gnome-session/gsm-shell.h @@ +66,3 @@ void (* end_session_dialog_canceled) (GsmShell *shell); void (* end_session_dialog_confirmed) (GsmShell *shell); + void (* end_session_dialog_closed) (GsmShell *shell); Last one that you didn't move before canceled/confirmed ;-)
Review of attachment 184106 [details] [review]: Looks good, thanks.
Thanks for trying to fix this... with gnome-shell 2.91.92-3.fc15 and gnome-session 2.91.93-1.fc15 , the behaviour is now just buggy. If I have gedit open with a single empty document (i.e. how it starts up), on my first attempt to shut down, the dialog claims gedit is blocking shutdown; it shouldn't be, it does not usually do so if the open document is completely empty. If I click on gedit in the dialog it switches to the gedit window, which indeed isn't displaying any dialog saying it's blocking shut down. Subsequent attempts to shut down simply fail: I hit the 'Adam Williamson' menu, hold down Alt, hit 'Shut down...', and nothing happens.
after the above, if I quit gedit, the Shell shutdown dialog suddenly pops up. so it looks like there's two issues: Shell wrongly thinks gedit is inhibiting shutdown if it's running with one, empty, un-named document After you hit the above issue, communication between Shell and gedit gets confused such that future attempts to shut down will get stuck until you quit gedit
Hey, To prevent this bug report from getting too overloaded, can you file that as a new report against gnome-session? FWIW, i just tried: 1) open gedit 2) click poweroff and I just get a "power off automatically in 60 seconds" dialog, so there must be more to the picture here. Maybe you have another gedit window on a different workspace and it's jumping to the wrong one or something like that? But let's pick it up on a new report.
Sure. I just tried and couldn't immediately reproduce the above, so I'll file a new report only if I can figure out a way to reproduce.