GNOME Bugzilla – Bug 586731
Should build with -DGSEAL_ENABLE
Last modified: 2010-07-04 08:56:02 UTC
To be ready for GNOME 3 we should be able to build with -DGSEAL_ENABLE Patch follows for libgames-support, glines and gnobots. The rest are WIP. Chpe: I need to add GTK_CHECK_VERSION to games-scores-dialog.c and games-gridframe.c for maemo right? What about aisleriot, should I ignore it or do you want a patch for it too? Currently it is not possible to fix 100% as there are no accessor functions for GtkObject flags and GtkWidget allocation yet.
Created attachment 137238 [details] [review] Gseal fixes for libgames-support, glines, and gnobots2
games-scores-dialog.c and games-gridframe.c are unused on maemo. Aisleriot already has quite enough #ifdef'd code already, I'd really hate to add more of it. Maybe a different approach would work: as far as I can tell, all of these new accessors just do "return widget->member" directly, without doing any additional work, right? In that case, we could add a games-gtk-compat.h header to libgames-support, that has #if !GTK_CHECK_VERSION (x, y, z) #define gtk_widget_get_window(widget) ((widget)->window) .... #endif /* GTK < x.y.z */ and just include that in aisleriot's code and then use the new accessors regardless of the gtk version. And if in future a new accessor is added to gtk and we'd have to bump our gtk req just for this reason, we could just add to the compat header and keep the lower gtk req, also for other games. In fact that fixes the biggest complaint I have about these accessors: the needless gtk req bumping :)
Yes the ifdef's are starting to become a pain. I was hoping that you had an idea how to avoid it :) The games-gtk-compat.h sounds like a good solution. It looks like most accessors were added in 2.13.4 (GSEAL branch merged) and then some extra in later versions. But I guess we only care about those accessors we actually use in aisleriot (or entire gnome-games). I think I will just commit the games I fix soon but leave a patch for aisleriot & games-gtk-compat.h for review by you here. Unless you do it before me that is... :)
Created attachment 137344 [details] [review] Gseal fixes for gnotravex, gnotski, and gtali
Created attachment 138773 [details] [review] Gseal fixes for mahjongg, same-gnome, and part of gnomine
Created attachment 139170 [details] [review] Add games-gtk-compat.h and use it for a few sealings in aisleriot Chpe: This is a first stab at the aisleriot/games-gtk-compat.h stuff. I only included gtk_widget_get_window and gtk_widget_get_state in this patch. I just wanted to make sure that this was what you had in mind.
Yes, that looks fine. A few comments: +// Temporary fix from http://live.gnome.org/GnomeGoals/UseGseal No C++ comments please. +#undef GTK_OBJECT_FLAGS +#define GTK_OBJECT_FLAGS(i) (GTK_OBJECT (i)->GSEAL(flags)) This block should be #ifdef GSEAL_ENABLE, I think? + source = gdk_bitmap_create_from_data (gtk_widget_get_window (widget), data, 20, 20); + mask = gdk_bitmap_create_from_data (gtk_widget_get_window (widget), mask_data, 20, 20); Where the same GtkWidget accessor is used more than once in a function, I'd prefer to call it only once, by using a local variable of the appropriate type. With these changes, please commit. Thanks!
Created attachment 141100 [details] [review] Updated patch. Add games-gtk-compat.h and use it for a few sealings in aisleriot Patch updated with comments. Thanks Christian
Committed a small patch adding getter/setter for allocation to games-gtk-compat.h http://git.gnome.org/cgit/gnome-games/commit/?id=3ef185bbda8d9fbba65ac4a76739ee63e29d7233
Created attachment 142157 [details] [review] More fixes related to gseal. Mostly ->allocation Mostly fixes for ->allocation. Also a few fixes other fixes to complete aisleriot (except for flags)
Created attachment 142173 [details] [review] More fixes related to gseal. Mostly ->allocation (v2) Same patch as before but adjusted for comments from chpe on irc
Created attachment 142174 [details] [review] More fixes related to gseal. Mostly ->allocation (v3) Same as previous. Just added a few small fixes to aisleriot/window.c In aisleriot_window_init the "widget surgery" in the statusbar code looks tricky to do with gseal. Chpe?
Created attachment 144853 [details] [review] GTK_WIDGET_HAS_FOCUS and GTK_WIDGET_VISIBLE touches aisleriot. Chpe, review?
Created attachment 144857 [details] [review] GTK_WIDGET_HAS_FOCUS and GTK_WIDGET_VISIBLE v2 updated as per comments on irc
Created attachment 144912 [details] [review] Patch for aisleriot statusbar issues patch based on comments from chpe on irc
patch untested btw
Comment on attachment 144912 [details] [review] Patch for aisleriot statusbar issues + GtkWidget *statusbar_hbox,*label, *time_box, *frame; + frame = GTK_FRAME (list->data); That's a type mismatch. Since we only need the frame as a container, let's declare it GtkContainer*, and use GTK_CONTAINER cast, and remove the 2 casts in the gtk_container_* calls. And let's call it statusbar_frame. + gtk_container_remove (GTK_CONTAINER (frame), gtk_bin_get_child (GTK_BIN (frame))); + gtk_box_pack_start (GTK_BOX (statusbar_hbox), gtk_bin_get_child (GTK_BIN (frame)), TRUE, TRUE, 0); This won't work; on the 2nd line the frame has no child anymore, obviously :) So you need a local statusbar_label var.
Created attachment 144918 [details] [review] Patch for aisleriot statusbar issues v2 make much more sense now. Thanks
Comment on attachment 144918 [details] [review] Patch for aisleriot statusbar issues v2 Looks ok now. Assuming it also _works_ as expected, ok to commit :)
Comment on attachment 144918 [details] [review] Patch for aisleriot statusbar issues v2 Thanks. Committed: http://git.gnome.org/cgit/gnome-games/commit/?id=7436c9e0f02797e0b1f2c5429266668baf00fd81
Created attachment 149308 [details] [review] GTK_WIDGET_DRAWABLE(), GTK_WIDGET_SET_FLAGS (widget, GTK_CAN_FOCUS) and GTK_WIDGET_HAS_FOCUS()
Created attachment 149309 [details] [review] minefield: Fix GSEAL issues
Created attachment 149310 [details] [review] gnibbles: Fix GSEAL issues
Still missing (no GTK+ api available): GTK_WIDGET_SET_FLAGS (widget, GTK_REALIZED) GTK_WIDGET_REALIZED() GTK_WIDGET_MAPPED()
Created attachment 149475 [details] [review] GTK_WIDGET_DRAWABLE(), GTK_WIDGET_SET_FLAGS (widget, GTK_CAN_FOCUS) and GTK_WIDGET_HAS_FOCUS().v2 Updated patch after yesterday Chpe changes
Created attachment 155880 [details] [review] Use widow directly as it is already available This broke with bf42fd6efd28fa415a8df6660649d2565e40a2b0 With this patch we build with GSEAL again (with gtk 2.19.6)
Comment on attachment 155880 [details] [review] Use widow directly as it is already available Please commit.
Also, ar-clutter-embed.c will need a similar fix.
Created attachment 157330 [details] [review] Similar fix as last for ar-clutter-embed.c Patch for ar-clutter-embed.c attached. Will commit after 2.30 is branched
Created attachment 158013 [details] [review] Use accessor functions. Final patch
Comment on attachment 158013 [details] [review] Use accessor functions. Final patch When you replace these functions with the new accessors in aisleriot, you need to make sure that the file includes <libgames-support/games-gtk-compat.h>, which ar-fullscreen-button.c is missing. Also, if the macro occurs twice or more in the same function, you should introduce a local variable to hold the result instead of calling the accessor multiple times.
And this: +#define gtk_widget_set_realized(widget, TRUE) GTK_WIDGET_SET_FLAGS(widget, GTK_REALIZED) doesn't work, of course.
Created attachment 158044 [details] [review] Use accessor functions. Final patch Updated patch with your comments About this: #define gtk_widget_set_realized(widget, TRUE) GTK_WIDGET_SET_FLAGS(widget, GTK_REALIZED) seems to work here.
But it doesn't when you pass FALSE. gtk_widget_set_realized needs to have identical function whether it's the new accessor or the compat macro. So you need to fix it to support FALSE too.
Created attachment 158049 [details] [review] Use accessor functions. Final patch.v3 Ok, I think this should work (thanks to ebassi here ;))
Looks good to me. We should probably also fix this in the same way as gtk_widget_set_realized: #define gtk_widget_set_can_focus(widget, can_focus) GTK_WIDGET_SET_FLAGS (widget, GTK_CAN_FOCUS) Chpe, okay to commit with that extra fix?
(In reply to comment #36) > We should probably also fix this in the same way as gtk_widget_set_realized: > #define gtk_widget_set_can_focus(widget, can_focus) GTK_WIDGET_SET_FLAGS > (widget, GTK_CAN_FOCUS) Yes. > Chpe, okay to commit with that extra fix? Sure.
Comment on attachment 158049 [details] [review] Use accessor functions. Final patch.v3 committed with the requested fixes commit da14cdf0c479d91a1eaa0eeffa4bea6e870c0115
This problem has been fixed in the development version. The fix will be available in the next major software release. Congrats! :)
I had a quick look at games-gtk-compat.h yesterday and noticed that gtk_widget_set_window has two problems. 1) The space between the macro name and the first parentheses will cause it to be treated like an object instead of a function. 2) Both tokens of "window" will be replaced on the right side. It currently looks like this: #define gtk_widget_set_window (widget, window) ((widget)->window=window) To illustrate what is going on I've put the following in a file (test.c) and let the preprocessor expand the macro. #define gtk_widget_set_window (widget, window) ((widget)->window=window) gtk_widget_set_window(my_widget, my_window); thomas@mosegris:~$ gcc -E test.c ... (widget, window) ((widget)->window=window)(my_widget, my_window); Not the result we wanted... With the fixed version: #define gtk_widget_set_window(widget, window_arg) ((widget)->window=window_arg) gtk_widget_set_window(my_widget, my_window); thomas@mosegris:~$ gcc -E test.c ... ((my_widget)->window=my_window); Much better :)
This isn't done completely: aisleriot/window.c still has this: gtk_box_pack_end (GTK_BOX (GTK_MESSAGE_DIALOG (dialog)->label->parent), entry, FALSE, FALSE, 0);
Created attachment 163655 [details] [review] Fix a GSEAL issue with GdkVisual I don't see that error when I'm compiling. Do I need to pass something specific to autogen/make for that code to be compiled? On the other hand I do see a problem in aisleriot/board-noclutter.c with GdkVisual->depth. The patch fixes that but I suppose we want to add a compat for that as well.
The aisleriot thing only happens with --enable-debug-ui (debug only code, but essential; fixing it depends on gtk bug 328069).
Comment on attachment 163655 [details] [review] Fix a GSEAL issue with GdkVisual Looks fine. We do need a compat define for this, of course.
Created attachment 163725 [details] [review] Fix a GSEAL issue with GdkVisual With compat define. (Untested as my build broke for unrelated reasons)
Can the last patch get a review please?
What patch? All patches are committed. AFAIK this is FIXED.
(In reply to comment #47) > What patch? All patches are committed. > > AFAIK this is FIXED. Oh true - compiling git master with DGSEAL_ENABLE worked fine for me.