GNOME Bugzilla – Bug 699112
Transition from desktop to locked screen looks broken
Last modified: 2013-08-20 20:04:01 UTC
In 3.8, if you manually lock the screen, first the "shield" rolls down and after this animation is finished, the screen suddenly turns black, which I first expected to be a bug, but I guess the screen is supposed to turn black? IMHO it would look better if the screen would fade to black during the shield rolldown animation.
(In reply to comment #0) > IMHO it would look better if the screen would fade to black during the shield > rolldown animation. That, or fade after the shield animation and before blanking the screen ... In any case, let's try to get some design feedback.
I like Florian's suggestion. Very rapid curtain fall followed by a fade to black in about 500ms
*** Bug 699494 has been marked as a duplicate of this bug. ***
Jon in bug 699494: My recommendation is to: * Lower the shield with an animation (to provide feedback on the lock action) * Take a beat * Quickly animate the brightness to zero (just enough to round off the edge of the discontinuity)
I don't think we can animate the actual brightness: for one, the minimum brightness supported by HW can still be very high, or the display might be external, in which case we don't control it. We can fade to black, like what we do when inactive, but for bright LCDs you still have a jump. Anyway, going for the fade option.
Created attachment 243926 [details] [review] ScreenShield: switch resetLockScreen to accept keyword arguments Having two booleans as argument is just confusing.
Created attachment 243927 [details] [review] Lightbox: have animation times passed as parameters to show() and hide() This way it is possible to use the same lightbox with different times.
Created attachment 243928 [details] [review] ScreenShield: fade the screen to black when locking manually When locking manually (or locking with an animation), fade the screen to black after a small timeout. This provides a smoother experience, instead of abruptly turning off the screen.
Created attachment 243937 [details] [review] ScreenShield: fade the screen to black when locking manually When locking manually (or locking with an animation), fade the screen to black after a small timeout. This provides a smoother experience, instead of abruptly turning off the screen.
Created attachment 246019 [details] [review] ScreenShield: fade the screen to black when locking manually When locking manually (or locking with an animation), fade the screen to black after a small timeout. This provides a smoother experience, instead of abruptly turning off the screen.
Review of attachment 243926 [details] [review]: ::: js/ui/screenShield.js @@ +933,3 @@ }, + _resetLockScreen: function(params) { Shouldn't we have some default values like we always do when using params?
Review of attachment 243927 [details] [review]: LG.
*** Bug 697493 has been marked as a duplicate of this bug. ***
(In reply to comment #11) > Review of attachment 243926 [details] [review]: > > ::: js/ui/screenShield.js > @@ +933,3 @@ > }, > > + _resetLockScreen: function(params) { > > Shouldn't we have some default values like we always do when using params? We always pass both params (and that's the point of the patch, it makes explicit at call site what's happening), so I don't think it is necessary.
Review of attachment 246019 [details] [review]: "ligthbox" is all throughout the code as well. ::: js/ui/screenShield.js @@ +835,3 @@ + // is fully shown; at this point isLocked is false, so we just hide + // the ligthbox, reset the activationTime and go back to the unlocked + // desktop I know you're reducing this from 4 to 2, but this is really confusing control flow and a giant comment here isn't helping me figure this out.
(In reply to comment #15) > Review of attachment 246019 [details] [review]: > > "ligthbox" is all throughout the code as well. > > ::: js/ui/screenShield.js > @@ +835,3 @@ > + // is fully shown; at this point isLocked is false, so we just hide > + // the ligthbox, reset the activationTime and go back to the > unlocked > + // desktop > > I know you're reducing this from 4 to 2, but this is really confusing control > flow and a giant comment here isn't helping me figure this out. Do you have an idea for a better flow? Basically, the problem is that weird stuff (loginctl unlock-session, gsd timeouts, XSync resets, laptop lid closing...) can happen at any time, so you need to be prepared to interrupt the regular flow.
Created attachment 249118 [details] [review] ScreenShield: fade the screen to black when locking manually When locking manually (or locking with an animation), fade the screen to black after a small timeout. This provides a smoother experience, instead of abruptly turning off the screen.
I'm afraid that this doesn't apply here.
Created attachment 251946 [details] [review] ScreenShield: switch resetLockScreen to accept keyword arguments Having two booleans as argument is just confusing.
Created attachment 251947 [details] [review] Lightbox: have animation times passed as parameters to show() and hide() This way it is possible to use the same lightbox with different times.
Created attachment 251948 [details] [review] ScreenShield: fade the screen to black when locking manually When locking manually (or locking with an animation), fade the screen to black after a small timeout. This provides a smoother experience, instead of abruptly turning off the screen.
Rebased. Also, me and Jasper discussed at Guadec the code flow of the screenshield in general, and in the end he agreed that it's a mess we can't get rid of it, life sucks, and everything. So it would be nice if he could approve these. Thank you :)
That's totally not what I said.
(In reply to comment #23) > That's totally not what I said. But I explained why every single bit of that was needed when you reviewed the other screenshield patches, and you agreed!
(In reply to comment #24) > But I explained why every single bit of that was needed when you reviewed the > other screenshield patches, and you agreed! What I said that we have two different animations (lower shield, fast fade lightbox vs. slow fade lightbox, insta-lower shield), and that they should be handled by two separate flow controls rather than tied together with boolean variables determining what to do.
Created attachment 252061 [details] [review] ScreenShield: fade the screen to black when locking manually When locking manually (or locking with an animation), fade the screen to black after a small timeout. This provides a smoother experience, instead of abruptly turning off the screen.
Review of attachment 252061 [details] [review]: Much better.
Review of attachment 251947 [details] [review]: I assume this is unnecessary now.
Review of attachment 251946 [details] [review]: Still not the greatest choice of names, but OK.
(In reply to comment #28) > Review of attachment 251947 [details] [review]: > > I assume this is unnecessary now. No, it's still used because I might pass the short timeout or 0, according to the animate argument.
Review of attachment 251947 [details] [review]: OK then.
*** Bug 704601 has been marked as a duplicate of this bug. ***
Attachment 251946 [details] pushed as 32613ba - ScreenShield: switch resetLockScreen to accept keyword arguments Attachment 251947 [details] pushed as 6b8339e - Lightbox: have animation times passed as parameters to show() and hide() Attachment 252061 [details] pushed as 2a2bcc8 - ScreenShield: fade the screen to black when locking manually
Nice work! Thanks for squeezing this in before the freeze.