After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 682285 - ScreenShield: rework the arrow in the lock screen
ScreenShield: rework the arrow in the lock screen
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-08-20 17:59 UTC by Giovanni Campagna
Modified: 2012-10-10 03:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ScreenShield: rework the arrow in the lock screen (7.88 KB, patch)
2012-08-20 17:59 UTC, Giovanni Campagna
none Details | Review
ScreenShield: rework the arrow in the lock screen (7.91 KB, patch)
2012-08-20 18:38 UTC, Giovanni Campagna
none Details | Review
Tweaked patch (7.91 KB, patch)
2012-08-20 21:22 UTC, Allan Day
committed Details | Review
ScreenShield: add a drop shadow to the animated arrows (9.71 KB, patch)
2012-08-21 19:15 UTC, Giovanni Campagna
none Details | Review
ScreenShield: add a drop shadow to the animated arrows (9.72 KB, patch)
2012-08-21 19:22 UTC, Giovanni Campagna
none Details | Review
ScreenShield: add a drop shadow to the animated arrows (9.72 KB, patch)
2012-08-21 19:30 UTC, Giovanni Campagna
committed Details | Review
St: Add a StShadowEffect for drop shadows (19.51 KB, patch)
2012-08-23 16:41 UTC, Giovanni Campagna
needs-work Details | Review
ScreenShield: add a drop shadow to the animated arrows (3.87 KB, patch)
2012-08-23 16:42 UTC, Giovanni Campagna
needs-work Details | Review
St: use StShadowEffect for StLabel and StIcon (10.26 KB, patch)
2012-08-23 16:42 UTC, Giovanni Campagna
needs-work Details | Review

Description Giovanni Campagna 2012-08-20 17:59:22 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.
Comment 1 Giovanni Campagna 2012-08-20 17:59:25 UTC
Created attachment 221866 [details] [review]
ScreenShield: rework the arrow in the lock screen
Comment 2 Giovanni Campagna 2012-08-20 18:38:11 UTC
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
Comment 3 Allan Day 2012-08-20 21:22:47 UTC
Created attachment 221921 [details] [review]
Tweaked patch

This looks really good to me. I've tweaked some of the values...
Comment 4 Allan Day 2012-08-20 22:14:55 UTC
One problem here - the white arrows can disappear on some backgrounds. On others they can be a bit too prominent.

Ideas?
Comment 5 Allan Day 2012-08-21 09:52:55 UTC
Giovanni - can we try adding a subtle shadow to the arrows? Jimmac suggests something like:

0 1px 1px rgba(0,0,0,0,4)
Comment 6 Giovanni Campagna 2012-08-21 19:15:50 UTC
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.
Comment 7 Giovanni Campagna 2012-08-21 19:22:45 UTC
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.
Comment 8 Giovanni Campagna 2012-08-21 19:30:36 UTC
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.)
Comment 9 Allan Day 2012-08-21 19:46:18 UTC
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.
Comment 10 Allan Day 2012-08-23 12:56:52 UTC
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.
Comment 11 Giovanni Campagna 2012-08-23 13:05:36 UTC
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.
Comment 12 Florian Müllner 2012-08-23 14:07:32 UTC
(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.
Comment 13 Giovanni Campagna 2012-08-23 14:19:17 UTC
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.
Comment 14 Florian Müllner 2012-08-23 14:34:47 UTC
(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 ;-)
Comment 15 Giovanni Campagna 2012-08-23 16:39:14 UTC
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...
Comment 16 Giovanni Campagna 2012-08-23 16:41:50 UTC
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.
Comment 17 Giovanni Campagna 2012-08-23 16:42:19 UTC
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.
Comment 18 Giovanni Campagna 2012-08-23 16:42:38 UTC
Created attachment 222242 [details] [review]
St: use StShadowEffect for StLabel and StIcon

Replace a bunch of custom code with the new StShadowEffect
Comment 19 Giovanni Campagna 2012-08-23 17:06:04 UTC
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.
Comment 20 Giovanni Campagna 2012-08-31 14:17:10 UTC
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.
Comment 21 Jasper St. Pierre (not reading bugmail) 2012-08-31 16:52:02 UTC
Review of attachment 222067 [details] [review]:

This looks fine to me.
Comment 22 Giovanni Campagna 2012-08-31 20:31:03 UTC
(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?
Comment 23 Jasper St. Pierre (not reading bugmail) 2012-08-31 20:33:17 UTC
Review of attachment 221921 [details] [review]:

Looks fine.
Comment 24 Giovanni Campagna 2012-09-01 14:53:45 UTC
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.
Comment 25 Jasper St. Pierre (not reading bugmail) 2012-09-22 13:59:41 UTC
Is there anything left to do here?
Comment 26 Matthias Clasen 2012-10-10 03:04:46 UTC
I think not