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 594957 - Use accessor functions instead direct access (use GSEAL GnomeGoal)
Use accessor functions instead direct access (use GSEAL GnomeGoal)
Status: RESOLVED FIXED
Product: glade
Classification: Applications
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: Glade 3 Maintainers
Glade 3 Maintainers
Depends on: 594960 594962 614510
Blocks: 585391 636726
 
 
Reported: 2009-09-12 01:43 UTC by Javier Jardón (IRC: jjardon)
Modified: 2010-12-30 16:08 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
Use accessor functions instead direct access (117.20 KB, patch)
2009-09-12 02:48 UTC, Javier Jardón (IRC: jjardon)
needs-work Details | Review
Use accessor functions instead direct access.v2 (117.14 KB, patch)
2009-09-12 23:01 UTC, Javier Jardón (IRC: jjardon)
needs-work Details | Review
Use accessor functions instead direct access.v3 (109.50 KB, patch)
2009-12-04 15:24 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Use accessor functions instead direct access.Second patch (10.75 KB, patch)
2009-12-04 15:26 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Use accessor functions instead direct accessv. Third patch (14.90 KB, patch)
2010-02-11 00:15 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Use accessor functions instead direct access. fourth patch (9.41 KB, patch)
2010-02-12 02:45 UTC, Javier Jardón (IRC: jjardon)
needs-work Details | Review
Use accessor functions instead direct access. fourth patch.v2 (10.72 KB, patch)
2010-06-21 02:54 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
Use accessor functions instead direct access. fourth patch.v3 (8.49 KB, patch)
2010-07-04 23:16 UTC, Javier Jardón (IRC: jjardon)
none Details | Review

Description Javier Jardón (IRC: jjardon) 2009-09-12 01:43:17 UTC
To be ready for GNOME 3 should be able to build with -DGSEAL_ENABLE

See http://live.gnome.org/GnomeGoals/UseGseal for more details
Comment 1 Javier Jardón (IRC: jjardon) 2009-09-12 02:48:50 UTC
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?
Comment 2 Tristan Van Berkom 2009-09-12 18:20:36 UTC
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 ?
Comment 3 Javier Jardón (IRC: jjardon) 2009-09-12 23:01:54 UTC
Created attachment 143088 [details] [review]
Use accessor functions instead direct access.v2

I've changed the functions as you requested
Comment 4 Tristan Van Berkom 2009-09-18 22:48:10 UTC
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.
Comment 5 Javier Jardón (IRC: jjardon) 2009-12-04 15:24:26 UTC
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
Comment 6 Javier Jardón (IRC: jjardon) 2009-12-04 15:26:22 UTC
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
Comment 7 Tristan Van Berkom 2009-12-04 19:04:59 UTC
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.
Comment 8 Tristan Van Berkom 2009-12-04 19:08:20 UTC
Review of attachment 149091 [details] [review]:

Please commit this one to master also.
Comment 9 Javier Jardón (IRC: jjardon) 2009-12-04 19:20:30 UTC
Comment on attachment 149090 [details] [review]
Use accessor functions instead direct access.v3

commit 2d7e9abe72d4ae63cdb7906751c6f85758a14593
Comment 10 Javier Jardón (IRC: jjardon) 2009-12-04 19:20:51 UTC
Comment on attachment 149091 [details] [review]
Use accessor functions instead direct access.Second patch

4dda3ade8851db79150af9865acc93855ee59e00
Comment 11 Javier Jardón (IRC: jjardon) 2009-12-04 19:22:36 UTC
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 ()
Comment 12 Javier Jardón (IRC: jjardon) 2010-02-11 00:15:48 UTC
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)
Comment 13 Tristan Van Berkom 2010-02-11 01:56:07 UTC
Review of attachment 153493 [details] [review]:

Thanks, please commit.
Comment 14 Javier Jardón (IRC: jjardon) 2010-02-11 03:29:04 UTC
Comment on attachment 153493 [details] [review]
Use accessor functions instead direct accessv. Third patch

commit f8bbea40f05850a40ebecd2d38378c59acc444e8
Comment 15 Javier Jardón (IRC: jjardon) 2010-02-12 02:45:47 UTC
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
Comment 16 Tristan Van Berkom 2010-02-12 21:34:04 UTC
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).
Comment 17 Tristan Van Berkom 2010-02-12 21:46:31 UTC

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).
Comment 18 Javier Jardón (IRC: jjardon) 2010-02-14 02:37:06 UTC
(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.
Comment 19 Tristan Van Berkom 2010-02-14 02:47:23 UTC
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...
Comment 20 Javier Jardón (IRC: jjardon) 2010-02-14 03:28:23 UTC
(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 :/
Comment 21 Cody Russell 2010-02-15 01:00:49 UTC
(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.
Comment 22 Tristan Van Berkom 2010-02-15 01:21:00 UTC
(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).
Comment 23 Javier Jardón (IRC: jjardon) 2010-02-15 01:57:52 UTC
Only note that GtkInput Dialog is marked as deprecated since GTK+ 2.20, but maybe we still need a solution for old code.
Comment 24 Javier Jardón (IRC: jjardon) 2010-06-21 02:54:29 UTC
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)
Comment 25 André Klapper 2010-06-21 07:35:00 UTC
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...
Comment 26 Tristan Van Berkom 2010-06-21 07:48:23 UTC
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.
Comment 27 Johannes Schmid 2010-06-27 18:57:02 UTC
Blocks anjuta release with gtk+-3.0...
Comment 28 Javier Jardón (IRC: jjardon) 2010-07-04 23:16:40 UTC
Created attachment 165249 [details] [review]
Use accessor functions instead direct access. fourth patch.v3

Only missing:

GTK_WIDGET_UNSET_FLAGS (widget, GTK_TOPLEVEL)
Comment 29 Johannes Schmid 2010-11-15 23:20:28 UTC
Was the last patch committed somewhere? Anyway, this is fixed in offscreen-gtk3 as it compiles against latest gtk+ master.
Comment 30 Javier Jardón (IRC: jjardon) 2010-11-16 00:05:38 UTC
No, AFAIK It hasn't been committed.
BTW, Thanks for your work in the offscreen-gtk3 branch ;)
Comment 31 Javier Jardón (IRC: jjardon) 2010-12-30 16:08:05 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release :)