Created attachment 143041 [details] [review]
Use accessor functions instead direct access

GTK+ 2.17.10 is now the required version
I've used all the GTK+ 2.17.11 api available, still missing:
GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL);
GTK_WIDGET_SET_FLAGS (widget, GTK_REALIZED);
GTK_WIDGET_REALIZED ()
GTK_WIDGET_MAPPED ()
GTK_VIEWPORT ()->bin_window
GTK_ENTRY ()->editing_canceled

Some work is still required in these files:

http://git.gnome.org/cgit/glade3/tree/gladeui/glade-editor-property.c#n3394

and

http://git.gnome.org/cgit/glade3/tree/plugins/gtk+/glade-gtk.c#n5155
http://git.gnome.org/cgit/glade3/tree/plugins/gtk+/glade-gtk.c#n5314
http://git.gnome.org/cgit/glade3/tree/plugins/gtk+/glade-gtk.c#n5314
http://git.gnome.org/cgit/glade3/tree/plugins/gtk+/glade-gtk.c#n5372

For the second group of files some solutions were proposed:
 - <kalikianatoli> jjardon, shouldn't you use gtk builder for that?
<jjardon> kalikianatoli, to rework the function?
<kalikianatoli> jjardon, yes. Thinking of obtaining children by their name
<kalikianatoli> or alternatively use gtk_container_foreach
<kalikianatoli> and don't use any names

- It seems the right approach would be simply to use gtk_buildable_get_internal_child() here (to access the dialog buttons).
The strings which should be used are the same ones we give to glade_widget_adaptor_create_internal().

What is the best?

Use gtk_buildable_get_internal_child() because thats what
its for, its used by GtkBuilder when loading the interface
to get at the internal children of composite widgets which 
may have properties set or children added.

So what a HUGE patch ! you must have learned alot about
Glade just by going through all that ;-)

The patch looks all around good, granted that the
get_visible() and is_toplevel() apis are doing the
right thing, which seems to be the case.

Theres one comment I would like to make which is 
really only relevant to good coding style, in a couple
of ->realize() handlers you make the code do this:

-	widget->window = gdk_window_new (gtk_widget_get_parent_window (widget), 
-					 &attributes, attributes_mask);
-	gdk_window_set_user_data (widget->window, custom);
+	gtk_widget_set_window (widget, gdk_window_new (gtk_widget_get_parent_window (widget),
+						       &attributes, attributes_mask));
+	window = gtk_widget_get_window (widget);
+	gdk_window_set_user_data (window, custom);

I think this is confusing and not straight forward, it should be:

   window = gdk_window_new (...);
   gtk_widget_set_window (widget, window);
   gdk_window_set_user_data (window, widget);

Other than that I would like to apply this patch soon, is she
running steady (or seemingly at least) after all those changes ?

Created attachment 143088 [details] [review]
Use accessor functions instead direct access.v2

I've changed the functions as you requested

Comment on attachment 143088 [details] [review]
Use accessor functions instead direct access.v2

As discussed on irc, this patch is unsafe as it introduces a crash when adding a toplevel window to the workspace.

Created attachment 149090 [details] [review]
Use accessor functions instead direct access.v3

I was worked in the former patch and found some errors.
Here a new patch that work correctly in my tests.

The new required GTK+ version is now 2.17.10

Created attachment 149091 [details] [review]
Use accessor functions instead direct access.Second patch

This patch substitutes new code with the new GTK+ api additions.

The required GTK+ version is now 2.19.0

Review of attachment 149090 [details] [review]:

Ok thanks for fixing the errors and for redoing the huge patch.

I looked briefly over it and tested it a bit, the previous crashes
arent there, so lets go ahead and commit this one.

Review of attachment 149091 [details] [review]:

Please commit this one to master also.

Comment on attachment 149090 [details] [review]
Use accessor functions instead direct access.v3

commit 2d7e9abe72d4ae63cdb7906751c6f85758a14593

Comment on attachment 149091 [details] [review]
Use accessor functions instead direct access.Second patch

4dda3ade8851db79150af9865acc93855ee59e00

For the record, still missing (no GTK+ api available):

    GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL);
    GTK_WIDGET_SET_FLAGS (widget, GTK_REALIZED);
    GTK_WIDGET_REALIZED ()
    GTK_WIDGET_MAPPED ()

Created attachment 153493 [details] [review]
Use accessor functions instead direct accessv. Third patch

Substitute GTK_WIDGET_REALIZED() and GTK_WIDGET_MAPPED()

Only remaining function: GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)

Review of attachment 153493 [details] [review]:

Thanks, please commit.

Comment on attachment 153493 [details] [review]
Use accessor functions instead direct accessv. Third patch

commit f8bbea40f05850a40ebecd2d38378c59acc444e8

Created attachment 153598 [details] [review]
Use accessor functions instead direct access. fourth patch

Only missing:

- GTK_INPUT_DIALOG ()->save_button

Maybe wa can file a bug to add GTK_RESPONSE_SAVE?

- GTK_FONT_SELECTION_DIALOG ()->fontsel

Dont' know how to soleve this

- GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)

This is really a bug because you should never need to use this

See mith coment here: https://bugzilla.gnome.org/show_bug.cgi?id=593601#c4
and ebassi one here: https://bugzilla.gnome.org/show_bug.cgi?id=69872#c27

Review of attachment 153598 [details] [review]:

Reviewed your patch, gtk_dialog_get_widget_for_response() will be fine
the way its used by Glade assuming it works properly (and we like to assume that ;-)),
do we need to bump the GTK+ required version in configure.ac yet ?

 - glade_gtk_dialog_get_internal_child ()
   In this function you add a case of strcmp() to get the response id;
   I think we can drop all the if (GTK_IS_MESSAGE_DIALOG(dialog) { ... } else if (... type code in there
   i.e. gtk_dialog_get_widget_for_response() should only return NULL if it doesnt have that response, not 
   fire any assertions or anything.

 - glade_gtk_dialog_get_children () could take a similar simplification (i.e. check if any of the stock
   responses are there, list them and drop the type casing would be nicer).

Regarding this one:
    GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)

This is a hack we have been depending on since 3.2.

If we cant compile in a way that lets us use this hack
basically Glade wont work on new GTK+ without a good
headache.

Working around it will involve tricking Glade to use
a different type for GtkWindow in the workspace (probably
not all that hard to do, but that hack has not been
broken for a long time, so it makes little sense to
go and fix it).

Note; ultimately if clutter is going to be portable
and usable everywhere; it would be nice to make a clutter
based workspace and allow drawing over widgets, maybe
also removing that hack by consequence (but I cant predict
its going to happen soon).

(In reply to comment #16)
> Review of attachment 153598 [details] [review]:
>  - glade_gtk_dialog_get_internal_child ()
>    In this function you add a case of strcmp() to get the response id;
>    I think we can drop all the if (GTK_IS_MESSAGE_DIALOG(dialog) { ... } else
> if (... type code in there
>    i.e. gtk_dialog_get_widget_for_response() should only return NULL if it
> doesnt have that response, not 
>    fire any assertions or anything.

OK, But How Can we solve these cases:

- GTK_INPUT_DIALOG (dialog)->save_button (there is no GTK_RESPONSE_SAVE)
- GTK_FONT_SELECTION_DIALOG (dialog)->fontsel

There is gtk_font_selection_get_font_name(), but this function returns the a gchar*, not the widget.

For GtkInputDialog, if it includes a save button, maybe it should
define a response id for it.

I'm not sure if its useful to expose the internal font selection
widget of a fontsel dialog anyway... but I suppose its the widget
in the dialog's action area...

(In reply to comment #19)
> For GtkInputDialog, if it includes a save button, maybe it should
> define a response id for it.

Bad news, GtkInputDialog doesn't define a response for its buttons :/
 
> I'm not sure if its useful to expose the internal font selection
> widget of a fontsel dialog anyway... but I suppose its the widget
> in the dialog's action area...

GTK_FONT_SELECTION_DIALOG is not a derived class of a GtkDialog :/

(In reply to comment #18)
> OK, But How Can we solve these cases:
> 
> - GTK_INPUT_DIALOG (dialog)->save_button (there is no GTK_RESPONSE_SAVE)

Can you just pick the most relevant one?  GTK_RESPONSE_APPLY seems like it should be sufficient.

(In reply to comment #21)
> (In reply to comment #18)
> > OK, But How Can we solve these cases:
> > 
> > - GTK_INPUT_DIALOG (dialog)->save_button (there is no GTK_RESPONSE_SAVE)
> 
> Can you just pick the most relevant one?  GTK_RESPONSE_APPLY seems like it
> should be sufficient.

AFAICS no, we would at least need GtkInputDialog to assign its stock
"save" button the response id on its own (best to document that behaviour
too), then we could later get at it with the known response id
(unless thats already the case, in which case yes, we can just use that).


The best thing would be if we had gtk_buildable_get_internal_child()
exposed without the irrelevant GtkBuilder * argument for that method.

The whole reason why we need access to these is to expose internal
widgets that will be recognized by builder.

If GTK+ does not expose the save button of an input dialog
as an internal widget already, then we can look at this as a simple
incompatability from libglade --> GtkBuilder, and we can look
into a way to disable access to those internal widgets while
working in GtkBuilder format (its possible, I carried 
over lots of code and some of it was based on assumption).

Only note that GtkInput Dialog is marked as deprecated since GTK+ 2.20, but maybe we still need a solution for old code.

Created attachment 164193 [details] [review]
Use accessor functions instead direct access. fourth patch.v2

Here a new patch against current master.

I tried something different here: I've treated all dialogs as normal derived dialogs (GtkFontSelectionDialog and GtkColoSelectionDialog are derived from GtkDialog too).

I think the result is more consistent with the rest of the dialogs.

The only missing issue:

GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)

Tristan: Can this please be reviewed very soon? We want to ship GNOME 2.31.4 (next week) with all GSeal issues fixed.

(In reply to comment #24)
> The only missing issue:
> GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)

Maybe that can be dropped? At least when fixing seahorse "GTK_WIDGET_SET_FLAGS (window, GTK_TOPLEVEL);" was simply dropped without any bad outcome...

Andre, I'm afraid it's wishful thinking to have Glade 3.0 ready in that
time (as soon as GTK_WIDGET_SET_FLAGS() goes away Glade wont compile
or just wont be usable without that line).

Glade unsets the toplevel flag on GtkWindow to avoid assertions
in GTK+ because GTK+ doesnt like it when we add a GtkWindow to a
container widget (GTK+ refuses to put our widgets in the workspace
otherwise).

Juan Pablo is working on something to solve this possibly by
rendering the toplevel project widgets with offscreen windows.

Blocks anjuta release with gtk+-3.0...

Created attachment 165249 [details] [review]
Use accessor functions instead direct access. fourth patch.v3

Only missing:

GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)

Was the last patch committed somewhere? Anyway, this is fixed in offscreen-gtk3 as it compiles against latest gtk+ master.

No, AFAIK It hasn't been committed.
BTW, Thanks for your work in the offscreen-gtk3 branch ;)

This problem has been fixed in the development version. The fix will be available in the next major software release :)