Merge lp:~3v1n0/unity/switcher-activenumber-overflow-fix-7.0 into lp:unity/7.0

Proposed by Brandon Schaefer on 2013-05-18
Status: Merged
Approved by: Francis Ginther on 2013-05-30
Approved revision: 3321
Merged at revision: 3317
Proposed branch: lp:~3v1n0/unity/switcher-activenumber-overflow-fix-7.0
Merge into: lp:unity/7.0
Diff against target: 307 lines (+75/-36)
8 files modified
launcher/SwitcherController.cpp (+6/-10)
tests/test_application_launcher_icon.cpp (+0/-1)
tests/test_switcher_controller.cpp (+27/-2)
tests/test_switcher_controller.h (+8/-7)
tests/test_switcher_controller_class.cpp (+15/-7)
tests/test_switcher_controller_slow.cpp (+12/-7)
unity-shared/StandaloneWindowManager.cpp (+5/-0)
unity-shared/StandaloneWindowManager.h (+2/-2)
To merge this branch: bzr merge lp:~3v1n0/unity/switcher-activenumber-overflow-fix-7.0
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration 2013-05-18 Approve on 2013-05-30
Andrea Azzarone (community) Approve on 2013-05-22
Brandon Schaefer (community) Approve on 2013-05-18
Review via email: mp+164564@code.launchpad.net

This proposal supersedes a proposal from 2013-05-17.

Commit message

SwitcherController: use proper long long types for WindowActiveNumber

Otherwise an overflow can cause bad window selection after long uptime.

Description of the change

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

LGTM. I resubmitted to unity/7.0 for you :)

review: Approve
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Andrea Azzarone (azzar1) :
review: Approve
Francis Ginther (fginther) wrote :

The build environment has been cleaned-up (a local archive was in use). Retrying this to see if that was the issue.

review: Approve (continuous-integration)
Marco Trevisan (Treviño) (3v1n0) wrote :

yay! :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/SwitcherController.cpp'
2--- launcher/SwitcherController.cpp 2013-04-18 14:04:10 +0000
3+++ launcher/SwitcherController.cpp 2013-05-18 01:15:39 +0000
4@@ -649,15 +649,15 @@
5 return;
6 }
7
8- unsigned int first_highest = 0;
9- unsigned int first_second = 0; // first icons second highest active
10- unsigned int second_first = 0; // second icons first highest active
11+ unsigned long long first_highest = 0;
12+ unsigned long long first_second = 0; // first icons second highest active
13+ unsigned long long second_first = 0; // second icons first highest active
14
15 WindowManager& wm = WindowManager::Default();
16 for (auto& window : first->Windows())
17 {
18- guint32 xid = window->window_id();
19- unsigned int num = wm.GetWindowActiveNumber(xid);
20+ Window xid = window->window_id();
21+ unsigned long long num = wm.GetWindowActiveNumber(xid);
22
23 if (num > first_highest)
24 {
25@@ -670,11 +670,7 @@
26 }
27 }
28
29- for (auto& window : second->Windows())
30- {
31- guint32 xid = window->window_id();
32- second_first = std::max<unsigned long long>(wm.GetWindowActiveNumber(xid), second_first);
33- }
34+ second_first = second->SwitcherPriority();
35
36 if (first_second > second_first)
37 model_->Select(first);
38
39=== modified file 'tests/test_application_launcher_icon.cpp'
40--- tests/test_application_launcher_icon.cpp 2013-04-17 19:16:54 +0000
41+++ tests/test_application_launcher_icon.cpp 2013-05-18 01:15:39 +0000
42@@ -29,7 +29,6 @@
43 #include "FavoriteStore.h"
44 #include "StandaloneWindowManager.h"
45 #include "mock-application.h"
46-#include "StandaloneWindowManager.h"
47 #include "test_utils.h"
48
49 using namespace testing;
50
51=== modified file 'tests/test_switcher_controller.cpp'
52--- tests/test_switcher_controller.cpp 2013-04-17 19:16:54 +0000
53+++ tests/test_switcher_controller.cpp 2013-05-18 01:15:39 +0000
54@@ -84,7 +84,7 @@
55 EXPECT_CALL(*mock_window_, ShowWindow(true, _)).Times(AtLeast(1));
56
57 controller_->Show(ShowMode::ALL, SortMode::LAUNCHER_ORDER, icons_);
58- Utils::WaitForTimeoutMSec(200);
59+ Utils::WaitUntilMSec([this] { return controller_->Visible(); });
60 EXPECT_TRUE(controller_->Visible());
61 EXPECT_TRUE(controller_->StartIndex() == 1);
62 }
63@@ -97,7 +97,7 @@
64 ASSERT_TRUE(controller_->IsShowDesktopDisabled());
65
66 controller_->Show(ShowMode::ALL, SortMode::LAUNCHER_ORDER, icons_);
67- Utils::WaitForTimeoutMSec(200);
68+ Utils::WaitUntilMSec([this] { return controller_->Visible(); });
69 ASSERT_TRUE(controller_->StartIndex() == 0);
70 Selection selection = controller_->GetCurrentSelection();
71 EXPECT_NE(selection.application_->tooltip_text(), "Show Desktop");
72@@ -120,6 +120,31 @@
73 EXPECT_FALSE(selection.application_);
74 }
75
76+TEST_F(TestSwitcherController, ShowSwitcherSelectsWindowOfActiveApp)
77+{
78+ // Making the first application active
79+ auto const& first_app = icons_[1];
80+ first_app->SetQuirk(launcher::AbstractLauncherIcon::Quirk::ACTIVE, true);
81+
82+ // Raising the priority of the second window of the first app
83+ auto first_app_windows = first_app->Windows();
84+ auto first_app_last_window = first_app_windows.back();
85+ auto standalone = WM->GetWindowByXid(first_app_last_window->window_id());
86+ standalone->active_number = WM->GetWindowActiveNumber(first_app_windows.front()->window_id()) - 1;
87+
88+ // Setting the active number of the first window of the second app to max uint
89+ auto second_app_first_window = icons_[2]->Windows().front();
90+ standalone = WM->GetWindowByXid(second_app_first_window->window_id());
91+ standalone->active_number = std::numeric_limits<unsigned>::max();
92+
93+ controller_->Show(ShowMode::CURRENT_VIEWPORT, SortMode::FOCUS_ORDER, icons_);
94+ Utils::WaitUntilMSec([this] { return controller_->Visible(); });
95+
96+ Selection selection = controller_->GetCurrentSelection();
97+ ASSERT_EQ(selection.application_, first_app);
98+ EXPECT_EQ(selection.window_, first_app_last_window->window_id());
99+}
100+
101 TEST_F(TestSwitcherController, Opacity)
102 {
103 EXPECT_EQ(controller_->Opacity(), 0.0f);
104
105=== modified file 'tests/test_switcher_controller.h'
106--- tests/test_switcher_controller.h 2013-04-17 19:16:54 +0000
107+++ tests/test_switcher_controller.h 2013-05-18 01:15:39 +0000
108@@ -28,6 +28,7 @@
109 #include "test_utils.h"
110 #include "DesktopLauncherIcon.h"
111 #include "SimpleLauncherIcon.h"
112+#include "StandaloneWindowManager.h"
113 #include "SwitcherController.h"
114 #include "SwitcherView.h"
115 #include "TimeUtil.h"
116@@ -51,7 +52,7 @@
117 class FakeApplicationWindow : public unity::ApplicationWindow
118 {
119 public:
120- FakeApplicationWindow(Window xid);
121+ FakeApplicationWindow(Window xid, unsigned long long active_number = 0);
122
123 std::string title() const;
124 virtual std::string icon() const;
125@@ -62,6 +63,7 @@
126 virtual unity::ApplicationPtr application() const;
127 virtual bool Focus() const;
128 virtual void Quit() const;
129+
130 private:
131 Window xid_;
132 };
133@@ -69,18 +71,16 @@
134 /**
135 * A fake LauncherIcon for verifying selection operations of the switcher.
136 */
137-class FakeLauncherIcon : public unity::launcher::SimpleLauncherIcon
138+struct FakeLauncherIcon : unity::launcher::SimpleLauncherIcon
139 {
140-public:
141- FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned priority);
142+ FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned long long priority);
143
144- unity::WindowList Windows()override;
145+ unity::WindowList Windows() override;
146 bool AllowDetailViewInSwitcher() const override;
147 unsigned long long SwitcherPriority() override;
148
149-private:
150 bool allow_detail_view_;
151- unsigned priority_;
152+ unsigned long long priority_;
153 unity::WindowList window_list;
154 };
155
156@@ -96,6 +96,7 @@
157 // required to create hidden secret global variables before test objects
158 unity::Settings unity_settings_;
159
160+ unity::StandaloneWindowManager* WM;
161 nux::animation::TickSource tick_source_;
162 nux::animation::AnimationController animation_controller_;
163 unity::testmocks::MockBaseWindow::Ptr mock_window_;
164
165=== modified file 'tests/test_switcher_controller_class.cpp'
166--- tests/test_switcher_controller_class.cpp 2013-04-17 19:16:54 +0000
167+++ tests/test_switcher_controller_class.cpp 2013-05-18 01:15:39 +0000
168@@ -25,7 +25,14 @@
169 using namespace unity::switcher;
170 using namespace std::chrono;
171
172-FakeApplicationWindow::FakeApplicationWindow(Window xid) : xid_(xid) {}
173+FakeApplicationWindow::FakeApplicationWindow(Window xid, unsigned long long active_number)
174+ : xid_(xid)
175+{
176+ auto WM = dynamic_cast<StandaloneWindowManager*>(&WindowManager::Default());
177+ auto standalone_window = std::make_shared<StandaloneWindow>(xid_);
178+ standalone_window->active_number = active_number;
179+ WM->AddStandaloneWindow(standalone_window);
180+}
181
182 std::string FakeApplicationWindow::title() const { return "FakeApplicationWindow"; }
183 std::string FakeApplicationWindow::icon() const { return ""; }
184@@ -35,14 +42,14 @@
185 int FakeApplicationWindow::monitor() const { return -1; }
186 ApplicationPtr FakeApplicationWindow::application() const { return ApplicationPtr(); }
187 bool FakeApplicationWindow::Focus() const { return false; }
188-void FakeApplicationWindow::Quit() const {}
189+void FakeApplicationWindow::Quit() const { WindowManager::Default().Close(xid_); }
190
191-FakeLauncherIcon::FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned priority)
192+FakeLauncherIcon::FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned long long priority)
193 : launcher::SimpleLauncherIcon(IconType::APPLICATION)
194 , allow_detail_view_(allow_detail_view)
195 , priority_(priority)
196- , window_list{ std::make_shared<FakeApplicationWindow>(priority_ | 0x0001),
197- std::make_shared<FakeApplicationWindow>(priority_ | 0x0002) }
198+ , window_list{ std::make_shared<FakeApplicationWindow>(priority_ | 0x0001, SwitcherPriority()),
199+ std::make_shared<FakeApplicationWindow>(priority_ | 0x0002, priority_) }
200 {
201 tooltip_text = app_name;
202 }
203@@ -59,7 +66,7 @@
204
205 unsigned long long FakeLauncherIcon::SwitcherPriority()
206 {
207- return 0xffffffff - priority_;
208+ return std::numeric_limits<unsigned long long>::max() - priority_;
209 }
210
211 /**
212@@ -67,7 +74,8 @@
213 */
214 //class TestSwitcherController : public testing::Test
215 TestSwitcherController::TestSwitcherController()
216- : animation_controller_(tick_source_)
217+ : WM(dynamic_cast<StandaloneWindowManager*>(&WindowManager::Default()))
218+ , animation_controller_(tick_source_)
219 , mock_window_(new NiceMock<testmocks::MockBaseWindow>())
220 {
221 ON_CALL(*mock_window_, SetOpacity(_))
222
223=== modified file 'tests/test_switcher_controller_slow.cpp'
224--- tests/test_switcher_controller_slow.cpp 2013-03-19 01:13:43 +0000
225+++ tests/test_switcher_controller_slow.cpp 2013-05-18 01:15:39 +0000
226@@ -69,17 +69,22 @@
227
228 controller_->Next();
229 selection = controller_->GetCurrentSelection();
230- EXPECT_EQ(selection.application_->tooltip_text(), "Show Desktop");
231- EXPECT_EQ(selection.window_, 0);
232-
233- controller_->Next();
234- selection = controller_->GetCurrentSelection();
235- EXPECT_EQ(selection.application_->tooltip_text(), "First");
236+ ASSERT_EQ(selection.application_->tooltip_text(), "Third");
237+ EXPECT_EQ(selection.window_, 0);
238+
239+ controller_->Next();
240+ selection = controller_->GetCurrentSelection();
241+ ASSERT_EQ(selection.application_->tooltip_text(), "Show Desktop");
242+ EXPECT_EQ(selection.window_, 0);
243+
244+ controller_->Next();
245+ selection = controller_->GetCurrentSelection();
246+ ASSERT_EQ(selection.application_->tooltip_text(), "First");
247 EXPECT_EQ(selection.window_, 0);
248
249 Utils::WaitForTimeoutMSec(details_timeout * 1.1);
250 selection = controller_->GetCurrentSelection();
251- EXPECT_EQ(selection.application_->tooltip_text(), "First");
252+ ASSERT_EQ(selection.application_->tooltip_text(), "First");
253 EXPECT_EQ(selection.window_, 0x0101);
254
255
256
257=== modified file 'unity-shared/StandaloneWindowManager.cpp'
258--- unity-shared/StandaloneWindowManager.cpp 2013-02-09 16:06:56 +0000
259+++ unity-shared/StandaloneWindowManager.cpp 2013-05-18 01:15:39 +0000
260@@ -39,6 +39,7 @@
261 , geo(nux::Geometry(0, 0, 10, 10))
262 , current_desktop(0)
263 , monitor(0)
264+ , active_number(0)
265 , active(false)
266 , mapped(true)
267 , visible(true)
268@@ -480,6 +481,10 @@
269
270 unsigned long long StandaloneWindowManager::GetWindowActiveNumber(Window window_id) const
271 {
272+ auto window = GetWindowByXid(window_id);
273+ if (window)
274+ return window->active_number;
275+
276 return 0;
277 }
278
279
280=== modified file 'unity-shared/StandaloneWindowManager.h'
281--- unity-shared/StandaloneWindowManager.h 2013-02-09 16:06:56 +0000
282+++ unity-shared/StandaloneWindowManager.h 2013-05-18 01:15:39 +0000
283@@ -44,6 +44,7 @@
284 nux::Size deco_sizes[4];
285 unsigned current_desktop;
286 unsigned monitor;
287+ unsigned long long active_number;
288 nux::Property<bool> active;
289 nux::Property<bool> mapped;
290 nux::Property<bool> visible;
291@@ -146,6 +147,7 @@
292 virtual std::string GetWindowName(Window window_id) const;
293
294 // Mock functions
295+ StandaloneWindow::Ptr GetWindowByXid(Window window_id) const;
296 void AddStandaloneWindow(StandaloneWindow::Ptr const& window);
297 std::list<StandaloneWindow::Ptr> GetStandaloneWindows() const;
298
299@@ -160,8 +162,6 @@
300 virtual void AddProperties(GVariantBuilder* builder);
301
302 private:
303- StandaloneWindow::Ptr GetWindowByXid(Window window_id) const;
304-
305 bool expo_state_;
306 bool in_show_desktop_;
307 bool scale_active_;

Subscribers

People subscribed via source and target branches