Merge lp:~sil2100/unity/launcher_dnd_flicker_new into lp:unity

Proposed by Łukasz Zemczak
Status: Merged
Approved by: Didier Roche-Tolomelli
Approved revision: no longer in the source branch.
Merged at revision: 2389
Proposed branch: lp:~sil2100/unity/launcher_dnd_flicker_new
Merge into: lp:unity
Diff against target: 35 lines (+10/-3)
1 file modified
launcher/Launcher.cpp (+10/-3)
To merge this branch: bzr merge lp:~sil2100/unity/launcher_dnd_flicker_new
Reviewer Review Type Date Requested Status
Jason Smith (community) Approve
Sam Spilsbury (community) Approve
Alex Launi quality Pending
Review via email: mp+105064@code.launchpad.net

This proposal supersedes a proposal from 2012-04-08.

Commit message

When dragging a non-application, do not saturate all icons automatically, but only those that are needed.

Eliminates flickering before 'prelighting' the Launcher icons that can handle the current drag-and-drop (LP #863230).

Description of the change

Problem description:

When drag-and-dropping from the Dash to the Launcher a non-application icon (for instance, a PNG file), the Launcher 'flickers' a bit. This flickering means that the Launcher icons get desaturated for a moment and then loose saturation again (as originally intended), tinted (LP #863230 and more specifically - LP #971086).

The fix:

During drag and drop, instead of saturating all icons automatically when overlay is present, we only do this when we know that we're not dragging a non-application. Otherwise, only saturate the icons that we will actually 'prelight' (i.e. indicate that can handle the given file). This way, there is no flickering and all icons visualize correctly, as intended. There are no longer any tinting problems as well (as noted by JohnLea).

Test coverage:

The following fix can be tested by manual testing of the specified bugs (LP #863230, LP #971086). Problem no longer occurs.

To post a comment you must log in.
Revision history for this message
Alex Launi (alexlauni) wrote : Posted in a previous version of this proposal

Please add tests to the suite in manual-tests/

review: Needs Fixing (quality)
Revision history for this message
Łukasz Zemczak (sil2100) wrote : Posted in a previous version of this proposal

There already seems to be a manual-test that theoretically tests the given behavior:
manual-tests/Dash.txt -> "Launcher drop target saturation test" (from the "Drag and drop tests" section).

Should I modify it more to add details regarding this bug? But it seems rather sufficient.

Revision history for this message
Alex Launi (alexlauni) wrote : Posted in a previous version of this proposal

Good catch! Thanks. Yeah the current test is sufficient.

review: Approve (quality)
Revision history for this message
Unity Merger (unity-merger) wrote : Posted in a previous version of this proposal

Attempt to merge into lp:unity failed due to conflicts:

text conflict in plugins/unityshell/src/Launcher.cpp

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

I resubmitted the merge with a new branch, based on the latest lp:unity on trunk (after the restructuring). Hope I didn't miss anything.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Merged in updated trunk.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

I'm happy with the code, although a flag that we need to get LauncherHideMachine under some sort of test because there's lots of state information there which can be easily clobbered by the sounds of things.

review: Approve
Revision history for this message
Jason Smith (jassmith) wrote :

looks wholly sane, +1

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

No commit message specified.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/Launcher.cpp'
2--- launcher/Launcher.cpp 2012-05-24 11:04:32 +0000
3+++ launcher/Launcher.cpp 2012-05-31 11:06:28 +0000
4@@ -2696,9 +2696,6 @@
5
6 _hide_machine->SetQuirk(LauncherHideMachine::EXTERNAL_DND_ACTIVE, true);
7
8- if (IsOverlayOpen())
9- SaturateIcons();
10-
11 for (auto it : _dnd_data.Uris())
12 {
13 if (g_str_has_suffix(it.c_str(), ".desktop"))
14@@ -2713,11 +2710,21 @@
15 for (auto it : *_model)
16 {
17 if (it->ShouldHighlightOnDrag(_dnd_data))
18+ {
19+ it->SetQuirk(AbstractLauncherIcon::QUIRK_DESAT, false);
20 it->SetQuirk(AbstractLauncherIcon::QUIRK_DROP_PRELIGHT, true);
21+ }
22 else
23+ {
24 it->SetQuirk(AbstractLauncherIcon::QUIRK_DROP_DIM, true);
25+ }
26 }
27 }
28+ else
29+ {
30+ if (IsOverlayOpen())
31+ SaturateIcons();
32+ }
33 }
34
35 void