Merge lp:~azzar1/unity/fix-1044681 into lp:unity

[Andrea Azzarone]

Refactor unity::Settings to use nux::RWProperty.

== Problem ==
Bug #1044681: [regression] Minimizing a window does not give focus to the window behind

== Fix ==
Refactored unity::Settings to use RWProperty.

== Test ==
Unit tests added for unity::Settings.

[Omer Akram (om26er)]

fix works for me.

review: Approve (functional)

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1334/console reported an error when processing this lp:~andyrock/unity/fix-1044681 branch.
Not merging it.

=== modified file 'dash/DashController.cpp'
--- dash/DashController.cpp	2012-09-17 10:00:38 +0000
+++ dash/DashController.cpp	2012-09-18 13:56:19 +0000
@@ -74,9 +74,9 @@
 
   SetupWindow();
 
-  Settings::Instance().changed.connect([&]()
+  Settings::Instance().form_factor.changed.connect([this](FormFactor)
   {
-    if (window_ && view_)
+    if (window_ && view_  && visible_)
     {
       window_->PushToFront();
       window_->SetInputFocus();

=== modified file 'dash/DashView.cpp'
--- dash/DashView.cpp	2012-09-18 09:05:50 +0000
+++ dash/DashView.cpp	2012-09-18 13:56:19 +0000
@@ -98,7 +98,10 @@
   SetupViews();
   SetupUBusConnections();
 
-  Settings::Instance().changed.connect(sigc::mem_fun(this, &DashView::Relayout));
+  Settings::Instance().form_factor.changed.connect([this](FormFactor) {
+    Relayout();
+  });
+
   lenses_.lens_added.connect(sigc::mem_fun(this, &DashView::OnLensAdded));
   mouse_down.connect(sigc::mem_fun(this, &DashView::OnMouseButtonDown));
   preview_state_machine_.PreviewActivated.connect(sigc::mem_fun(this, &DashView::BuildPreview));
@@ -337,7 +340,6 @@
       content_geo_ = geo;
   }
 
-
   // kinda hacky, but it makes sure the content isn't so big that it throws
   // the bottom of the dash off the screen
   // not hugely happy with this, so FIXME
@@ -422,7 +424,7 @@
   dash::Style& style = dash::Style::Instance();
   nux::Geometry geo(content_geo_);
 
-  if (Settings::Instance().GetFormFactor() == FormFactor::DESKTOP)
+  if (Settings::Instance().form_factor() == FormFactor::DESKTOP)
   {
     geo.width += style.GetDashRightTileWidth();
     geo.height += style.GetDashBottomTileHeight();
@@ -854,11 +856,11 @@
 
   std::string form_factor("unknown");
 
-  if (Settings::Instance().GetFormFactor() == FormFactor::NETBOOK)
+  if (Settings::Instance().form_factor() == FormFactor::NETBOOK)
     form_factor = "netbook";
-  else if (Settings::Instance().GetFormFactor() == FormFactor::DESKTOP)
+  else if (Settings::Instance().form_factor() == FormFactor::DESKTOP)
     form_factor = "desktop";
-  else if (Settings::Instance().GetFormFactor() == FormFactor::TV)
+  else if (Settings::Instance().form_factor() == FormFactor::TV)
     form_factor = "tv";
 
   unity::variant::BuilderWrapper wrapper(builder);

=== modified file 'panel/WindowButtons.cpp'
--- panel/WindowButtons.cpp	2012-07-06 22:36:27 +0000
+++ panel/WindowButtons.cpp	2012-09-18 13:56:19 +0000
@@ -395,7 +395,7 @@
 
   ubus_manager_.RegisterInterest(UBUS_OVERLAY_SHOWN, sigc::mem_fun(this, &WindowButtons::OnOverlayShown));
   ubus_manager_.RegisterInterest(UBUS_OVERLAY_HIDDEN, sigc::mem_fun(this, &WindowButtons::OnOverlayHidden));
-  Settings::Instance().changed.connect(sigc::mem_fun(this, &WindowButtons::OnDashSettingsUpdated));
+  Settings::Instance().form_factor.changed.connect(sigc::mem_fun(this, &WindowButtons::OnDashSettingsUpdated));
 }
 
 nux::Area* WindowButtons::FindAreaUnderMouse(const nux::Point& mouse, nux::NuxEventType event_type)
@@ -470,7 +470,7 @@
 
   if (win_button->IsOverlayOpen())
   {
-    Settings::Instance().SetFormFactor(FormFactor::DESKTOP);
+    Settings::Instance().form_factor = FormFactor::DESKTOP;
   }
   else
   {
@@ -494,7 +494,7 @@
 
   if (win_button->IsOverlayOpen())
   {
-    Settings::Instance().SetFormFactor(FormFactor::NETBOOK);
+    Settings::Instance().form_factor = FormFactor::NETBOOK;
   }
 
   maximize_clicked.emit();
@@ -550,7 +550,7 @@
   if (restore_button && maximize_button)
   {
     Settings &dash_settings = Settings::Instance();
-    bool maximizable = (dash_settings.GetFormFactor() == FormFactor::DESKTOP);
+    bool maximizable = (dash_settings.form_factor() == FormFactor::DESKTOP);
 
     restore_button->SetEnabled(can_maximise);
     maximize_button->SetEnabled(can_maximise);
@@ -645,7 +645,7 @@
   }
 }
 
-void WindowButtons::OnDashSettingsUpdated()
+void WindowButtons::OnDashSettingsUpdated(FormFactor form_factor)
 {
   WindowButton* maximize_button = nullptr;
   WindowButton* restore_button = nullptr;
@@ -672,8 +672,7 @@
 
   if (restore_button && restore_button->IsOverlayOpen() && maximize_button)
   {
-    Settings &dash_settings = Settings::Instance();
-    bool maximizable = (dash_settings.GetFormFactor() == FormFactor::DESKTOP);
+    bool maximizable = (form_factor == FormFactor::DESKTOP);
 
     if (maximizable != maximize_button->IsVisible())
     {

=== modified file 'panel/WindowButtons.h'
--- panel/WindowButtons.h	2012-05-06 23:48:38 +0000
+++ panel/WindowButtons.h	2012-09-18 13:56:19 +0000
@@ -26,6 +26,7 @@
 
 #include "unity-shared/UBusWrapper.h"
 #include "unity-shared/Introspectable.h"
+#include "unity-shared/UnitySettings.h"
 
 namespace unity
 {
@@ -70,7 +71,7 @@
   void OnMaximizeClicked(nux::Button *button);
   void OnOverlayShown(GVariant* data);
   void OnOverlayHidden(GVariant* data);
-  void OnDashSettingsUpdated();
+  void OnDashSettingsUpdated(FormFactor form_factor);
 
   int monitor_;
   double opacity_;

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2012-09-17 14:07:30 +0000
+++ tests/CMakeLists.txt	2012-09-18 13:56:19 +0000
@@ -231,6 +231,7 @@
                  test_texture_cache.cpp
                  test_thumbnail_generator.cpp
                  test_launcher_minimize_speed.cpp
+                 test_unity_settings.cpp
                  test_volume_imp.cpp
                  test_volume_launcher_icon.cpp
                  gmockmount.c

=== added file 'tests/test_unity_settings.cpp'
--- tests/test_unity_settings.cpp	1970-01-01 00:00:00 +0000
+++ tests/test_unity_settings.cpp	2012-09-18 13:56:19 +0000
@@ -0,0 +1,110 @@
+/*
+ * Copyright 2012 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the  Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 3 along with this program.  If not, see
+ * <http://www.gnu.org/licenses/>
+ *
+ * Authored by: Andrea Azzarone <[e-mail]>
+ *
+ */
+
+#include <gio/gio.h>
+#include <gtest/gtest.h>
+
+#include "test_utils.h"
+#include "unity-shared/UnitySettings.h"
+
+#include <UnityCore/GLibWrapper.h>
+
+namespace
+{
+
+class TestUnitySettings : public testing::Test
+{
+public:
+  unity::glib::Object<GSettings> gsettings;
+  std::unique_ptr<unity::Settings> unity_settings;
+
+  void SetUp()
+  {
+    g_setenv("GSETTINGS_BACKEND", "memory", TRUE);
+
+    gsettings = g_settings_new("com.canonical.Unity");
+    g_settings_set_enum(gsettings, "form-factor", static_cast<int>(unity::FormFactor::DESKTOP));
+
+    unity_settings.reset(new unity::Settings);
+  }
+
+  void TearDown()
+  {
+    g_setenv("GSETTINGS_BACKEND", "", TRUE);
+  }
+};
+
+TEST_F(TestUnitySettings, SetFormFactor)
+{
+  unity_settings->form_factor = unity::FormFactor::NETBOOK;
+
+  int raw_form_factor = g_settings_get_enum(gsettings, "form-factor");
+  EXPECT_EQ(raw_form_factor, static_cast<int>(unity::FormFactor::NETBOOK));
+}
+
+TEST_F(TestUnitySettings, GetFormFactor)
+{
+  EXPECT_EQ(unity_settings->form_factor(), unity::FormFactor::DESKTOP);
+
+  g_settings_set_enum(gsettings, "form-factor", static_cast<int>(unity::FormFactor::NETBOOK));
+  EXPECT_EQ(unity_settings->form_factor(), unity::FormFactor::NETBOOK);
+}
+
+TEST_F(TestUnitySettings, FormFactorChangedSignal_Extern)
+{
+  bool signal_received = false;
+  unity::FormFactor new_form_factor;
+  unity_settings->form_factor.changed.connect([&](unity::FormFactor form_factor) {
+    signal_received = true;
+    new_form_factor = form_factor;
+  });
+
+  g_settings_set_enum(gsettings, "form-factor", static_cast<int>(unity::FormFactor::NETBOOK));
+  Utils::WaitUntil(signal_received);
+  EXPECT_EQ(new_form_factor, unity::FormFactor::NETBOOK);
+}
+
+TEST_F(TestUnitySettings, FormFactorChangedSignal_Extern_OtherKeys)
+{
+  bool signal_received = false;
+  unity_settings->form_factor.changed.connect([&](unity::FormFactor form_factor) {
+    signal_received = true;
+  });
+
+  g_settings_set_int(gsettings, "minimize-count", 0);
+  Utils::WaitForTimeout(1);
+  EXPECT_FALSE(signal_received);
+}
+
+TEST_F(TestUnitySettings, FormFactorChangedSignal_Inter)
+{
+  bool signal_received = false;
+  unity::FormFactor new_form_factor;
+  unity_settings->form_factor.changed.connect([&](unity::FormFactor form_factor) {
+    signal_received = true;
+    new_form_factor = form_factor;
+  });
+
+  unity_settings->form_factor = unity::FormFactor::NETBOOK;
+  Utils::WaitUntil(signal_received);
+  EXPECT_EQ(new_form_factor, unity::FormFactor::NETBOOK);
+}
+
+}

=== modified file 'unity-shared/DashStyle.cpp'
--- unity-shared/DashStyle.cpp	2012-09-16 23:45:17 +0000
+++ unity-shared/DashStyle.cpp	2012-09-18 13:56:19 +0000
@@ -427,14 +427,14 @@
     style_instance = this;
   }
 
-  auto formfactor_lambda = [this] () 
+  auto formfactor_lambda = [this] (FormFactor)
   {
-    FormFactor formfactor = Settings::Instance().GetFormFactor();
+    FormFactor formfactor = Settings::Instance().form_factor();
     always_maximised = (formfactor == FormFactor::NETBOOK || formfactor == FormFactor::TV); 
   };
 
-  Settings::Instance().changed.connect(formfactor_lambda);
-  formfactor_lambda();
+  Settings::Instance().form_factor.changed.connect(formfactor_lambda);
+  formfactor_lambda(FormFactor());
 }
 
 Style::~Style ()

=== modified file 'unity-shared/OverlayRenderer.cpp'
--- unity-shared/OverlayRenderer.cpp	2012-09-14 10:26:33 +0000
+++ unity-shared/OverlayRenderer.cpp	2012-09-18 13:56:19 +0000
@@ -438,7 +438,7 @@
   bool paint_blur = BackgroundEffectHelper::blur_type != BLUR_NONE;
   nux::Geometry geo(content_geo);
 
-  int excess_border = (Settings::Instance().GetFormFactor() != FormFactor::NETBOOK || force_edges) ? EXCESS_BORDER : 0;
+  int excess_border = (Settings::Instance().form_factor() != FormFactor::NETBOOK || force_edges) ? EXCESS_BORDER : 0;
 
   nux::Geometry larger_content_geo = content_geo;
   larger_content_geo.OffsetSize(excess_border, excess_border);
@@ -578,7 +578,7 @@
   }
 
 
-  if (Settings::Instance().GetFormFactor() != FormFactor::NETBOOK || force_edges)
+  if (Settings::Instance().form_factor() != FormFactor::NETBOOK || force_edges)
   {
     // Paint the edges
     {
@@ -835,7 +835,7 @@
   nux::Geometry geo = geometry;
   bgs = 0;
 
-  int excess_border = (Settings::Instance().GetFormFactor() != FormFactor::NETBOOK) ? EXCESS_BORDER : 0;
+  int excess_border = (Settings::Instance().form_factor() != FormFactor::NETBOOK) ? EXCESS_BORDER : 0;
 
   nux::Geometry larger_content_geo = content_geo;
   larger_content_geo.OffsetSize(excess_border, excess_border);

=== modified file 'unity-shared/PanelStyle.cpp'
--- unity-shared/PanelStyle.cpp	2012-08-29 18:29:18 +0000
+++ unity-shared/PanelStyle.cpp	2012-09-18 13:56:19 +0000
@@ -73,12 +73,12 @@
     style_instance = this;
   }
 
-  if (Settings::Instance().GetFormFactor() == FormFactor::TV)
+  if (Settings::Instance().form_factor() == FormFactor::TV)
     panel_height = 0;
   
-  Settings::Instance().changed.connect([this]() 
+  Settings::Instance().form_factor.changed.connect([this](FormFactor form_factor)
   {
-    if (Settings::Instance().GetFormFactor() == FormFactor::TV)
+    if (form_factor == FormFactor::TV)
       panel_height = 0;
   });
 

=== modified file 'unity-shared/UnitySettings.cpp'
--- unity-shared/UnitySettings.cpp	2012-05-22 14:33:41 +0000
+++ unity-shared/UnitySettings.cpp	2012-09-18 13:56:19 +0000
@@ -1,6 +1,6 @@
 // -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
 /*
-* Copyright (C) 2010, 2011 Canonical Ltd
+* Copyright (C) 2010, 2011, 2012 Canonical Ltd
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
@@ -15,6 +15,7 @@
 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 *
 * Authored by: Neil Jagdish Patel <[e-mail]>
+*              Andrea Azzarone <[e-mail]>
 */
 
 #include <gdk/gdk.h>
@@ -32,81 +33,69 @@
 nux::logging::Logger logger("unity");
 
 Settings* settings_instance = nullptr;
-const char* const FORM_FACTOR = "form-factor";
+
+const std::string SETTINGS_NAME = "com.canonical.Unity";
+const std::string FORM_FACTOR = "form-factor";
 }
 
+//
+// Start private implementation
+//
 class Settings::Impl
 {
 public:
-  Impl(Settings* owner);
-  ~Impl();
-
-  FormFactor GetFormFactor() const;
-  void SetFormFactor(FormFactor factor);
-
-private:
-  void Refresh();
-  static void Changed(GSettings* settings, gchar* key, Impl* self);
-
-private:
-  Settings* owner_;
-  GSettings* settings_;
-  FormFactor form_factor_;
+  Impl(Settings* owner)
+    : parent_(owner)
+    , gsettings_(g_settings_new(SETTINGS_NAME.c_str()))
+    , cached_form_factor_(FormFactor::DESKTOP)
+  {
+    CacheFormFactor();
+
+    form_factor_changed_.Connect(gsettings_, "changed::" + FORM_FACTOR, [this] (GSettings*, gchar*) {
+      CacheFormFactor();
+      parent_->form_factor.changed.emit(cached_form_factor_);
+    });
+  }
+
+  void CacheFormFactor()
+  {
+    int raw_from_factor = g_settings_get_enum(gsettings_, FORM_FACTOR.c_str());
+
+    if (raw_from_factor == 0) //Automatic
+    {
+      auto uscreen = UScreen::GetDefault();
+      int primary_monitor = uscreen->GetMonitorWithMouse();
+      auto const& geo = uscreen->GetMonitorGeometry(primary_monitor);
+
+      cached_form_factor_ = geo.height > 799 ? FormFactor::DESKTOP : FormFactor::NETBOOK;
+    }
+    else
+    {
+      cached_form_factor_ = static_cast<FormFactor>(raw_from_factor);
+    }
+  }
+
+  FormFactor GetFormFactor() const
+  {
+    return cached_form_factor_;
+  }
+
+  bool SetFormFactor(FormFactor factor)
+  {
+    g_settings_set_enum(gsettings_, FORM_FACTOR.c_str(), static_cast<int>(factor));
+    return true;
+  }
+
+  Settings* parent_;
+  glib::Object<GSettings> gsettings_;
+  FormFactor cached_form_factor_;
+
+  glib::Signal<void, GSettings*, gchar* > form_factor_changed_;
 };
 
-
-Settings::Impl::Impl(Settings* owner)
-  : owner_(owner)
-  , settings_(nullptr)
-  , form_factor_(FormFactor::DESKTOP)
-{
-  settings_ = g_settings_new("com.canonical.Unity");
-  g_signal_connect(settings_, "changed",
-                   (GCallback)(Impl::Changed), this);
-  Refresh();
-}
-
-Settings::Impl::~Impl()
-{
-  g_object_unref(settings_);
-}
-
-void Settings::Impl::Refresh()
-{
-  int raw_from_factor = g_settings_get_enum(settings_, FORM_FACTOR);
-
-  if (raw_from_factor == 0) //Automatic
-  {
-    UScreen *uscreen = UScreen::GetDefault();
-    int primary_monitor = uscreen->GetMonitorWithMouse();
-    auto geo = uscreen->GetMonitorGeometry(primary_monitor);
-
-    form_factor_ = geo.height > 799 ? FormFactor::DESKTOP : FormFactor::NETBOOK;
-  }
-  else
-  {
-    form_factor_ = static_cast<FormFactor>(raw_from_factor);
-  }
-
-  owner_->changed.emit();
-}
-
-void Settings::Impl::Changed(GSettings* settings, char* key, Impl* self)
-{
-  self->Refresh();
-}
-
-FormFactor Settings::Impl::GetFormFactor() const
-{
-  return form_factor_;
-}
-
-void Settings::Impl::SetFormFactor(FormFactor factor)
-{
-  form_factor_ = factor;
-  g_settings_set_enum(settings_, FORM_FACTOR, static_cast<int>(factor));
-  owner_->changed.emit();
-}
+//
+// End private implementation
+//
 
 Settings::Settings()
   : is_standalone(false)
@@ -118,13 +107,15 @@
   }
   else
   {
+    form_factor.SetGetterFunction(sigc::mem_fun(*pimpl, &Impl::GetFormFactor));
+    form_factor.SetSetterFunction(sigc::mem_fun(*pimpl, &Impl::SetFormFactor));
+
     settings_instance = this;
   }
 }
 
 Settings::~Settings()
 {
-  delete pimpl;
   if (settings_instance == this)
     settings_instance = nullptr;
 }
@@ -139,15 +130,5 @@
   return *settings_instance;
 }
 
-FormFactor Settings::GetFormFactor() const
-{
-  return pimpl->GetFormFactor();
-}
-
-void Settings::SetFormFactor(FormFactor factor)
-{
-  pimpl->SetFormFactor(factor);
-}
-
 
 } // namespace unity

=== modified file 'unity-shared/UnitySettings.h'
--- unity-shared/UnitySettings.h	2012-05-22 14:33:41 +0000
+++ unity-shared/UnitySettings.h	2012-09-18 13:56:19 +0000
@@ -1,6 +1,6 @@
 // -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
 /*
-* Copyright (C) 2010, 2011 Canonical Ltd
+* Copyright (C) 2010, 2011, 2012 Canonical Ltd
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
@@ -15,11 +15,13 @@
 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 *
 * Authored by: Neil Jagdish Patel <[e-mail]>
+*              Andrea Azzarone <[e-mail]>
 */
 
-#ifndef UNITY_SETTINGS_H
-#define UNITY_SETTINGS_H
+#ifndef UNITYSHELL_SETTINGS_H
+#define UNITYSHELL_SETTINGS_H
 
+#include <memory>
 #include <sigc++/signal.h>
 #include <Nux/Nux.h>
 
@@ -29,7 +31,7 @@
 enum class FormFactor
 {
   DESKTOP = 1,
-  NETBOOK = 2,
+  NETBOOK,
   TV
 };
 
@@ -41,18 +43,14 @@
 
   static Settings& Instance();
 
-  // NOTE: could potentially refactor this into a nux::Property
-  FormFactor GetFormFactor() const;
-  void SetFormFactor(FormFactor factor);
-
+  nux::RWProperty<FormFactor> form_factor;
   nux::Property<bool> is_standalone;
-  sigc::signal<void> changed;
 
 private:
   class Impl;
-  Impl* pimpl;
+  std::unique_ptr<Impl> pimpl;
 };
 
 }
 
-#endif // UNITY_SETTINGS_H
+#endif

=== modified file 'unity-standalone/StandaloneUnity.cpp'
--- unity-standalone/StandaloneUnity.cpp	2012-09-11 13:57:19 +0000
+++ unity-standalone/StandaloneUnity.cpp	2012-09-18 13:56:19 +0000
@@ -160,7 +160,7 @@
   // The instances for the pseudo-singletons.
   Settings settings;
   settings.is_standalone = true;
-  if (force_tv) Settings::Instance().SetFormFactor(FormFactor::TV);
+  if (force_tv) Settings::Instance().form_factor(FormFactor::TV);
   
   PluginAdapter::Initialize(NULL);
   dash::Style dash_style;