GNOME Bugzilla – Bug 630195
Use GDK error trapping straight-up
Last modified: 2010-09-20 22:41:48 UTC
The hacks we were playing by calling gdk_error_trap_push() and then resetting the error handler are incompatible with the rewrite of GDK error traps. Since the new error code has some features that simplify what we are doing (like automatically figuring out whether a XSync() is needed) and because our custom error handler didn't have a lot of a point), use a separate code path for GTK+ 3.0 builds that just uses the GDK error traps straight-up without a custom error handler.
Created attachment 170691 [details] [review] Use GDK error trapping straight-up
Comment on attachment 170691 [details] [review] Use GDK error trapping straight-up >are doing (like automatically figuring out whether a XSync() is needed) >and because our custom error handler didn't have a lot of a point), extra ")" there >+ * Since the main point of our custom error trap was to get the error logged >+ * to the right place, with GTK+-3.0 we simply omit our own error handler and >+ * use the GTK+ handling straight-up. I don't get it... Is there an implied "but getting the error logged to the right place isn't actually very important so we don't care"? If so, then why don't we just use gdk's error handling under 2.x too? (And also, is there any reason for using our own IOErrorHandler if we're using gdk's ErrorHandler?) The patch otherwise seems sane, although while investigating the existing error traps to compare against gtk3, I noticed that the existing meta_error_trap_push_with_return() is obviously broken; it computes need_sync but then ignores it and just always passes FALSE to meta_error_trap_push_internal() anyway (which also means all the bookkeeping of display->error_trap_synced_at_last_pop is entirely pointless since nothing other than that code looks at it). Maybe another argument for "always use the gtk versions"...
*** Bug 630207 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > (From update of attachment 170691 [details] [review]) > >are doing (like automatically figuring out whether a XSync() is needed) > >and because our custom error handler didn't have a lot of a point), > > extra ")" there > > >+ * Since the main point of our custom error trap was to get the error logged > >+ * to the right place, with GTK+-3.0 we simply omit our own error handler and > >+ * use the GTK+ handling straight-up. > > I don't get it... Is there an implied "but getting the error logged to the > right place isn't actually very important so we don't care"? There's an implied "this is silly, let's just fix GTK+ to g_log()" and an implied "Getting the error to the right place isn't as important as getting the shell working again right now". > If so, then why > don't we just use gdk's error handling under 2.x too? Because a) harder to make it g_log() b) I could drastically simplify the code with GTK+ 3 with performance advantages, and didn't really want to have a GTK+ 3 version, and a modified GTK+ 2 version that I wasn't going to test. > (And also, is there any > reason for using our own IOErrorHandler if we're using gdk's ErrorHandler?) GDK doesn't use an IOErrorHandler, so if we want IO errors to go to the log, we need to do go it ourselves. Err, wait, GDK does have an IOError handler. > The patch otherwise seems sane, although while investigating the existing error > traps to compare against gtk3, I noticed that the existing > meta_error_trap_push_with_return() is obviously broken; it computes need_sync > but then ignores it and just always passes FALSE to > meta_error_trap_push_internal() anyway (which also means all the bookkeeping of > display->error_trap_synced_at_last_pop is entirely pointless since nothing > other than that code looks at it). Maybe another argument for "always use the > gtk versions"... Does it sound good: - To move the IO Error handler along with the X error handler - Fix the extra ) - File a bug/patch against GTK+ to log X[IO]Errors rather than fprintf'ing them ?
sure
Bug 630216 filed with a GDK patch.
Attachment 170691 [details] pushed as c2f8949 - Use GDK error trapping straight-up