GNOME Bugzilla – Bug 691964
ActiveChanged() signal not very useful
Last modified: 2013-03-22 13:59:35 UTC
The ActiveChanged tells us that the screensaver is about to come on. The problem is that gnome-settings-daemon's power plugin has no way to know whether the screenlock is coming down because the idle finished, or if it was requested (especially if the lock is requested near enough the idle being triggered). This is important so that the blanking doesn't cover the fade animation. We could add an argument to the ActiveChanged() signal (or create a new signal with the added arg), but easier for g-s-d would be to trigger the signal when the fade has finished. That way we'd now for sure that the user didn't cancel the fade, and avoid the possible 9.3 seconds of black but not blanked screen if the user requested the locking.
In bug 691965, I have 2 patches, one for the current case (with the possible 9.3 seconds delay in blanking) and one for if gnome-shell sent the ActiveChanged() signal only once the shield was down, and the fade to black done. The latter is much simpler and cleaner.
The only users of org.gnome.ScreenSaver are gnome-settings-daemon and gnome-session, so we can define the semantics in a way that makes sense for us, but let me understand this. When do you want the signal? Options: - When the fade begins (transition from non-idle to idle in gnome-session) - When the fade ends (fade beginning + fade duration) - When the screen is "virtually" locked (fade beginning + lock delay) - When the shield is actually activated (transition from idle to non-idle, after being virtually locked) - None of the above Also, do you want ActiveChanged when locking manually (from the menu, from ScreenSaver.Lock, from login1.Lock)? Do you want ActiveChanged when the shield comes up, but before the session is unlocked? Btw, ActiveChanged and IsActive are used by gnome-session to denote the "locked" status to logind. I don't know what it is used for, but if we break the semantics we should probably get a new signal.
(In reply to comment #2) > The only users of org.gnome.ScreenSaver are gnome-settings-daemon and > gnome-session, so we can define the semantics in a way that makes sense for us, > but let me understand this. When do you want the signal? > Options: > > - When the fade begins (transition from non-idle to idle in gnome-session) > - When the fade ends (fade beginning + fade duration) Here. The screen shield is down (even though it's invisible until you go back to non-idle). At this point, we have a much more aggressive blank strategy, and disable dimming (because the blank is very aggressive). > - When the screen is "virtually" locked (fade beginning + lock delay) > - When the shield is actually activated (transition from idle to non-idle, > after being virtually locked) > - None of the above > > Also, do you want ActiveChanged when locking manually (from the menu, from > ScreenSaver.Lock, from login1.Lock)? Yes. That allows us to blank the screen straight away. Right now, we have a 10 second delay before blanking (so we line up with the fade ending), and that's too long when locking manually (about 9.3 seconds too long. > Do you want ActiveChanged when the shield > comes up, but before the session is unlocked? Only when coming back to the full unlocked desktop. > Btw, ActiveChanged and IsActive are used by gnome-session to denote the > "locked" status to logind. I don't know what it is used for, but if we break > the semantics we should probably get a new signal. I expect "IsActive" to denote the shield's status, not whether or not the session is locked (and would require a password/PIN to unlock).
(In reply to comment #3) > (In reply to comment #2) > > The only users of org.gnome.ScreenSaver are gnome-settings-daemon and > > gnome-session, so we can define the semantics in a way that makes sense for us, > > but let me understand this. When do you want the signal? > > Options: > > > > - When the fade begins (transition from non-idle to idle in gnome-session) > > - When the fade ends (fade beginning + fade duration) > > Here. The screen shield is down (even though it's invisible until you go back > to non-idle). At this point, we have a much more aggressive blank strategy, and > disable dimming (because the blank is very aggressive). Uhm. The shield is not down at this point, if lock-delay > 20: if you move the mouse, you're back to the desktop immediately. But I agree that's the right point to start applying aggressive powersaving. > [...] > > Btw, ActiveChanged and IsActive are used by gnome-session to denote the > > "locked" status to logind. I don't know what it is used for, but if we break > > the semantics we should probably get a new signal. > > I expect "IsActive" to denote the shield's status, not whether or not the > session is locked (and would require a password/PIN to unlock). Then what would you use in gnome-session to export to logind? Also, the locked state is pretty tied to active in the shell, we don't even have a active-but-not-locked state internally. We could easily fake one, if really necessary... Bah.
I believe that's what the designers want (at least that's what we discussed in bug 691002). I would expect a behaviour similar to the "no password/live user" one before lock-delay, and the current one after it. We might indeed need API changes if gnome-session/logind expect "Active" to mean active lock (as opposed to screen shield down/aggressive screensaving).
Created attachment 233703 [details] [review] ScreenShield: only emit ActiveChanged at the end of the idle fading gnome-settings-daemon wants to use ActiveChanged to drive screen blanking policies. I also added two big comments that should cover all cases, to clear up what's happening when the idle timers fire. I was wrong with IsActive propagation to logind (I wonder where I got that idea...). gnome-session uses IsActive to constrain presence to IDLE when the screensaver is active - a GNOME 2 heritage probably, but it's ok, because IsActive is always true when locked. The screen shield occasionally emits more signals than it should, I hope that's not a problem for gnome-settings-daemon.
Review of attachment 233703 [details] [review]: Would have tested, but it doesn't apply against master... ::: js/ui/screenShield.js @@ +631,3 @@ + // showing. + // The latter is a very unlikely condition (it requires + // idle-delay < 20), but in any case we have nothing idle-delay < 20 and > 0 (meaning never).
(In reply to comment #7) > Review of attachment 233703 [details] [review]: > > Would have tested, but it doesn't apply against master... Uh right, but I think you need just bug 689106.
(In reply to comment #8) > (In reply to comment #7) > > Review of attachment 233703 [details] [review] [details]: > > > > Would have tested, but it doesn't apply against master... > > Uh right, but I think you need just bug 689106. Still not sufficient.
Created attachment 233824 [details] [review] ScreenShield: only emit ActiveChanged at the end of the idle fading gnome-settings-daemon wants to use ActiveChanged to drive screen blanking policies. I also added two big comments that should cover all cases, to clear up what's happening when the idle timers fire. Rebased to be on top of bug 689106 only.
The blanking seems to work as expected (it's triggered as soon as the animation finishes). Though I found 2 problems: - the comment in screenShield.js, with "If you are setting org.gnome.desktop.session.idle-delay directly in dconf," needs updating - gnome-session seems to signal idleness too early, and changing the idle-delay seems to have no effect on the actual configuration (it was 60 seconds when I started the session, gnome-session still kicks off at 60 seconds even though I have changed it to 600 seconds). I'll file a separate bug about that.
(In reply to comment #11) > - gnome-session seems to signal idleness too early, and changing the idle-delay > seems to have no effect on the actual configuration (it was 60 seconds when I > started the session, gnome-session still kicks off at 60 seconds even though I > have changed it to 600 seconds). I'll file a separate bug about that. See bug 692082
As mentioned in bug 691965, I've pushed the g-s-d patch that makes use of the one in this bug. It will need to be merged before 3.7.5 gets released, otherwise it'll appear pretty broken.
(In reply to comment #13) > As mentioned in bug 691965, I've pushed the g-s-d patch that makes use of the > one in this bug. It will need to be merged before 3.7.5 gets released, > otherwise it'll appear pretty broken. Uhm... I does already appear pretty broken: ActiveChanged(true) is emitted when you lock the screen manually, so the screen blanks before the shield goes down. Then, for whatever reason, the screen reactivates itself at minimum brightness, always.
(In reply to comment #14) > (In reply to comment #13) > > As mentioned in bug 691965, I've pushed the g-s-d patch that makes use of the > > one in this bug. It will need to be merged before 3.7.5 gets released, > > otherwise it'll appear pretty broken. > > Uhm... I does already appear pretty broken: ActiveChanged(true) is emitted when > you lock the screen manually, so the screen blanks before the shield goes down. Right, the patch is incomplete then? That's not what we discussed in comment 3. > Then, for whatever reason, the screen reactivates itself at minimum brightness, > always. That's a separate issue, caused by c4bd6ca742ad8039b41aff7edc0dc80d4fabc747 (which was a rework of 168d3ee1a08be38051e0cc054659ad7946b1c14d). It's pretty much irrelevant to this bug though.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > As mentioned in bug 691965, I've pushed the g-s-d patch that makes use of the > > > one in this bug. It will need to be merged before 3.7.5 gets released, > > > otherwise it'll appear pretty broken. > > > > Uhm... I does already appear pretty broken: ActiveChanged(true) is emitted when > > you lock the screen manually, so the screen blanks before the shield goes down. > > Right, the patch is incomplete then? That's not what we discussed in comment 3. Uhm... > > Also, do you want ActiveChanged when locking manually (from the menu, from > > ScreenSaver.Lock, from login1.Lock)? > > Yes. That allows us to blank the screen straight away. Right now, we have a 10 > second delay before blanking (so we line up with the fade ending), and that's > too long when locking manually (about 9.3 seconds too long. And indeed, you have an ActiveChanged when locking manually...
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #13) > > > > As mentioned in bug 691965, I've pushed the g-s-d patch that makes use of the > > > > one in this bug. It will need to be merged before 3.7.5 gets released, > > > > otherwise it'll appear pretty broken. > > > > > > Uhm... I does already appear pretty broken: ActiveChanged(true) is emitted when > > > you lock the screen manually, so the screen blanks before the shield goes down. > > > > Right, the patch is incomplete then? That's not what we discussed in comment 3. > > Uhm... > > > > Also, do you want ActiveChanged when locking manually (from the menu, from > > > ScreenSaver.Lock, from login1.Lock)? > > > > Yes. That allows us to blank the screen straight away. Right now, we have a 10 > > second delay before blanking (so we line up with the fade ending), and that's > > too long when locking manually (about 9.3 seconds too long. > > And indeed, you have an ActiveChanged when locking manually... Right. Sorry if I wasn't clear. I'd want the signal *after* any animation. We don't want to trample on the animation with the blanking. If I were to add a 0.3 sec timeout for the manual lock case, I'd have the same problem as originally (meaning that I wouldn't know what the timeout should be). Unless you make the fadeout uncancellable from 0.3 seconds before, and send the ActiveChanged() signal at that point, and I make the g-s-d code as convoluted as it was before. I don't think we want to go down that path.
(In reply to comment #15) > > Then, for whatever reason, the screen reactivates itself at minimum brightness, > > always. > > That's a separate issue, caused by c4bd6ca742ad8039b41aff7edc0dc80d4fabc747 > (which was a rework of 168d3ee1a08be38051e0cc054659ad7946b1c14d). It's pretty > much irrelevant to this bug though. Which should be fixed by one-liner: commit b24f223613921bd22377ce0778b4ef5d6ec37472 Author: Bastien Nocera <hadess@hadess.net> Date: Tue Jan 29 14:39:26 2013 +0100 power: Fix incorrect backlight level on restore https://bugzilla.gnome.org/show_bug.cgi?id=691964#c14 If that doesn't fix it, I'll have to add more tests to the test suite :)
(In reply to comment #17) > [...] > > Right. Sorry if I wasn't clear. I'd want the signal *after* any animation. We > don't want to trample on the animation with the blanking. If I were to add a > 0.3 sec timeout for the manual lock case, I'd have the same problem as > originally (meaning that I wouldn't know what the timeout should be). > > Unless you make the fadeout uncancellable from 0.3 seconds before, and send the > ActiveChanged() signal at that point, and I make the g-s-d code as convoluted > as it was before. I don't think we want to go down that path. You're trading complexity in gnome-settings-daemon for complexity in gnome-shell. I'm not sure I like it :) I'll prepare a better patch, anyway.
(In reply to comment #19) > (In reply to comment #17) > > [...] > > > > Right. Sorry if I wasn't clear. I'd want the signal *after* any animation. We > > don't want to trample on the animation with the blanking. If I were to add a > > 0.3 sec timeout for the manual lock case, I'd have the same problem as > > originally (meaning that I wouldn't know what the timeout should be). > > > > Unless you make the fadeout uncancellable from 0.3 seconds before, and send the > > ActiveChanged() signal at that point, and I make the g-s-d code as convoluted > > as it was before. I don't think we want to go down that path. > > You're trading complexity in gnome-settings-daemon for complexity in > gnome-shell. I'm not sure I like it :) I really don't think I am. I'm trading complexity in only gnome-shell with complexity in both gnome-shell and gnome-settings-daemon. I also didn't think that the work to make the signal be sent at the end of the animation instead of the beginning would be that much work... I guess that's what happens when one makes assumptions on code bases they don't know ;) > I'll prepare a better patch, anyway. Thanks.
Still, there is a problem: the animation is jerky and most of time it is not visible at all. Comparing immediate blanking to no blanking, it slightly looks better to me. I'm not sure if we can consider this just a performance problem. I tried the best possible combination: warm cache (disk, texture and CSS), bug 689858 applied, nothing else in the system taking CPU. The alternative is a longer animation, or no animation at all.
Created attachment 234744 [details] [review] ScreenShield: emit lock-status-changed at the end of animation for manual locking too gnome-settings-daemon uses lock-status-changed/ActiveChanged to drive screen blanking, so must wait for the animation end before emitting it.
Created attachment 234745 [details] [review] ScreenShield: use a longer animation when locking manually The curtain animation looks jerky at its current speed, and more so if we blank the screen immediately at the end. Make it a little slower and it becomes more confortable.
(The last two patches are on top of bug 692560, which has some refactoring of the state handling)
Review of attachment 233824 [details] [review]: One minor nit, otherwise OK. ::: js/ui/screenShield.js @@ +651,3 @@ + } + + // assert(!this._isActive); ???
Review of attachment 234744 [details] [review]: OK.
Review of attachment 234745 [details] [review]: OK.
Attachment 233824 [details] pushed as 3f6f597 - ScreenShield: only emit ActiveChanged at the end of the idle fading Attachment 234744 [details] pushed as 7ad881d - ScreenShield: emit lock-status-changed at the end of animation for manual locking too Not committing the animation patch until I get designer approval. In particular, it looks slightly weird because raising and lowering are not symmetric.
I'm getting the ActiveChanged signal with "false" as the boolean argument when lock-delay isn't 0, and seem to never get the ActiveChanged() signal (with true or something else) even when the display is effectively locked. ActiveChanged(true) should be sent as soon as the shield is down, even if not locked yet (so, after idle-delay). It works properly when lock-delay is 0.
Created attachment 234922 [details] [review] ScreenShield: fix wrong signal value in ActiveChanged ActiveChanged should be about active, not locked. Not my lucky day...
Review of attachment 234922 [details] [review]: Yikes.
Review of attachment 234922 [details] [review]: Works for me.
Comment on attachment 234922 [details] [review] ScreenShield: fix wrong signal value in ActiveChanged Attachment 234922 [details] pushed as 644b830 - ScreenShield: fix wrong signal value in ActiveChanged
Should we get the last acn patch here merged ?
I asked for a designer review, since the time was originally 0.8, then it was changed to 0.3 before 3.6.
FWIW, since I was the one who changed it to 0.3, I agree with changing the locking animation to 0.8.
We got a designer OK.
Attachment 234745 [details] pushed as 2b1a661 - ScreenShield: use a longer animation when locking manually
FWIW 3.6 curtain fall speed seems just right, and is stutter-free. 3.8 feels laggy.