After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710793 - GtkDialog destroy event allocation size== 1, 1
GtkDialog destroy event allocation size== 1, 1
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
3.8.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2013-10-24 12:13 UTC by IgnorantGuru
Modified: 2014-12-07 23:13 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description IgnorantGuru 2013-10-24 12:13:11 UTC
First noticed in GTK 3.8 (not in 3.4):  Getting the allocation in the handler of the destroy event for a dialog window gives allocation width,height as 1,1.  This breaks code that saves the dialog size here.  (May affect other widget's destroy events too.)

void on_dlg_destroy( GtkDialog* dlg, ... )
{
    GtkAllocation allocation;
    
    gtk_widget_get_allocation ( GTK_WIDGET( dlg ), &allocation );
    /* allocation.width == 1   allocation.height == 1
     * previously returned window size instead of 1,1 */
}

g_signal_connect( dlg, "destroy", G_CALLBACK( on_dlg_destroy ), NULL );
Comment 1 Emmanuele Bassi (:ebassi) 2013-10-24 12:36:28 UTC
GtkWidget::destroy documentation says:

"""
Signals that all holders of a reference to the widget should release the reference that they hold. May result in finalization of the widget if all references are released.
"""

admittedly, the documentation for GtkWidget::destroy should be much clearer on this point, but: GtkWidget::destroy is for releasing references held on the widget. any code that relies on valid widget state that is not explicitly added by that very same code is broken, as there should be no expectation of defined behaviour. a documentation patch would be very much welcome.

in this particular case: your code should use GtkWidget::size-allocate to track the size of a widget, and use GtkWidget::delete-event (which, unlike GtkWidget::destroy, is recoverable by returning TRUE, and thus keeps the widget state valid until the default class handler is called) on the top-level to store the allocation.
Comment 2 IgnorantGuru 2013-10-24 13:29:51 UTC
If I'm not mistaken, delete-event only fires when the dialog is closed by the user pressing the X button or escape key to close the window.  I used the configure event to work around this.

The signal handler runs *before* destroy, so seemingly the reference should still be valid.  Regardless, as this change in GTK 3.8 breaks common existing code for saving dialog sizes (and other widgets?), you may want to revert it.
Comment 3 Emmanuele Bassi (:ebassi) 2013-10-24 14:16:51 UTC
(In reply to comment #2)
> If I'm not mistaken, delete-event only fires when the dialog is closed by the
> user pressing the X button or escape key to close the window.  I used the
> configure event to work around this.

no, don't use the configure-event signal. I already told you: use the ::size-allocate signal, which is meant *exactly* for that use (tracking size and position changes). ::configure-event is used for tracking changes in underlying GdkWindow backed by a native surface, and it is just legacy.

if you are tracking the GtkWidget's allocation using ::size-allocate and your own code, then you can access your values during the ::destroy signal emission, e.g.:

    typedef struct {
      int widget_width;
      int widget_height;
    } WidgetState;

    static void
    on_allocate (GtkWidget *widget, GtkAllocation *alloc, gpointer data)
    {
      WidgetState *state = g_object_get_data (widget, "widget-state");

      state->widget_width = allocation.width;
      state->widget_height = allocation.height;
    }

    static void
    on_destroy (GtkWidget *widget, gpointer data)
    {
      WidgetState *state = g_object_get_data (widget, "widget-state");

      save_widget_state (state);
    }

    ...
    WidgetState *state = g_new0 (WidgetState, 1);
    g_object_set_data_full (widget, "widget-state", state, g_free);

    g_signal_connect (widget, "size-allocate", on_allocate, NULL);
    g_signal_connect (widget, "destroy", on_destroy, NULL);
    ...

bonus point if you use GtkWidget::window-state-event to track fullscreen or maximized state changes.

> The signal handler runs *before* destroy, so seemingly the reference should
> still be valid.  Regardless, as this change in GTK 3.8 breaks common existing
> code for saving dialog sizes (and other widgets?), you may want to revert it.

the *reference* is valid, because that's all you need to use to release a reference of disconnect a signal handler.. nowhere in the documentation says that the *state* of the widget matches some expectation, except being "valid" (in this case, a 1x1 allocation is perfectly valid - just not what you expected).

undefined behaviour is not guaranteed to persist across releases. that's the whole point of "undefined behaviour".
Comment 4 IgnorantGuru 2013-10-24 18:04:05 UTC
> :configure-event is used for tracking changes in underlying GdkWindow backed by a native surface, and it is just legacy.

Your current docs:
> To avoid race conditions, connect to "configure-event" on the window and adjust your size-dependent state to match the size delivered in the GdkEventConfigure. 
https://developer.gnome.org/gtk3/unstable/GtkWindow.html

Nowhere in GtkWindow is size-allocate mentioned, only configure.  (I realize size-allocate can be used on any widget, but the docs make it sound like configure is recommended.)

The state of the widget is "valid", but it's not.  Common sense would be to invalidate the reference and then invalidate the data it points to.  What event invalidates the data of a widget if not destroy?  It's ridiculous to have to store the same data and track every resize just to have that same data available at window close.  And now there is no single event (that I see) which fires just before the window closes, while the data is still valid.

> undefined behaviour is not guaranteed to persist across releases. that's the whole point of "undefined behaviour".

Thanks for the lecture.  And a bug report is not a legal proceeding.  I thought you might be interested in knowing where you're causing unnecessary breakage with unnecessary changes in well-established, if not documented behavior (keeping in mind that gtk is poorly documented, which leaves a lot of room for chaos).  This helps to make transitions to higher versions go more smoothly.  Maybe now is a good time to define the behavior in a logical, consistent way, instead of a way which no one would guess.  As you have (un)defined it, the destroy event doesn't seem very useful or reliable for much of anything.

Work for backwards compatibility whenever possible.  Anyone can break everything with each release and claim you never defined it.  The art is to smooth that process out with intelligence and common sense.  Returning a size of 1, 1 just before the window closes is not common sense, especially as newly introduced behavior.

Don't worry - I never thought you'd fix it (I've learned), just giving you feedback on why developers are leaving GTK.  You're known for being dismissive and uncooperative, defending any breakage you can get away with causing, instead of seeing how it might be avoided and improved.  Nice attitude, again.
Comment 5 Emmanuele Bassi (:ebassi) 2013-10-24 19:00:05 UTC
(In reply to comment #4)
> > :configure-event is used for tracking changes in underlying GdkWindow backed by a native surface, and it is just legacy.
> 
> Your current docs:
> > To avoid race conditions, connect to "configure-event" on the window and adjust your size-dependent state to match the size delivered in the GdkEventConfigure. 
> https://developer.gnome.org/gtk3/unstable/GtkWindow.html

yes, and again: a patch for the documentation would be welcome.

> Nowhere in GtkWindow is size-allocate mentioned, only configure.  (I realize
> size-allocate can be used on any widget, but the docs make it sound like
> configure is recommended.)

indeed, and it should not be the case.

> The state of the widget is "valid", but it's not.

an allocation of 1x1 is valid.

>  Common sense would be to
> invalidate the reference and then invalidate the data it points to.

precisely, but nowhere it's written that the state of the widget is going to be exactly the same as it was before gtk_widget_destroy() is called.

>  What event
> invalidates the data of a widget if not destroy?  It's ridiculous to have to
> store the same data and track every resize just to have that same data
> available at window close.

this is not "window close": it's widget destroy - i.e. the ::destroy signal is emitted while the object is being disposed. the only guarantee made for the instance is that it's in a valid state, but not that the state matches exactly the one that it had prior to calling gtk_widget_destroy().

>  And now there is no single event (that I see) which
> fires just before the window closes, while the data is still valid.

for user interaction, that signal is ::delete-event.

for programmatic action, you can save the state before calling gtk_widget_destroy().

> > undefined behaviour is not guaranteed to persist across releases. that's the whole point of "undefined behaviour".
> 
> Thanks for the lecture.  And a bug report is not a legal proceeding.

no, but the documentation is binding. when the documentation is unclear or misleading, it needs to be fixed - which is why this bug is still open.

GTK+ already provides you with a perfectly safe way to track the state of a widget; saving state during destruction is not that way.

>  I thought
> you might be interested in knowing where you're causing unnecessary breakage
> with unnecessary changes in well-established, if not documented behavior
> (keeping in mind that gtk is poorly documented, which leaves a lot of room for
> chaos).

"thanks for the lecture".

> Work for backwards compatibility whenever possible.  Anyone can break
> everything with each release and claim you never defined it.  The art is to
> smooth that process out with intelligence and common sense.  Returning a size
> of 1, 1 just before the window closes is not common sense, especially as newly
> introduced behavior.
> 
> Don't worry - I never thought you'd fix it (I've learned), just giving you
> feedback on why developers are leaving GTK.  You're known for being dismissive
> and uncooperative, defending any breakage you can get away with causing,
> instead of seeing how it might be avoided and improved.  Nice attitude, again.

if I wanted to be dismissive and uncooperative, I would have closed this issue as RESOLVED NOTABUG - whereas I'm leaving it open because a) there is a bug in the documentation and b) this leads to broken code being written and used in the wild. I don't believe it's in our best interest to support broken code, so our documentation should be improved.

on a more personal note, I'd appreciate it if you stopped being unnecessarily hostile and passive-aggressive on Bugzilla; it's counter-productive, as it doesn't put anybody in their best mood to help you, and it doesn't strike me as a particularly useful attitude.