Merge lp:~ev/compiz/call-apport-on-hangs into lp:compiz/0.9.8

Proposed by Evan on 2012-07-04
Status: Work in progress
Proposed branch: lp:~ev/compiz/call-apport-on-hangs
Merge into: lp:compiz/0.9.8
Diff against target: 181 lines (+68/-1)
7 files modified
cmake/base.cmake (+3/-0)
config.h.core.in (+3/-0)
gtk/config.h.gtk.in (+3/-0)
gtk/window-decorator/forcequit.c (+39/-0)
gtk/window-decorator/gtk-window-decorator.h (+2/-0)
plugins/fade/src/fade.cpp (+16/-1)
plugins/fade/src/fade.h (+2/-0)
To merge this branch: bzr merge lp:~ev/compiz/call-apport-on-hangs
Reviewer Review Type Date Requested Status
Sam Spilsbury Needs Information on 2012-10-11
Daniel van Vugt 2012-07-04 Resubmit on 2012-09-28
Francis Ginther Abstain on 2012-08-01
jenkins (community) continuous-integration Needs Fixing on 2012-07-11
Review via email: mp+113436@code.launchpad.net

Description of the change

This branch adds a USE_APPORT flag. If set, when faced with a hanging application, compiz will call apport with the --hanging option (in apport trunk; I'll be releasing shortly), rather than show its own dialog. It will also automatically show the dialog if the fade plugin triggers.

By calling into apport in this way, we can track application hangs as errors on http://errors.ubuntu.com

I'll happily take care of coordinating the upload to Ubuntu, if the branch is accepted and no one else beats me to it. :)

To post a comment you must log in.
lp:~ev/compiz/call-apport-on-hangs updated on 2012-07-04
3274. By Evan on 2012-07-04

Report the error if we cannot spawn apport-gtk.

Sam Spilsbury (smspillaz) wrote :

this looks okay, although the design feels a bit weird to me.

145 +#ifdef USE_APPORT
146 + Time serverTime;
147 +
148 + if (!window->alive () && !forceQuitTriggered) {
149 + serverTime = screen->getCurrentTime ();
150 + screen->toolkitAction (Atoms::toolkitActionForceQuitDialog, serverTime, window->id (), true, 0, 0);
151 + forceQuitTriggered = true;
152 + }
153 +#endif
154 + if (window->alive ())
155 + forceQuitTriggered = false;
156
157 value = fScreen->optionGetUnresponsiveBrightness ();
158 if (value != 100)
159 @@ -361,7 +375,8 @@
160 fadeTime (0),
161 opacityDiff (0),
162 brightnessDiff (0),
163 - saturationDiff (0)
164 + saturationDiff (0),
165 + forceQuitTriggered (false)

Currently it pops up the apport dialog as soon as the application becomes unresponsive. There are quite a few cases where applications can become temporarily unresponsive and it isn't a bug because the system is just under really heavy load and the UI thread of every application is starved. It doesn't make sense to report those.

I noticed that because of this, you need to have checks in both gtk-window-decorator and in the fade plugin to see if the apport dialog has already come up.

What I'd suggest is just showing the dialog when the force quit dialog normally comes up - except it might be better to add a checkbox which says "report this unresponsive application"

Then you can make the code slightly more modular too by delegating the call through a standard interface that all reporters might accept.

This way you only have check if the apport dialog has appeared once, because the user will be closing and reporting the hang at the same time.

Also, some indentation notes:

72 + app = wnck_window_get_application (win);
73 + if (!app)
74 + return FALSE;

You need a one-tab indent for the return FALSE;

76 + pid = wnck_application_get_pid (app);
77 + if (d->apport_dialog) {
78 + /* Is Apport already running? */
79 + if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
80 + return TRUE;
81 + }

pid is indented incorrectly, the brace should fall on the next line, return TRUE needs an indent (4 spaces)

85 + if (error) {
86 + g_printerr ("Could not spawn apport-gtk: %s\n", error->message);
87 + g_error_free (error);
88 + }

brace needs to fall on the next line.

Daniel van Vugt (vanvugt) wrote :

As above, please fix the formatting. Please also rethink as Sam says:

"Currently it pops up the apport dialog as soon as the application becomes unresponsive. There are quite a few cases where applications can become temporarily unresponsive and it isn't a bug because the system is just under really heavy load and the UI thread of every application is starved. It doesn't make sense to report those."

review: Needs Fixing
Evan (ev) wrote :

Matthew Paul Thomas is working on addressing this in the UI. I'll update the merge when we have something concrete.

jenkins (martin-mrazik+qa) wrote :
review: Needs Fixing (continuous-integration)
Martin Mrazik (mrazik) wrote :

The jenkins failure above seems to be be related to us.archive.ubuntu.com connectivity issues :-/

Evan (ev) wrote :

On Wed, Jul 4, 2012 at 8:41 PM, Sam Spilsbury
<email address hidden> wrote:
> Also, some indentation notes:
>
> 72 + app = wnck_window_get_application (win);
> 73 + if (!app)
> 74 + return FALSE;
>
> You need a one-tab indent for the return FALSE;
>
> 76 + pid = wnck_application_get_pid (app);
> 77 + if (d->apport_dialog) {
> 78 + /* Is Apport already running? */
> 79 + if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
> 80 + return TRUE;
> 81 + }
>
> pid is indented incorrectly, the brace should fall on the next line, return TRUE needs an indent (4 spaces)

Does compiz mix tabs and spaces? If so, why?

Also, is there a codified coding style to be followed?

Sam Spilsbury (smspillaz) wrote :

> On Wed, Jul 4, 2012 at 8:41 PM, Sam Spilsbury
> <email address hidden> wrote:
> > Also, some indentation notes:
> >
> > 72 + app = wnck_window_get_application (win);
> > 73 + if (!app)
> > 74 + return FALSE;
> >
> > You need a one-tab indent for the return FALSE;
> >
> > 76 + pid = wnck_application_get_pid (app);
> > 77 + if (d->apport_dialog) {
> > 78 + /* Is Apport already running? */
> > 79 + if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
> > 80 + return TRUE;
> > 81 + }
> >
> > pid is indented incorrectly, the brace should fall on the next line, return
> TRUE needs an indent (4 spaces)
>
> Does compiz mix tabs and spaces? If so, why?

Yes, history (its how X11 does it, davidr was an X11 developer, that coding style got followed in compiz). Yes, its stupid. No we're not changing it (mucks up bzr histories etc, better to just be consistent).

>
> Also, is there a codified coding style to be followed?

I would point you to the compiz wiki, but it seems to be down again :(

Evan (ev) wrote :

On Fri, Jul 13, 2012 at 9:59 AM, Sam Spilsbury
<email address hidden> wrote:
>> Does compiz mix tabs and spaces? If so, why?
>
> Yes, history (its how X11 does it, davidr was an X11 developer, that coding style got followed in compiz). Yes, its stupid. No we're not changing it (mucks up bzr histories etc, better to just be consistent).

I'm sure "lets change it!" is the normal response, but I'm all for
staying consistent.
>>
>> Also, is there a codified coding style to be followed?
>
> I would point you to the compiz wiki, but it seems to be down again :(

I've fetched it out of Google's cache. Thanks!

Evan (ev) wrote :

Matthew has finished updating the specification to account for temporarily unresponsive applications:

https://wiki.ubuntu.com/ErrorTracker#app-hang

I'll get started on this now, but if there are concerns do let me know.

Tim Penhey (thumper) wrote :

Evan, I agree with you on the "change it" for mixed tabs and spaces (my emacs hates it). Personally I think history for that sake is more of a hindrance, and not something that couldn't be worked around when necessary, but hey, it isn't my project.

Francis Ginther (fginther) wrote :

Review was claimed by accident, please ignore.

review: Abstain
Daniel van Vugt (vanvugt) wrote :

The branches have changed. Please resubmit targeting lp:compiz

Daniel van Vugt (vanvugt) :
review: Resubmit
Sam Spilsbury (smspillaz) wrote :

Is this still being worked on or can we drop it from the merge queue?

review: Needs Information
Evan (ev) wrote :

It's being worked on - slowly :)

Unmerged revisions

3274. By Evan on 2012-07-04

Report the error if we cannot spawn apport-gtk.

3273. By Evan on 2012-07-04

Put USE_APPORT in the right place so that forcequit.c picks it up.

3272. By Evan on 2012-07-04

Make apport dialog optional.

3271. By Evan on 2012-07-03

Initial commit of calling apport when applications hang under compiz.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmake/base.cmake'
2--- cmake/base.cmake 2012-05-24 00:55:17 +0000
3+++ cmake/base.cmake 2012-07-04 17:10:24 +0000
4@@ -1,3 +1,5 @@
5+option (BUILD_APPORT "Use apport instead of the built-in application hang dialog" ON)
6+compiz_set (USE_APPORT ${BUILD_APPORT})
7 set (USE_GCONF 1 CACHE BOOL "Install core GConf schemas")
8
9 if (USE_GCONF)
10@@ -29,6 +31,7 @@
11 compiz_print_result_message ("gconf schemas" USE_GCONF)
12 compiz_print_result_message ("gnome" USE_GNOME)
13 compiz_print_result_message ("kde4 window decorator" USE_KDE4)
14+ compiz_print_result_message ("apport hang dialog" USE_APPORT)
15
16 compiz_print_result_message ("protocol buffers" USE_PROTOBUF)
17 compiz_print_result_message ("file system change notifications" HAVE_INOTIFY)
18
19=== modified file 'config.h.core.in'
20--- config.h.core.in 2012-05-16 17:40:13 +0000
21+++ config.h.core.in 2012-07-04 17:10:24 +0000
22@@ -3,3 +3,6 @@
23 #define PACKAGE "@_compiz_package@"
24
25 #define GETTEXT_PACKAGE "@GETTEXT_PACKAGE@"
26+
27+/* Define to 1 if Apport support is enabled */
28+#cmakedefine USE_APPORT 1
29
30=== modified file 'gtk/config.h.gtk.in'
31--- gtk/config.h.gtk.in 2008-10-14 10:27:55 +0000
32+++ gtk/config.h.gtk.in 2012-07-04 17:10:24 +0000
33@@ -4,6 +4,9 @@
34 /* Define to 1 if Gconf support is enabled */
35 #cmakedefine USE_GCONF 1
36
37+/* Define to 1 if Apport support is enabled */
38+#cmakedefine USE_APPORT 1
39+
40 /* Define to 1 if you have the `wnck_window_has_name' function. */
41 #cmakedefine HAVE_WNCK_WINDOW_HAS_NAME 1
42
43
44=== modified file 'gtk/window-decorator/forcequit.c'
45--- gtk/window-decorator/forcequit.c 2011-02-21 09:53:08 +0000
46+++ gtk/window-decorator/forcequit.c 2012-07-04 17:10:24 +0000
47@@ -23,6 +23,9 @@
48 * Authored By: Sam Spilsbury <sam.spilsbury@canonical.com>
49 */
50
51+#ifdef HAVE_CONFIG_H
52+#include <config.h>
53+#endif
54 #include "gtk-window-decorator.h"
55
56 static char *
57@@ -127,6 +130,37 @@
58 }
59 }
60
61+gboolean
62+show_apport_dialog (WnckWindow *win)
63+{
64+ WnckApplication *app;
65+ GError *error = NULL;
66+ gulong flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
67+ decor_t *d = g_object_get_data (G_OBJECT (win), "decor");
68+ int pid;
69+ char pid_s[16];
70+ gchar *argv[] = { "/usr/share/apport/apport-gtk", "--hanging", NULL, NULL };
71+
72+ app = wnck_window_get_application (win);
73+ if (!app)
74+ return FALSE;
75+
76+ pid = wnck_application_get_pid (app);
77+ if (d->apport_dialog) {
78+ /* Is Apport already running? */
79+ if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
80+ return TRUE;
81+ }
82+ sprintf (pid_s, "%d", pid);
83+ argv[2] = pid_s;
84+ g_spawn_async (NULL, argv, NULL, flags, NULL, NULL, &(d->apport_dialog), &error);
85+ if (error) {
86+ g_printerr ("Could not spawn apport-gtk: %s\n", error->message);
87+ g_error_free (error);
88+ }
89+ return TRUE;
90+}
91+
92 void
93 show_force_quit_dialog (WnckWindow *win,
94 Time timestamp)
95@@ -135,6 +169,11 @@
96 GtkWidget *dialog;
97 gchar *str, *tmp;
98
99+#ifdef USE_APPORT
100+ if (show_apport_dialog(win))
101+ return;
102+#endif
103+
104 if (d->force_quit_dialog)
105 return;
106
107
108=== modified file 'gtk/window-decorator/gtk-window-decorator.h'
109--- gtk/window-decorator/gtk-window-decorator.h 2012-05-27 04:32:55 +0000
110+++ gtk/window-decorator/gtk-window-decorator.h 2012-07-04 17:10:24 +0000
111@@ -100,6 +100,7 @@
112 #include <unistd.h>
113 #include <sys/types.h>
114 #include <signal.h>
115+#include <errno.h>
116
117 #include <libintl.h>
118 #define _(x) gettext (x)
119@@ -464,6 +465,7 @@
120 WnckWindowActions actions;
121 XID prop_xid;
122 GtkWidget *force_quit_dialog;
123+ int apport_dialog;
124 Bool created;
125 void (*draw) (struct _decor *d);
126 } decor_t;
127
128=== modified file 'plugins/fade/src/fade.cpp'
129--- plugins/fade/src/fade.cpp 2011-06-27 13:32:58 +0000
130+++ plugins/fade/src/fade.cpp 2012-07-04 17:10:24 +0000
131@@ -23,6 +23,9 @@
132 * Author: David Reveman <davidr@novell.com>
133 */
134
135+#ifdef HAVE_CONFIG_H
136+# include <config.h>
137+#endif
138 #include "fade.h"
139 #include <core/atoms.h>
140
141@@ -195,6 +198,17 @@
142 fScreen->optionGetDimUnresponsive ())
143 {
144 GLuint value;
145+#ifdef USE_APPORT
146+ Time serverTime;
147+
148+ if (!window->alive () && !forceQuitTriggered) {
149+ serverTime = screen->getCurrentTime ();
150+ screen->toolkitAction (Atoms::toolkitActionForceQuitDialog, serverTime, window->id (), true, 0, 0);
151+ forceQuitTriggered = true;
152+ }
153+#endif
154+ if (window->alive ())
155+ forceQuitTriggered = false;
156
157 value = fScreen->optionGetUnresponsiveBrightness ();
158 if (value != 100)
159@@ -361,7 +375,8 @@
160 fadeTime (0),
161 opacityDiff (0),
162 brightnessDiff (0),
163- saturationDiff (0)
164+ saturationDiff (0),
165+ forceQuitTriggered (false)
166 {
167 if (window->isViewable ())
168 addDisplayModal ();
169
170=== modified file 'plugins/fade/src/fade.h'
171--- plugins/fade/src/fade.h 2012-01-18 16:26:45 +0000
172+++ plugins/fade/src/fade.h 2012-07-04 17:10:24 +0000
173@@ -94,6 +94,8 @@
174 int opacityDiff;
175 int brightnessDiff;
176 int saturationDiff;
177+
178+ bool forceQuitTriggered;
179 };
180
181 class FadePluginVTable :

Subscribers

People subscribed via source and target branches