Merge lp:~nick-dedekind/unity/lp1044823.glib.Source into lp:unity

[Nick Dedekind]

Fixed ability to delete glib::Source wrapper during its callback.

Fixed ability to delete glib::Source wrapper during its callback.
Updated gtests to better test deletion during callback.

[Nick Dedekind (nick-dedekind)]

Callback pointer moved into callback data to ensure lambda function "context" is valid if Source is deleted during the callback.

[Michal Hruby (mhr3)]

83 + Callback callback_fn_;
93 - Callback callback_;

I think these two changes are everything that's needed to make it work, unless I'm missing something the two new bools aren't really necessary.

98 === modified file 'dash/previews/PreviewContainer.cpp'

Guess this shouldn't be part of this MP?

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

I believe we should keep the bools so that if you call Source::Remove (directly, or on destruction) during the callback, then the Remove will not destroy the GSource immediately, and will cause the callback to return false to destroy when it's complete.

[Nick Dedekind (nick-dedekind)]

Removed crossed-over MP files.

[Nick Dedekind (nick-dedekind)]

Removed management of gsource destruction during callback (let glib handle it).

[Michal Hruby (mhr3)]

Looks good to me now. +1

review: Approve

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

Looking good, let's see if we can have more Callback const& bits around ;)

review: Approve

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1183/console reported an error when processing this lp:~nick-dedekind/unity/lp1044823.glib.Source branch.
Not merging it.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1204/console reported an error when processing this lp:~nick-dedekind/unity/lp1044823.glib.Source branch.
Not merging it.

=== modified file 'UnityCore/GLibSource.cpp'
--- UnityCore/GLibSource.cpp	2012-07-18 14:24:49 +0000
+++ UnityCore/GLibSource.cpp	2012-09-05 08:40:28 +0000
@@ -69,13 +69,12 @@
   return static_cast<Priority>(prio);
 }
 
-bool Source::Run(Callback callback)
+bool Source::Run(Callback const& callback)
 {
   if (!source_ || source_id_ || IsRunning())
     return false;
 
-  callback_ = callback;
-  callback_data_ = new CallBackData(this);
+  callback_data_ = new CallBackData(this, callback);
 
   g_source_set_callback(source_, SourceCallback, callback_data_, DestroyCallback);
   source_id_ = g_source_attach(source_, nullptr);
@@ -101,16 +100,14 @@
   if (!data)
     return G_SOURCE_REMOVE;
 
-  auto self = static_cast<CallBackData*>(data)->self;
+  auto cb_data = static_cast<CallBackData*>(data);
 
-  if (self && self->callback_ && self->callback_())
+  if (cb_data && cb_data->callback_fn_ && cb_data->callback_fn_())
   {
     return G_SOURCE_CONTINUE;
   }
-  else
-  {
-    return G_SOURCE_REMOVE;
-  }
+
+  return G_SOURCE_REMOVE;
 }
 
 void Source::DestroyCallback(gpointer data)
@@ -133,7 +130,7 @@
 }
 
 
-Timeout::Timeout(unsigned int milliseconds, Callback cb, Priority prio)
+Timeout::Timeout(unsigned int milliseconds, Callback const& cb, Priority prio)
 {
   Init(milliseconds, prio);
   Run(cb);
@@ -151,7 +148,7 @@
 }
 
 
-TimeoutSeconds::TimeoutSeconds(unsigned int seconds, Callback cb, Priority prio)
+TimeoutSeconds::TimeoutSeconds(unsigned int seconds, Callback const& cb, Priority prio)
 {
   Init(seconds, prio);
   Run(cb);
@@ -169,7 +166,7 @@
 }
 
 
-Idle::Idle(Callback cb, Priority prio)
+Idle::Idle(Callback const& cb, Priority prio)
 {
   Init(prio);
   Run(cb);
@@ -255,7 +252,7 @@
   return nullptr;
 }
 
-Source::Ptr SourceManager::AddTimeout(unsigned int milliseconds, Source::Callback cb, std::string const& nick)
+Source::Ptr SourceManager::AddTimeout(unsigned int milliseconds, Source::Callback const& cb, std::string const& nick)
 {
   auto timeout = std::make_shared<Timeout>(milliseconds);
 
@@ -280,7 +277,7 @@
   return nullptr;
 }
 
-Source::Ptr SourceManager::AddTimeoutSeconds(unsigned int seconds, Source::Callback cb, std::string const& nick)
+Source::Ptr SourceManager::AddTimeoutSeconds(unsigned int seconds, Source::Callback const& cb, std::string const& nick)
 {
   auto timeout = std::make_shared<TimeoutSeconds>(seconds);
 
@@ -305,7 +302,7 @@
   return nullptr;
 }
 
-Source::Ptr SourceManager::AddIdle(Source::Callback cb, std::string const& nick)
+Source::Ptr SourceManager::AddIdle(Source::Callback const& cb, std::string const& nick)
 {
   auto idle = std::make_shared<Idle>();
 

=== modified file 'UnityCore/GLibSource.h'
--- UnityCore/GLibSource.h	2012-07-16 11:03:32 +0000
+++ UnityCore/GLibSource.h	2012-09-05 08:40:28 +0000
@@ -77,7 +77,7 @@
    * This Run a source using the @callback function as Source's callback.
    * The method will return false if the source is already running, true otherwise.
    */
-  bool Run(Callback callback);
+  bool Run(Callback const& callback);
   bool IsRunning() const;
 
   /**
@@ -104,11 +104,13 @@
 private:
   struct CallBackData
   {
-    CallBackData(Source* src)
+    CallBackData(Source* src, Callback const& callback)
       : self(src)
+      , callback_fn_(callback)
     {}
 
     Source* self;
+    Callback callback_fn_;
   };
 
   static gboolean SourceCallback(gpointer data);
@@ -116,7 +118,6 @@
 
   unsigned int source_id_;
   CallBackData* callback_data_;
-  Callback callback_;
 };
 
 
@@ -133,7 +134,7 @@
 {
 public:
   Timeout(unsigned int milliseconds, Priority prio = Priority::DEFAULT);
-  Timeout(unsigned int milliseconds, Callback cb, Priority prio = Priority::DEFAULT);
+  Timeout(unsigned int milliseconds, Callback const& cb, Priority prio = Priority::DEFAULT);
 
 private:
   void Init(unsigned int milliseconds, Priority prio);
@@ -153,7 +154,7 @@
 {
 public:
   TimeoutSeconds(unsigned int seconds, Priority prio = Priority::DEFAULT);
-  TimeoutSeconds(unsigned int seconds, Callback cb, Priority prio = Priority::DEFAULT);
+  TimeoutSeconds(unsigned int seconds, Callback const& cb, Priority prio = Priority::DEFAULT);
 
 private:
   void Init(unsigned int seconds, Priority prio);
@@ -173,7 +174,7 @@
 {
 public:
   Idle(Priority prio = Priority::DEFAULT_IDLE);
-  Idle(Callback cb, Priority prio = Priority::DEFAULT_IDLE);
+  Idle(Callback const& cb, Priority prio = Priority::DEFAULT_IDLE);
 
 private:
   void Init(Priority prio);
@@ -206,13 +207,13 @@
   bool Add(Source::Ptr const& source, std::string const& nick = "");
 
   Source::Ptr AddTimeout(unsigned int milliseconds, std::string const& nick = "");
-  Source::Ptr AddTimeout(unsigned int milliseconds, Source::Callback cb, std::string const& nick = "");
+  Source::Ptr AddTimeout(unsigned int milliseconds, Source::Callback const& cb, std::string const& nick = "");
 
   Source::Ptr AddTimeoutSeconds(unsigned int seconds, std::string const& nick = "");
-  Source::Ptr AddTimeoutSeconds(unsigned int seconds, Source::Callback cb, std::string const& nick = "");
+  Source::Ptr AddTimeoutSeconds(unsigned int seconds, Source::Callback const& cb, std::string const& nick = "");
 
   Source::Ptr AddIdle(std::string const& nick = "");
-  Source::Ptr AddIdle(Source::Callback cb, std::string const& nick = "");
+  Source::Ptr AddIdle(Source::Callback const& cb, std::string const& nick = "");
 
   bool Remove(std::string const& nick);
   bool Remove(unsigned int id);

=== modified file 'tests/test_glib_source.cpp'
--- tests/test_glib_source.cpp	2012-07-09 11:21:08 +0000
+++ tests/test_glib_source.cpp	2012-09-05 08:40:28 +0000
@@ -247,11 +247,14 @@
   unsigned int local_callback_call_count = 0;
 
   Source::UniquePtr timeout(new Timeout(10, [&] {
+    unsigned int id = timeout->Id();
     local_callback_called = true;
-    ++local_callback_call_count;
+    local_callback_call_count++;
+
     timeout.reset();
-
-    // this function would be called more than once if we had not removed the source.
+    // resetting the ptr should destroy the source.
+    EXPECT_TRUE(g_main_context_find_source_by_id(NULL, id) == nullptr);
+    // this function would be called more than once (local_callback_call_count > 1) if we had not removed the source.
     return true;
   }));
 
@@ -262,6 +265,31 @@
   EXPECT_EQ(local_callback_call_count, 1);
 }
 
+TEST(TestGLibTimeout, AutoRemoveSourceOnCallback)
+{
+  bool local_callback_called = false;
+  unsigned int local_callback_call_count = 0;
+
+  Source::UniquePtr timeout(new Timeout(10, [&] {
+    local_callback_called = true;
+    ++local_callback_call_count;
+
+    // return false to remove source.
+    return false;
+  }));
+  unsigned int id = timeout->Id();
+
+  Utils::WaitForTimeoutMSec(100);
+
+  timeout.reset();
+  EXPECT_EQ(local_callback_called, true);
+  EXPECT_EQ(local_callback_call_count, 1);
+
+  // source should be removed by now.
+  EXPECT_TRUE(g_main_context_find_source_by_id(NULL, id) == nullptr);
+}
+
+
 // GLib TimeoutSeconds tests
 
 TEST(TestGLibTimeoutSeconds, Construction)