Merge lp:~macslow/unity/unity.fix-863230 into lp:unity

Proposed by Mirco Müller
Status: Merged
Approved by: Didier Roche
Approved revision: 1972
Merged at revision: 2104
Proposed branch: lp:~macslow/unity/unity.fix-863230
Merge into: lp:unity
Diff against target: 42 lines (+20/-0)
2 files modified
manual-tests/DragDropDashLauncher.txt (+6/-0)
plugins/unityshell/src/Launcher.cpp (+14/-0)
To merge this branch: bzr merge lp:~macslow/unity/unity.fix-863230
Reviewer Review Type Date Requested Status
John Lea (community) design Needs Fixing
Tim Penhey (community) Approve
Review via email:

Description of the change

Trying to understand how and where changes to an icons alpha- and saturation-value are taking place is material to get insane. By now I've spent many many many hours trying to get behind what's going on. I can't say I fully grasp
the intricate ways of icons opacity and saturation in class Launcher.

As far as I can tell, there are four valid "cases" for the launcher icons in terms of alpha and saturation. To make it easier for you to follow, have a look at this table:

                                            alpha | saturation
| dash open | 0.5 | 0.0 |
| dash closed | 1.0 | 1.0 |
| drop-target (dragging & dash open) | 1.0 | 1.0 |
| no drop-target (dragging & dash open) | 0.5 | 0.0 |

So alpha should never be outside [0.5 .. 1.0] and saturation should never be outside [0.0 .. 1.0]. From my investigation it is sufficient to protect only against the lower threshold, where alpha needs to be 0.5 and saturation 0.0

without fix:

with fix:

If anybody ever causes this to regress, I'll ... !

To post a comment you must log in.
Mirco Müller (macslow) wrote :

Screw LP text-formatting of merge-proposal messages!

Tim Penhey (thumper) wrote :

This sounds like a classic use-case for a Clamp.

See tests/test_properties.cpp in nux.

If we changed alpha and saturation to be properties with a clamp, you wouldn't have to manually check them.

However that would change all uses of RenderArg, and I'm not sure if that is desirable.

Due to the visual nature of this, I accept that we can't have automated tests for it.

We do however want design signoff, so getting John to look.

review: Approve
Alex Launi (alexlauni) wrote :

Actually Tim, we should be able to export the alpha and saturation values via introspection, and check them at various interaction states. However they're just values, a test like that doesn't seem appropriate.

Mirco Müller (macslow) wrote :

Setting it to approved after catch up with Tim, yesterday.

Unity Merger (unity-merger) wrote :

The Jenkins job reported an error when processing this lp:~macslow/unity/unity.fix-863230 branch.
Not merging it.

John Lea (johnlea) wrote :

If you watch the Chromium icon in the video, it's tinting is changing at a time when it should remain static. Look at the Chromium icon at 0:06 in the video, this is the correct state. Now if you scrub back to 0:04, you will see the tinting on the Chromium icon has changed slightly, even though it is not a valid drop target.

Requested fix in order for this change to be complete and ready to land: When a user starts dragging a icon in the Dash, the tinting of Launcher icons that are not valid drop targets should not change in *any* way, even temporally.

review: Needs Fixing (design)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'manual-tests/DragDropDashLauncher.txt'
2--- manual-tests/DragDropDashLauncher.txt 1970-01-01 00:00:00 +0000
3+++ manual-tests/DragDropDashLauncher.txt 2012-02-29 21:18:19 +0000
4@@ -0,0 +1,6 @@
5+- open Dash
6+- go to Files-lens
7+- select and drag a document-file (e.g. text or image) to the launcher
8+- as you drag the document-file watch the drag-targets (drop-recepticals) saturate
9+- this saturation-process should be smooth and without flicker
10+- the tinting of the non-drag-targets should not change
12=== modified file 'plugins/unityshell/src/Launcher.cpp'
13--- plugins/unityshell/src/Launcher.cpp 2012-02-27 22:53:53 +0000
14+++ plugins/unityshell/src/Launcher.cpp 2012-02-29 21:18:19 +0000
15@@ -899,6 +899,13 @@
16 icon->GetIconType() == AbstractLauncherIcon::TYPE_DEVICE ||
17 icon->GetIconType() == AbstractLauncherIcon::TYPE_EXPO;
19+ // trying to protect against flickering when icon is dragged from dash LP: #863230
20+ if (arg.alpha < 0.5)
21+ {
22+ arg.alpha = 0.5;
23+ arg.saturation = 0.0;
24+ }
26 if (_dash_is_open)
27 arg.active_arrow = icon->GetIconType() == AbstractLauncherIcon::TYPE_HOME;
28 else
29@@ -978,6 +985,13 @@
30 if (drop_dim_value < 1.0f)
31 arg.alpha *= drop_dim_value;
33+ // trying to protect against flickering when icon is dragged from dash LP: #863230
34+ if (arg.alpha < 0.5)
35+ {
36+ arg.alpha = 0.5;
37+ arg.saturation = 0.0;
38+ }
40 if (icon == _drag_icon)
41 {
42 if (MouseBeyondDragThreshold())