In brief, what breaks it is that cell_area is created in the constructor, which runs after the instance_init() sequence.

+  if (!priv->cell_area)
+    {
+      priv->cell_area = gtk_cell_area_box_new ();
+      g_object_ref_sink (priv->cell_area);
+    }

This is also true for combo boxes.  I had to rewrite the init code in a bunch of custom Evolution widgets to work around this.

I think this is a big API issue. In GTK2 you could subclass every widget just by overriding init(). If most widgets now require to instead put stuff into constructed(), we should at least advertise this widely in the migration guide and other documentation.

My preference would be finding ways to avoid this requirement, because it'll be really confusing to newcomers.

Also, do we need to document functions that do not work in init()?

(In reply to comment #1)
> In brief, what breaks it is that cell_area is created in the constructor, which
> runs after the instance_init() sequence.
> 
> +  if (!priv->cell_area)
> +    {
> +      priv->cell_area = gtk_cell_area_box_new ();
> +      g_object_ref_sink (priv->cell_area);
> +    }

One way around this particular issue might be to create a default cell area in instance_init() and let the constructor property (if provided) replace it.  That limits the stricter subclassing requirements to new code that overrides the cell area.

Created attachment 177969 [details] [review]
Ensure that gtk_icon_view_cell_layout_get_area() doesn't return NULL

This will avoid GtkIconView subclasses being initialized in their
init() method from  accessing a GtkCellArea that is not yet created.

The rationale behind this patch is that if someone starts initializing the iconview in the init, then she really doesn't care about overriding the cellarea and there's no point in delaying the creation of it.

If this patch or something along these lines is welcome I can cook a few more for the other widgets affected.

That patch de facto makes the cell-area property a non-construct property in subclasses. I'm not a fan of this change, because it breaks g_object_new (MY_TYPE_SUBCLASS, "cell-area", some_cell_area, NULL);

Currently it just silently overrides the cell area, but we could make that an assertion. But do we want those inconsistencies in our API?

Created attachment 178084 [details] [review]
iconview: Make cell-area settable

This makes the cell area changeable at runtime and not only during
construction. A side effect is that it allows init functions of
subclasses to modify the cell area again.

Makes sense to me.

There are several things I dont like about the proposed patch:

 a.) If at _init() time a subclass modifies the cell area, and
     an "area" parameter is provided to g_object_new() for that
     class, then there are no warnings.

     The subclass silently fails. The area which was accessed and
     created at _init() time, and the cell renderers which the
     subclass surely added at _init() time are simply silently
     dropped and ignored.

b.) Yanking the cell area from under the feet of a functioning layouting 
    widget is not exactly a simple exercise. gtk_icon_view_unset_cell_area()
    gets is wrong off the bat.

    For the icon view's specific case, the icon view needs to further
    free the array ->row_contexts and invalidate the sizes of all the current
    icons and queue a resize on the widget.

c.) All of this because what ? people dont feel comfortable with properly
    initializing their subclass from the ->constructor instead of the _init() ??

This technical backflipping and recalculating size on the fly to support
ripping out the guts of a live layouting widget would have to be done for:
 - GtkTreeViewColumn (will have to ensure it's treeview requests a new size)
 - GtkCellView
 - GtkComboBox
 - GtkIconView
 - GtkEntryCompletion

Using a construct-only property here reduces possible points of failure
at a good rate for a small price IMO, i.e. you create a layouting widget
for one purpose (and it sounds fair to me to say the CellArea is a constant
for the life-cycle of a layouting widget).

Is it that important that a GtkCellAreaBox be changed in the middle of
runtime for a GtkCellAreaTable ?

No ? then please, lets just grow up and properly initialize our dependant
counterparts from the ->constructor() instead of more naively from _init().

(In reply to comment #10)
>  a.) If at _init() time a subclass modifies the cell area, and
>      an "area" parameter is provided to g_object_new() for that
>      class, then there are no warnings.
> 
Yes, this is roughly equivalent to calling gtk_cell_layout_clear() on the subclass. Nobody does that.

> b.) Yanking the cell area from under the feet of a functioning layouting 
>     widget is not exactly a simple exercise. gtk_icon_view_unset_cell_area()
>     gets is wrong off the bat.
> 
Isn't that roughly equivalent to the work that needs to be done whenever somebody modifies the cell area? Like adding renderers at runtime or even setting the spacing of a CellAreaBox? Don't we need an "invalidate everything" anyway?
Could you give me specific examples for what gets so much harder if we allow changing the cell area at runtime?

> c.) All of this because what ? people dont feel comfortable with properly
>     initializing their subclass from the ->constructor instead of the _init()
> ??
>
All of this because the current approach breaks existing applications without gcc warnings. And I suspect you didn't bother to fix a single one of those. Especially because you didn't provide and easy way to find _all_ of them.

(In reply to comment #11)
> (In reply to comment #10)
> >  a.) If at _init() time a subclass modifies the cell area, and
> >      an "area" parameter is provided to g_object_new() for that
> >      class, then there are no warnings.
> > 
> Yes, this is roughly equivalent to calling gtk_cell_layout_clear() on the
> subclass. Nobody does that.

Except providing a customized area at construct time is something that
is interesting and useful, unlike explicitly clearing the layout.

> > b.) Yanking the cell area from under the feet of a functioning layouting 
> >     widget is not exactly a simple exercise. gtk_icon_view_unset_cell_area()
> >     gets is wrong off the bat.
> > 
> Isn't that roughly equivalent to the work that needs to be done whenever
> somebody modifies the cell area? Like adding renderers at runtime or even
> setting the spacing of a CellAreaBox? Don't we need an "invalidate everything"
> anyway?
> Could you give me specific examples for what gets so much harder if we allow
> changing the cell area at runtime?

I did give you specific examples.

In the case of icon-view, every GtkCellAreaContext needs to be rebuilt,
i.e. the per-row contexts... every icon needs to have its size invalidated,
all signals need to be reconnected to the new context(s).

All this work is unnecessary, because we can simply make the assumption
that the cell area is constant after the layouting widget returns from
g_object_new().

> 
> > c.) All of this because what ? people dont feel comfortable with properly
> >     initializing their subclass from the ->constructor instead of the _init()
> > ??
> >
> All of this because the current approach breaks existing applications without
> gcc warnings. And I suspect you didn't bother to fix a single one of those.
> Especially because you didn't provide and easy way to find _all_ of them.

No I did not anticipate that apps would break, neither did I anticipate such
a holy war insisting that delegate objects and all object members should be
accessible from the instance struct initializer.

We have construct-only properties in GObject, it's a perfectly valid 
programming construct to use them, why should we deprive ourselves use
of construct-only properties from GTK+ ?

You are saying we should refrain from using perfectly valid GObject
facilities for the sake of making everything unconditionally available
in the instance _init() stub ? at the cost of making our own code
drastically more complex ?

My answer to that is simple, just use the constructor instead of the
initializer, that's a perfectly good rule for any object that needs
to make assumptions about how it's superclass was built, i.e. _init()
can only really be relied upon for initializing your own instance
structure.

(In reply to comment #12)
> Except providing a customized area at construct time is something that
> is interesting and useful, unlike explicitly clearing the layout.
>
customized areas at runtime still works fine. It just doesn't work fine on subclasses, where it's not interesting. Just like clearing the layout. SO I consider your argument entirely moot.
 
> In the case of icon-view, every GtkCellAreaContext needs to be rebuilt,
> i.e. the per-row contexts... every icon needs to have its size invalidated,
> all signals need to be reconnected to the new context(s).
> 
> All this work is unnecessary, because we can simply make the assumption
> that the cell area is constant after the layouting widget returns from
> g_object_new().
> 
All this work has code for it inside the icon view code already, because we already support APIs that do parts or all of these things. There is not a single thing we'd need to write new code for apart from hooking all of it up. I suppose we just need to gtk_icon_view_queue_layout()?

I was looking for things that we need to write new code for. I still don't see any.

> No I did not anticipate that apps would break, neither did I anticipate such
> a holy war insisting that delegate objects and all object members should be
> accessible from the instance struct initializer.
> 
My arguments are only about existing code breaking and making sure developers know what they are doing. It's about fixing existing code and educating developers.
The thing we're discussing here breaks both of these:
- Apps break at runtime, not even compile-time. And there's no way to find all these cases for the developers.
- Developers don't know what constructed is and how it should behave. That's not just app developers, but we ourselves aren't clear on that. So we'd need a massive education campaign to make sure that people get it right in the future.

And I haven't had any suggestions on how we fix these things - in 2 months for GNOME, in a bit longer for the rest of the world, from Inkscape to Audacity.

Can we please forget about these details and address the general
issue that these classes claim to implement the GtkCellLayout
interface?

So they *are* GtkCellLayouts.

But wait, they are, but not in init()...

That can't be, if a class implements an interface, it has to do
so unconditionally, also if the internal method of implementing
it is delegation. That's an implementation detail, and bothering
the user with it is painful.

Created attachment 178622 [details] [review]
add demo plugin

This adds a demo plugin based on the one from bug 639597 (I think it's based on libpeas' demo plugin) that I used for testing.

It spits warnings now when the window is closed, maybe something changed in gtk+ again. If these concern you can comment out the eog functionality and just keep the printfs. It should still fail to release the window ref then.

Comment on attachment 178622 [details] [review]
add demo plugin

Whoops! This shouldn't have landed here. Bugzilla tricked me here. :?

I've committed an elaboration of Claudios initial patch, with a warning and some documentation about the issue.