Merge lp:~gordallott/unity/unity.fix-bg-hash-crashers into lp:unity

[Gord Allott]

Fixes a few (hopefully linked) bugs in the bghash code, the race seems to be fixed on my machine as well as the crazy alpha problem, segfaults should be gone too

[Sam Spilsbury (smspillaz)]

Looks good to me

review: Approve

=== modified file 'plugins/unityshell/src/BGHash.cpp'
--- plugins/unityshell/src/BGHash.cpp	2011-08-12 04:16:44 +0000
+++ plugins/unityshell/src/BGHash.cpp	2011-08-18 11:16:32 +0000
@@ -18,6 +18,7 @@
 
 #include "BGHash.h"
 #include <Nux/Nux.h>
+#include <NuxCore/Logger.h>
 #include <gdk-pixbuf/gdk-pixbuf.h>
 #include <libgnome-desktop/gnome-bg.h>
 #include <unity-misc/gnome-bg-slideshow.h>
@@ -25,6 +26,11 @@
 #include "UBusMessages.h"
 #include "UnityCore/GLibWrapper.h"
 
+namespace
+{
+  nux::logging::Logger logger("unity.BGHash");
+}
+
 namespace {
   int level_of_recursion;
   const int MAX_LEVEL_OF_RECURSION = 16;
@@ -37,9 +43,9 @@
       _bg_slideshow (NULL),
       _current_slide (NULL),
       _slideshow_handler(0),
-      _current_color (nux::color::DimGray),
-      _new_color (nux::color::DimGray),
-      _old_color (nux::color::DimGray),
+      _current_color (unity::colors::Aubergine),
+      _new_color (unity::colors::Aubergine),
+      _old_color (unity::colors::Aubergine),
       _hires_time_start(10),
       _hires_time_end(20),
       _ubus_handle_request_colour(0)
@@ -47,9 +53,6 @@
     background_monitor = gnome_bg_new ();
     client = g_settings_new ("org.gnome.desktop.background");
 
-    glib::Object<GdkPixbuf> pixbuf(GetPixbufFromBG());
-    LoadPixbufToHash(pixbuf);
-
     signal_manager_.Add(
       new glib::Signal<void, GnomeBG*>(background_monitor,
                                        "changed",
@@ -64,6 +67,9 @@
 
     gnome_bg_load_from_preferences (background_monitor, client);
 
+    glib::Object<GdkPixbuf> pixbuf(GetPixbufFromBG());
+    LoadPixbufToHash(pixbuf);
+
     g_timeout_add (0, (GSourceFunc)ForceUpdate, (gpointer)this);
 
     // avoids making a new object method when all we are doing is
@@ -90,7 +96,7 @@
   gboolean BGHash::ForceUpdate (BGHash *self)
   {
     self->OnBackgroundChanged(self->background_monitor);
-    return FALSE;  
+    return FALSE;
   }
 
   void BGHash::OnGSettingsChanged (GSettings *settings, gchar *key)
@@ -101,98 +107,135 @@
   void BGHash::OnBackgroundChanged (GnomeBG *bg)
   {
     const gchar *filename = gnome_bg_get_filename (bg);
-    if (_bg_slideshow != NULL)
-    {
-      slideshow_unref (_bg_slideshow);
-      _bg_slideshow = NULL;
-      _current_slide = NULL;
-    }
-
-    if (_slideshow_handler)
-    {
-      g_source_remove (_slideshow_handler);
-      _slideshow_handler = 0;
-    }
-
-    if (g_str_has_suffix (filename, "xml"))
-    {
-      GError *error = NULL;
-
+    if (filename == NULL)
+    {
+      // we might have a gradient instead
+      GdkColor color_primary, color_secondary;
+      GDesktopBackgroundShading shading_type;
+      nux::Color parsed_color;
+
+      gnome_bg_get_color (bg, &shading_type, &color_primary, &color_secondary);
+      if (shading_type == G_DESKTOP_BACKGROUND_SHADING_HORIZONTAL ||
+          shading_type == G_DESKTOP_BACKGROUND_SHADING_SOLID)
+      {
+        parsed_color = nux::Color(static_cast<float>(color_primary.red) / 65535,
+                                  static_cast<float>(color_primary.green) / 65535,
+                                  static_cast<float>(color_primary.blue) / 65535,
+                                  1.0f);
+      }
+      else
+      {
+        nux::Color primary = nux::Color(static_cast<float>(color_primary.red) / 65535,
+                                        static_cast<float>(color_primary.green) / 65535,
+                                        static_cast<float>(color_primary.blue) / 65535,
+                                        1.0f);
+
+        nux::Color secondary = nux::Color(static_cast<float>(color_secondary.red) / 65535,
+                                          static_cast<float>(color_secondary.green) / 65535,
+                                          static_cast<float>(color_secondary.blue) / 65535,
+                                          1.0f);
+
+        parsed_color = (primary + secondary) * 0.5f;
+      }
+
+      nux::Color new_color = MatchColor (parsed_color);
+      TransitionToNewColor (new_color);
+    }
+    else
+    {
       if (_bg_slideshow != NULL)
       {
         slideshow_unref (_bg_slideshow);
         _bg_slideshow = NULL;
-      }
-
-      _bg_slideshow = read_slideshow_file (filename, &error);
-
-      if (error != NULL)
-      {
-        g_warning ("BGHash.cpp: could not load filename %s: %s", filename, error->message);
-        g_error_free (error);
-      }
-      else if (_bg_slideshow == NULL)
-      {
-        g_warning ("BGHash.cpp: could not load filename %s", filename);
-      }
-      else
-      {
-        // we loaded fine, hook up to the slideshow
-        time_t current_time = time(0);
-        double now = (double) current_time;
-
-        double time_diff = now - _bg_slideshow->start_time;
-        double progress = fmod (time_diff, _bg_slideshow->total_duration);
-
-        // progress now holds how many seconds we are in to this slideshow.
-        // iterate over the slideshows until we get in to the current slideshow
-        Slide *slide_iteration;
-        Slide *slide_current = NULL;
-        double elapsed = 0;
-        double time_to_next_change = 0;
-        GList *list;
-
-        for (list = _bg_slideshow->slides->head; list != NULL; list = list->next)
-        {
-          slide_iteration = reinterpret_cast<Slide *>(list->data);
-          if (elapsed + slide_iteration->duration > progress)
-          {
-            slide_current = slide_iteration;
-            time_to_next_change = slide_current->duration- (progress - elapsed);
-            break;
-          }
-
-          elapsed += slide_iteration->duration;
-        }
-
-        if (slide_current == NULL)
-        {
-          slide_current = reinterpret_cast<Slide *>(g_queue_peek_head(_bg_slideshow->slides));
-          time_to_next_change = slide_current->duration;
-        }
-
-        // time_to_next_change now holds the seconds until the next slide change
-        // the next slide change may or may not be a fixed slide.
-        _slideshow_handler = g_timeout_add ((guint)(time_to_next_change * 1000),
-                                                  (GSourceFunc)OnSlideshowTransition,
-                                                  (gpointer)this);
-
-        // find our current slide now
-        if (slide_current->file1 == NULL)
-        {
-          g_warning ("BGHash.cpp: could not load filename %s - slide has no filename", filename);
+        _current_slide = NULL;
+      }
+
+      if (_slideshow_handler)
+      {
+        g_source_remove (_slideshow_handler);
+        _slideshow_handler = 0;
+      }
+
+      if (g_str_has_suffix (filename, "xml"))
+      {
+        GError *error = NULL;
+
+        if (_bg_slideshow != NULL)
+        {
+          slideshow_unref (_bg_slideshow);
+          _bg_slideshow = NULL;
+        }
+
+        _bg_slideshow = read_slideshow_file (filename, &error);
+
+        if (error != NULL)
+        {
+          LOG_WARNING(logger) << "Could not load filename \"" << filename << "\": " << error->message;
+          g_error_free (error);
+        }
+        else if (_bg_slideshow == NULL)
+        {
+          LOG_WARNING(logger) << "Could not load filename \"" << filename << "\"";
         }
         else
         {
-          FileSize *fs = reinterpret_cast<FileSize *>(slide_current->file1->data);
-          filename = reinterpret_cast<gchar *>(fs->file);
+          // we loaded fine, hook up to the slideshow
+          time_t current_time = time(0);
+          double now = (double) current_time;
+
+          double time_diff = now - _bg_slideshow->start_time;
+          double progress = fmod (time_diff, _bg_slideshow->total_duration);
+
+          // progress now holds how many seconds we are in to this slideshow.
+          // iterate over the slideshows until we get in to the current slideshow
+          Slide *slide_iteration;
+          Slide *slide_current = NULL;
+          double elapsed = 0;
+          double time_to_next_change = 0;
+          GList *list;
+
+          for (list = _bg_slideshow->slides->head; list != NULL; list = list->next)
+          {
+            slide_iteration = reinterpret_cast<Slide *>(list->data);
+            if (elapsed + slide_iteration->duration > progress)
+            {
+              slide_current = slide_iteration;
+              time_to_next_change = slide_current->duration- (progress - elapsed);
+              break;
+            }
+
+            elapsed += slide_iteration->duration;
+          }
+
+          if (slide_current == NULL)
+          {
+            slide_current = reinterpret_cast<Slide *>(g_queue_peek_head(_bg_slideshow->slides));
+            time_to_next_change = slide_current->duration;
+          }
+
+          // time_to_next_change now holds the seconds until the next slide change
+          // the next slide change may or may not be a fixed slide.
+          _slideshow_handler = g_timeout_add ((guint)(time_to_next_change * 1000),
+                                                    (GSourceFunc)OnSlideshowTransition,
+                                                    (gpointer)this);
+
+          // find our current slide now
+          if (slide_current->file1 == NULL)
+          {
+            LOG_WARNING(logger) << "Could not load filename \"" << filename << "\"";
+          }
+          else
+          {
+            FileSize *fs = reinterpret_cast<FileSize *>(slide_current->file1->data);
+            filename = reinterpret_cast<gchar *>(fs->file);
+          }
+
+          _current_slide = slide_current;
         }
-
-        _current_slide = slide_current;
       }
+
+      LoadFileToHash(filename);
     }
-
-    LoadFileToHash(filename);
   }
 
   gboolean BGHash::OnSlideshowTransition (BGHash *self)
@@ -235,7 +278,7 @@
       const gchar *filename = NULL;
       if (proposed_slide->file1 == NULL)
       {
-        g_warning ("BGHash.cpp: could not load filename %s - slide has no filename", filename);
+        LOG_WARNING(logger) << "Could not load filename \"" << filename << "\"";
       }
       else
       {
@@ -310,7 +353,7 @@
                                             _current_color.red * 0.7f,
                                             _current_color.green * 0.7f,
                                             _current_color.blue * 0.7f,
-                                            _current_color.alpha)
+                                            0.5)
                             );
   }
 
@@ -336,10 +379,17 @@
 
   void BGHash::LoadPixbufToHash(GdkPixbuf *pixbuf)
   {
+    nux::Color new_color;
     if (pixbuf == NULL)
-      return;
+    {
+      LOG_WARNING(logger) << "Passed in a bad pixbuf, defaulting colour";
+      new_color = unity::colors::Aubergine;
+    }
+    else
+    {
+      new_color = HashColor (pixbuf);
+    }
 
-    nux::Color new_color = HashColor (pixbuf);
     TransitionToNewColor (new_color);
   }
 
@@ -350,8 +400,12 @@
 
     if (error)
     {
+      LOG_WARNING(logger) << "Could not load filename \"" << path << "\": " << error.Message();
       _current_color = nux::Color(0.2, 0.2, 0.2, 0.9);
-      return;
+
+      // try and get a colour from gnome-bg, for various reasons, gnome bg might not
+      // return a correct image which sucks =\ but this is a fallback
+      pixbuf = GetPixbufFromBG();
     }
 
     LoadPixbufToHash (pixbuf);
@@ -507,8 +561,6 @@
     // apply design to the colour
     chosen_color.alpha = 0.5f;
 
-
-
     return chosen_color;
   }
 

=== modified file 'plugins/unityshell/src/BGHash.h'
--- plugins/unityshell/src/BGHash.h	2011-08-11 16:47:44 +0000
+++ plugins/unityshell/src/BGHash.h	2011-08-18 11:16:32 +0000
@@ -25,11 +25,19 @@
 #include <unity-misc/gnome-bg-slideshow.h>
 #include <UnityCore/GLibSignal.h>
 
+namespace unity {
+namespace colors {
+  const nux::Color Aubergine(0x3E, 0x20, 0x60);
+};
+};
+
 namespace unity
 {
   class BGHash
   {
   public:
+
+
     BGHash ();
     ~BGHash ();
 
@@ -67,7 +75,7 @@
 
     guint64 _hires_time_start;
     guint64 _hires_time_end;
-    glib::SignalManager signal_manager_; 
+    glib::SignalManager signal_manager_;
     uint _ubus_handle_request_colour;
   };
 };

=== modified file 'tests/TestBGHash.cpp'
--- tests/TestBGHash.cpp	2011-07-28 11:02:56 +0000
+++ tests/TestBGHash.cpp	2011-08-18 11:16:32 +0000
@@ -16,9 +16,10 @@
  *
  * Authored by: Gordon Allott <[e-mail]>
  */
-#include "Nux/Nux.h"
-#include "Nux/WindowThread.h"
-#include "NuxGraphics/GraphicsEngine.h"
+#include <Nux/Nux.h>
+#include <NuxCore/Logger.h>
+#include <Nux/WindowThread.h>
+#include <NuxGraphics/GraphicsEngine.h>
 #include <gtk/gtk.h>
 
 #include "BGHash.h"
@@ -84,7 +85,7 @@
 int main(int argc, char **argv)
 {
   UBusServer *ubus;
-  unity::BGHash bg_hash;
+
   nux::SystemThread* st = NULL;
   nux::WindowThread* wt = NULL;
 
@@ -97,6 +98,9 @@
   gtk_init (&argc, &argv);
 
   nux::NuxInitialize(0);
+  nux::logging::configure_logging(::getenv("UNITY_LOG_SEVERITY"));
+
+  unity::BGHash bg_hash;
 
   ubus = ubus_server_get_default ();
   ubus_server_register_interest (ubus, UBUS_BACKGROUND_COLOR_CHANGED,