GNOME Bugzilla – Bug 682285
ScreenShield: rework the arrow in the lock screen
Last modified: 2012-10-10 03:04:46 UTC
User testing has shown that it is not discoverable that the whole lock screen can be dragged. A new mockup includes more arrows and a short animation every 4 seconds.
Created attachment 221866 [details] [review] ScreenShield: rework the arrow in the lock screen
Created attachment 221872 [details] [review] ScreenShield: rework the arrow in the lock screen User testing has shown that it is not discoverable that the whole lock screen can be dragged. A new mockup includes more arrows and a short animation every 4 seconds. Rebased on master
Created attachment 221921 [details] [review] Tweaked patch This looks really good to me. I've tweaked some of the values...
One problem here - the white arrows can disappear on some backgrounds. On others they can be a bit too prominent. Ideas?
Giovanni - can we try adding a subtle shadow to the arrows? Jimmac suggests something like: 0 1px 1px rgba(0,0,0,0,4)
Created attachment 222065 [details] [review] ScreenShield: add a drop shadow to the animated arrows Introduce a StShadowHelper to manage drop shadows from JS (which cannot use Cogl directly), and use it in a new StWidget-derived JS class to draw the arrow. This is a very quick and very dirty patch. After the release I plan to adapt StShadowHelper to StIcon and StLabel, and then maybe make it a ClutterOffscreenEffect.
Created attachment 222066 [details] [review] ScreenShield: add a drop shadow to the animated arrows Introduce a StShadowHelper to manage drop shadows from JS (which cannot use Cogl directly), and use it in a new StWidget-derived JS class to draw the arrow. Rebased on master rather than my local branch, although there shouldn't be merge conflicts.
Created attachment 222067 [details] [review] ScreenShield: add a drop shadow to the animated arrows Introduce a StShadowHelper to manage drop shadows from JS (which cannot use Cogl directly), and use it in a new StWidget-derived JS class to draw the arrow. Fixed build. (Pressure is not my best friend. Good thing this is not Pascal.)
This looks great to me, and works well with both light and dark backgrounds. My only suggestion (passed on from Jimmac) - lower the opacity of the arrows a bit more. 0.4 looks good to me.
Giovanni - do you want to do any more work on this before we ask for a freeze exception, or do you think it is ready to land now? Also, updating the component.
It depends. This is not the best fix for the shadow, but it would take time (a week, given my current time availability) to do it right (with a ClutterOffscreenEffect). If the freeze exception can be granted at any time, then sure, I can make it right from the start (and fix 677412 at the same time), otherwise it's best to have this in soon (so the screenshots / videos / release notes can be written), and then fix the implementation before hard code freeze.
(In reply to comment #11) > It depends. This is not the best fix for the shadow, but it would take time (a > week, given my current time availability) to do it right (with a > ClutterOffscreenEffect). The reason for shadows being implemented in software rather than using shaders (ClutterOffscreenEffect didn't exist at that time) was that not all hardware capable of running gnome-shell supports them - given that we sometimes use shadows to improve legibility, I'm wary of throwing out the software implementation completely. I'm of course fine with using an effect when possible.
A ClutterOffscreenEffect does not need to be implemented using shaders, the only requirement over GL 2.1 (which includes GLSL, btw) is EXT_framebuffer_object, which is already used by current drop shadows. Anyway, turns out code might be less than I expected, patch coming in a not so distant future.
(In reply to comment #13) > [...] the only requirement over GL 2.1 (which includes GLSL, btw) is > EXT_framebuffer_object, which is already used by current drop shadows. Guess who owns a graphics card that supports EXT_framebuffer_object and no GLSL ;-)
Ok, I implemented the ClutterOffscreenEffect approach now, and while I like it from an implementation POV, it does not seem correct in the way it handle transformations. Maybe the StShadowHelper was not that a bad idea...
Created attachment 222240 [details] [review] St: Add a StShadowEffect for drop shadows The previous way to handle drop shadows was hacky and not extensible. Plus it would break in presence of opacity transitions. Therefore abstract the drop shadow handling in a ClutterOffscreenEffect.
Created attachment 222241 [details] [review] ScreenShield: add a drop shadow to the animated arrows Using the new StShadowEffect and a new StWidget-derived JS class, add a drop shadow to the arrows in the lock screen, to facilitate seeing them in white backgrounds.
Created attachment 222242 [details] [review] St: use StShadowEffect for StLabel and StIcon Replace a bunch of custom code with the new StShadowEffect
Comment on attachment 222240 [details] [review] St: Add a StShadowEffect for drop shadows Gghghgh... Comparing to master, it's clear this is just broken, and I have no idea why. Well, let's go with ShadowHelper for now.
Any one for a review of the shadow helper patch (patch 221921 and patch 222067)? They break affect UI freeze and it would be good to land them before 3.5.91 and before the doc team starts taking screenshots of the lock screen.
Review of attachment 222067 [details] [review]: This looks fine to me.
(In reply to comment #21) > Review of attachment 222067 [details] [review]: > > This looks fine to me. What about the other one (patch 221921), on which this depends?
Review of attachment 221921 [details] [review]: Looks fine.
Attachment 222067 [details] pushed as a76cc79 - ScreenShield: add a drop shadow to the animated arrows Attachment 221921 [details] pushed as something else Leaving the bug open in case we want to fix the StShadowEffect in the future and use that.
Is there anything left to do here?
I think not