I went back and compared a GtkFrame generated by glade and one generated by gtk-builder-convert.  The labels attached to frames generated by glade have type "label_item".  Those generated by gtk-builder-convert have type "label".  When attempting to load a file generated by glade with this label_item type, the application seg faults while loading xml file.

Perhaps this is a result of changes in how gtkbuilder works that I do not yet have?

Running Fedora 9, gtk2-2.12, glade3 svn1823

its a bug in glade, gtkbuilder saves uses a different name for the child
type, so we need to adjust our code to allow different child type names
for different formats.

Created attachment 114887 [details] [review]
Possible solution

If GtkFrame is the only problem of that kind, then this patch should help.

Well, this patch is worth considering yes - as I understand it, it loads and
saves the special child type depending on the format, without touching the core.

Well, We have notebook tab child types also, and expander labels too... are we
lucky enough to have them named the same in builder ? if so then I would be 
prepared to live with this, if not it might be less code all around to try
to bookkeep separate child type names from the catalog, but that also sounds
unattractive (we might try using your approach for other occurrences, maybe 
reusing the code with some static functions ?).

Created attachment 114945 [details] [review]
Patch

I've added the same functionality to GtkExpander. Additionally, I've fixed inconsistency while deleting widgets, so remove-child function seems to become useless.

Nice, so, questions, do GtkNotebook tabs share this symptom ?

Also, removing the expander_remove_child() function makes me 
nervous... what happens when you delete the expander label ?
add another widget in its place... undo undo, redo redo... good ?
how about deleting the expander label and saving and loading,
is the placeholder properly saved/loaded into the expander
label position ?

(In reply to comment #6)
> Nice, so, questions, do GtkNotebook tabs share this symptom ?

Yes, they do.

> Also, removing the expander_remove_child() function makes me 
> nervous... what happens when you delete the expander label ?
> add another widget in its place... undo undo, redo redo... good ?
> how about deleting the expander label and saving and loading,
> is the placeholder properly saved/loaded into the expander
> label position ?

A widget with fixed amount of children needs add_child function only while loading and doesn't needs remove_child at all (if core functions correctly). I found that remove_child appears to be called after delete=>undo=>redo and fixed this issue.
And about placeholder as label. It appears to be saved with special-child-type="label_item" because placeholder isn't a widget, so write_child doesn't get called. But this case is handled correctly by both glade and gtk-builder, because "label_item" is native for glade and gtk-buider ignores placeholders.

(In reply to comment #7)
> (In reply to comment #6)
> > Nice, so, questions, do GtkNotebook tabs share this symptom ?
> 
> Yes, they do.

Oops, no!

(In reply to comment #7)
> (In reply to comment #6)
> > Nice, so, questions, do GtkNotebook tabs share this symptom ?
> 
> Yes, they do.
> 
> > Also, removing the expander_remove_child() function makes me 
> > nervous... what happens when you delete the expander label ?
> > add another widget in its place... undo undo, redo redo... good ?
> > how about deleting the expander label and saving and loading,
> > is the placeholder properly saved/loaded into the expander
> > label position ?
> 
> A widget with fixed amount of children needs add_child function only while
> loading and doesn't needs remove_child at all (if core functions correctly). I
> found that remove_child appears to be called after delete=>undo=>redo and fixed
> this issue.

I dont understand what you mean here, how did you fix the issue, you
left the remove function in the plugin after all ?

Please explain your modifications to glade-command, I didnt look to 
closely, but as I recall, its important to leave it up to the plugin
to create its placeholders - this is also a delicate area.

> And about placeholder as label. It appears to be saved with
> special-child-type="label_item" because placeholder isn't a widget, so
> write_child doesn't get called. But this case is handled correctly by both
> glade and gtk-builder, because "label_item" is native for glade and gtk-buider
> ignores placeholders.

Yes thats fine with me, when the placeholder is saved without being
marked as the child label, bad things happen ;-)

(In reply to comment #9)
> I dont understand what you mean here, how did you fix the issue, you
> left the remove function in the plugin after all ?
> 
> Please explain your modifications to glade-command, I didnt look to 
> closely, but as I recall, its important to leave it up to the plugin
> to create its placeholders - this is also a delicate area.

This modification is the solution. I haven't made any changes since I posted the last patch. glade_command_remove creates a new placeholder and connects to its "destroyed" signal. But the placeholder appears to be destroyed after undo so I just recreate it. It's destroyed because it is not refed when connected to command.

I was going through the bugs today and want to commit this, but Im
not convinced that glade command should be inventing placeholders,
placeholders should always be left to the plugin (its a rule of thumb
that emerged out of a world of bugs).

Maybe I need to go back and review the patch again, is there a
reason why adding the placeholder cant be done in the _remove_child()
function in the plugin and the behaviour of glade-command must be
changed ?

The rest of this patch is probably the singly most important patch
for glade right now - Im not aware of many other builder compatibility bugs.

Sorry if you think I should relook at it all again please tell me
and explain :)

Created attachment 116060 [details] [review]
A new patch

> is there a reason why adding the placeholder cant be done in the 
> _remove_child() function in the plugin and the behaviour of 
> glade-command must be changed ?

It's already changed. glade-command creates a new placeholder in glade_command_remove but doesn't refs it, so the placeholder gets deleted after undo. In this new patch I ref the placeholder on command creation and unref on finalization.

Created attachment 116067 [details] [review]
Patch

Sorry, I didn't refresh the patch. This one is correct.

Ok Im gonna have to take another look at the code, and I asked Juan to
look at it too.

My guess is that the placeholder should dissapear in Undo, and adding
the placeholder in glade_command_remove() is just a detail of gladecommand,
maybe glade-command could be honed down a little so that it doesnt need
to add a placeholder at all in glade_command_remove() (remember, removing
a widget from the interface does not mean there will be a placeholder
where the widget was - that part is up to the plugin).

GladeCommand is obviously the central hub of our business logic code, so
for this it gets really complex and ugly in there:

Ok, so Im still here, the relevent code is this:

	if (cdata->parent != NULL &&
	    glade_widget_placeholder_relation 
	    (cdata->parent, cdata->widget))
	{
		placeholder = glade_placeholder_new ();
		glade_command_placeholder_connect
			(cdata, GLADE_PLACEHOLDER (placeholder));
	}

Here, we are inventing a placeholder because a widget is being
removed, from a container that uses placeholders to add child 
widgets of that specific child type (i.e. its possible for a
container to hold GtkWidget children and other GObject children).

This business logic is needed to make sure to use glade_widget_replace()
and give it a placeholder the first time when a GtkWidget is removed
from a GtkContainer, as you noted, on following calls to undo/redo,
glade_widget_replace() is no longer used, cause the placeholder
dissapears.

Now as far as I can see, this is working ok, is there something in
particular that you find is not working ?

(In reply to comment #7)
> (In reply to comment #6)
[...]
> A widget with fixed amount of children needs add_child function only while
> loading and doesn't needs remove_child at all (if core functions correctly). I
> found that remove_child appears to be called after delete=>undo=>redo and fixed
> this issue.

Hmmm, That assumption might be true if we were also assuming that a container
has GtkWidget type children, or if we were assuming that GtkWidget type
children are replaced by placeholders in all types of containers.

Unless there is a real problem with GladeCommand I am not going to change
the policy (which is to let placeholders die and just watch them, with the
ugly added exception of inventing one upon _remove()) so I'll ask you to
simply leave the expander_remove() function in place and leave GladeCommand
alone, so I can go ahead and apply the rest of your patch :)

Created attachment 116106 [details] [review]
Patch

As you wish

Thanks, applied in svn, its working nicely ;-)