Merge lp:~nick-dedekind/unity/lp923657.dash-dismiss into lp:unity

Proposed by Nick Dedekind on 2012-09-06
Status: Merged
Approved by: Brandon Schaefer on 2012-09-12
Approved revision: 2682
Merged at revision: 2684
Proposed branch: lp:~nick-dedekind/unity/lp923657.dash-dismiss
Merge into: lp:unity
Diff against target: 367 lines (+144/-50)
8 files modified
dash/DashController.cpp (+8/-10)
dash/DashController.h (+2/-1)
launcher/LauncherController.cpp (+1/-1)
manual-tests/Dash.txt (+13/-0)
plugins/unityshell/src/unityshell.cpp (+13/-4)
plugins/unityshell/src/unityshell.h (+1/-1)
tests/autopilot/unity/tests/test_dash.py (+55/-17)
tests/autopilot/unity/tests/test_hud.py (+51/-16)
To merge this branch: bzr merge lp:~nick-dedekind/unity/lp923657.dash-dismiss
Reviewer Review Type Date Requested Status
Brandon Schaefer (community) Approve on 2012-09-12
Christopher Lee (community) Needs Fixing on 2012-09-11
Andrea Azzarone (community) 2012-09-06 Needs Fixing on 2012-09-11
Review via email: mp+123128@code.launchpad.net

Commit message

Fixed dash dismissal when changing focus to a window on monitor external to that of the dash. (LP#923657)
Improved panel update speed when opening dash. (LP#1044086)

Description of the change

Fixed dash dismissal when changing focus to a window on monitor external to that of the dash. (LP#923657)
Improved panel update speed when opening dash. (LP#1044086)

To post a comment you must log in.
Brandon Schaefer (brandontschaefer) wrote :

Looks good, all that is missing are tests!

Andrea Azzarone (azzar1) wrote :

# Open the terminal.
# Run: sleep 10; gnome-calc
# Open the dash.

After 10 seconds, gnome-calc is opened and dash is dismissed. By design window opened without user interaction (see update-manager windows) should not steal focus (or dismiss the dash).

review: Needs Fixing
Nick Dedekind (nick-dedekind) wrote :

Added AP tests.
Also fixed a drawing issue where if you clicked across monitors for dash/hud closure, the dash/hud would intermittently be drawn on the monitor where the mouse is located.

Nick Dedekind (nick-dedekind) wrote :

Right, now the dash will not close on an async app open when the dash is open, but will close when mouse is clicked on another desktop.

Andrea Azzarone (azzar1) wrote :

86 +
87 +Test the Panel does not lose track of when the Dash is opened then closed.
88 +--------------------------------------------------------------------------
89 +This tests shows the panel will not think the Dash is opened when it is closed.
90 +(see lp:1044086)
91 +
92 +Setup:

Please use Actions:

93 +#. ress Super twice (Quickly)
94 +
95 +Expected Result:
96 + The panel should look the same as if you had never opened the dash.
97 +
98 +Wrong Result:
99 + The panel still thinks the dash is open and is blured to match your background.

Also Wrong Result section is not listed in TEST_TEMPLATE.txt

The cpp looks good to me and works here. Ask thomi to review the AP test.

review: Needs Fixing
Christopher Lee (veebers) wrote :

Reviewing the AP test, instead of thomi :)
I'm informed that the merge conflict is being fixed up already.

A couple of minor things:
202 + prev_monitor = None
  - Never actually used
203 + .. range(0, self...
  - The 0 isn't required, start defaults to 0
204 +
  - Extra newline
206 + self.dash.ensure_visible()
  - Please also add self.addCleanup(self.dash.ensure_hidden) to make sure things are cleaned up in case of any issues.

Otherwise looks good.

Christopher Lee (veebers) wrote :

Oops, wrong comment type before :P

review: Needs Fixing
Nick Dedekind (nick-dedekind) wrote :

Not sure how the setup/teaddown work for each test, but don't they run after each test? meaning that ensure_hidden is called after each test.
Anyways, I've added the ensure_hidden cleanup to the tests.

Nick Dedekind (nick-dedekind) wrote :

I can't seem to test the new Hud tests. Keybindings aren't working correctly, when the keypress happens I get some garbage in the stdout and failed test when the hud fails to open.

Can some-one test the hud AP tests for me?

Brandon Schaefer (brandontschaefer) wrote :

These tests pass, the code looks good :). +1.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'dash/DashController.cpp'
2--- dash/DashController.cpp 2012-09-11 12:01:20 +0000
3+++ dash/DashController.cpp 2012-09-12 16:39:23 +0000
4@@ -284,17 +284,8 @@
5 return;
6 }
7
8- /* GetIdealMonitor must get called before visible_ is set */
9 monitor_ = GetIdealMonitor();
10
11- // The launcher must receive UBUS_OVERLAY_SHOW before window_->EnableInputWindow().
12- // Other wise the Launcher gets focus for X, which causes XIM to fail.
13- sources_.AddIdle([this] {
14- GVariant* info = g_variant_new(UBUS_OVERLAY_FORMAT_STRING, "dash", TRUE, monitor_);
15- ubus_manager_.SendMessage(UBUS_OVERLAY_SHOWN, info);
16- return false;
17- });
18-
19 view_->AboutToShow();
20
21 window_->ShowWindow(true);
22@@ -311,6 +302,9 @@
23 visible_ = true;
24
25 StartShowHideTimeline();
26+
27+ GVariant* info = g_variant_new(UBUS_OVERLAY_FORMAT_STRING, "dash", TRUE, monitor_);
28+ ubus_manager_.SendMessage(UBUS_OVERLAY_SHOWN, info);
29 }
30
31 void Controller::HideDash(bool restore)
32@@ -400,6 +394,11 @@
33 .add("monitor", monitor_);
34 }
35
36+bool Controller::IsVisible() const
37+{
38+ return visible_;
39+}
40+
41 void Controller::OnBusAcquired(GObject *obj, GAsyncResult *result, gpointer user_data)
42 {
43 glib::Error error;
44@@ -447,6 +446,5 @@
45 }
46
47
48-
49 }
50 }
51
52=== modified file 'dash/DashController.h'
53--- dash/DashController.h 2012-09-11 10:47:15 +0000
54+++ dash/DashController.h 2012-09-12 16:39:23 +0000
55@@ -59,6 +59,8 @@
56
57 void HideDash(bool restore_focus = true);
58
59+ bool IsVisible() const;
60+
61 protected:
62 std::string GetName() const;
63 void AddProperties(GVariantBuilder* builder);
64@@ -104,7 +106,6 @@
65 sigc::connection screen_ungrabbed_slot_;
66 glib::SignalManager sig_manager_;
67 glib::TimeoutSeconds ensure_timeout_;
68- glib::SourceManager sources_;
69 Animator timeline_animator_;
70 UBusManager ubus_manager_;
71 unsigned int dbus_owner_;
72
73=== modified file 'launcher/LauncherController.cpp'
74--- launcher/LauncherController.cpp 2012-09-04 16:40:30 +0000
75+++ launcher/LauncherController.cpp 2012-09-12 16:39:23 +0000
76@@ -244,7 +244,7 @@
77 {
78 static bool keynav_first_focus = false;
79
80- if (parent_->IsOverlayOpen())
81+ if (parent_->IsOverlayOpen() || launcher_->GetParent()->GetInputWindowId() == xid)
82 keynav_first_focus = false;
83
84 if (keynav_first_focus)
85
86=== modified file 'manual-tests/Dash.txt'
87--- manual-tests/Dash.txt 2012-08-28 12:23:15 +0000
88+++ manual-tests/Dash.txt 2012-09-12 16:39:23 +0000
89@@ -100,6 +100,19 @@
90 * When a single row of results isn't enough to contain all returned search results, and there are no other category headers, the displayed category
91 header expands automatically
92
93+
94+Test the Panel does not lose track of when the Dash is opened then closed.
95+--------------------------------------------------------------------------
96+This tests shows the panel will not think the Dash is opened when it is closed.
97+(see lp:1044086)
98+
99+Actions:
100+#. Press Super twice (Quickly)
101+
102+Expected Result:
103+ The screen should look the same as if you had never opened the dash.
104+
105+
106 Filter Results Tests
107 ========================
108 These tests show that the dash "All" button works well.
109
110=== modified file 'plugins/unityshell/src/unityshell.cpp'
111--- plugins/unityshell/src/unityshell.cpp 2012-09-12 15:24:08 +0000
112+++ plugins/unityshell/src/unityshell.cpp 2012-09-12 16:39:23 +0000
113@@ -358,7 +358,7 @@
114 g_variant_get(data, UBUS_OVERLAY_FORMAT_STRING,
115 &overlay_identity, &can_maximise, &overlay_monitor);
116
117- dash_monitor_ = overlay_monitor;
118+ overlay_monitor_ = overlay_monitor;
119
120 RaiseInputWindows();
121 });
122@@ -569,7 +569,7 @@
123 i++;
124 }
125
126- if (!(launcher_controller_->IsOverlayOpen() && current_monitor == dash_monitor_)
127+ if (!(launcher_controller_->IsOverlayOpen() && current_monitor == overlay_monitor_)
128 && panel_controller_->opacity() > 0.0f)
129 {
130 foreach(GLTexture * tex, _shadow_texture)
131@@ -1420,7 +1420,15 @@
132 if (CompWindow *w = screen->findWindow(ss->getSelectedWindow()))
133 skip_other_plugins = UnityWindow::get(w)->handleEvent(event);
134 }
135-
136+ if (launcher_controller_->IsOverlayOpen())
137+ {
138+ int monitor_with_mouse = UScreen::GetDefault()->GetMonitorWithMouse();
139+ if (overlay_monitor_ != monitor_with_mouse)
140+ {
141+ dash_controller_->HideDash(false);
142+ hud_controller_->HideHud(false);
143+ }
144+ }
145 break;
146 case ButtonRelease:
147 if (switcher_controller_ && switcher_controller_->Visible())
148@@ -2560,7 +2568,8 @@
149 UnityScreen* us = UnityScreen::get(screen);
150 CompWindow *lw;
151
152- if (us->launcher_controller_->IsOverlayOpen())
153+ // can't rely on launcher->IsOverlayVisible on focus change (because ubus is async close on focus change.)
154+ if (us && (us->dash_controller_->IsVisible() || us->hud_controller_->IsVisible()))
155 {
156 lw = screen->findWindow(us->launcher_controller_->LauncherWindowId(0));
157 lw->moveInputFocusTo();
158
159=== modified file 'plugins/unityshell/src/unityshell.h'
160--- plugins/unityshell/src/unityshell.h 2012-09-12 15:24:08 +0000
161+++ plugins/unityshell/src/unityshell.h 2012-09-12 16:39:23 +0000
162@@ -312,7 +312,7 @@
163
164 bool queryForShader ();
165
166- int dash_monitor_;
167+ int overlay_monitor_;
168 CompScreen::GrabHandle grab_index_;
169 CompWindowList fullscreen_windows_;
170 bool painting_tray_;
171
172=== modified file 'tests/autopilot/unity/tests/test_dash.py'
173--- tests/autopilot/unity/tests/test_dash.py 2012-09-11 12:01:20 +0000
174+++ tests/autopilot/unity/tests/test_dash.py 2012-09-12 16:39:23 +0000
175@@ -107,22 +107,6 @@
176 self.dash.reveal_application_lens()
177 self.assertThat(self.dash.active_lens, Eventually(Equals('applications.lens')))
178
179- def test_dash_stays_on_same_monitor(self):
180- """If the dash is opened, then the mouse is moved to another monitor and
181- the keyboard is used. The Dash must not move to that monitor.
182- """
183-
184- if self.screen_geo.get_num_monitors() < 2:
185- self.skip ("This test must be ran with more then 1 monitor.")
186-
187- self.dash.ensure_visible()
188- self.addCleanup(self.dash.ensure_hidden)
189-
190- self.screen_geo.move_mouse_to_monitor(1)
191- self.keyboard.type("abc")
192-
193- self.assertThat(self.dash.ideal_monitor, Eventually(Equals(0)))
194-
195
196 class DashSearchInputTests(DashTestCase):
197 """Test features involving input to the dash search"""
198@@ -435,6 +419,9 @@
199 class DashKeyboardFocusTests(DashTestCase):
200 """Tests that keyboard focus works."""
201
202+ def assertSearchText(self, text):
203+ self.assertThat(self.dash.search_string, Eventually(Equals(text)))
204+
205 def test_filterbar_expansion_leaves_kb_focus(self):
206 """Expanding or collapsing the filterbar must keave keyboard focus in the
207 search bar.
208@@ -447,7 +434,19 @@
209 filter_bar.ensure_expanded()
210 self.addCleanup(filter_bar.ensure_collapsed)
211 self.keyboard.type(" world")
212- self.assertThat(self.dash.search_string, Eventually(Equals("hello world")))
213+ self.assertSearchText("hello world")
214+
215+ def test_keep_focus_on_application_opens(self):
216+ """The Dash must keep key focus as well as stay open if an app gets opened from an external source. """
217+
218+ self.dash.ensure_visible()
219+ self.addCleanup(self.hud.ensure_hidden)
220+
221+ self.start_app_window("Calculator")
222+ sleep(1)
223+
224+ self.keyboard.type("HasFocus")
225+ self.assertSearchText("HasFocus")
226
227
228 class DashLensResultsTests(DashTestCase):
229@@ -847,6 +846,7 @@
230
231 self.assertThat(self.dash.preview_displaying, Eventually(Equals(False)))
232
233+
234 class DashDBusIfaceTests(DashTestCase):
235 """Test the Unity dash DBus interface."""
236
237@@ -856,3 +856,41 @@
238 self.dash.controller.hide_dash_via_dbus()
239 self.assertThat(self.dash.visible, Eventually(Equals(False)))
240 self.dash.ensure_hidden()
241+
242+
243+class DashCrossMonitorsTests(DashTestCase):
244+ """Multi-monitor dash tests."""
245+
246+ def setUp(self):
247+ super(DashCrossMonitorsTests, self).setUp()
248+ if self.screen_geo.get_num_monitors() < 2:
249+ self.skipTest("This test requires more than 1 monitor.")
250+
251+ def test_dash_stays_on_same_monitor(self):
252+ """If the dash is opened, then the mouse is moved to another monitor and
253+ the keyboard is used. The Dash must not move to that monitor.
254+ """
255+ current_monitor = self.dash.ideal_monitor
256+
257+ self.dash.ensure_visible()
258+ self.addCleanup(self.dash.ensure_hidden)
259+
260+ self.screen_geo.move_mouse_to_monitor((current_monitor + 1) % self.screen_geo.get_num_monitors())
261+ self.keyboard.type("abc")
262+
263+ self.assertThat(self.dash.ideal_monitor, Eventually(Equals(current_monitor)))
264+
265+ def test_dash_close_on_cross_monitor_click(self):
266+ """Dash must close when clicking on a window in a different screen."""
267+
268+ self.addCleanup(self.dash.ensure_hidden)
269+
270+ for monitor in range(self.screen_geo.get_num_monitors()-1):
271+ self.screen_geo.move_mouse_to_monitor(monitor)
272+ self.dash.ensure_visible()
273+
274+ self.screen_geo.move_mouse_to_monitor(monitor+1)
275+ sleep(.5)
276+ self.mouse.click()
277+
278+ self.assertThat(self.dash.visible, Eventually(Equals(False)))
279
280=== modified file 'tests/autopilot/unity/tests/test_hud.py'
281--- tests/autopilot/unity/tests/test_hud.py 2012-09-07 17:36:50 +0000
282+++ tests/autopilot/unity/tests/test_hud.py 2012-09-12 16:39:23 +0000
283@@ -357,22 +357,6 @@
284
285 self.assertThat(self.hud.visible, Eventually(Equals(False)))
286
287- def test_hud_stays_on_same_monitor(self):
288- """If the hud is opened, then the mouse is moved to another monitor and
289- the keyboard is used. The hud must not move to that monitor.
290- """
291-
292- if self.screen_geo.get_num_monitors() < 2:
293- self.skip ("This test must be ran with more then 1 monitor.")
294-
295- self.hud.ensure_visible()
296- self.addCleanup(self.hud.ensure_hidden)
297-
298- self.screen_geo.move_mouse_to_monitor(1)
299- self.keyboard.type("abc")
300-
301- self.assertThat(self.hud.ideal_monitor, Eventually(Equals(0)))
302-
303 def test_mouse_changes_selected_hud_button(self):
304 """This tests moves the mouse from the top of the screen to the bottom, this must
305 change the selected button from 1 to 5.
306@@ -408,6 +392,18 @@
307
308 self.assertThat(self.hud.view.selected_button, Eventually(Equals(1)))
309
310+ def test_keep_focus_on_application_opens(self):
311+ """The Hud must keep key focus as well as stay open if an app gets opened from an external source. """
312+
313+ self.hud.ensure_visible()
314+ self.addCleanup(self.hud.ensure_hidden)
315+
316+ self.start_app_window("Calculator")
317+ sleep(1)
318+
319+ self.keyboard.type("HasFocus")
320+ self.assertThat(self.hud.search_string, Eventually(Equals("HasFocus")))
321+
322
323 class HudLauncherInteractionsTests(HudTestsBase):
324
325@@ -661,3 +657,42 @@
326 # Don't use reveal_hud, but be explicit in the keybindings.
327 self.keyboard.press_and_release("Ctrl+Alt+h")
328 self.assertThat(self.hud.visible, Eventually(Equals(True)))
329+
330+
331+class HudCrossMonitorsTests(HudTestsBase):
332+ """Multi-monitor hud tests."""
333+
334+ def setUp(self):
335+ super(HudCrossMonitorsTests, self).setUp()
336+ if self.screen_geo.get_num_monitors() < 2:
337+ self.skipTest("This test requires more than 1 monitor.")
338+
339+ def test_hud_stays_on_same_monitor(self):
340+ """If the hud is opened, then the mouse is moved to another monitor and
341+ the keyboard is used. The hud must not move to that monitor.
342+ """
343+
344+ current_monitor = self.hud.ideal_monitor
345+
346+ self.hud.ensure_visible()
347+ self.addCleanup(self.hud.ensure_hidden)
348+
349+ self.screen_geo.move_mouse_to_monitor((current_monitor + 1) % self.screen_geo.get_num_monitors())
350+ self.keyboard.type("abc")
351+
352+ self.assertThat(self.hud.ideal_monitor, Eventually(Equals(current_monitor)))
353+
354+ def test_hud_close_on_cross_monitor_click(self):
355+ """Hud must close when clicking on a window in a different screen."""
356+
357+ self.addCleanup(self.hud.ensure_hidden)
358+
359+ for monitor in range(self.screen_geo.get_num_monitors()-1):
360+ self.screen_geo.move_mouse_to_monitor(monitor)
361+ self.hud.ensure_visible()
362+
363+ self.screen_geo.move_mouse_to_monitor(monitor+1)
364+ sleep(.5)
365+ self.mouse.click()
366+
367+ self.assertThat(self.hud.visible, Eventually(Equals(False)))