Merge lp:~vanvugt/unity/fix-742664-trunk into lp:unity

[Daniel van Vugt]

Make panel menus more responsive, less laggy, when scrubbing. Removed logic that used to send pointer motion events over dbus. (LP: #742664)

Currently unity-panel-service receives all pointer motion (scrubbing) events and sends them ALL to unity(compiz) via dbus. unity(compiz) then decides which indicator/menu this is and sends a message back to unity-panel-service telling it which menu to display. Once the menu is visible unity-panel-service sends another message back to unity(compiz) so it can then display the panel menu background. So that's 3 interprocess communication stages for every motion event. Not to mention the fact that it's responding to every single motion event which is known to be bad practice in GUI design.

This proposal reduces the number of interprocess communication stages between pointer movement and the panel repainting from 3 to 0 (but still 1 for the menu to draw). So the end result is that indicators and panel menus feel much more responsive and always draw where the mouse pointer actually is.

Disclaimer: I developed and tested this fix on nattywith unity 3. I have only hand-ported it to unity-4 and made sure it builds in oneiric. So there has been no functional testing on oneiric yet, only natty.

[Andrea Azzarone (azzar1)]

Review: Need Fixing

87 +static
88 +gboolean
89 +track_menu_pointer (gpointer data)

Should be

static gboolean track_menu_pointer(gpointer data).

91 + PanelView *self = (PanelView*)data;

Please avoid C cast.

93 + gdk_display_get_pointer (gdk_display_get_default (), NULL, &x, &y, NULL);

Please no space between the function name and the open parenthesis.

_menu_view->AllMenusClosed ();

The same applies here.

Please note that I have reviewed only the style roughly.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

A C cast is correct for gpointer data because it is the parameter to a C callback (g_timeout_add). Not C++.

And I agree with no space after function names. Was only copying the style that the unity code already used :)

[Neil J. Patel (njpatel)]

Excellent work.

review: Approve

=== modified file 'UnityCore/DBusIndicators.cpp'
--- UnityCore/DBusIndicators.cpp	2011-07-21 20:24:04 +0000
+++ UnityCore/DBusIndicators.cpp	2011-07-29 08:57:18 +0000
@@ -438,13 +438,6 @@
       RequestSyncAll();
     }
   }
-  else if (signal_name == "ActiveMenuPointerMotion")
-  {
-    int x = 0;
-    int y = 0;
-    g_variant_get(parameters, "(ii)", &x, &y);
-    owner_->on_menu_pointer_moved.emit(x, y);
-  }
   else if (signal_name == "EntryShowNowChanged")
   {
     gchar*    id = NULL;

=== modified file 'UnityCore/Indicators.h'
--- UnityCore/Indicators.h	2011-07-21 20:24:04 +0000
+++ UnityCore/Indicators.h	2011-07-29 08:57:18 +0000
@@ -64,14 +64,6 @@
   sigc::signal<void, Indicator::Ptr const&> on_object_removed;
 
   /**
-   * This signal is emitted when an entry is activated and the user moves the
-   * mouse.
-   * @param x x coordinate
-   * @param y y coordinate
-   */
-  sigc::signal<void, int, int> on_menu_pointer_moved;
-
-  /**
    * Service wants the view to activate an entry.
    * Example use-case: user has activated an entry with the mouse and pressed
    * Left or Right key to activate previous or next entry.

=== modified file 'plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp'
--- plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp	2011-07-22 14:04:58 +0000
+++ plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp	2011-07-29 08:57:18 +0000
@@ -101,11 +101,15 @@
                      GetAbsoluteY() + PANEL_HEIGHT,
                      time(NULL),
                      nux::GetEventButton(button_flags));
-  }
-  else
-  {
-    Refresh();
-  }
+    proxy_->set_active(true);
+    //
+    // ^ Set active even before the menu appears. This allows the below
+    //   Refresh call to know it should draw_menu_bg() immediately
+    //   rather than waiting for slow inter-process communication with
+    //   unity-panel-service, which causes visible lag in many cases.
+    //
+  }
+  Refresh();
 }
 
 void PanelIndicatorObjectEntryView::OnMouseUp(int x, int y, long button_flags, long key_flags)

=== modified file 'plugins/unityshell/src/PanelView.cpp'
--- plugins/unityshell/src/PanelView.cpp	2011-07-28 11:02:56 +0000
+++ plugins/unityshell/src/PanelView.cpp	2011-07-29 08:57:18 +0000
@@ -79,7 +79,6 @@
 
   _remote = indicator::DBusIndicators::Ptr(new indicator::DBusIndicators());
   _remote->on_object_added.connect(sigc::mem_fun(this, &PanelView::OnObjectAdded));
-  _remote->on_menu_pointer_moved.connect(sigc::mem_fun(this, &PanelView::OnMenuPointerMoved));
   _remote->on_entry_activate_request.connect(sigc::mem_fun(this, &PanelView::OnEntryActivateRequest));
   _remote->on_entry_activated.connect(sigc::mem_fun(this, &PanelView::OnEntryActivated));
   _remote->on_synced.connect(sigc::mem_fun(this, &PanelView::OnSynced));
@@ -100,10 +99,14 @@
                                                      this);
    // request the latest colour from bghash
    ubus_server_send_message (ubus, UBUS_BACKGROUND_REQUEST_COLOUR_EMIT, NULL);
+
+  _track_menu_pointer_id = 0;
 }
 
 PanelView::~PanelView()
 {
+  if (_track_menu_pointer_id)
+    g_source_remove(_track_menu_pointer_id);
   _style->UnReference();
   UBusServer *ubus = ubus_server_get_default();
   ubus_server_unregister_interest(ubus, _handle_bg_color_update);
@@ -343,10 +346,41 @@
   }
 }
 
+static gboolean track_menu_pointer(gpointer data)
+{
+  PanelView *self = (PanelView*)data;
+  gint x, y;
+  gdk_display_get_pointer(gdk_display_get_default(), NULL, &x, &y, NULL);
+  self->OnMenuPointerMoved(x, y);
+  return TRUE;
+}
+
 void PanelView::OnEntryActivated(std::string const& entry_id)
 {
-  if (entry_id == "")
+  bool active = (entry_id.size() > 0);
+  if (active && !_track_menu_pointer_id)
+  {
+    //
+    // Track menus being scrubbed at 60Hz (about every 16 millisec)
+    // It might sound ugly, but it's far nicer (and more responsive) than the
+    // code it replaces which used to capture motion events in another process
+    // (unity-panel-service) and send them to us over dbus.
+    // NOTE: The reason why we have to use a timer instead of tracking motion
+    // events is because the motion events will never be delivered to this
+    // process. All the motion events will go to unity-panel-service while
+    // scrubbing because the active panel menu has (needs) the pointer grab.
+    //
+    _track_menu_pointer_id = g_timeout_add(16, track_menu_pointer, this);
+  }
+  else if (!active)
+  {
+    if (_track_menu_pointer_id)
+    {
+      g_source_remove(_track_menu_pointer_id);
+      _track_menu_pointer_id = 0;
+    }
     _menu_view->AllMenusClosed();
+  }
 }
 
 void PanelView::OnSynced()

=== modified file 'plugins/unityshell/src/PanelView.h'
--- plugins/unityshell/src/PanelView.h	2011-07-28 11:02:56 +0000
+++ plugins/unityshell/src/PanelView.h	2011-07-29 08:57:18 +0000
@@ -116,6 +116,7 @@
   guint       _handle_dash_hidden;
   guint       _handle_dash_shown;
   guint       _handle_bg_color_update;
+  guint       _track_menu_pointer_id;
 };
 
 }

=== modified file 'services/panel-main.c'
--- services/panel-main.c	2011-07-21 20:24:04 +0000
+++ services/panel-main.c	2011-07-29 08:57:18 +0000
@@ -85,11 +85,6 @@
   "     <arg type='s' name='indicator_id' />"
   "    </signal>"
   ""
-  "    <signal name='ActiveMenuPointerMotion'>"
-  "     <arg type='i' name='x' />"
-  "     <arg type='i' name='y' />"
-  "    </signal>"
-  ""
   "    <signal name='EntryActivateRequest'>"
   "     <arg type='s' name='entry_id' />"
   "    </signal>"
@@ -254,30 +249,6 @@
 }
 
 static void
-on_service_active_menu_pointer_motion (PanelService    *service,
-                                       GDBusConnection *connection)
-{
-  GError *error = NULL;
-  gint    x=0, y=0;
-
-  panel_service_get_last_xy (service, &x, &y);
-
-  g_dbus_connection_emit_signal (connection,
-                                 S_NAME,
-                                 S_PATH,
-                                 S_IFACE,
-                                 "ActiveMenuPointerMotion",
-                                 g_variant_new ("(ii)", x, y),
-                                 &error);
-
-  if (error)
-    {
-      g_warning ("Unable to emit ActiveMenuPointerMotionsignal: %s", error->message);
-      g_error_free (error);
-    }
-}
-
-static void
 on_service_entry_activate_request (PanelService    *service,
                                    const gchar     *entry_id,
                                    GDBusConnection *connection)
@@ -339,8 +310,6 @@
                     G_CALLBACK (on_service_resync), connection);
   g_signal_connect (service, "entry-activated",
                     G_CALLBACK (on_service_entry_activated), connection);
-  g_signal_connect (service, "active-menu-pointer-motion",
-                    G_CALLBACK (on_service_active_menu_pointer_motion), connection);
   g_signal_connect (service, "entry-activate-request",
                     G_CALLBACK (on_service_entry_activate_request), connection);
   g_signal_connect (service, "entry-show-now-changed",
@@ -370,7 +339,6 @@
   {
     g_signal_handlers_disconnect_by_func (service, on_service_resync, connection);
     g_signal_handlers_disconnect_by_func (service, on_service_entry_activated, connection);
-    g_signal_handlers_disconnect_by_func (service, on_service_active_menu_pointer_motion, connection);
     g_signal_handlers_disconnect_by_func (service, on_service_entry_activate_request, connection);
     g_signal_handlers_disconnect_by_func (service, on_service_entry_show_now_changed, connection);
   }

=== modified file 'services/panel-service.c'
--- services/panel-service.c	2011-07-22 14:05:38 +0000
+++ services/panel-service.c	2011-07-29 08:57:18 +0000
@@ -62,9 +62,6 @@
   gint     last_right;
   gint     last_bottom;
   guint32  last_menu_button;
-
-  gint     last_menu_x;
-  gint     last_menu_y;
 };
 
 /* Globals */
@@ -74,7 +71,6 @@
 {
   ENTRY_ACTIVATED = 0,
   RE_SYNC,
-  ACTIVE_MENU_POINTER_MOTION,
   ENTRY_ACTIVATE_REQUEST,
   ENTRY_SHOW_NOW_CHANGED,
   GEOMETRIES_CHANGED,
@@ -173,15 +169,6 @@
                   g_cclosure_marshal_VOID__STRING,
                   G_TYPE_NONE, 1, G_TYPE_STRING);
 
- _service_signals[ACTIVE_MENU_POINTER_MOTION] =
-    g_signal_new ("active-menu-pointer-motion",
-                  G_OBJECT_CLASS_TYPE (obj_class),
-                  G_SIGNAL_RUN_LAST,
-                  0,
-                  NULL, NULL,
-                  g_cclosure_marshal_VOID__VOID,
-                  G_TYPE_NONE, 0);
-
  _service_signals[ENTRY_ACTIVATE_REQUEST] =
     g_signal_new ("entry-activate-request",
                   G_OBJECT_CLASS_TYPE (obj_class),
@@ -249,16 +236,6 @@
 
           priv->last_menu_button = 0;
         }
-      else if (event && event->evtype == XI_Motion)
-        {
-          priv->last_menu_x = event->root_x;
-          priv->last_menu_y = event->root_y;
-
-          if (priv->last_menu_y <= priv->last_y)
-            {
-              g_signal_emit (self, _service_signals[ACTIVE_MENU_POINTER_MOTION], 0);
-            }
-        }
     }
 
   return ret;
@@ -1185,11 +1162,3 @@
                         abs(delta/120), direction);
 }
 
-void
-panel_service_get_last_xy   (PanelService  *self,
-                             gint          *x,
-                             gint          *y)
-{
-  *x = self->priv->last_menu_x;
-  *y = self->priv->last_menu_y;
-}

=== modified file 'services/panel-service.h'
--- services/panel-service.h	2011-07-21 20:24:04 +0000
+++ services/panel-service.h	2011-07-29 08:57:18 +0000
@@ -103,10 +103,6 @@
 						const gchar  *entry_id,
 						gint32       delta);
 
-void              panel_service_get_last_xy   (PanelService  *self,
-					       gint          *x,
-					       gint          *y);
-
 G_END_DECLS
 
 #endif /* _PANEL_SERVICE_H_ */