Merge lp:~3v1n0/unity/dbus-indicators-proxy into lp:unity

[Marco Trevisan (Treviño)]

DBusIndicators code rewritten to use glib::DBusProxy instead of raw gdbus calls

DBusIndicators code rewritten to use glib::DBusProxy instead of raw gdbus calls.

The code now is more simple, clean and C++ conformant. Also, now we always make sure that the panel is launched when it is not available (before this was not happening in some corner cases).

Tests are covered by lp:~3v1n0/unity/indicators-tests

[Michal Hruby (mhr3)]

787 + bool auto_reconnect)

What are you actually trying to achieve with this? GDBusProxy automatically reconnects itself if you construct it using a known service name (as opposed to unique bus address). Therefore it seems redundant to me.

If you want to make sure that a service is alive, all you need to do is watch changes to the connected state and if it disconnects, just call any method from given interface and that will dbus-activate the service again.

review: Needs Information

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

> 787 + bool auto_reconnect)
>
> What are you actually trying to achieve with this? GDBusProxy automatically
> reconnects itself if you construct it using a known service name (as opposed
> to unique bus address). Therefore it seems redundant to me.

Yeah, I know, but it doesn't work here for the reason we discovered...

> If you want to make sure that a service is alive, all you need to do is watch
> changes to the connected state and if it disconnects, just call any method
> from given interface and that will dbus-activate the service again.

That's what I've tried to do, but it doesn't work here because glib::DBusProxy notify us about the disconnection event when actually the name has vanished, but the proxy is not disconnected yet.
Adding a timeout fixes it, but we should handle that in a better way.

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

> 787 + bool auto_reconnect)
>
> What are you actually trying to achieve with this? GDBusProxy automatically
> reconnects itself if you construct it using a known service name (as opposed
> to unique bus address). Therefore it seems redundant to me.

Ok this is now fixed into lp:~3v1n0/unity/dbus-indicators-proxy that will merge in this one as you know ;)

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

> Ok this is now fixed into lp:~3v1n0/unity/dbus-indicators-proxy that will
> merge in this one as you know ;)

Ops, I meant lp:~3v1n0/unity/dbus-proxy-name-owner-watch

[Unity Merger (unity-merger)]

The prerequisite lp:~3v1n0/unity/indicators-p has not yet been merged into lp:unity.

[Michal Hruby (mhr3)]

Overall I'm liking this a lot - mostly because of "(+234/-516)" :) I have some concerns though:

269 + RequestSyncAll();
270 +
271 + reconnect_timeout_id_ = g_timeout_add_seconds(1, [](gpointer data) -> gboolean {
272 + auto self = static_cast<DBusIndicators::Impl*>(data);
273 +
274 + if (!self->gproxy_.IsConnected())
275 + {
276 + self->RequestSyncAll();
277 + return TRUE;
278 + }
279 +
280 + self->reconnect_timeout_id_ = 0;
281 + return FALSE;
282 + }, this);

Are you sure you want to call RequestSyncAll also outside of the lambda expression? Plus you should be checking if reconnect_timeout_id == 0, before calling g_timeout_add...

146 + struct CallData
147 + {
148 + Impl* self;
149 + GVariant *parameters;
150 + };

I'm not really liking this, it's unclear who owns the variant (or rather why isn't it unreffed) - using our Variant wrapper should fix this. What's worse is that the CallData can be alive even after the Impl instance is destroyed which can lead to a crash.

review: Needs Fixing

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

> Overall I'm liking this a lot - mostly because of "(+234/-516)" :)

Ehehe ;)

> Are you sure you want to call RequestSyncAll also outside of the lambda
> expression?

Yes. Because after we get a disconnection event we can try immediately to restore the service, it that fails we retry every 1 second. This assures that if we have a valid u-p-s running or in the system as well, we use it!

> Plus you should be checking if reconnect_timeout_id == 0, before
> calling g_timeout_add...

Yes, sure.

> 146 + struct CallData
> 147 + {
> 148 + Impl* self;
> 149 + GVariant *parameters;
> 150 + };
>
> I'm not really liking this, it's unclear who owns the variant (or rather why
> isn't it unreffed) - using our Variant wrapper should fix this.

Well, as you know the proxy handles it, so I didn't ref/unreff'ed it... But I'll move to glib::Variant.

> What's worse
> is that the CallData can be alive even after the Impl instance is destroyed
> which can lead to a crash.

This should be very unlikely using an idle, but I'll make that more safe as well.

[Michal Hruby (mhr3)]

Awesome, thanks!

review: Approve

[Unity Merger (unity-merger)]

No proposals found for merge of lp:~3v1n0/unity/indicators-p into lp:unity.

=== modified file 'UnityCore/DBusIndicators.cpp'
--- UnityCore/DBusIndicators.cpp	2012-03-20 10:16:27 +0000
+++ UnityCore/DBusIndicators.cpp	2012-03-20 10:16:27 +0000
@@ -1,6 +1,6 @@
 // -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
 /*
- * Copyright (C) 2010, 2011 Canonical Ltd
+ * Copyright (C) 2010-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
@@ -18,18 +18,11 @@
  *              Marco Trevisan (Treviño) <[e-mail]>
  */
 
+#include "config.h"
 #include "DBusIndicators.h"
 
-#include <algorithm>
-#include <iostream>
-
-#include <gio/gio.h>
-
-#include <X11/Xlib.h>
-
-#include "config.h"
-#include "GLibSignal.h"
 #include "GLibWrapper.h"
+#include "GLibDBusProxy.h"
 #include "Variant.h"
 
 namespace unity
@@ -39,87 +32,33 @@
 
 namespace
 {
-// This anonymous namespace holds the DBus callback methods.
-
-struct SyncData
-{
-  SyncData(DBusIndicators::Impl* self_)
-    : self(self_)
-    , cancel(g_cancellable_new())
-  {
-  }
-
-  ~SyncData()
-  {
-    if (cancel)
-    {
-      g_cancellable_cancel(cancel);
-      g_object_unref(cancel);
-    }
-  }
-
-  void SyncComplete()
-  {
-    if (cancel)
-    {
-      g_object_unref(cancel);
-    }
-    cancel = NULL;
-  }
-
-  DBusIndicators::Impl* self;
-  GCancellable* cancel;
-};
-
-typedef boost::shared_ptr<SyncData> SyncDataPtr;
-
-const char* const S_NAME = "com.canonical.Unity.Panel.Service";
-const char* const S_PATH = "/com/canonical/Unity/Panel/Service";
-const char* const S_IFACE = "com.canonical.Unity.Panel.Service";
-
-struct ShowEntryData
-{
-  glib::Object<GDBusProxy> proxy;
-  std::string entry_id;
-  guint xid;
-  int x;
-  int y;
-  guint button;
-  guint timestamp;
-};
-
-bool run_local_panel_service();
-gboolean reconnect_to_service(gpointer data);
-void on_proxy_ready_cb(GObject* source, GAsyncResult* res, gpointer data);
-void request_sync(GDBusProxy* proxy, const char* method, GVariant* name,
-                  SyncData* data);
-void on_sync_ready_cb(GObject* source, GAsyncResult* res, gpointer data);
-
-bool send_show_entry(ShowEntryData* data);
-bool send_show_appmenu(ShowEntryData* data);
-
+const std::string SERVICE_NAME("com.canonical.Unity.Panel.Service");
+const std::string SERVICE_PATH("/com/canonical/Unity/Panel/Service");
+const std::string SERVICE_IFACE("com.canonical.Unity.Panel.Service");
 } // anonymous namespace
 
 
-// Connects to the remote panel service (unity-panel-service) and translates
-// that into something that the panel can show
+/* Connects to the remote panel service (unity-panel-service) and translates
+ * that into something that the panel can show */
 class DBusIndicators::Impl
 {
 public:
   Impl(DBusIndicators* owner);
+  ~Impl();
 
-  void OnRemoteProxyReady(GDBusProxy* proxy);
-  void Reconnect();
+  void CheckLocalService();
   void RequestSyncAll();
   void RequestSyncIndicator(std::string const& name);
-  void Sync(GVariant* args, SyncData* data);
-  void SyncGeometries(std::string const& name,
-                      EntryLocationMap const& locations);
-  void OnProxyNameOwnerChanged(GDBusProxy* proxy, GParamSpec* pspec);
-  void OnProxySignalReceived(GDBusProxy* proxy,
-                             char* sender_name,
-                             char* signal_name_,
-                             GVariant* parameters);
+  void Sync(GVariant* args);
+  void SyncGeometries(std::string const& name, EntryLocationMap const& locations);
+
+  void OnConnected();
+  void OnDisconnected();
+
+  void OnReSync(GVariant* parameters);
+  void OnEntryActivated(GVariant* parameters);
+  void OnEntryActivatedRequest(GVariant* parameters);
+  void OnEntryShowNowChanged(GVariant* parameters);
 
   virtual void OnEntryScroll(std::string const& entry_id, int delta);
   virtual void OnEntryShowMenu(std::string const& entry_id, unsigned int xid,
@@ -130,16 +69,17 @@
   virtual void OnShowAppMenu(unsigned int xid, int x, int y,
                              unsigned int timestamp);
 
-  std::string name() const;
-  std::string owner_name() const;
-  bool using_local_service() const;
+  struct CallData
+  {
+    Impl* self;
+    glib::Variant parameters;
+  };
 
   DBusIndicators* owner_;
-  glib::Object<GDBusProxy> proxy_;
-  typedef std::vector<SyncDataPtr> PendingSyncs;
-  PendingSyncs pending_syncs_;
-
-  glib::SignalManager signal_manager_;
+  guint reconnect_timeout_id_;
+  guint show_entry_idle_id_;
+  guint show_appmenu_idle_id_;
+  glib::DBusProxy gproxy_;
   std::map<std::string, EntryLocationMap> cached_locations_;
 };
 
@@ -147,62 +87,149 @@
 // Public Methods
 DBusIndicators::Impl::Impl(DBusIndicators* owner)
   : owner_(owner)
-{
-  Reconnect();
-}
-
-void DBusIndicators::Impl::Reconnect()
-{
-  g_spawn_command_line_sync("killall -9 unity-panel-service",
-                            NULL, NULL, NULL, NULL);
-
+  , reconnect_timeout_id_(0)
+  , show_entry_idle_id_(0)
+  , show_appmenu_idle_id_(0)
+  , gproxy_(SERVICE_NAME, SERVICE_PATH, SERVICE_IFACE,
+            G_BUS_TYPE_SESSION, G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES)
+{
+  gproxy_.Connect("ReSync", sigc::mem_fun(this, &DBusIndicators::Impl::OnReSync));
+  gproxy_.Connect("EntryActivated", sigc::mem_fun(this, &DBusIndicators::Impl::OnEntryActivated));
+  gproxy_.Connect("EntryActivateRequest", sigc::mem_fun(this, &DBusIndicators::Impl::OnEntryActivatedRequest));
+  gproxy_.Connect("EntryShowNowChanged", sigc::mem_fun(this, &DBusIndicators::Impl::OnEntryShowNowChanged));
+
+  gproxy_.connected.connect(sigc::mem_fun(this, &DBusIndicators::Impl::OnConnected));
+  gproxy_.disconnected.connect(sigc::mem_fun(this, &DBusIndicators::Impl::OnDisconnected));
+
+  CheckLocalService();
+}
+
+DBusIndicators::Impl::~Impl()
+{
+  if (reconnect_timeout_id_)
+    g_source_remove(reconnect_timeout_id_);
+
+  if (show_entry_idle_id_)
+    g_source_remove(show_entry_idle_id_);
+
+  if (show_appmenu_idle_id_)
+    g_source_remove(show_appmenu_idle_id_);
+}
+
+void DBusIndicators::Impl::CheckLocalService()
+{
   if (g_getenv("PANEL_USE_LOCAL_SERVICE"))
   {
-    run_local_panel_service();
-    g_timeout_add_seconds(1, reconnect_to_service, this);
-  }
-  else
-  {
-    // We want to grab the Panel Service object. This is async, which is fine
-    reconnect_to_service(this);
-  }
-}
-
-void DBusIndicators::Impl::OnRemoteProxyReady(GDBusProxy* proxy)
-{
-  if (proxy_)
-  {
-    // We've been connected before; We don't need new proxy, just continue
-    // rocking with the old one.
-    g_object_unref(proxy);
-  }
-  else
-  {
-    proxy_ = proxy;
-    // Connect to interesting signals
-    signal_manager_.Add(new glib::Signal<void, GDBusProxy*, char*, char*, GVariant*>
-                        (proxy_, "g-signal", sigc::mem_fun(this, &Impl::OnProxySignalReceived)));
-
-    signal_manager_.Add(new glib::Signal<void, GDBusProxy*, GParamSpec*>
-                        (proxy_, "notify::g-name-owner", sigc::mem_fun(this, &Impl::OnProxyNameOwnerChanged)));
-  }
-  RequestSyncAll();
+    glib::Error error;
+
+    g_spawn_command_line_sync("killall -9 unity-panel-service",
+                              nullptr, nullptr, nullptr, nullptr);
+
+    // This is obviously hackish, but this part of the code is mostly hackish...
+    // Let's attempt to run it from where we expect it to be
+    std::string cmd = PREFIXDIR + std::string("/lib/unity/unity-panel-service");
+    std::cerr << "\nWARNING: Couldn't load panel from installed services, "
+              << "so trying to load panel from known location: "
+              << cmd << "\n";
+
+    g_spawn_command_line_async(cmd.c_str(), &error);
+
+    if (error)
+    {
+      std::cerr << "\nWARNING: Unable to launch remote service manually: "
+                << error.Message() << "\n";
+    }
+  }
+}
+
+void DBusIndicators::Impl::OnConnected()
+{
+  RequestSyncAll();
+}
+
+void DBusIndicators::Impl::OnDisconnected()
+{
+  for (auto indicator : owner_->GetIndicators())
+  {
+    owner_->RemoveIndicator(indicator->name());
+  }
+
+  cached_locations_.clear();
+
+  CheckLocalService();
+  RequestSyncAll();
+
+  if (reconnect_timeout_id_)
+    g_source_remove(reconnect_timeout_id_);
+
+  reconnect_timeout_id_ = g_timeout_add_seconds(1, [](gpointer data) -> gboolean {
+    auto self = static_cast<DBusIndicators::Impl*>(data);
+
+    if (!self->gproxy_.IsConnected())
+    {
+      self->RequestSyncAll();
+      return TRUE;
+    }
+
+    self->reconnect_timeout_id_ = 0;
+    return FALSE;
+  }, this);
+}
+
+void DBusIndicators::Impl::OnReSync(GVariant* parameters)
+{
+  glib::String indicator_name;
+  g_variant_get(parameters, "(s)", &indicator_name);
+
+  if (indicator_name && !indicator_name.Str().empty())
+  {
+    RequestSyncIndicator(indicator_name);
+  }
+  else
+  {
+    RequestSyncAll();
+  }
+}
+
+void DBusIndicators::Impl::OnEntryActivated(GVariant* parameters)
+{
+  glib::String entry_name;
+  nux::Rect geo;
+  g_variant_get(parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y, &geo.width, &geo.height);
+
+  if (entry_name)
+    owner_->ActivateEntry(entry_name, geo);
+}
+
+void DBusIndicators::Impl::OnEntryActivatedRequest(GVariant* parameters)
+{
+  glib::String entry_name;
+  g_variant_get(parameters, "(s)", &entry_name);
+
+  if (entry_name)
+    owner_->on_entry_activate_request.emit(entry_name);
+}
+
+void DBusIndicators::Impl::OnEntryShowNowChanged(GVariant* parameters)
+{
+  glib::String entry_name;
+  gboolean  show_now;
+  g_variant_get(parameters, "(sb)", &entry_name, &show_now);
+
+  if (entry_name)
+    owner_->SetEntryShowNow(entry_name, show_now);
 }
 
 void DBusIndicators::Impl::RequestSyncAll()
 {
-  SyncDataPtr data(new SyncData(this));
-  pending_syncs_.push_back(data);
-  request_sync(proxy_, "Sync", NULL, data.get());
+  gproxy_.Call("Sync", nullptr, sigc::mem_fun(this, &DBusIndicators::Impl::Sync));
 }
 
 void DBusIndicators::Impl::RequestSyncIndicator(std::string const& name)
 {
-  SyncDataPtr data(new SyncData(this));
-  pending_syncs_.push_back(data);
-  // The ownership of this variant is taken by the g_dbus_proxy_call.
-  GVariant* v_name = g_variant_new("(s)", name.c_str());
-  request_sync(proxy_, "SyncOne", v_name, data.get());
+  GVariant* parameter = g_variant_new("(s)", name.c_str());
+
+  gproxy_.Call("SyncOne", parameter, sigc::mem_fun(this, &DBusIndicators::Impl::Sync));
 }
 
 
@@ -216,16 +243,20 @@
   // We have to do this because on certain systems X won't have time to
   // respond to our request for XUngrabPointer and this will cause the
   // menu not to show
-  auto data = new ShowEntryData();
-  data->proxy = proxy_;
-  data->entry_id = entry_id;
-  data->xid = xid;
-  data->x = x;
-  data->y = y;
-  data->button = button;
-  data->timestamp = timestamp;
-
-  g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_entry, data, NULL);
+  auto data = new CallData();
+  data->self = this;
+  data->parameters = g_variant_new("(suiiuu)", entry_id.c_str(), xid, x, y,
+                                   button, timestamp);
+
+  if (show_entry_idle_id_)
+    g_source_remove(show_entry_idle_id_);
+
+  show_entry_idle_id_ = g_idle_add_full (G_PRIORITY_DEFAULT, [] (gpointer data) -> gboolean {
+    auto call_data = static_cast<CallData*>(data);
+    call_data->self->gproxy_.Call("ShowEntry", call_data->parameters);
+
+    return FALSE;
+  }, data, [] (gpointer data) { delete static_cast<CallData*>(data); });
 }
 
 void DBusIndicators::Impl::OnShowAppMenu(unsigned int xid, int x, int y,
@@ -236,43 +267,43 @@
   // We have to do this because on certain systems X won't have time to
   // respond to our request for XUngrabPointer and this will cause the
   // menu not to show
-  auto data = new ShowEntryData();
-  data->proxy = proxy_;
-  data->xid = xid;
-  data->x = x;
-  data->y = y;
-  data->timestamp = timestamp;
-
-  g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_appmenu, data, NULL);
+  auto data = new CallData();
+  data->self = this;
+  data->parameters = g_variant_new("(uiiu)", xid, x, y, timestamp);
+
+  if (show_appmenu_idle_id_)
+    g_source_remove(show_appmenu_idle_id_);
+
+  show_appmenu_idle_id_ = g_idle_add_full (G_PRIORITY_DEFAULT, [] (gpointer data) -> gboolean {
+    auto call_data = static_cast<CallData*>(data);
+    call_data->self->gproxy_.Call("ShowAppMenu", call_data->parameters);
+
+    return FALSE;
+  }, data, [] (gpointer data) { delete static_cast<CallData*>(data); });
 }
 
 void DBusIndicators::Impl::OnEntrySecondaryActivate(std::string const& entry_id,
                                                     unsigned int timestamp)
 {
-  g_dbus_proxy_call(proxy_, "SecondaryActivateEntry",
-                    g_variant_new("(su)", entry_id.c_str(), timestamp),
-                    G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL);
+  gproxy_.Call("SecondaryActivateEntry", g_variant_new("(su)", entry_id.c_str(), timestamp));
 }
 
 void DBusIndicators::Impl::OnEntryScroll(std::string const& entry_id, int delta)
 {
-  g_dbus_proxy_call(proxy_, "ScrollEntry",
-                    g_variant_new("(si)", entry_id.c_str(), delta),
-                    G_DBUS_CALL_FLAGS_NONE,
-                    -1, NULL, NULL, NULL);
+  gproxy_.Call("ScrollEntry", g_variant_new("(si)", entry_id.c_str(), delta));
 }
 
-void DBusIndicators::Impl::Sync(GVariant* args, SyncData* data)
+void DBusIndicators::Impl::Sync(GVariant* args)
 {
-  GVariantIter* iter            = NULL;
-  gchar*        name_hint       = NULL;
-  gchar*        indicator_id    = NULL;
-  gchar*        entry_id        = NULL;
-  gchar*        label           = NULL;
+  GVariantIter* iter            = nullptr;
+  gchar*        name_hint       = nullptr;
+  gchar*        indicator_id    = nullptr;
+  gchar*        entry_id        = nullptr;
+  gchar*        label           = nullptr;
   gboolean      label_sensitive = false;
   gboolean      label_visible   = false;
   guint32       image_type      = 0;
-  gchar*        image_data      = NULL;
+  gchar*        image_data      = nullptr;
   gboolean      image_sensitive = false;
   gboolean      image_visible   = false;
   gint32        priority        = -1;
@@ -308,7 +339,7 @@
 
     Indicator::Entries& entries = indicators[indicator];
 
-    // NULL entries (entry_id == "") are empty indicators.
+    // Null entries (entry_id == "") are empty indicators.
     if (entry != "")
     {
       Entry::Ptr e = indicator->GetEntry(entry_id);
@@ -343,20 +374,6 @@
     i->first->Sync(indicators[i->first]);
   }
 
-  // Clean up the SyncData.  NOTE: don't use find when passing in a raw
-  // pointer due to explicit construction of the shared pointer.  Could write
-  // a predicate, but often a for loop is easier to understand.
-  data->SyncComplete();
-  for (PendingSyncs::iterator i = pending_syncs_.begin(), end = pending_syncs_.end();
-       i != end; ++i)
-  {
-    if (i->get() == data)
-    {
-      pending_syncs_.erase(i);
-      break;
-    }
-  }
-
   // Notify listeners we have new data
   owner_->on_synced.emit();
 }
@@ -364,7 +381,7 @@
 void DBusIndicators::Impl::SyncGeometries(std::string const& name,
                                           EntryLocationMap const& locations)
 {
-  if (!proxy_)
+  if (!gproxy_.IsConnected())
     return;
 
   GVariantBuilder b;
@@ -414,116 +431,17 @@
   }
 
   g_variant_builder_close(&b);
-  g_dbus_proxy_call(proxy_, "SyncGeometries",
-                    g_variant_builder_end(&b),
-                    G_DBUS_CALL_FLAGS_NONE,
-                    -1,
-                    NULL,
-                    NULL,
-                    NULL);
 
+  gproxy_.Call("SyncGeometries", g_variant_builder_end(&b));
   cached_loc = locations;
 }
 
-std::string DBusIndicators::Impl::name() const
-{
-  glib::String name;
-  g_object_get(proxy_, "g-name", &name, NULL);
-  return name.Str();
-}
-
-std::string DBusIndicators::Impl::owner_name() const
-{
-  glib::String owner_name;
-  g_object_get(proxy_, "g-name-owner", &owner_name, NULL);
-  return owner_name.Str();
-}
-
-bool DBusIndicators::Impl::using_local_service() const
-{
-  return g_getenv("PANEL_USE_LOCAL_SERVICE") != NULL;
-}
-
-void DBusIndicators::Impl::OnProxyNameOwnerChanged(GDBusProxy* proxy,
-                                                   GParamSpec* pspec)
-{
-  char* name_owner = g_dbus_proxy_get_name_owner(proxy);
-
-  if (name_owner == NULL)
-  {
-    for (auto indicator : owner_->GetIndicators())
-    {
-      owner_->RemoveIndicator(indicator->name());
-    }
-
-    // The panel service has stopped for some reason.  Restart it if not in
-    // dev mode
-    if (!g_getenv("UNITY_DEV_MODE"))
-      Reconnect();
-  }
-
-  g_free(name_owner);
-}
-
-void DBusIndicators::Impl::OnProxySignalReceived(GDBusProxy* proxy,
-                                                 char* sender_name,
-                                                 char* signal_name_,
-                                                 GVariant* parameters)
-{
-  std::string signal_name(signal_name_);
-  if (signal_name == "EntryActivated")
-  {
-    glib::String entry_name;
-    nux::Rect geo;
-    g_variant_get(parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y, &geo.width, &geo.height);
-
-    if (entry_name)
-    {
-      owner_->ActivateEntry(entry_name.Str(), geo);
-    }
-  }
-  else if (signal_name == "EntryActivateRequest")
-  {
-    const char* entry_name = g_variant_get_string(g_variant_get_child_value(parameters, 0), NULL);
-    if (entry_name)
-    {
-      owner_->on_entry_activate_request.emit(entry_name);
-    }
-  }
-  else if (signal_name == "ReSync")
-  {
-    const char* id = g_variant_get_string(g_variant_get_child_value(parameters, 0), NULL);
-    bool sync_one = !g_strcmp0(id, "") == 0;
-
-    if (sync_one)
-    {
-      RequestSyncIndicator(id);
-    }
-    else
-    {
-      RequestSyncAll();
-    }
-  }
-  else if (signal_name == "EntryShowNowChanged")
-  {
-    gchar*    id = NULL;
-    gboolean  show_now;
-
-    g_variant_get(parameters, "(sb)", &id, &show_now);
-    owner_->SetEntryShowNow(id, show_now);
-
-    g_free(id);
-  }
-}
-
 DBusIndicators::DBusIndicators()
   : pimpl(new Impl(this))
 {}
 
 DBusIndicators::~DBusIndicators()
-{
-  delete pimpl;
-}
+{}
 
 void DBusIndicators::SyncGeometries(std::string const& name,
                                     EntryLocationMap const& locations)
@@ -555,171 +473,5 @@
   pimpl->OnShowAppMenu(xid, x, y, timestamp);
 }
 
-std::string DBusIndicators::name() const
-{
-  return pimpl->name();
-}
-
-std::string DBusIndicators::owner_name() const
-{
-  return pimpl->owner_name();
-}
-
-bool DBusIndicators::using_local_service() const
-{
-  return pimpl->using_local_service();
-}
-
-
-namespace
-{
-
-// Initialise DBus for the panel service, and let us know when it is
-// ready.  The unused bool return is to fit with the GSourceFunc.
-gboolean reconnect_to_service(gpointer data)
-{
-  g_dbus_proxy_new_for_bus(G_BUS_TYPE_SESSION,
-                           G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES,
-                           NULL,
-                           S_NAME,
-                           S_PATH,
-                           S_IFACE,
-                           NULL,
-                           on_proxy_ready_cb,
-                           data);
-  return false;
-}
-
-// Make sure the proxy object exists and has a name, and if so, pass
-// that on to the DBusIndicators.
-void on_proxy_ready_cb(GObject* source, GAsyncResult* res, gpointer data)
-{
-  DBusIndicators::Impl* remote = reinterpret_cast<DBusIndicators::Impl*>(data);
-  GError* error = NULL;
-  GDBusProxy* proxy = g_dbus_proxy_new_for_bus_finish(res, &error);
-
-  static bool force_tried = false;
-
-  char* name_owner;
-  if (G_IS_DBUS_PROXY(proxy) && (name_owner = g_dbus_proxy_get_name_owner(proxy)))
-  {
-    remote->OnRemoteProxyReady(G_DBUS_PROXY(proxy));
-    g_free(name_owner);
-    return;
-  }
-  else
-  {
-    if (force_tried)
-    {
-      g_warning("WARNING: Unable to connect to the unity-panel-service %s",
-                error ? error->message : "Unknown");
-      if (error)
-        g_error_free(error);
-    }
-    else
-    {
-      force_tried = true;
-      run_local_panel_service();
-      g_timeout_add_seconds(2, reconnect_to_service, remote);
-    }
-  }
-
-  g_object_unref(proxy);
-}
-
-
-bool run_local_panel_service()
-{
-  GError* error = NULL;
-
-  // This is obviously hackish, but this part of the code is mostly hackish...
-  // Let's attempt to run it from where we expect it to be
-  std::string cmd = PREFIXDIR + std::string("/lib/unity/unity-panel-service");
-  std::cerr << "\nWARNING: Couldn't load panel from installed services, "
-            << "so trying to load panel from known location: "
-            << cmd << "\n";
-
-  g_spawn_command_line_async(cmd.c_str(), &error);
-  if (error)
-  {
-    std::cerr << "\nWARNING: Unable to launch remote service manually: "
-              << error->message << "\n";
-    g_error_free(error);
-    return false;
-  }
-  return true;
-}
-
-void request_sync(GDBusProxy* proxy, const char* method, GVariant* name, SyncData* data)
-{
-  g_dbus_proxy_call(proxy, method, name, G_DBUS_CALL_FLAGS_NONE,
-                    -1, data->cancel, on_sync_ready_cb, data);
-}
-
-void on_sync_ready_cb(GObject* source, GAsyncResult* res, gpointer data)
-{
-  SyncData* sync_data = reinterpret_cast<SyncData*>(data);
-  GError* error = NULL;
-  GVariant* args = g_dbus_proxy_call_finish((GDBusProxy*)source, res, &error);
-
-  if (args == NULL)
-  {
-    g_warning("Unable to perform Sync() on panel service: %s", error->message);
-    g_error_free(error);
-    return;
-  }
-
-  sync_data->self->Sync(args, sync_data);
-  g_variant_unref(args);
-}
-
-bool send_show_entry(ShowEntryData* data)
-{
-  g_return_val_if_fail(data != NULL, FALSE);
-  g_return_val_if_fail(G_IS_DBUS_PROXY(data->proxy.RawPtr()), FALSE);
-
-  g_dbus_proxy_call(data->proxy,
-                    "ShowEntry",
-                    g_variant_new("(suiiuu)",
-                                  data->entry_id.c_str(),
-                                  data->xid,
-                                  data->x,
-                                  data->y,
-                                  data->button,
-                                  data->timestamp),
-                    G_DBUS_CALL_FLAGS_NONE,
-                    -1,
-                    NULL,
-                    NULL,
-                    NULL);
-
-  delete data;
-  return FALSE;
-}
-
-bool send_show_appmenu(ShowEntryData* data)
-{
-  g_return_val_if_fail(data != NULL, FALSE);
-  g_return_val_if_fail(G_IS_DBUS_PROXY(data->proxy.RawPtr()), FALSE);
-
-  g_dbus_proxy_call(data->proxy,
-                    "ShowAppMenu",
-                    g_variant_new("(uiiu)",
-                                  data->xid,
-                                  data->x,
-                                  data->y,
-                                  data->timestamp),
-                    G_DBUS_CALL_FLAGS_NONE,
-                    -1,
-                    NULL,
-                    NULL,
-                    NULL);
-
-  delete data;
-  return FALSE;
-}
-
-} // anonymous namespace
-
 } // namespace indicator
 } // namespace unity

=== modified file 'UnityCore/DBusIndicators.h'
--- UnityCore/DBusIndicators.h	2012-03-20 10:16:27 +0000
+++ UnityCore/DBusIndicators.h	2012-03-20 10:16:27 +0000
@@ -50,15 +50,9 @@
   virtual void OnShowAppMenu(unsigned int xid, int x, int y,
                              unsigned int timestamp);
 
-  std::string name() const;
-  std::string owner_name() const;
-  bool using_local_service() const;
-
-  // Due to the callback nature, the Impl class must be declared public, but
-  // it is not available to anyone except the internal implementation.
+private:
   class Impl;
-private:
-  Impl* pimpl;
+  std::unique_ptr<Impl> pimpl;
 };
 
 }

=== modified file 'UnityCore/GLibDBusProxy.cpp'
--- UnityCore/GLibDBusProxy.cpp	2012-02-20 09:29:55 +0000
+++ UnityCore/GLibDBusProxy.cpp	2012-03-20 10:16:27 +0000
@@ -39,13 +39,6 @@
 
 using std::string;
 
-struct CallData
-{
-  DBusProxy::ReplyCallback callback;
-  DBusProxy::Impl* impl;
-  std::string method_name;
-};
-
 class DBusProxy::Impl
 {
 public:
@@ -62,8 +55,7 @@
 
   void StartReconnectionTimeout();
   void Connect();
-  void OnProxySignal(GDBusProxy* proxy, char* sender_name, char* signal_name,
-                     GVariant* parameters);
+
   void Call(string const& method_name,
             GVariant* parameters,
             ReplyCallback callback,
@@ -74,12 +66,20 @@
   void Connect(string const& signal_name, ReplyCallback callback);
   bool IsConnected();
 
-  static void OnNameAppeared(GDBusConnection* connection, const char* name,
-                             const char* name_owner, gpointer impl);
-  static void OnNameVanished(GDBusConnection* connection, const char* name, gpointer impl);
+  void OnProxyNameOwnerChanged(GDBusProxy*, GParamSpec*);
+  void OnProxySignal(GDBusProxy* proxy, char* sender_name, char* signal_name,
+                     GVariant* parameters);
+
   static void OnProxyConnectCallback(GObject* source, GAsyncResult* res, gpointer impl);
   static void OnCallCallback(GObject* source, GAsyncResult* res, gpointer call_data);
- 
+
+  struct CallData
+  {
+    DBusProxy::ReplyCallback callback;
+    DBusProxy::Impl* impl;
+    std::string method_name;
+  };
+
   DBusProxy* owner_;
   string name_;
   string object_path_;
@@ -94,6 +94,7 @@
   bool connected_;
 
   glib::Signal<void, GDBusProxy*, char*, char*, GVariant*> g_signal_connection_;
+  glib::Signal<void, GDBusProxy*, GParamSpec*> name_owner_signal_;
 
   SignalHandlers handlers_;
 };
@@ -115,14 +116,6 @@
   , reconnect_timeout_id_(0)
   , connected_(false)
 {
-  LOG_DEBUG(logger) << "Watching name " << name_;
-  watcher_id_ = g_bus_watch_name(bus_type_,
-                                 name.c_str(),
-                                 G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
-                                 DBusProxy::Impl::OnNameAppeared,
-                                 DBusProxy::Impl::OnNameVanished,
-                                 this,
-                                 NULL);
   StartReconnectionTimeout();
 }
 
@@ -135,33 +128,6 @@
     g_source_remove(reconnect_timeout_id_);
 }
 
-void DBusProxy::Impl::OnNameAppeared(GDBusConnection* connection,
-                                     const char* name,
-                                     const char* name_owner,
-                                     gpointer impl)
-{
-  DBusProxy::Impl* self = static_cast<DBusProxy::Impl*>(impl);
-  LOG_DEBUG(logger) << self->name_ << " appeared";
-
-  if (self->proxy_)
-  {
-    self->connected_ = true;
-    self->owner_->connected.emit();
-  }
-}
-
-void DBusProxy::Impl::OnNameVanished(GDBusConnection* connection,
-                                     const char* name,
-                                     gpointer impl)
-{
-  DBusProxy::Impl* self = static_cast<DBusProxy::Impl*>(impl);
-
-  LOG_DEBUG(logger) << self->name_ << " vanished";
-
-  self->connected_ = false;
-  self->owner_->disconnected.emit();
-}
-
 void DBusProxy::Impl::StartReconnectionTimeout()
 {
   LOG_DEBUG(logger) << "Starting reconnection timeout for " << name_;
@@ -204,6 +170,8 @@
                                              GAsyncResult* res,
                                              gpointer impl)
 {
+  DBusProxy::Impl* self = static_cast<DBusProxy::Impl*>(impl);
+
   glib::Error error;
   glib::Object<GDBusProxy> proxy(g_dbus_proxy_new_for_bus_finish(res, &error));
 
@@ -215,14 +183,44 @@
     return;
   }
 
-  DBusProxy::Impl* self = static_cast<DBusProxy::Impl*>(impl);
   LOG_DEBUG(logger) << "Sucessfully created proxy: " << self->object_path_;
 
   self->proxy_ = proxy;
   self->g_signal_connection_.Connect(self->proxy_, "g-signal",
-                                     sigc::mem_fun(self, &DBusProxy::Impl::OnProxySignal));
-  self->connected_ = true;
-  self->owner_->connected.emit();
+                                     sigc::mem_fun(self, &Impl::OnProxySignal));
+  self->name_owner_signal_.Connect(self->proxy_, "notify::g-name-owner",
+                                   sigc::mem_fun(self, &Impl::OnProxyNameOwnerChanged));
+
+  // If a proxy cannot autostart a service, it doesn't throw an error, but
+  // sets name_owner to NULL
+  if (glib::String(g_dbus_proxy_get_name_owner(proxy)))
+  {
+    self->connected_ = true;
+    self->owner_->connected.emit();
+  }
+}
+
+void DBusProxy::Impl::OnProxyNameOwnerChanged(GDBusProxy* proxy, GParamSpec* param)
+{
+  glib::String name_owner(g_dbus_proxy_get_name_owner(proxy));
+
+  if (name_owner)
+  {
+    if (!connected_)
+    {
+      LOG_DEBUG(logger) << name_ << " appeared";
+
+      connected_ = true;
+      owner_->connected.emit();
+    }
+  }
+  else if (connected_)
+  {
+    LOG_DEBUG(logger) << name_ << " vanished";
+
+    connected_ = false;
+    owner_->disconnected.emit();
+  }
 }
 
 void DBusProxy::Impl::OnProxySignal(GDBusProxy* proxy,
@@ -308,9 +306,7 @@
 {}
 
 DBusProxy::~DBusProxy()
-{
-  delete pimpl;
-}
+{}
 
 void DBusProxy::Call(string const& method_name,
                      GVariant* parameters,

=== modified file 'UnityCore/GLibDBusProxy.h'
--- UnityCore/GLibDBusProxy.h	2012-02-04 05:28:23 +0000
+++ UnityCore/GLibDBusProxy.h	2012-03-20 10:16:27 +0000
@@ -62,9 +62,9 @@
   static void NoReplyCallback(GVariant* v) {};
 
   // Public due to use in some callbacks
+private:
   class Impl;
-private:
-  Impl* pimpl;
+  std::unique_ptr<Impl> pimpl;
 };
 
 }

=== modified file 'UnityCore/Indicators.cpp'
--- UnityCore/Indicators.cpp	2012-03-20 10:16:27 +0000
+++ UnityCore/Indicators.cpp	2012-03-20 10:16:27 +0000
@@ -58,13 +58,10 @@
 
 Indicators::Indicators()
   : pimpl(new Impl(this))
-{
-}
+{}
 
 Indicators::~Indicators()
-{
-  delete pimpl;
-}
+{}
 
 void Indicators::ActivateEntry(std::string const& entry_id, nux::Rect const& geometry)
 {

=== modified file 'UnityCore/Indicators.h'
--- UnityCore/Indicators.h	2012-03-20 10:16:27 +0000
+++ UnityCore/Indicators.h	2012-03-20 10:16:27 +0000
@@ -124,7 +124,7 @@
 
 private:
   class Impl;
-  Impl* pimpl;
+  std::unique_ptr<Impl> pimpl;
 };
 
 }

=== modified file 'plugins/unityshell/src/PanelView.cpp'
--- plugins/unityshell/src/PanelView.cpp	2012-03-20 10:16:27 +0000
+++ plugins/unityshell/src/PanelView.cpp	2012-03-20 10:16:27 +0000
@@ -215,9 +215,6 @@
 {
   variant::BuilderWrapper(builder)
   .add("backend", "remote")
-  .add("service-name", _remote->name())
-  .add("service-unique-name", _remote->owner_name())
-  .add("using-local-service", _remote->using_local_service())
   .add(GetGeometry());
 }