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

[Brandon Schaefer]

SwitcherController: use proper long long types for WindowActiveNumber

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

Backporting lp:~3v1n0/unity/switcher-activenumber-overflow-fix/+merge/164549 to unity-7.0

[Brandon Schaefer (brandontschaefer)]

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

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3321
http://jenkins.qa.ubuntu.com/job/unity-7.0-ci/10/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-armhf-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-i386-ci/11/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-7.0-ci/10/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3321
http://jenkins.qa.ubuntu.com/job/unity-ci/77/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-amd64-ci/81
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-armhf-ci/77
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-raring-i386-ci/77

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/77/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-7.0-autolanding/11/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-amd64-autolanding/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-armhf-autolanding/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-i386-autolanding/11/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-7.0-autolanding/12/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-amd64-autolanding/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-armhf-autolanding/12/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-i386-autolanding/12/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-7.0-autolanding/13/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-amd64-autolanding/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-armhf-autolanding/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-i386-autolanding/13/console

review: Needs Fixing (continuous-integration)

[Andrea Azzarone (azzar1)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-7.0-autolanding/16/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-amd64-autolanding/16/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-armhf-autolanding/16/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-7.0-raring-i386-autolanding/16/console

review: Needs Fixing (continuous-integration)

[Francis Ginther (fginther)]

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

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[Marco Trevisan (Treviño) (3v1n0)]

yay! :)

=== modified file 'launcher/SwitcherController.cpp'
--- launcher/SwitcherController.cpp	2013-04-18 14:04:10 +0000
+++ launcher/SwitcherController.cpp	2013-05-18 01:15:39 +0000
@@ -649,15 +649,15 @@
     return;
   }
 
-  unsigned int first_highest = 0;
-  unsigned int first_second = 0; // first icons second highest active
-  unsigned int second_first = 0; // second icons first highest active
+  unsigned long long first_highest = 0;
+  unsigned long long first_second = 0; // first icons second highest active
+  unsigned long long second_first = 0; // second icons first highest active
 
   WindowManager& wm = WindowManager::Default();
   for (auto& window : first->Windows())
   {
-    guint32 xid = window->window_id();
-    unsigned int num = wm.GetWindowActiveNumber(xid);
+    Window xid = window->window_id();
+    unsigned long long num = wm.GetWindowActiveNumber(xid);
 
     if (num > first_highest)
     {
@@ -670,11 +670,7 @@
     }
   }
 
-  for (auto& window : second->Windows())
-  {
-    guint32 xid = window->window_id();
-    second_first = std::max<unsigned long long>(wm.GetWindowActiveNumber(xid), second_first);
-  }
+  second_first = second->SwitcherPriority();
 
   if (first_second > second_first)
     model_->Select(first);

=== modified file 'tests/test_application_launcher_icon.cpp'
--- tests/test_application_launcher_icon.cpp	2013-04-17 19:16:54 +0000
+++ tests/test_application_launcher_icon.cpp	2013-05-18 01:15:39 +0000
@@ -29,7 +29,6 @@
 #include "FavoriteStore.h"
 #include "StandaloneWindowManager.h"
 #include "mock-application.h"
-#include "StandaloneWindowManager.h"
 #include "test_utils.h"
 
 using namespace testing;

=== modified file 'tests/test_switcher_controller.cpp'
--- tests/test_switcher_controller.cpp	2013-04-17 19:16:54 +0000
+++ tests/test_switcher_controller.cpp	2013-05-18 01:15:39 +0000
@@ -84,7 +84,7 @@
   EXPECT_CALL(*mock_window_, ShowWindow(true, _)).Times(AtLeast(1));
 
   controller_->Show(ShowMode::ALL, SortMode::LAUNCHER_ORDER, icons_);
-  Utils::WaitForTimeoutMSec(200);
+  Utils::WaitUntilMSec([this] { return controller_->Visible(); });
   EXPECT_TRUE(controller_->Visible());
   EXPECT_TRUE(controller_->StartIndex() == 1);
 }
@@ -97,7 +97,7 @@
   ASSERT_TRUE(controller_->IsShowDesktopDisabled());
 
   controller_->Show(ShowMode::ALL, SortMode::LAUNCHER_ORDER, icons_);
-  Utils::WaitForTimeoutMSec(200);
+  Utils::WaitUntilMSec([this] { return controller_->Visible(); });
   ASSERT_TRUE(controller_->StartIndex() == 0);
   Selection selection = controller_->GetCurrentSelection();
   EXPECT_NE(selection.application_->tooltip_text(), "Show Desktop");
@@ -120,6 +120,31 @@
   EXPECT_FALSE(selection.application_);
 }
 
+TEST_F(TestSwitcherController, ShowSwitcherSelectsWindowOfActiveApp)
+{
+  // Making the first application active
+  auto const& first_app = icons_[1];
+  first_app->SetQuirk(launcher::AbstractLauncherIcon::Quirk::ACTIVE, true);
+
+  // Raising the priority of the second window of the first app
+  auto first_app_windows = first_app->Windows();
+  auto first_app_last_window = first_app_windows.back();
+  auto standalone = WM->GetWindowByXid(first_app_last_window->window_id());
+  standalone->active_number = WM->GetWindowActiveNumber(first_app_windows.front()->window_id()) - 1;
+
+  // Setting the active number of the first window of the second app to max uint
+  auto second_app_first_window = icons_[2]->Windows().front();
+  standalone = WM->GetWindowByXid(second_app_first_window->window_id());
+  standalone->active_number = std::numeric_limits<unsigned>::max();
+
+  controller_->Show(ShowMode::CURRENT_VIEWPORT, SortMode::FOCUS_ORDER, icons_);
+  Utils::WaitUntilMSec([this] { return controller_->Visible(); });
+
+  Selection selection = controller_->GetCurrentSelection();
+  ASSERT_EQ(selection.application_, first_app);
+  EXPECT_EQ(selection.window_, first_app_last_window->window_id());
+}
+
 TEST_F(TestSwitcherController, Opacity)
 {
   EXPECT_EQ(controller_->Opacity(), 0.0f);

=== modified file 'tests/test_switcher_controller.h'
--- tests/test_switcher_controller.h	2013-04-17 19:16:54 +0000
+++ tests/test_switcher_controller.h	2013-05-18 01:15:39 +0000
@@ -28,6 +28,7 @@
 #include "test_utils.h"
 #include "DesktopLauncherIcon.h"
 #include "SimpleLauncherIcon.h"
+#include "StandaloneWindowManager.h"
 #include "SwitcherController.h"
 #include "SwitcherView.h"
 #include "TimeUtil.h"
@@ -51,7 +52,7 @@
 class FakeApplicationWindow : public unity::ApplicationWindow
 {
 public:
-  FakeApplicationWindow(Window xid);
+  FakeApplicationWindow(Window xid, unsigned long long active_number = 0);
 
   std::string title() const;
   virtual std::string icon() const;
@@ -62,6 +63,7 @@
   virtual unity::ApplicationPtr application() const;
   virtual bool Focus() const;
   virtual void Quit() const;
+
 private:
   Window xid_;
 };
@@ -69,18 +71,16 @@
 /**
  * A fake LauncherIcon for verifying selection operations of the switcher.
  */
-class FakeLauncherIcon : public unity::launcher::SimpleLauncherIcon
+struct FakeLauncherIcon : unity::launcher::SimpleLauncherIcon
 {
-public:
-  FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned priority);
+  FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned long long priority);
 
-  unity::WindowList Windows()override;
+  unity::WindowList Windows() override;
   bool AllowDetailViewInSwitcher() const override;
   unsigned long long SwitcherPriority() override;
 
-private:
   bool allow_detail_view_;
-  unsigned   priority_;
+  unsigned long long priority_;
   unity::WindowList window_list;
 };
 
@@ -96,6 +96,7 @@
   // required to create hidden secret global variables before test objects
   unity::Settings unity_settings_;
 
+  unity::StandaloneWindowManager* WM;
   nux::animation::TickSource tick_source_;
   nux::animation::AnimationController animation_controller_;
   unity::testmocks::MockBaseWindow::Ptr mock_window_;

=== modified file 'tests/test_switcher_controller_class.cpp'
--- tests/test_switcher_controller_class.cpp	2013-04-17 19:16:54 +0000
+++ tests/test_switcher_controller_class.cpp	2013-05-18 01:15:39 +0000
@@ -25,7 +25,14 @@
 using namespace unity::switcher;
 using namespace std::chrono;
 
-FakeApplicationWindow::FakeApplicationWindow(Window xid) : xid_(xid) {}
+FakeApplicationWindow::FakeApplicationWindow(Window xid, unsigned long long active_number)
+  : xid_(xid)
+{
+  auto WM = dynamic_cast<StandaloneWindowManager*>(&WindowManager::Default());
+  auto standalone_window = std::make_shared<StandaloneWindow>(xid_);
+  standalone_window->active_number = active_number;
+  WM->AddStandaloneWindow(standalone_window);
+}
 
 std::string FakeApplicationWindow::title() const { return "FakeApplicationWindow"; }
 std::string FakeApplicationWindow::icon() const { return ""; }
@@ -35,14 +42,14 @@
 int FakeApplicationWindow::monitor() const { return -1; }
 ApplicationPtr FakeApplicationWindow::application() const { return ApplicationPtr(); }
 bool FakeApplicationWindow::Focus() const { return false; }
-void FakeApplicationWindow::Quit() const {}
+void FakeApplicationWindow::Quit() const { WindowManager::Default().Close(xid_); }
 
-FakeLauncherIcon::FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned priority)
+FakeLauncherIcon::FakeLauncherIcon(std::string const& app_name, bool allow_detail_view, unsigned long long priority)
   : launcher::SimpleLauncherIcon(IconType::APPLICATION)
   , allow_detail_view_(allow_detail_view)
   , priority_(priority)
-  , window_list{ std::make_shared<FakeApplicationWindow>(priority_ | 0x0001),
-                 std::make_shared<FakeApplicationWindow>(priority_ | 0x0002) }
+  , window_list{ std::make_shared<FakeApplicationWindow>(priority_ | 0x0001, SwitcherPriority()),
+                 std::make_shared<FakeApplicationWindow>(priority_ | 0x0002, priority_) }
 {
   tooltip_text = app_name;
 }
@@ -59,7 +66,7 @@
 
 unsigned long long FakeLauncherIcon::SwitcherPriority()
 {
-  return 0xffffffff - priority_;
+  return std::numeric_limits<unsigned long long>::max() - priority_;
 }
 
 /**
@@ -67,7 +74,8 @@
  */
 //class TestSwitcherController : public testing::Test
 TestSwitcherController::TestSwitcherController()
-  : animation_controller_(tick_source_)
+  : WM(dynamic_cast<StandaloneWindowManager*>(&WindowManager::Default()))
+  , animation_controller_(tick_source_)
   , mock_window_(new NiceMock<testmocks::MockBaseWindow>())
 {
   ON_CALL(*mock_window_, SetOpacity(_))

=== modified file 'tests/test_switcher_controller_slow.cpp'
--- tests/test_switcher_controller_slow.cpp	2013-03-19 01:13:43 +0000
+++ tests/test_switcher_controller_slow.cpp	2013-05-18 01:15:39 +0000
@@ -69,17 +69,22 @@
 
   controller_->Next();
   selection = controller_->GetCurrentSelection();
-  EXPECT_EQ(selection.application_->tooltip_text(), "Show Desktop");
-  EXPECT_EQ(selection.window_, 0);
-
-  controller_->Next();
-  selection = controller_->GetCurrentSelection();
-  EXPECT_EQ(selection.application_->tooltip_text(), "First");
+  ASSERT_EQ(selection.application_->tooltip_text(), "Third");
+  EXPECT_EQ(selection.window_, 0);
+
+  controller_->Next();
+  selection = controller_->GetCurrentSelection();
+  ASSERT_EQ(selection.application_->tooltip_text(), "Show Desktop");
+  EXPECT_EQ(selection.window_, 0);
+
+  controller_->Next();
+  selection = controller_->GetCurrentSelection();
+  ASSERT_EQ(selection.application_->tooltip_text(), "First");
   EXPECT_EQ(selection.window_, 0);
 
   Utils::WaitForTimeoutMSec(details_timeout * 1.1);
   selection = controller_->GetCurrentSelection();
-  EXPECT_EQ(selection.application_->tooltip_text(), "First");
+  ASSERT_EQ(selection.application_->tooltip_text(), "First");
   EXPECT_EQ(selection.window_, 0x0101);
 
 

=== modified file 'unity-shared/StandaloneWindowManager.cpp'
--- unity-shared/StandaloneWindowManager.cpp	2013-02-09 16:06:56 +0000
+++ unity-shared/StandaloneWindowManager.cpp	2013-05-18 01:15:39 +0000
@@ -39,6 +39,7 @@
   , geo(nux::Geometry(0, 0, 10, 10))
   , current_desktop(0)
   , monitor(0)
+  , active_number(0)
   , active(false)
   , mapped(true)
   , visible(true)
@@ -480,6 +481,10 @@
 
 unsigned long long StandaloneWindowManager::GetWindowActiveNumber(Window window_id) const
 {
+  auto window = GetWindowByXid(window_id);
+  if (window)
+    return window->active_number;
+
   return 0;
 }
 

=== modified file 'unity-shared/StandaloneWindowManager.h'
--- unity-shared/StandaloneWindowManager.h	2013-02-09 16:06:56 +0000
+++ unity-shared/StandaloneWindowManager.h	2013-05-18 01:15:39 +0000
@@ -44,6 +44,7 @@
   nux::Size deco_sizes[4];
   unsigned current_desktop;
   unsigned monitor;
+  unsigned long long active_number;
   nux::Property<bool> active;
   nux::Property<bool> mapped;
   nux::Property<bool> visible;
@@ -146,6 +147,7 @@
   virtual std::string GetWindowName(Window window_id) const;
 
   // Mock functions
+  StandaloneWindow::Ptr GetWindowByXid(Window window_id) const;
   void AddStandaloneWindow(StandaloneWindow::Ptr const& window);
   std::list<StandaloneWindow::Ptr> GetStandaloneWindows() const;
 
@@ -160,8 +162,6 @@
   virtual void AddProperties(GVariantBuilder* builder);
 
 private:
-  StandaloneWindow::Ptr GetWindowByXid(Window window_id) const;
-
   bool expo_state_;
   bool in_show_desktop_;
   bool scale_active_;