GNOME Bugzilla – Bug 613302
GtkWidget not emitting the unmap signal
Last modified: 2010-05-21 15:13:17 UTC
Created attachment 156538 [details] [review] [GtkWidget] Properly call gtk_widget_unmap() during unrealize For a reason I still don't understand, gtk_widget_real_unrealize() calls directly gtk_widget_real_unmap(), which is the base implementation of the GtkWidget.unmap virtual method. This has the consecuence of: 1) not emitting GtkWidget::unmap 2) not calling the GtkWidget.unmap() implementation of GtkWidget derived classes. According to git blame, this has been the case for 12 years now: commit 57bae555749aec7e0ff016a3db35af53d85ca844 Author: Tim Janik <timj@src.gnome.org> Date: Mon Feb 2 05:35:59 1998 +0000 fixed a refresh bug with unrealization of no_window widgets. (dialog button in tesgtk) -timj I've been running the attached patch in an N900 without issues. Needless to say, it fixes a crasher due to a unmap() method not being executed when assumed it will.
Created attachment 156539 [details] Small test case showing the issues, click the button to destroy it
Tim, do you have any comments on this?
Created attachment 156560 [details] [review] [GtkWidget] Call gtk_widget_unmap() during unrealize Above patch is wrong. It is not wise to call gtk_widget_unmap() at that point because triggering the unmap signal so late is wrong (pg, some unmap() methods might be called after their unrealize() counterparts). A different fix would be to call gtk_widget_unmap() in gtk_widget_unrealize(), before emitting unrealize. This ensures that all unmap() methods in derived classes are ran before the unrealize() ones.
(In reply to comment #0) > Created an attachment (id=156538) [details] [review] > [GtkWidget] Properly call gtk_widget_unmap() during unrealize > > For a reason I still don't understand, gtk_widget_real_unrealize() calls > directly gtk_widget_real_unmap(), which is the base implementation of the > GtkWidget.unmap virtual method. This has the consecuence of: unmap/hide aren't neccessarily called during unrealization. This was an optimization decision about 12 years ago, iirc. real_unmap being called directly sounds odd indeed, it'd probably suffice to instead just call widget_unmap from the start of gtk_window_unrealize instead and possibly some other cases like handle box or menus. Owen might possibly have more ideas on this.
I think it's actually working as was intended: * We need to unset the MAPPED flag when unsetting the REALIZED flags because that's implied by the GTK+ state invariants. * If we are unrealizing all the child widgets, we want to unmap the parent window first or the child windows being destroyed from the leaves will be seen by the user. * But we *don't* need to unmap any other windows of the widget other than widget->window and we don't need to emit signals. So gtk_widget_real_unrealize() did exactly what was needed. A comment, or even a separate static function would have made the code more legible and less likely to be "fixed". (The unmap() is actually a bit too aggressive - we could only hide widget->window if gdk_window_is_viewable(widget->window) ... but saving a few async X requests isn't going to make a big difference.)
(In reply to comment #4) > unmap/hide aren't neccessarily called during unrealization. But in the cases when it happens to be called (like during a gtk_widget_destroy() sequence in the example above), it is called. Just that only the base implementation and code that was supposed to be executed during unmapping won't run at all.. (In reply to comment #5) > * But we *don't* need to unmap any other windows of the widget other than > widget->window and we don't need to emit signals. But do you understand that with this we are telling widget implementers that that whatever they do in foo_widget_map(), they can't reliably undo in foo_widget_unmap(), because there are no guarantees that this will actually be executed? This is what I find mostly odd. If this is a design decision, it must be at least documented.
FWIW, I had to commit this to hildon, in order to avoid a crasher that's consequence of this particularity: http://maemo.gitorious.org/hildon/hildon/commit/f2c9bc3cdc7658fe4382186d0d9dfb8c39b1d669.patch
I remember this bug in Evolution. We were leaking something because it was allocated in ::map(), and deallocated in ::unmap(). Then we fixed it to deallocate in ::unrealize() if it wasn't deallocated yet. This is an uncomfortable asymmetry, but it's not a huge deal...
How about documenting this with a short example snippet? The behaviour is certainly not obvious even if it makes sense as an optimization.
(In reply to comment #9) > How about documenting this with a short example snippet? The > behaviour is certainly not obvious even if it makes sense as an > optimization. Yes, I'd make it very clear in the documentation
Created attachment 160706 [details] [review] Explain that unmap-event may never be emitted
Review of attachment 160706 [details] [review]: looks good to me.
Pushed to master.