GNOME Bugzilla – Bug 769968
Screensaver kicks in while playing
Last modified: 2016-09-15 05:28:17 UTC
I was playing a game yesterday evening, in full screen, with a joypad, in flatpak. Much happiness. Suddenly, the screen started getting darker, and eventually it got blanked and locked by the screensaver. That made me much less happy. Games should inhibit the screensaver when fullscreening a game. Seems we can use gtk_application_inhibit() with the GTK_APPLICATION_INHIBIT_IDLE. https://developer.gnome.org/gtk3/stable/GtkApplication.html#gtk-application-inhibit
Created attachment 333481 [details] [review] ui: Inhibit the screensaver when playing Nobody wants their screen to blank and lock while they are playing. With this change, we now inhibit the screensaver when the user is playing a game, and the window is focused.
Created attachment 333482 [details] [review] ui: Factorize the inhibition code We're going to add other types of inhibitors soon, but we want to always have only a single inhibitor with multiple inhibition flags.
Created attachment 333483 [details] [review] ui: Inhibit logout while playing This allows notifying the user when they try to logout that a game was running, giving them a chance to cancel the logout, go back to the game, save their progress, play for an hour or two because they got caught back in, save their progress again and then finally logout. This also handles poweroff, since it first logs out.
Created attachment 333565 [details] [review] ui: Inhibit the screensaver when playing Nobody wants their screen to blank and lock while they are playing. With this change, we now inhibit the screensaver when the user is playing a game, and the window is focused.
Created attachment 333566 [details] [review] ui: Factorize the inhibition code We're going to add other types of inhibitors soon, but we want to always have only a single inhibitor with multiple inhibition flags.
Created attachment 333567 [details] [review] ui: Inhibit logout while playing This allows notifying the user when they try to logout that a game was running, giving them a chance to cancel the logout, go back to the game, save their progress, play for an hour or two because they got caught back in, save their progress again and then finally logout. This also handles poweroff, since it first logs out.
Review of attachment 333565 [details] [review]: You're missing something to trigger the inhibition when you start to play, I think. This would only start the inhibition if you lost and then gained focus again. ::: src/ui/application-window.vala @@ +158,3 @@ [GtkCallback] public bool on_window_state_event (Gdk.EventWindowState event) { + var focused = (bool) (event.new_window_state & Gdk.WindowState.FOCUSED); To avoid doing all this work, you could also check for if (!(event.changed_mask & Gdk.WindowState.FOCUSED)) return; @@ +159,3 @@ public bool on_window_state_event (Gdk.EventWindowState event) { + var focused = (bool) (event.new_window_state & Gdk.WindowState.FOCUSED); + var playing = (ui_state == UiState.DISPLAY); Do you need to check whether that's !paused as well?
Review of attachment 333566 [details] [review]: This is good, but you could/should have factorised in the previous patch. 1st patch: add uninhibition/inhibition in separate functions, without any arguments to your functions 2nd patch: add the support for flags in those new separate functions 3rd patch: add logout inhibition
Review of attachment 333567 [details] [review]: ::: src/ui/application-window.vala @@ +107,3 @@ run_game_with_cancellable (game, run_game_cancellable); + + inhibit (Gtk.ApplicationInhibitFlags.LOGOUT); You need to inhibit idle as well, when starting a game (but in the 1st patch)
(In reply to Bastien Nocera from comment #7) > @@ +159,3 @@ > public bool on_window_state_event (Gdk.EventWindowState event) { > + var focused = (bool) (event.new_window_state & Gdk.WindowState.FOCUSED); > + var playing = (ui_state == UiState.DISPLAY); > > Do you need to check whether that's !paused as well? Maybe? Games wasn't pausing games on its own when I wrote those patches, so at the time no I shouldn't have checked for it. Right now, I don't know. Games will only pause the game when the window loses the focus, in which case `focused` will be false, and as such the `if (focused && playing)` won't pass and we won't take an inhibitor. So that wouldn't change anything functionally, but it might be more correct, maybe?
Created attachment 334554 [details] [review] ui: Inhibit the screensaver when playing Nobody wants their screen to blank and lock while they are playing. With this change, we now inhibit the screensaver when the user is playing a game, and the window is focused.
Created attachment 334555 [details] [review] ui: Improve the inhibition code This makes the code a bit more flexible, introducing a new parameters to the (un)inhibition functions, which will allow taking/releasing other kinds of inhibitors than just the idle one.
Created attachment 334556 [details] [review] ui: Inhibit logout while playing This allows notifying the user when they try to logout that a game was running, giving them a chance to cancel the logout, go back to the game, save their progress, play for an hour or two because they got caught back in, save their progress again and then finally logout. This also handles poweroff, since it first logs out.
Comment on attachment 333566 [details] [review] ui: Factorize the inhibition code >From 111167c2914404aaf557b4081880987dbe9400ca Mon Sep 17 00:00:00 2001 >From: Mathieu Bridon <bochecha@daitauha.fr> >Date: Wed, 17 Aug 2016 11:44:50 +0200 >Subject: [PATCH] ui: Factorize the inhibition code > >We're going to add other types of inhibitors soon, but we want to always >have only a single inhibitor with multiple inhibition flags. > >https://bugzilla.gnome.org/show_bug.cgi?id=769968 >--- > src/ui/application-window.vala | 45 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 9 deletions(-) > >diff --git a/src/ui/application-window.vala b/src/ui/application-window.vala >index ca1ced3..6fc2492 100644 >--- a/src/ui/application-window.vala >+++ b/src/ui/application-window.vala >@@ -77,6 +77,7 @@ private class Games.ApplicationWindow : Gtk.ApplicationWindow { > private Cancellable quit_game_cancellable; > > private uint inhibit_cookie; >+ private Gtk.ApplicationInhibitFlags inhibit_flags; > > public ApplicationWindow (ListModel collection) { > collection_box.collection = collection; >@@ -94,6 +95,7 @@ private class Games.ApplicationWindow : Gtk.ApplicationWindow { > BindingFlags.BIDIRECTIONAL); > > inhibit_cookie = 0; >+ inhibit_flags = 0; > } > > public void run_game (Game game) { >@@ -155,19 +157,45 @@ private class Games.ApplicationWindow : Gtk.ApplicationWindow { > return false; > } > >+ public void inhibit (Gtk.ApplicationInhibitFlags flag) { >+ if ((bool) (inhibit_flags & flag)) >+ return; >+ >+ Gtk.ApplicationInhibitFlags new_flags = (inhibit_flags | flag); >+ uint new_cookie = application.inhibit (this, new_flags, _("Playing a game")); >+ >+ if (inhibit_cookie != 0) >+ application.uninhibit (inhibit_cookie); >+ >+ inhibit_cookie = new_cookie; >+ inhibit_flags = new_flags; >+ } >+ >+ public void uninhibit (Gtk.ApplicationInhibitFlags flag) { >+ if (!(bool) ((inhibit_flags & flag))) >+ return; >+ >+ Gtk.ApplicationInhibitFlags new_flags = (inhibit_flags & ~flag); >+ uint new_cookie = 0; >+ >+ if ((bool) new_flags) >+ new_cookie = application.inhibit (this, new_flags, _("Playing a game")); >+ >+ application.uninhibit (inhibit_cookie); >+ inhibit_cookie = new_cookie; >+ inhibit_flags = new_flags; >+ } >+ > [GtkCallback] > public bool on_window_state_event (Gdk.EventWindowState event) { > var focused = (bool) (event.new_window_state & Gdk.WindowState.FOCUSED); > var playing = (ui_state == UiState.DISPLAY); > >- if (focused && playing && inhibit_cookie == 0) { >- inhibit_cookie = application.inhibit (this, Gtk.ApplicationInhibitFlags.IDLE, _("Playing a game")); >- } >+ if (focused && playing) >+ inhibit (Gtk.ApplicationInhibitFlags.IDLE); > >- if (!focused && inhibit_cookie != 0) { >- application.uninhibit (inhibit_cookie); >- inhibit_cookie = 0; >- } >+ if (!focused) >+ uninhibit (Gtk.ApplicationInhibitFlags.IDLE); > > update_toplevel_focus (); > >@@ -184,8 +212,7 @@ private class Games.ApplicationWindow : Gtk.ApplicationWindow { > if (quit_game ()) > ui_state = UiState.COLLECTION; > >- application.uninhibit (inhibit_cookie); >- inhibit_cookie = 0; >+ uninhibit (Gtk.ApplicationInhibitFlags.IDLE); > } > > private void run_game_with_cancellable (Game game, Cancellable cancellable) { >-- >2.7.4
I have no idea where this last comment with the whole patch comes from, I must have clicked wrong in BZ. Apologies for that. Bastien, I have reworked the patch series, splitting it up as you suggested. I have also added the missing IDLE inhibitor when starting to play a game, that was indeed an oversight. :) Finally I added the check on whether the focused state of the window changed, you're right there is no need to do anything if it didn't. Due to checks of whether we already have inhibitors, nothing would have happened anyway, but it's still a good thing to bail out as early as possible.
Created attachment 334703 [details] [review] ui: Inhibit the screensaver when playing Nobody wants their screen to blank and lock while they are playing. With this change, we now inhibit the screensaver when the user is playing a game, and the window is focused.
Created attachment 334704 [details] [review] ui: Improve the inhibition code This makes the code a bit more flexible, introducing a new parameters to the (un)inhibition functions, which will allow taking/releasing other kinds of inhibitors than just the idle one.
Created attachment 334705 [details] [review] ui: Inhibit logout while playing This allows notifying the user when they try to logout that a game was running, giving them a chance to cancel the logout, go back to the game, save their progress, play for an hour or two because they got caught back in, save their progress again and then finally logout. This also handles poweroff, since it first logs out.
Anything else I should do to get this merged in? Would be great to have it for 3.22 :)
Review of attachment 334703 [details] [review]: Looks good.
Review of attachment 334704 [details] [review]: > a new parameters a new parameter Looks fine, though you should add a big warning at the top of the inhibit and uninhibit that a single flag should be passed. Passing 2 of them will not work due to the way you're doing the bit operations (checking for bool, instead of exact values).
Review of attachment 334705 [details] [review]: ::: src/ui/application-window.vala @@ +119,3 @@ run_game_cancellable = null; + inhibit (Gtk.ApplicationInhibitFlags.IDLE | Gtk.ApplicationInhibitFlags.LOGOUT); That's dangerous, because the code in inhibit isn't prepared to handle multiple flags at once. Do this as two separate calls instead.
Created attachment 335460 [details] [review] ui: Improve the inhibition code This makes the code a bit more flexible, introducing a new parameter to the (un)inhibition functions, which will allow taking/releasing other kinds of inhibitors than just the idle one.
Created attachment 335461 [details] [review] ui: Inhibit logout while playing This allows notifying the user when they try to logout that a game was running, giving them a chance to cancel the logout, go back to the game, save their progress, play for an hour or two because they got caught back in, save their progress again and then finally logout. This also handles poweroff, since it first logs out.
(In reply to Bastien Nocera from comment #21) > Review of attachment 334704 [details] [review] [review]: > > > a new parameters > > a new parameter > > Looks fine, though you should add a big warning at the top of the inhibit > and uninhibit that a single flag should be passed. Passing 2 of them will > not work due to the way you're doing the bit operations (checking for bool, > instead of exact values). I (hopefully) fixed this in the last version of the patch. This would also take care of comment 22.
I found some small problems with the current patches. The idle inhibitor is active when the 'Resume failed' dialog is presented, it probably shouldn't. When playing a resumable game, click on a menu from the shell: the game will be paused and saved but the session inhibitor will still be active. BTW, should we use session inhibitors for savable/resumable games?
What about splitting this patch into two? One adding the idle inhibitor and the other adding the logout inhibitor, especially as the first one seems more important and polished than the second one.
Review of attachment 334703 [details] [review]: We should probably reorganize the code of the 3 patches and fix some bugs, but they overall look good. ::: src/ui/application-window.vala @@ +166,3 @@ } + public void inhibit () { This should probably be private as we only use it in the class itself. Private methods go after the public ones. @@ +175,3 @@ + } + + public void uninhibit () { Ditto.
Review of attachment 334703 [details] [review]: ::: src/ui/application-window.vala @@ -163,3 @@ [GtkCallback] public bool on_window_state_event (Gdk.EventWindowState event) { - is_fullscreen = (bool) (event.new_window_state & Gdk.WindowState.FULLSCREEN); Why is this removed? I suppose it's an error while rebasing.
Created attachment 335564 [details] [review] ui: Inhibit the screensaver when playing Nobody wants their screen to blank and lock while they are playing. With this change, we now inhibit the screensaver when the user is playing a game, and the window is focused.
Created attachment 335565 [details] [review] ui: Improve the inhibition code This makes the code a bit more flexible, introducing a new parameter to the (un)inhibition functions, which will allow taking/releasing other kinds of inhibitors than just the idle one.
Created attachment 335566 [details] [review] ui: Inhibit logout while playing This allows notifying the user when they try to logout that a game was running, giving them a chance to cancel the logout, go back to the game, save their progress, play for an hour or two because they got caught back in, save their progress again and then finally logout. This also handles poweroff, since it first logs out.
Thanks Bastien for the help with flags, that was a pretty bad mistake on my part. :) Latest patches make the two methods private as you requested Adrien. > - is_fullscreen = (bool) (event.new_window_state & Gdk.WindowState.FULLSCREEN); > > Why is this removed? I suppose it's an error while rebasing. As discussed on IRC I removed it on purpose because I thought it was an unused local variable. I predict that the lack of explicit "this." is going to keep tripping me over and over. Anyway, that's also fixed in the latest patch series.
Review of attachment 335564 [details] [review]: LGTM
Review of attachment 335565 [details] [review]: This code changes almost all of the code introduced in the previous commit so I wonder if it wouldn't be better to merge them into one. Besides that it LGTM.
Review of attachment 335566 [details] [review]: LGTM
> This code changes almost all of the code introduced in the previous commit so I wonder if it wouldn't be better to merge them into one. I don't think so, and here's why. Keeping it as it is and merging the first two patches are very different things, because they don't tell the same story. The way the patch series is structured, the story being told is the following: 1. we started by taking an idle inhibitor 2. we then realized we'd want to inhibit other things in the future (e.g logout) and as such made the function accept parameters 3. and so we finally added the logout inhibitor That is a cohesive story. Following it with `git log -p` makes it very clear what happens at each step, and how we got to the last point. Each commit makes sense on its own. Months could have happened between the first and second commit without it impacting that story. The last two are actually more closely related than the first two (we made the function generic specifically because we wanted to take a second inhibitor, not because making it generic was « right » in itself). But merging them wouldn't be right either, because making the function generic is infrastructure/refactoring, whereas adding the logout inhibitor is the actual feature we wanted. On the other hand, merging the first two patches tells a different story: 1. we started by taking an idle inhibitor... but we made it generic because we knew in advance something else was coming up but we didn't feel like doing it in the same commit 2. we then added the logout inhibitor Sure, that's not entirely terrible. By looking at the second patch you quickly figure out why the first one was made in a generic way. But you have to look at that second commit to understand the first one. It's just not as obvious as with the first story. The way we commit to the VCS tells a story, which will be read by others, and it's important to write that story so that it is easy to follow (and hopefully pleasant to read). I believe the 3-patches story is a much nicer read than the 2-patches one.
Attachment 335564 [details] pushed as 326b58d - ui: Inhibit the screensaver when playing Attachment 335565 [details] pushed as 6d47246 - ui: Improve the inhibition code Attachment 335566 [details] pushed as 6a086ad - ui: Inhibit logout while playing