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

Proposed by Daniel van Vugt on 2011-07-29
Status: Merged
Approved by: Neil J. Patel on 2011-08-01
Approved revision: 1319
Merged at revision: 1325
Proposed branch: lp:~vanvugt/unity/fix-742664-trunk
Merge into: lp:unity
Diff against target: 289 lines (+46/-89)
8 files modified
UnityCore/DBusIndicators.cpp (+0/-7)
UnityCore/Indicators.h (+0/-8)
plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp (+9/-5)
plugins/unityshell/src/PanelView.cpp (+36/-2)
plugins/unityshell/src/PanelView.h (+1/-0)
services/panel-main.c (+0/-32)
services/panel-service.c (+0/-31)
services/panel-service.h (+0/-4)
To merge this branch: bzr merge lp:~vanvugt/unity/fix-742664-trunk
Reviewer Review Type Date Requested Status
Neil J. Patel (community) 2011-07-29 Approve on 2011-08-01
Andrea Azzarone (community) Needs Fixing on 2011-07-29
Review via email: mp+69751@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Andrea Azzarone (azzar1) wrote :

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) wrote :

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 :)

lp:~vanvugt/unity/fix-742664-trunk updated on 2011-07-29
1318. By Daniel van Vugt on 2011-07-29

Minor style and spacing cleanup.

1319. By Daniel van Vugt on 2011-07-29

Add missing destructor code, got lost when porting the change from
unity-3 to unity-4.

Neil J. Patel (njpatel) wrote :

Excellent work.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'UnityCore/DBusIndicators.cpp'
2--- UnityCore/DBusIndicators.cpp 2011-07-21 20:24:04 +0000
3+++ UnityCore/DBusIndicators.cpp 2011-07-29 08:57:18 +0000
4@@ -438,13 +438,6 @@
5 RequestSyncAll();
6 }
7 }
8- else if (signal_name == "ActiveMenuPointerMotion")
9- {
10- int x = 0;
11- int y = 0;
12- g_variant_get(parameters, "(ii)", &x, &y);
13- owner_->on_menu_pointer_moved.emit(x, y);
14- }
15 else if (signal_name == "EntryShowNowChanged")
16 {
17 gchar* id = NULL;
18
19=== modified file 'UnityCore/Indicators.h'
20--- UnityCore/Indicators.h 2011-07-21 20:24:04 +0000
21+++ UnityCore/Indicators.h 2011-07-29 08:57:18 +0000
22@@ -64,14 +64,6 @@
23 sigc::signal<void, Indicator::Ptr const&> on_object_removed;
24
25 /**
26- * This signal is emitted when an entry is activated and the user moves the
27- * mouse.
28- * @param x x coordinate
29- * @param y y coordinate
30- */
31- sigc::signal<void, int, int> on_menu_pointer_moved;
32-
33- /**
34 * Service wants the view to activate an entry.
35 * Example use-case: user has activated an entry with the mouse and pressed
36 * Left or Right key to activate previous or next entry.
37
38=== modified file 'plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp'
39--- plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp 2011-07-22 14:04:58 +0000
40+++ plugins/unityshell/src/PanelIndicatorObjectEntryView.cpp 2011-07-29 08:57:18 +0000
41@@ -101,11 +101,15 @@
42 GetAbsoluteY() + PANEL_HEIGHT,
43 time(NULL),
44 nux::GetEventButton(button_flags));
45- }
46- else
47- {
48- Refresh();
49- }
50+ proxy_->set_active(true);
51+ //
52+ // ^ Set active even before the menu appears. This allows the below
53+ // Refresh call to know it should draw_menu_bg() immediately
54+ // rather than waiting for slow inter-process communication with
55+ // unity-panel-service, which causes visible lag in many cases.
56+ //
57+ }
58+ Refresh();
59 }
60
61 void PanelIndicatorObjectEntryView::OnMouseUp(int x, int y, long button_flags, long key_flags)
62
63=== modified file 'plugins/unityshell/src/PanelView.cpp'
64--- plugins/unityshell/src/PanelView.cpp 2011-07-28 11:02:56 +0000
65+++ plugins/unityshell/src/PanelView.cpp 2011-07-29 08:57:18 +0000
66@@ -79,7 +79,6 @@
67
68 _remote = indicator::DBusIndicators::Ptr(new indicator::DBusIndicators());
69 _remote->on_object_added.connect(sigc::mem_fun(this, &PanelView::OnObjectAdded));
70- _remote->on_menu_pointer_moved.connect(sigc::mem_fun(this, &PanelView::OnMenuPointerMoved));
71 _remote->on_entry_activate_request.connect(sigc::mem_fun(this, &PanelView::OnEntryActivateRequest));
72 _remote->on_entry_activated.connect(sigc::mem_fun(this, &PanelView::OnEntryActivated));
73 _remote->on_synced.connect(sigc::mem_fun(this, &PanelView::OnSynced));
74@@ -100,10 +99,14 @@
75 this);
76 // request the latest colour from bghash
77 ubus_server_send_message (ubus, UBUS_BACKGROUND_REQUEST_COLOUR_EMIT, NULL);
78+
79+ _track_menu_pointer_id = 0;
80 }
81
82 PanelView::~PanelView()
83 {
84+ if (_track_menu_pointer_id)
85+ g_source_remove(_track_menu_pointer_id);
86 _style->UnReference();
87 UBusServer *ubus = ubus_server_get_default();
88 ubus_server_unregister_interest(ubus, _handle_bg_color_update);
89@@ -343,10 +346,41 @@
90 }
91 }
92
93+static gboolean track_menu_pointer(gpointer data)
94+{
95+ PanelView *self = (PanelView*)data;
96+ gint x, y;
97+ gdk_display_get_pointer(gdk_display_get_default(), NULL, &x, &y, NULL);
98+ self->OnMenuPointerMoved(x, y);
99+ return TRUE;
100+}
101+
102 void PanelView::OnEntryActivated(std::string const& entry_id)
103 {
104- if (entry_id == "")
105+ bool active = (entry_id.size() > 0);
106+ if (active && !_track_menu_pointer_id)
107+ {
108+ //
109+ // Track menus being scrubbed at 60Hz (about every 16 millisec)
110+ // It might sound ugly, but it's far nicer (and more responsive) than the
111+ // code it replaces which used to capture motion events in another process
112+ // (unity-panel-service) and send them to us over dbus.
113+ // NOTE: The reason why we have to use a timer instead of tracking motion
114+ // events is because the motion events will never be delivered to this
115+ // process. All the motion events will go to unity-panel-service while
116+ // scrubbing because the active panel menu has (needs) the pointer grab.
117+ //
118+ _track_menu_pointer_id = g_timeout_add(16, track_menu_pointer, this);
119+ }
120+ else if (!active)
121+ {
122+ if (_track_menu_pointer_id)
123+ {
124+ g_source_remove(_track_menu_pointer_id);
125+ _track_menu_pointer_id = 0;
126+ }
127 _menu_view->AllMenusClosed();
128+ }
129 }
130
131 void PanelView::OnSynced()
132
133=== modified file 'plugins/unityshell/src/PanelView.h'
134--- plugins/unityshell/src/PanelView.h 2011-07-28 11:02:56 +0000
135+++ plugins/unityshell/src/PanelView.h 2011-07-29 08:57:18 +0000
136@@ -116,6 +116,7 @@
137 guint _handle_dash_hidden;
138 guint _handle_dash_shown;
139 guint _handle_bg_color_update;
140+ guint _track_menu_pointer_id;
141 };
142
143 }
144
145=== modified file 'services/panel-main.c'
146--- services/panel-main.c 2011-07-21 20:24:04 +0000
147+++ services/panel-main.c 2011-07-29 08:57:18 +0000
148@@ -85,11 +85,6 @@
149 " <arg type='s' name='indicator_id' />"
150 " </signal>"
151 ""
152- " <signal name='ActiveMenuPointerMotion'>"
153- " <arg type='i' name='x' />"
154- " <arg type='i' name='y' />"
155- " </signal>"
156- ""
157 " <signal name='EntryActivateRequest'>"
158 " <arg type='s' name='entry_id' />"
159 " </signal>"
160@@ -254,30 +249,6 @@
161 }
162
163 static void
164-on_service_active_menu_pointer_motion (PanelService *service,
165- GDBusConnection *connection)
166-{
167- GError *error = NULL;
168- gint x=0, y=0;
169-
170- panel_service_get_last_xy (service, &x, &y);
171-
172- g_dbus_connection_emit_signal (connection,
173- S_NAME,
174- S_PATH,
175- S_IFACE,
176- "ActiveMenuPointerMotion",
177- g_variant_new ("(ii)", x, y),
178- &error);
179-
180- if (error)
181- {
182- g_warning ("Unable to emit ActiveMenuPointerMotionsignal: %s", error->message);
183- g_error_free (error);
184- }
185-}
186-
187-static void
188 on_service_entry_activate_request (PanelService *service,
189 const gchar *entry_id,
190 GDBusConnection *connection)
191@@ -339,8 +310,6 @@
192 G_CALLBACK (on_service_resync), connection);
193 g_signal_connect (service, "entry-activated",
194 G_CALLBACK (on_service_entry_activated), connection);
195- g_signal_connect (service, "active-menu-pointer-motion",
196- G_CALLBACK (on_service_active_menu_pointer_motion), connection);
197 g_signal_connect (service, "entry-activate-request",
198 G_CALLBACK (on_service_entry_activate_request), connection);
199 g_signal_connect (service, "entry-show-now-changed",
200@@ -370,7 +339,6 @@
201 {
202 g_signal_handlers_disconnect_by_func (service, on_service_resync, connection);
203 g_signal_handlers_disconnect_by_func (service, on_service_entry_activated, connection);
204- g_signal_handlers_disconnect_by_func (service, on_service_active_menu_pointer_motion, connection);
205 g_signal_handlers_disconnect_by_func (service, on_service_entry_activate_request, connection);
206 g_signal_handlers_disconnect_by_func (service, on_service_entry_show_now_changed, connection);
207 }
208
209=== modified file 'services/panel-service.c'
210--- services/panel-service.c 2011-07-22 14:05:38 +0000
211+++ services/panel-service.c 2011-07-29 08:57:18 +0000
212@@ -62,9 +62,6 @@
213 gint last_right;
214 gint last_bottom;
215 guint32 last_menu_button;
216-
217- gint last_menu_x;
218- gint last_menu_y;
219 };
220
221 /* Globals */
222@@ -74,7 +71,6 @@
223 {
224 ENTRY_ACTIVATED = 0,
225 RE_SYNC,
226- ACTIVE_MENU_POINTER_MOTION,
227 ENTRY_ACTIVATE_REQUEST,
228 ENTRY_SHOW_NOW_CHANGED,
229 GEOMETRIES_CHANGED,
230@@ -173,15 +169,6 @@
231 g_cclosure_marshal_VOID__STRING,
232 G_TYPE_NONE, 1, G_TYPE_STRING);
233
234- _service_signals[ACTIVE_MENU_POINTER_MOTION] =
235- g_signal_new ("active-menu-pointer-motion",
236- G_OBJECT_CLASS_TYPE (obj_class),
237- G_SIGNAL_RUN_LAST,
238- 0,
239- NULL, NULL,
240- g_cclosure_marshal_VOID__VOID,
241- G_TYPE_NONE, 0);
242-
243 _service_signals[ENTRY_ACTIVATE_REQUEST] =
244 g_signal_new ("entry-activate-request",
245 G_OBJECT_CLASS_TYPE (obj_class),
246@@ -249,16 +236,6 @@
247
248 priv->last_menu_button = 0;
249 }
250- else if (event && event->evtype == XI_Motion)
251- {
252- priv->last_menu_x = event->root_x;
253- priv->last_menu_y = event->root_y;
254-
255- if (priv->last_menu_y <= priv->last_y)
256- {
257- g_signal_emit (self, _service_signals[ACTIVE_MENU_POINTER_MOTION], 0);
258- }
259- }
260 }
261
262 return ret;
263@@ -1185,11 +1162,3 @@
264 abs(delta/120), direction);
265 }
266
267-void
268-panel_service_get_last_xy (PanelService *self,
269- gint *x,
270- gint *y)
271-{
272- *x = self->priv->last_menu_x;
273- *y = self->priv->last_menu_y;
274-}
275
276=== modified file 'services/panel-service.h'
277--- services/panel-service.h 2011-07-21 20:24:04 +0000
278+++ services/panel-service.h 2011-07-29 08:57:18 +0000
279@@ -103,10 +103,6 @@
280 const gchar *entry_id,
281 gint32 delta);
282
283-void panel_service_get_last_xy (PanelService *self,
284- gint *x,
285- gint *y);
286-
287 G_END_DECLS
288
289 #endif /* _PANEL_SERVICE_H_ */