Merge lp:~unity-team/unity/autohide-load-934514 into lp:unity

Proposed by Bilal Akhtar on 2012-07-31
Status: Needs review
Proposed branch: lp:~unity-team/unity/autohide-load-934514
Merge into: lp:unity
Diff against target: 56 lines (+29/-0)
2 files modified
launcher/LauncherController.cpp (+10/-0)
manual-tests/Launcher.txt (+19/-0)
To merge this branch: bzr merge lp:~unity-team/unity/autohide-load-934514
Reviewer Review Type Date Requested Status
Thomi Richards (community) quality 2012-08-08 Approve on 2012-09-10
Tim Penhey (community) 2012-07-31 Needs Information on 2012-08-27
Review via email: mp+117367@code.launchpad.net

Commit message

LauncherController: Don't hide the launcher for 3 seconds after it finishes loading on login (LP: #934513)

Description of the change

Fixes bug #934513 by ensuring the launcher doesn't start hidden, but instead hides 3 seconds after starting up (when autohide is enabled, that is).

To post a comment you must log in.
Tim Penhey (thumper) wrote :

Do we care about the situation where someone has autohide enabled, and moves the mouse over the launcher within the first three seconds of being logged in?

review: Needs Information
Bilal Akhtar (bilalakhtar) wrote :

Yes, this works. I just re-built and re-tested it, and if the user moves the mouse over the launcher, the ForceReveal(false) does nothing until the mouse moves away from the launcher.

Tim Penhey (thumper) wrote :

OK. Since this is a "start unity" type test, can I get you to write a manual test for it?

review: Needs Fixing
Tim Penhey (thumper) :
review: Approve
Unity Merger (unity-merger) wrote :

No commit message specified.

Marco Trevisan (Treviño) (3v1n0) wrote :

 auto hide_all_launchers = [&]()
10 + {
11 + for (auto launcher : launchers)
12 + launcher->ForceReveal(false);
13 + return true;
14 + };
15 + sources_.AddTimeout(3000, hide_all_launchers);

Whi is this timeout always running every 3 seconds?
I'm not sure this is really needed (or a good thing).

Also, probably you could add a sort of unit tests in test_launcher_controller (checking if the force-reveal quirk is correctly set on the launcher's hide machine)...

Tim Penhey (thumper) wrote :

Marco, could you provide more of a guideline for an automated test?

review: Needs Information
Bilal Akhtar (bilalakhtar) wrote :

Marco, the test doesn't run every three seconds. Once the "return true" call is reached, the timeout stops running. So it runs only once.

Bilal Akhtar (bilalakhtar) wrote :

Actually, I was wrong. I need to return false rather than true. Change made.

2533. By Bilal Akhtar on 2012-08-28

Change "return true" to "return false" to destroy timeout

Thomi Richards (thomir) wrote :

Has conflicts with trunk. needs re-merging.

2534. By Bilal Akhtar on 2012-09-04

Merge from trunk and fix conflicts

Bilal Akhtar (bilalakhtar) wrote :

Re-merged, fixed conflicts.

Marco Trevisan (Treviño) (3v1n0) wrote :

Are 3 seconds enough when loading unity on startup? Couldn't this timeout happen when the interface is still loading?

PS: You can use AddTimeoutSeconds and maybe reduce the timeout priority.

Bilal Akhtar (bilalakhtar) wrote :

Not sure; John preferred to go for 3 seconds (see the description of the linked bug report).

Thomi Richards (thomir) wrote :

From a quality POV, I'm happy that this code is merged with only a manual test.

review: Approve (quality)

Unmerged revisions

2534. By Bilal Akhtar on 2012-09-04

Merge from trunk and fix conflicts

2533. By Bilal Akhtar on 2012-08-28

Change "return true" to "return false" to destroy timeout

2532. By Bilal Akhtar on 2012-08-08

Add manual test for login autohide change

2531. By Bilal Akhtar on 2012-08-08

Merge from trunk

2530. By Bilal Akhtar on 2012-07-31

Don't hide the launcher for 3 seconds after it finishes loading on login (LP: #934513)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/LauncherController.cpp'
2--- launcher/LauncherController.cpp 2012-08-30 21:09:02 +0000
3+++ launcher/LauncherController.cpp 2012-09-04 08:17:19 +0000
4@@ -173,6 +173,15 @@
5 launcher->QueueDraw();
6 });
7
8+ // Hide launcher after 3 seconds
9+ auto hide_all_launchers = [&]()
10+ {
11+ for (auto launcher : launchers)
12+ launcher->ForceReveal(false);
13+ return false;
14+ };
15+ sources_.AddTimeout(3000, hide_all_launchers);
16+
17 dbus_owner_ = g_bus_own_name(G_BUS_TYPE_SESSION, DBUS_NAME.c_str(), G_BUS_NAME_OWNER_FLAGS_NONE,
18 OnBusAcquired, nullptr, nullptr, this, nullptr);
19 }
20@@ -219,6 +228,7 @@
21 launchers[i]->monitor(monitor);
22 launchers[i]->Resize();
23 edge_barriers_.Subscribe(launchers[i].GetPointer(), launchers[i]->monitor);
24+ launchers[i]->ForceReveal(true);
25 }
26
27 for (unsigned int i = last_launcher; i < launchers_size; ++i)
28
29=== modified file 'manual-tests/Launcher.txt'
30--- manual-tests/Launcher.txt 2012-09-03 00:07:32 +0000
31+++ manual-tests/Launcher.txt 2012-09-04 08:17:19 +0000
32@@ -703,6 +703,25 @@
33 * See https://bugs.launchpad.net/unity/+bug/980942/+attachment/3059914/+files/launcher-accordion-effect-tooltip-bug.ogv.
34
35
36+Test launcher autohide on login
37+------------------------------------
38+
39+Setup:
40+ * Ensure launcher autohide is enabled.
41+
42+Action:
43+ * Log into Unity
44+ * Immediately after the launcher loads, move the mouse over it
45+ * Keep mouse over launcher for more than three seconds
46+ * Move mouse away from launcher
47+
48+Expected results:
49+ * The launcher loads in a revealed (not hidden) state.
50+ * If the user takes no action, the launcher hides after three seconds
51+ * Moving the mouse over the launcher prevents the launcher from hiding
52+ * After the user moves the mouse away, the launcher hides
53+
54+
55 Stop SD card by dragging it's icon to trash
56 ---------------------------------------------------
57