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 587991 - Remove deprecated GTK+ symbols
Remove deprecated GTK+ symbols
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 620772 (view as bug list)
Depends on:
Blocks: 585692 622303
 
 
Reported: 2009-07-07 16:24 UTC by André Klapper
Modified: 2010-07-02 13:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch #1 adapted from Metacity (2.51 KB, patch)
2009-08-01 10:41 UTC, Tomas Frydrych
committed Details | Review
Patch #2 adapted from metacity (14.58 KB, patch)
2009-08-01 10:43 UTC, Tomas Frydrych
committed Details | Review
Remove more deprecated GTK+ symbols (9.34 KB, patch)
2009-08-10 10:59 UTC, Javier Jardón (IRC: jjardon)
needs-work Details | Review
Remove deprecated GTK+ symbols_v2 (2.66 KB, patch)
2009-08-22 04:41 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Remove deprecated GTK+ symbols_v2_part2 (6.52 KB, patch)
2009-08-22 07:11 UTC, Javier Jardón (IRC: jjardon)
accepted-commit_now Details | Review
Remove deprecated GTK+ symbols_v2_part2_v2 (6.56 KB, patch)
2009-08-26 01:27 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
Remove deprecated GTK+ symbols_v2_part2_v2 (6.87 KB, patch)
2009-08-26 01:33 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Stop using gdk_window_get_type, gtk_widget_set_uposition (1.79 KB, patch)
2009-08-26 18:14 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
[PATCH] - replace use of GtkObject system (18.94 KB, patch)
2009-11-25 19:24 UTC, Nickolas Lloyd
none Details | Review
[PATCH] - replace use of GtkObject system (18.88 KB, patch)
2009-11-25 20:12 UTC, Nickolas Lloyd
none Details | Review
replace use of deprecated GtkItemFactory framework (7.57 KB, patch)
2009-12-15 21:57 UTC, Nickolas Lloyd
none Details | Review
remove use of GtkItemFactory from src/ui/theme-viewer.c (7.81 KB, patch)
2009-12-16 05:32 UTC, Nickolas Lloyd
none Details | Review
remove use of GtkItemFactory from src/tools/mutter-window-demo.c (17.43 KB, patch)
2009-12-16 05:37 UTC, Nickolas Lloyd
none Details | Review
remove use of gdk_window_get_type () (853 bytes, patch)
2009-12-17 01:16 UTC, Nickolas Lloyd
none Details | Review
Remove use of all deprecated Gtk symbols in mutter (44.62 KB, patch)
2009-12-24 04:51 UTC, Nickolas Lloyd
needs-work Details | Review
fixed patch to remove deprecated symbols (30.09 KB, patch)
2010-05-28 20:48 UTC, Nickolas Lloyd
needs-work Details | Review
remove deprecated gtk symbols from mutter (29.00 KB, patch)
2010-06-11 19:25 UTC, Nickolas Lloyd
needs-work Details | Review
remove use of deprecated gtk symbols (29.85 KB, patch)
2010-06-13 23:56 UTC, Nickolas Lloyd
reviewed Details | Review
Replace deprecated GDK symbols (2.68 KB, patch)
2010-06-14 11:30 UTC, Florian Müllner
none Details | Review
Add compatibility with GTK+ 2.18 (2.34 KB, patch)
2010-06-14 11:31 UTC, Florian Müllner
none Details | Review
Replace deprecated GDK symbols (2.69 KB, patch)
2010-06-14 11:52 UTC, Florian Müllner
none Details | Review
Add compatibility with GTK+ 2.18 (2.36 KB, patch)
2010-06-14 12:14 UTC, Florian Müllner
none Details | Review
remove deprecated gtk symbols (29.57 KB, patch)
2010-06-14 17:52 UTC, Nickolas Lloyd
accepted-commit_now Details | Review
Replace deprecated GDK symbols (3.15 KB, patch)
2010-06-19 20:48 UTC, Florian Müllner
none Details | Review
Add compatibility with GTK+ 2.18 (2.36 KB, patch)
2010-06-19 20:48 UTC, Florian Müllner
none Details | Review
Replace deprecated GDK symbols (4.05 KB, patch)
2010-06-27 04:38 UTC, Florian Müllner
none Details | Review
Replace deprecated GDK symbols (3.97 KB, patch)
2010-06-27 06:53 UTC, Florian Müllner
committed Details | Review
Add compatibility with GTK+ 2.18 (3.68 KB, patch)
2010-06-27 07:27 UTC, Florian Müllner
committed Details | Review
Remove deprecated Gtk+ symbols (23.05 KB, patch)
2010-06-30 12:46 UTC, Florian Müllner
committed Details | Review
Use cairo_region_t when building with gtk+-3.0 (31.46 KB, patch)
2010-06-30 13:32 UTC, Florian Müllner
none Details | Review
Use cairo_region_t when building with gtk+-3.0 (26.66 KB, patch)
2010-07-01 15:52 UTC, Florian Müllner
needs-work Details | Review
Use cairo_region_t when building with gtk+-3.0 (27.93 KB, patch)
2010-07-01 18:46 UTC, Florian Müllner
none Details | Review
Use cairo_region_t when building with gtk+-3.0 (27.93 KB, patch)
2010-07-02 03:08 UTC, Florian Müllner
committed Details | Review

Description André Klapper 2009-07-07 16:24:20 UTC
http://live.gnome.org/GnomeGoals/RemoveDeprecatedSymbols/GTK%2B

According to http://www.gnome.org/~fpeters/299.html mutter uses deprecated GTK+ symbols.

GTK_CHECK_CAST, GTK_CHECK_CLASS_CAST, GTK_CHECK_CLASS_TYPE, GTK_CHECK_GET_CLASS, GTK_CHECK_TYPE, GTK_SIGNAL_FUNC, GtkClassInitFunc, GtkItemFactoryEntry, GtkObjectInitFunc, GtkType, GtkTypeInfo, gdk_pixbuf_render_to_drawable, gdk_window_get_type, gtk_item_factory_create_items, gtk_item_factory_get_widget, gtk_item_factory_new, gtk_item_factory_set_translate_func, gtk_object_sink, gtk_signal_connect_full, gtk_signal_disconnect_by_func, gtk_style_ref, gtk_toolbar_insert_stock, gtk_type_class, gtk_type_new, gtk_type_unique, gtk_widget_set_uposition, gtk_window_set_policy


Potential patch contributors see http://library.gnome.org/devel/gtk/ for API references.

Also see Metacity bug 572332 about this - maybe efforts can be shared.
Comment 1 Tomas Frydrych 2009-08-01 08:36:03 UTC
Looks like all of this is in src/ui, and we have not touched that too much so, yes, we should be able to share the effort. The Metacity bug has got a bunch of patches; some which have already been committed -- any objections if just go ahead and merge in at least the committed ones ?
Comment 2 Tomas Frydrych 2009-08-01 10:41:49 UTC
Created attachment 139677 [details] [review]
Patch #1 adapted from Metacity
Comment 3 Tomas Frydrych 2009-08-01 10:43:22 UTC
Created attachment 139678 [details] [review]
Patch #2 adapted from metacity

Added 2 patches that are equivalent to the two from bug #572332 that are already committed in metacity.
Comment 4 Owen Taylor 2009-08-03 18:08:20 UTC
(In reply to comment #2)
> Created an attachment (id=139677) [edit]
> Patch #1 [edit] adapted from Metacity

Looks good.
Comment 5 Owen Taylor 2009-08-03 18:24:03 UTC
(In reply to comment #3)
> Created an attachment (id=139678) [edit]
> Patch #2 [edit] adapted from metacity
> 
> Added 2 patches that are equivalent to the two from bug #572332 that are
> already committed in metacity.

   g_object_ref (item_factory);
-  gtk_object_sink (GTK_OBJECT (item_factory));
+  g_object_ref_sink (item_factory);
+  g_object_unref (item_factory);

The ref/unref left-over after this change is pointless. Other than that, the patch looks good.
Comment 6 Tomas Frydrych 2009-08-04 08:27:51 UTC
Patches 1 and 2 committed as e985bf0e7aaaf51a1bacc37ea8f0ed3130821612 and ff9400abde7681529e3741bad26bba8a80e99717 (the latter without the unnecessary ref/unref mentioned in comment #5.

Leaving the bug open, as there is more to do.
Comment 7 Javier Jardón (IRC: jjardon) 2009-08-10 10:59:19 UTC
Created attachment 140314 [details] [review]
Remove more deprecated GTK+ symbols

Removed gtk_toolbar_insert_stock, GTK_SIGNAL_FUNC, gdk_pixbuf_render_to_draw in this patch
Comment 8 Owen Taylor 2009-08-19 14:48:43 UTC
Comment on attachment 140314 [details] [review]
Remove more deprecated GTK+ symbols

Mixed declarations and code throughout patch.

I would suggest adding a insert_stock_button() helper and using that. (Do it separately in each file, meeting just the requirements of that file.)

gdk_pixbuf_render_to_drawable() portions look OK. Probably better to split the patch in half.

Would prefer not to have the magic GnomeBug:587991 in the header. Best option is to just have the bug URL in the body. (take a look at 'git log')
Comment 9 Javier Jardón (IRC: jjardon) 2009-08-22 04:41:05 UTC
Created attachment 141384 [details] [review]
Remove deprecated GTK+ symbols_v2

Patch to remove GTK_SIGNAL_FUNC, gdk_pixbuf_render_to_drawable
Comment 10 Javier Jardón (IRC: jjardon) 2009-08-22 07:11:09 UTC
Created attachment 141385 [details] [review]
Remove deprecated GTK+ symbols_v2_part2

Patch to remove gtk_toolbar_insert_stock
Comment 11 Owen Taylor 2009-08-24 18:20:31 UTC
Comment on attachment 141384 [details] [review]
Remove deprecated GTK+ symbols_v2

Looks good
Comment 12 Owen Taylor 2009-08-24 18:26:05 UTC
Comment on attachment 141385 [details] [review]
Remove deprecated GTK+ symbols_v2_part2

Looks pretty good. Just two minor things to fix up before committing:

Mutter (Metacity) is that function parameters should be one-per-line with the variable names aligned. See the other functions in mutter-window-demo.c for examples.

+  gtk_tool_item_set_tooltip_text (button,
+                                  _(text));

You should leave the _() on the strings - moving it like this means that they will no longer be extracted for the .po file.

(You could use N_() on the strings and then _() in the helper functions, but I don't see a point in that.)
Comment 13 Javier Jardón (IRC: jjardon) 2009-08-26 01:27:28 UTC
Created attachment 141714 [details] [review]
Remove deprecated GTK+ symbols_v2_part2_v2

Thank you for all your comments Owen.
Please, tell me if something is still wrong

Regards
Comment 14 Javier Jardón (IRC: jjardon) 2009-08-26 01:33:36 UTC
Created attachment 141717 [details] [review]
Remove deprecated GTK+ symbols_v2_part2_v2

Fixed some indentation problems.
Comment 15 Owen Taylor 2009-08-26 17:04:17 UTC
Comment on attachment 141384 [details] [review]
Remove deprecated GTK+ symbols_v2

Pushed (rewrote the two commits messages to have different Subject: lines)

http://git.gnome.org/cgit/mutter/commit/?id=cc46d2ebb438979fc00ec062cd301dcfced36d42
Comment 16 Owen Taylor 2009-08-26 17:05:29 UTC
Comment on attachment 141717 [details] [review]
Remove deprecated GTK+ symbols_v2_part2_v2

Looks, good, pushed as well.

http://git.gnome.org/cgit/mutter/commit/?id=d0510d8ea25a89de9c8e5af7c60fda7255227483

(Two CGIT links are reversed.)
Comment 17 Owen Taylor 2009-08-26 17:08:04 UTC
Are there more outstanding things that need to be fixed, or can this be closed?
Comment 18 André Klapper 2009-08-26 17:20:04 UTC
(In reply to comment #17)
> Are there more outstanding things that need to be fixed, or can this be closed?

See http://www.gnome.org/~fpeters/299.html .

You should get rid very quickly of:

GTK_SIGNAL_FUNC, GtkClassInitFunc, GtkItemFactoryEntry, GtkObjectInitFunc, GtkTypeInfo, gdk_pixbuf_render_to_drawable, gdk_window_get_type, gtk_item_factory_create_items, gtk_item_factory_get_widget, gtk_item_factory_new, gtk_item_factory_set_translate_func, gtk_signal_connect_full, gtk_toolbar_insert_stock, gtk_type_class, gtk_type_new, gtk_type_unique, gtk_widget_set_uposition
Comment 19 André Klapper 2009-08-26 17:20:54 UTC
(hmm, might be that the list is not yet updated to reflect the last two commits)
Comment 20 Javier Jardón (IRC: jjardon) 2009-08-26 18:14:03 UTC
Created attachment 141772 [details] [review]
Stop using gdk_window_get_type, gtk_widget_set_uposition
Comment 21 Owen Taylor 2009-08-26 18:16:54 UTC
Please add a note here when patches are attached that handle all deprecated symbols. I'd like to do the review of the rest as a group.
Comment 22 André Klapper 2009-08-26 18:24:08 UTC
While I understand that from a maintainer point of view, it's not really motivating if you want to have code contributors in this (boring) case.
Comment 23 Owen Taylor 2009-08-26 18:55:47 UTC
I just want to be up front about setting the correct expectations here. I'm willing to put in the time to review patches, but it's a lot harder and more time-consuming if I have to context switch many times to review small patches individually.
Comment 24 Nickolas Lloyd 2009-11-25 19:24:09 UTC
Created attachment 148473 [details] [review]
[PATCH] - replace use of GtkObject system

works for me.  hope it's satisfactory.
Comment 25 Nickolas Lloyd 2009-11-25 20:12:05 UTC
Created attachment 148475 [details] [review]
[PATCH] - replace use of GtkObject system

updated patch to remove compilation error
Comment 26 Nickolas Lloyd 2009-12-15 21:57:49 UTC
Created attachment 149804 [details] [review]
replace use of deprecated GtkItemFactory framework

this patch replaces all uses of the deprecated GtkItemFactory system with GtkUIManager.  this patch will break translations of the related menu and button names, so a patch will/should be forthcoming for that.  also, once (if) i am done removing all deprecated symbols from mutter, i will bundle all the changes into a single patch as requested
Comment 27 Nickolas Lloyd 2009-12-16 05:32:53 UTC
Created attachment 149814 [details] [review]
remove use of GtkItemFactory from src/ui/theme-viewer.c
Comment 28 Nickolas Lloyd 2009-12-16 05:37:04 UTC
Created attachment 149815 [details] [review]
remove use of GtkItemFactory from src/tools/mutter-window-demo.c
Comment 29 Nickolas Lloyd 2009-12-17 01:16:23 UTC
Created attachment 149891 [details] [review]
remove use of gdk_window_get_type ()

last patch...
Comment 30 Nickolas Lloyd 2009-12-24 04:51:59 UTC
Created attachment 150333 [details] [review]
Remove use of all deprecated Gtk symbols in mutter

this patch replaces all uses of deprecated Gtk symbols in mutter, except gtk_widget_set_uposition(), which it appears can only be set now with direct calls to the X server.
Comment 31 Nickolas Lloyd 2009-12-24 04:52:56 UTC
(In reply to comment #21)
> Please add a note here when patches are attached that handle all deprecated
> symbols. I'd like to do the review of the rest as a group.

patch attached below which removes all but one deprecated Gtk symbol
Comment 32 André Klapper 2010-01-05 12:41:56 UTC
ultrageek: Thanks a lot for the patch! Which exact symbol is missing?
Comment 33 Nickolas Lloyd 2010-01-06 05:52:29 UTC
the last symbol is gtk_widget_set_uposition(), which doesn't look like it will be replaced
Comment 34 Nickolas Lloyd 2010-01-12 04:42:48 UTC
Andre, could you test that patch and report any findings here?  perhaps the devs will take notice and consider merging it.

Thanks
Comment 35 Javier Jardón (IRC: jjardon) 2010-03-31 22:01:34 UTC
Hello!

Could the ultrageek.lloyd patch get a review?

Thanks!
Comment 36 Colin Walters 2010-04-10 20:51:14 UTC
Review of attachment 141772 [details] [review]:

This looks fine.
Comment 37 Colin Walters 2010-04-10 21:03:15 UTC
Review of attachment 150333 [details] [review]:

Thanks a lot for working on this!  The major issue here is definitely that this patch would be *significantly* simpler if you used G_DEFINE_TYPE.  There are a few other minor issues, see below.

::: src/ui/frames.c
@@ +137,1 @@
 }

No, just replace all of this with G_DEFINE_TYPE.

@@ +581,1 @@
 

This is unnecessary with G_DEFINE_TYPE.

@@ +584,2 @@
                                    screen_number);
+  return g_object_new (frames_type,

Keep this as META_TYPE_FRAMES.

::: src/ui/frames.h
@@ +113,3 @@
 };
+
+GTypeInfo        meta_frames_get_type_info          (void) G_GNUC_CONST;

This is not needed.

::: src/ui/metaaccellabel.c
@@ +78,3 @@
+      (GClassInitFunc) meta_accel_label_class_init,
+      (GBaseInitFunc) NULL,
+      sizeof (MetaAccelLabelClass),

Use G_DEFINE_TYPE, same as my comments for frames.c.

::: src/ui/metaaccellabel.h
@@ +93,3 @@
 };
+GType      meta_accel_label_get_type          (void);
+GTypeInfo  meta_accel_label_get_type_info     (void) G_GNUC_CONST;

Like the change for frames.c this is wrong, we only need the _get_type to be public.

::: src/ui/preview-widget.c
@@ +65,3 @@
+      (GClassInitFunc) meta_preview_class_init,
+      (GBaseInitFunc) NULL,
+      sizeof (MetaPreviewClass),

G_DEFINE_TYPE.

::: src/ui/preview-widget.h
@@ +65,3 @@
 GType    meta_preview_get_type (void) G_GNUC_CONST;
 
+GTypeInfo    meta_preview_get_type_info (void);

Not necessary.

::: src/ui/themewidget.c
@@ +59,3 @@
+      (GClassInitFunc) meta_area_class_init,
+      (GBaseInitFunc) NULL,
+      sizeof (MetaAreaClass),

G_DEFINE_TYPE.

::: src/ui/themewidget.h
@@ +69,3 @@
+
+GTypeInfo meta_area_get_type_info (void);
+

Not necessary.

::: src/ui/theme-viewer.c
@@ +139,1 @@
       

That's not how GError works; use:

GError *error = NULL;

Then you pass it by reference using &error.  See:
file:///usr/share/gtk-doc/html/glib/glib-Error-Reporting.html
Comment 38 Javier Jardón (IRC: jjardon) 2010-04-11 16:37:33 UTC
Comment on attachment 141772 [details] [review]
Stop using gdk_window_get_type, gtk_widget_set_uposition

commit fac482c44265f5edd642b49957a8e5261179e338
Comment 39 Nickolas Lloyd 2010-04-12 22:05:33 UTC
Collin, thanks for reviewing that.  I wasn't aware of G_DEFINE_TYPE (first time using glib), but I'll get to work changing that and making corrections according to your suggestions.  Thanks again!
Comment 40 Nickolas Lloyd 2010-05-28 20:48:41 UTC
Created attachment 162237 [details] [review]
fixed patch to remove deprecated symbols

This one uses G_DEFINE_TYPE() rather than writing everything out manually, and pretty much leaves the current structure in tact.  much simpler :)
Comment 41 André Klapper 2010-06-07 16:35:44 UTC
*** Bug 620772 has been marked as a duplicate of this bug. ***
Comment 42 Florian Müllner 2010-06-11 02:46:49 UTC
Review of attachment 162237 [details] [review]:

Note that your patch does not remove all calls to deprecated API (e.g. preview-widget.c, fixedtip.c). In GNOME Shell we append the full URL of the bugzilla bug to the commit message - not sure whether mutter follows the same policy.

A whole bunch of comments:

::: src/tools/mutter-window-demo.c
@@ +24,3 @@
 #include <unistd.h>
 #include <X11/Xatom.h>
+#include <string.h>

Not needed if you pass -1 as length parameter below

@@ +663,3 @@
+  name = gtk_action_get_name (action);
+  const gchar *name;
+  guint callback_action;

Not too elegant, but okay-ish. You could cut out the enum and use the action name directly as parameter to make_dock - not sure if it's worth the trouble though.

@@ +894,1 @@
+#define NUM_MENU_ITEMS 16

This is just asking for trouble - do not hardcode a length parameter which is guaranteed to go out of sync with the actual array one day or another. Instead use G_N_ELEMENTS below.

@@ +910,3 @@
+{
+
+#define NUM_MENU_ITEMS 16

Dito.

@@ +957,3 @@
+  action_group = gtk_action_group_new ("themenu");
+
+  contents = gtk_text_view_new ();

"mainmenu" would be more in line with the original code ("<main>")

@@ +971,3 @@
+  action_group = gtk_action_group_new ("themenu");
+
+  contents = gtk_text_view_new ();

See comment below about tearoffs in theme-viewer.c

@@ +981,1 @@
                           (GDestroyNotify) g_object_unref);

Replace the whole block with a single call to g_object_unref() at the end of the function. See the comment about this in theme-viewer.c

@@ +985,3 @@
+                                     strlen (menu_item_string),
+  gtk_ui_manager_add_ui_from_string (ui_manager, menu_item_string,
+

Pass NULL instead of error if you don't look at the result anyway - in which case the variable declaration should be removed as well.

::: src/ui/menu.c
@@ +110,3 @@
+  g_free(data);
+{
+meta_closure_notify (gpointer data, GClosure *closure)

Not needed, see below. If you insist on keeping it, add a space before the parameter list.

@@ +456,3 @@
+                                             G_CALLBACK (activate_cb),
+                                             "activate",
+                      g_signal_connect_data (GTK_OBJECT (submi),

You can use (GClosureNotify)g_free instead of meta_closure_notify()

@@ +484,3 @@
+                                     G_CALLBACK (activate_cb),
+                                     "activate",
+              g_signal_connect_data (GTK_OBJECT (mi),

Dito.

::: src/ui/theme-viewer.c
@@ +113,2 @@
 };
+#define NUM_MENU_ITEMS 11

Eeeek - use G_N_ELEMENTS below - doing a define here is almost guaranteed to get out of sync sooner or later.

@@ +127,1 @@
+#define NUM_TOOL_ITEMS 3

Dito.

@@ +154,3 @@
+                                menu_item_struct,
+  gtk_action_group_add_actions (action_group,
+  action_group = gtk_action_group_new ("themenu");

gettext is already the default, so you don't need this

@@ +157,3 @@
+                                menu_item_struct,
+  gtk_action_group_add_actions (action_group,
+  action_group = gtk_action_group_new ("themenu");

Not sure about that one - metacity-theme-viewer does use tearoffs, so this is a straightforward port, but note that it looks like support for tearoffs will be removed (for good reasons I might add). Also, the purpose of the program is to give an impression how different window types are going to look with a specific theme - I don't know any GNOME application which uses tearoff menus, so we probably shouldn't either.

@@ +164,3 @@
+  g_object_ref (ui_manager);
+  g_object_ref_sink (ui_manager);
+  g_object_unref (ui_manager);

You do not "set up ui manager to go away", you are leaking memory:

- gtk_ui_manager_new() gives you ref_count == 1
- g_object_ref gives you ref_count == 2
- g_object_ref_sink gives you ref_count == 3
- g_object_unref gives you ref_count == 2

GtkUIManager inherits from GObject, so all you need to do is g_object_unref() to free the reference created by gtk_ui_manager_new() - obviously you don't want to do it while you're still using it, so you must move the call to the end of the function.

@@ +171,1 @@
                           (GDestroyNotify) g_object_unref);

No longer necessary.

@@ +173,3 @@
 
+                                     strlen (menu_item_string),
+  gtk_ui_manager_add_ui_from_string (ui_manager, menu_item_string,

Not wrong, but you can just pass -1 for the length, as the string is \0 terminated

@@ +174,3 @@
+                                     &error);
+                                     strlen (menu_item_string),
+  gtk_ui_manager_add_ui_from_string (ui_manager, menu_item_string,

It is pointless to pass in a GError if you are not interested in the result - just pass in NULL (and don't forget to remove the variable declaration above)
Comment 43 Nickolas Lloyd 2010-06-11 19:19:37 UTC
(In reply to comment #42)
> Review of attachment 162237 [details] [review]:
> 

Thanks for your feedback.  I believe this latest patch fixes all of the problems you listed.  I do apologize for the profusion of errors in this code.  I still have a lot to learn about glib/gtk/etc.

> Note that your patch does not remove all calls to deprecated API (e.g.
> preview-widget.c, fixedtip.c). 

What symbols did I miss in fixedtip.c?  I checked, and it doesn't include any of the ones listed in this bug.

> In GNOME Shell we append the full URL of the
> bugzilla bug to the commit message - not sure whether mutter follows the same
> policy.

It appears that is the format that is being used for mutter as well.  I will use that from now on.  Thanks.

> +#include <string.h>
> 
> Not needed if you pass -1 as length parameter below

fixed.

> 
> @@ +663,3 @@
> +  name = gtk_action_get_name (action);
> +  const gchar *name;
> +  guint callback_action;
> 
> Not too elegant, but okay-ish. You could cut out the enum and use the action
> name directly as parameter to make_dock - not sure if it's worth the trouble
> though.
> 

I'm not sure that moving that functionality to make_dock() would  make the code that much simpler or more efficient.  I've left it the way it is for the time being, to be changed later if necessary.

> @@ +894,1 @@
> +#define NUM_MENU_ITEMS 16
> 
> @@ +910,3 @@
> +{
> +
> +#define NUM_MENU_ITEMS 16
> 

fixed these.

> "mainmenu" would be more in line with the original code ("<main>")
> 
> @@ +971,3 @@
> +  action_group = gtk_action_group_new ("themenu");

fixed.

> Pass NULL instead of error if you don't look at the result anyway - in which
> case the variable declaration should be removed as well.
> 

*doh.  Really though, is it possible and/or likely enough for the function to fail that gerror should be checked and a warning printed on failure?  Changed to NULL for now.

> @@ +456,3 @@
> +                                             G_CALLBACK (activate_cb),
> +                                             "activate",
> +                      g_signal_connect_data (GTK_OBJECT (submi),
> 
> You can use (GClosureNotify)g_free instead of meta_closure_notify()

Changed to g_free and removed the function.

> ::: src/ui/theme-viewer.c
> @@ +113,2 @@
>  };
> +#define NUM_MENU_ITEMS 11
> 
> @@ +127,1 @@
> +#define NUM_TOOL_ITEMS 3

fixed these as well.

> gettext is already the default, so you don't need this

fixed.

> Not sure about that one - metacity-theme-viewer does use tearoffs...

Based on what you said, it sounds to me like including tear-offs is pointless.  Removed.


> You do not "set up ui manager to go away", you are leaking memory:

fixed.

> 
> GtkUIManager inherits from GObject, so all you need to do is g_object_unref()
> to free the reference created by gtk_ui_manager_new() - obviously you don't
> want to do it while you're still using it, so you must move the call to the end
> of the function.

Regarding this, doesn't ui_manager need to stick around until the top window is closed?  The way I understand it, lowering the reference count to zero will destroy the object.  Shouldn't the reference count then be left at one upon function return?

> 
> @@ +171,1 @@
>                            (GDestroyNotify) g_object_unref);
> 
> No longer necessary.

removed.

> Not wrong, but you can just pass -1 for the length, as the string is \0
> terminated

fixed.

> It is pointless to pass in a GError if you are not interested in the result -
> just pass in NULL (and don't forget to remove the variable declaration above)

as above, fixed.
Comment 44 Nickolas Lloyd 2010-06-11 19:25:38 UTC
Created attachment 163412 [details] [review]
remove deprecated gtk symbols from mutter

latest and greatest version...
Comment 45 Florian Müllner 2010-06-13 02:28:43 UTC
(In reply to comment #43)
> > Note that your patch does not remove all calls to deprecated API (e.g.
> > preview-widget.c, fixedtip.c). 
> 
> What symbols did I miss in fixedtip.c?  I checked, and it doesn't include any
> of the ones listed in this bug.

True, it's not in the list - nevertheless, if you try to compile with CFLAGS=-DGTK_DISABLE_DEPRECATED you will notice that it fails due to the use of GtkTooltips (which has been deprecated for quite some time).
Just changing that to GtkTooltip (note the missing "s") appears to work for me, but I have no idea if that's the correct fix - it would be great if you could check that.


> I'm not sure that moving that functionality to make_dock() would  make the code
> that much simpler or more efficient.  I've left it the way it is for the time
> being, to be changed later if necessary.

OK.


> Regarding this, doesn't ui_manager need to stick around until the top window is
> closed?  The way I understand it, lowering the reference count to zero will
> destroy the object.  Shouldn't the reference count then be left at one upon
> function return?

Nope. Quoting the corresponding documentation:
"Also note that the widgets constructed by a ui manager are not tied to the lifecycle of the ui manager. If you add the widgets returned by this function to some container or explicitly ref them, they will survive the destruction of the ui manager."

Also note that GtkUIManager will _never_ have a floating reference, so calling g_object_ref_sink() on it is just equivalent to g_object_ref() (although more confusing because it suggest that there was a floating reference involved).
Comment 46 Florian Müllner 2010-06-13 02:33:45 UTC
Review of attachment 163412 [details] [review]:

Much better, but still some stuff left:

1) You managed to segfault both mutter-theme-viewer and mutter-window-demo
2) The memory management regarding GtkUIManager still isn't quite right
3) Some stylistic inconsistency

::: src/tools/mutter-window-demo.c
@@ +888,3 @@
+  { "Border Only",		NULL,	"Border Only",		NULL,
+    NULL,	G_CALLBACK (border_only_cb) },
+  { }

Remove empty {}

@@ +904,3 @@
+    "Open another one of these windows", G_CALLBACK (do_appwindow) },
+{
+static GtkActionEntry tool_item_struct[] =

Dito

@@ +920,3 @@
+  static const GtkActionEntry ratio_tool_button = {
+  GtkUIManager *ui_manager;
+  GtkActionGroup *action_group;

Unusual syntax, inconsistent with menu_item_struct / tool_item_struct - it's also not obvious why this is defined inside the function while the others are global.

@@ +962,3 @@
+  action_group = gtk_action_group_new ("mainmenu");
+
+  contents = gtk_text_view_new ();

Should work, but it's special-casing n_elements == 1 - better to define ratio_tool_button as array and use G_N_ELEMENTS (ratio_tool_button) here as well.

@@ +966,2 @@
+  g_object_ref_sink (ui_manager);
+  ui_manager = gtk_ui_manager_new ();

Remove the g_object_ref_sink() - this will leak.

::: src/ui/theme-viewer.c
@@ +109,3 @@
+  { "Utility",		NULL, N_("_Utility"),		"<control>u",	NULL, NULL },
+  { "Dialog",		NULL, N_("_Dialog"),		"<control>d",	NULL, NULL },
+  { "Windows",		NULL, N_("_Windows"),		NULL,		NULL, NULL },

Remove {}

@@ +120,3 @@
+    N_("This is a demo button with an 'open' icon"),	NULL },
+    N_("Open another one of these windows"),		NULL },
+  { "New",	GTK_STOCK_NEW,	NULL,	NULL,

Here as well

@@ +151,2 @@
+  g_object_ref_sink (ui_manager);
+  ui_manager = gtk_ui_manager_new ();

There is no floating reference - unless you call g_object_unref() twice (hint: you don't) you will leak memory
Comment 47 Florian Müllner 2010-06-13 02:34:50 UTC
Review of attachment 163412 [details] [review]:

Much better, but still some stuff left:

1) You managed to segfault both mutter-theme-viewer and mutter-window-demo
2) The memory management regarding GtkUIManager still isn't quite right
3) Some stylistic inconsistency

::: src/tools/mutter-window-demo.c
@@ +888,3 @@
+  { "Border Only",		NULL,	"Border Only",		NULL,
+    NULL,	G_CALLBACK (border_only_cb) },
+  { }

Remove empty {}

@@ +904,3 @@
+    "Open another one of these windows", G_CALLBACK (do_appwindow) },
+{
+static GtkActionEntry tool_item_struct[] =

Dito

@@ +920,3 @@
+  static const GtkActionEntry ratio_tool_button = {
+  GtkUIManager *ui_manager;
+  GtkActionGroup *action_group;

Unusual syntax, inconsistent with menu_item_struct / tool_item_struct - it's also not obvious why this is defined inside the function while the others are global.

@@ +962,3 @@
+  action_group = gtk_action_group_new ("mainmenu");
+
+  contents = gtk_text_view_new ();

Should work, but it's special-casing n_elements == 1 - better to define ratio_tool_button as array and use G_N_ELEMENTS (ratio_tool_button) here as well.

@@ +966,2 @@
+  g_object_ref_sink (ui_manager);
+  ui_manager = gtk_ui_manager_new ();

Remove the g_object_ref_sink() - this will leak.

::: src/ui/theme-viewer.c
@@ +109,3 @@
+  { "Utility",		NULL, N_("_Utility"),		"<control>u",	NULL, NULL },
+  { "Dialog",		NULL, N_("_Dialog"),		"<control>d",	NULL, NULL },
+  { "Windows",		NULL, N_("_Windows"),		NULL,		NULL, NULL },

Remove {}

@@ +120,3 @@
+    N_("This is a demo button with an 'open' icon"),	NULL },
+    N_("Open another one of these windows"),		NULL },
+  { "New",	GTK_STOCK_NEW,	NULL,	NULL,

Here as well

@@ +151,2 @@
+  g_object_ref_sink (ui_manager);
+  ui_manager = gtk_ui_manager_new ();

There is no floating reference - unless you call g_object_unref() twice (hint: you don't) you will leak memory
Comment 48 Nickolas Lloyd 2010-06-13 23:55:27 UTC
Thanks again for your help.  I believe that I've taken care of everything you outlined.

> True, it's not in the list - nevertheless, if you try to compile with
> CFLAGS=-DGTK_DISABLE_DEPRECATED you will notice that it fails due to the use
> of GtkTooltips (which has been deprecated for quite some time).
> Just changing that to GtkTooltip (note the missing "s") appears to work for
> me, but I have no idea if that's the correct fix - it would be great if you
> could check that.

I went ahead and changed this.  It seems safe, since the function it appears in doesn't even use it.  In fact, fixedtip.c doesn't even seem to use the GtkTooltips API.  Depending on what it's actually doing, using GtkTooltip might actually be simpler than the current code.

> 1) You managed to segfault both mutter-theme-viewer and mutter-window-demo

LOL.  Apparently I forgot to test my work? :)

> ::: src/tools/mutter-window-demo.c
> @@ +888,3 @@
> +  { "Border Only",        NULL,    "Border Only",        NULL,
> +    NULL,    G_CALLBACK (border_only_cb) },
> +  { }
> 
> Remove empty {}

This actually appears to have been the cause of the segfaults.  I've removed all of these and it works just fine now.

> @@ +920,3 @@
> +  static const GtkActionEntry ratio_tool_button = {
> +  GtkUIManager *ui_manager;
> +  GtkActionGroup *action_group;
> 
> Unusual syntax, inconsistent with menu_item_struct / tool_item_struct - it's
> also not obvious why this is defined inside the function while the others are
> global.
> 
> @@ +962,3 @@
> +  action_group = gtk_action_group_new ("mainmenu");
> +
> +  contents = gtk_text_view_new ();
> 
> Should work, but it's special-casing n_elements == 1 - better to define
> ratio_tool_button as array and use G_N_ELEMENTS (ratio_tool_button) here as
> well.

Moved outside of function, changed initializer to a list, and made it a single-element array.

> 
> @@ +966,2 @@
> +  g_object_ref_sink (ui_manager);
> +  ui_manager = gtk_ui_manager_new ();
> 
> Remove the g_object_ref_sink() - this will leak.

Fixed.  I guess I just assumed that the UI Manager would be linked to the window, and therefore destroyed with the window.  Now I know...
Comment 49 Nickolas Lloyd 2010-06-13 23:56:22 UTC
Created attachment 163542 [details] [review]
remove use of deprecated gtk symbols
Comment 50 Florian Müllner 2010-06-14 03:13:37 UTC
(In reply to comment #48)
> > Remove empty {}
> 
> This actually appears to have been the cause of the segfaults.  I've removed
> all of these and it works just fine now.

Well, yes - I didn't tell you to remove those because I hold an irrational grudge against empty brackets ;-)
Comment 51 Florian Müllner 2010-06-14 03:14:29 UTC
Review of attachment 163542 [details] [review]:

Thanks for the update - much better now (in fact, pretty much ready to go). I have some (mostly) stylistic comments left, feel free to ignore the first and last comment.

::: src/tools/mutter-window-demo.c
@@ +738,3 @@
     gtk_window_set_geometry_hints (GTK_WINDOW (window),
   if (window)
+                                  GTK_WIDGET (data),

Instead of doing multiple GTK_WIDGET(data) casts it is slightly more efficient to use a local variable:

GtkWidget *widget = GTK_WIDGET (data);

Not that it matters in a demo application like this though ...

@@ +824,2 @@
 }
+static gchar menu_item_string[] =

More conventional:
static const gchar *menu_item_string = ...

@@ +853,3 @@
+  "</ui>\n";
+
+static GtkActionEntry menu_item_struct[] =

The _struct suffix has an uncomfortable touch of Hungarian notation - also, array variables should use plural. Suggestion: menu_items.

@@ +889,3 @@
+};
+
+static GtkActionEntry tool_item_struct[] =

See above. The above comments also apply to theme-viewer.c.

@@ +909,3 @@
+      "This is a demo button that locks the aspect ratio "
+{
+static const GtkActionEntry ratio_tool_button[] =

Still a little weird - logically this belongs to the other tool_items, so maybe it's cleaner to ignore the different callback data in the original code and just move it into the above array.
The visual result at least is the same ...
Comment 52 Florian Müllner 2010-06-14 11:30:37 UTC
Created attachment 163572 [details] [review]
Replace deprecated GDK symbols

The previous patches allow building succesfully with GTK_DISABLE_DEPRECATED - there is still some deprecated Gdk code though, which has to be dealt with. The replacement code depends on new API introduced in the upcoming GTK+ release, so the patch bumps the required version to 2.22 - a compatibility patch for 2.18 follows.
Comment 53 Florian Müllner 2010-06-14 11:31:06 UTC
Created attachment 163573 [details] [review]
Add compatibility with GTK+ 2.18

In order to replace calls to deprecated GDK code, yet-unreleased
GTK+ 2.22 is required. Add some basic compatibility code to allow
building mutter with GTK+ 2.18.
Comment 54 André Klapper 2010-06-14 11:34:56 UTC
gtk+ 2.22?! Can you change that to 2.21.1 please?
I prefer not to wait months to be able to compile mutter if it works with earlier (unstable) gtk+ versions that I use for testing anyway. Thanks.
Comment 55 Tomas Frydrych 2010-06-14 11:42:42 UTC
(In reply to comment #53)
> Created an attachment (id=163573) [details] [review]
> Add compatibility with GTK+ 2.18
> 
> In order to replace calls to deprecated GDK code, yet-unreleased
> GTK+ 2.22 is required. Add some basic compatibility code to allow
> building mutter with GTK+ 2.18.

We can't push a patch that requires an unstable version of Gtk, never mind an unreleased one, and I am not too happy about the compatibility shim -- features are not deprecated until they are deprecated by a stable release, until then any changes that anticipate that should remain as patches in bugzilla.
Comment 56 André Klapper 2010-06-14 11:50:04 UTC
(In reply to comment #55)
> We can't push a patch that requires an unstable version of Gtk

Why not? (I don't know mutter development and if a stable and unstable branch exists - if the latter is missing it would be welcome).

> features are not deprecated until they are deprecated by a stable release, 

That's unfortunately not how GTK+ development on the way to GTK+ 3.0 currently works.

> until then any changes that anticipate that should remain as patches in 
> bugzilla.

Impossible. Following this would make it impossible to ship GNOME 3.0 with gnome-shell and to receive any testing for it before.
Comment 57 Florian Müllner 2010-06-14 11:52:47 UTC
Created attachment 163576 [details] [review]
Replace deprecated GDK symbols

(In reply to comment #54)
> gtk+ 2.22?! Can you change that to 2.21.1 please?

Sure.
Comment 58 Florian Müllner 2010-06-14 12:14:10 UTC
Created attachment 163578 [details] [review]
Add compatibility with GTK+ 2.18

(In reply to comment #55)
> features are not deprecated until they are deprecated by a stable release,

Like GTK+ 2.20? So yes, those "features" are already deprecated in at least one stable release (didn't go back in history further than 2.20.0). The problem is that until recently, the code was deprecated without replacement.


> until then any changes that anticipate that should remain as patches in
> bugzilla.

I don't have a problem with the patch sitting in bugzilla for a while - what I would really hate though is a GNOME 3.0 release with all applications using GTK-3 and just the "new" shiny window manager depending on the old GTK-2. Technically that is not a problem at all, but I see the risk of bad PR here.
Comment 59 Tomas Frydrych 2010-06-14 12:38:05 UTC
(In reply to comment #56)
> (In reply to comment #55)
> > We can't push a patch that requires an unstable version of Gtk
> 
> Why not? (I don't know mutter development and if a stable and unstable branch
> exists - if the latter is missing it would be welcome).

If someone wants to create (and maintain) a branch for testing this, that is fine, but the master is what we make releases from and it should not depend on an unreleased versions of libraries, certainly not months before the anticipated release.
Comment 60 André Klapper 2010-06-14 12:42:31 UTC
(In reply to comment #59)
> master should not depend on an unreleased versions of libraries

Fixed anyway by Florian's latest patch (which is a better approach it seems)
Comment 61 Florian Müllner 2010-06-14 13:08:30 UTC
(In reply to comment #59)
> master is what we make releases from and it should not depend on
> an unreleased versions of libraries, certainly not months before the
> anticipated release.

Which is why the combination of both patches does not change the requirements - the only reason for the split is to make it possible to remove the compatibility code with a simple git-revert at a future date (@André: this is also the reason why the original patch used 2.22 for pkg-config - gdk-compat.h was already checking for the correct 2.21.1)

Maybe I'm missing something obvious, but looking at these alternatives:

 - having GNOME Shell depend on GTK-2
 - pushing those patches (mostly) untested into mutter-3.0 when 
   GTK-3 is officially released
 - cluttering the code with #if GTK_CHECK_VERSION() checks

this approach simply looks like the lesser evil to me.
Comment 62 Owen Taylor 2010-06-14 17:42:38 UTC
This seems fine to me... the code that depends on unreleased versions of GTK+ is all effectively conditional. And while you may not agree 100% with the "compatibility shim" approach, we are already using it (see src/gtk-compat.h) - with earlier work it was chosen as a lesser evil than having #ifdefs scattered all over the code.
Comment 63 Nickolas Lloyd 2010-06-14 17:52:24 UTC
Created attachment 163616 [details] [review]
remove deprecated gtk symbols

This patch has been through so many revisions that I've forgotten the reason for most of the original changes.  I think that all of the vestiges of the original straight-port version are gone now.
Comment 64 Florian Müllner 2010-06-14 18:30:20 UTC
Review of attachment 163616 [details] [review]:

Looks good to me - Tomas: anything you'd like to add?
Comment 65 Tomas Frydrych 2010-06-15 07:20:43 UTC
(In reply to comment #64)

> Looks good to me - Tomas: anything you'd like to add?

Really just thanks for working on this guys :)
Comment 66 Florian Müllner 2010-06-19 20:48:30 UTC
Created attachment 164095 [details] [review]
Replace deprecated GDK symbols

Fix non-toplevel include. Updated patch:

--- a/src/compositor/mutter-shaped-texture.h
+++ b/src/compositor/mutter-shaped-texture.h
@@ -31,7 +31,7 @@
 #include <clutter/glx/clutter-glx.h>
 #endif /* HAVE_GLX_TEXTURE_PIXMAP */
 
-#include <gdk/gdkregion.h>
+#include <gdk/gdk.h>
 
 G_BEGIN_DECLS
 
--
Comment 67 Florian Müllner 2010-06-19 20:48:59 UTC
Created attachment 164096 [details] [review]
Add compatibility with GTK+ 2.18

Reattaching to maintain patch order.
Comment 68 Nickolas Lloyd 2010-06-22 00:32:08 UTC
IMHO, adding gdk-compat.h is the way to go.  Sure, it adds another file to the tree, increased complexity, etc, but it allows avoiding the use of deprecated functions without breaking things long-term.  Seems like a win-win to me.  Just my two cents.

One question/thought about the patch however:

+#if !GTK_CHECK_VERSION (2, 21, 1)
+
+#define gdk_window_get_back_pixmap(w,p,r) *p = GDK_WINDOW_OBJECT (w)->bg_pixmap
+#define gdk_window_get_background(w,c)    *c = GDK_WINDOW_OBJECT (w)->bg_color
+
+#endif /*GTK_CHECK_VERSION */

I could see this going wrong in the long term if, say, at a later date a call to gdk_window_get_back_pixmap() is added to the mutter code-base which relies on the value stored in r.  If compiled against Gtk 2.18, that will of course cause Bad Things.

Even though it's not pretty, what about something like the following?

#define gdk_window_get_back_pixmap(w,p,r) do { \
        _Bool *q = r; \
        *p = GDK_WINDOW_OBJECT (w)->bg_pixmap; \
        if (r) \
                *q = (*p == GDK_PARENT_RELATIVE_BG) \
} while (0)

Or would this not end up being a problem?
Comment 69 Florian Müllner 2010-06-27 04:38:19 UTC
Created attachment 164728 [details] [review]
Replace deprecated GDK symbols

Update to include GdkVisual->depth => gdk_visual_get_depth()
Comment 70 Florian Müllner 2010-06-27 06:53:20 UTC
Created attachment 164729 [details] [review]
Replace deprecated GDK symbols

There was a bug in the previous patch:

-  if (private->bg_pixmap == GDK_PARENT_RELATIVE_BG && private->parent)
+  gdk_window_get_back_pixmap (window, &back_pixmap, NULL);
+  if (back_pixmap == GDK_PARENT_RELATIVE_BG && parent)

The last conditional always evaluates to false, because gdk_window_get_back_pixmap() special cases this condition and sets back_pixmap to NULL.
Comment 71 Florian Müllner 2010-06-27 07:27:14 UTC
Created attachment 164731 [details] [review]
Add compatibility with GTK+ 2.18

Update compatibility layer - the problem outlined in the last comment unfortunately implies that we need a full implementation of gdk_window_get_back_pixmap()
Comment 72 André Klapper 2010-06-27 08:34:14 UTC
Comment on attachment 163616 [details] [review]
remove deprecated gtk symbols

Adjusting status as per Tomas' comment 65.
Comment 73 Florian Müllner 2010-06-30 12:46:38 UTC
Created attachment 164964 [details] [review]
Remove deprecated Gtk+ symbols

Rebased to master.
Comment 74 Florian Müllner 2010-06-30 13:32:23 UTC
Created attachment 164965 [details] [review]
Use cairo_region_t when building with gtk+-3.0

GdkRegion has been removed from Gtk+. The replacement is a
yet-unreleased cairo API, so use it only when building with
Gtk+-3.0.

Tomas:
Now that we scatter #ifdefs all over the code anyway, I can drop
the "compatibility shim" if you prefer - the few additional #ifdefs
hardly matter ...
Comment 75 Nickolas Lloyd 2010-06-30 13:40:39 UTC
(In reply to comment #73)
> Created an attachment (id=164964) [details] [review]
> Remove deprecated Gtk+ symbols
> 
> Rebased to master.

jooc, why is it that almost half this patch was ripped off and pushed by somebody else?
Comment 76 Florian Müllner 2010-06-30 13:46:31 UTC
(In reply to comment #75)
> jooc, why is it that almost half this patch was ripped off and pushed by
> somebody else?

I'd say because Claudio was unaware that this patch already existed?
Comment 77 Owen Taylor 2010-06-30 16:46:09 UTC
(In reply to comment #76)
> (In reply to comment #75)
> > jooc, why is it that almost half this patch was ripped off and pushed by
> > somebody else?
> 
> I'd say because Claudio was unaware that this patch already existed?

Sorry for giving the commit approval for that. I had forgotten that this patch was still outstanding and was surprised that we still had deprecated usages in Mutter. Since the patches were correct I figured that it wasn't worth tracking down how they had been missed earlier.

I thought this was all set to commit after commit 65, but I guess since it's been delayed enough that the cairo stuff landed, we have more stuffo deal with.

I really don't like scattering the ifdefs throughout the code. Regions are going to be more of an issue if we add Cody Russell's opaque region stuff. What I said in a private mail to him yesterday:

 Regions are a bit of a pain, since we are conditionally compiling for
 GTK+ 2 and GTK+ 3, so GdkRegion or cairo_region_t - I think I'd add a
 private header file that #defined MetaRegion and meta_region_union(),
 etc based on a <config.h> #define.

That is, take advantage of the fact that the cairo and GDK region API's are basically identical and compatible.

I *wouldn't* try to take the compat header approach here - #defining GddkRegion to cairo_region_t or vice versa strikes me as a bad idea.
Comment 78 Florian Müllner 2010-06-30 17:16:38 UTC
(In reply to comment #77)
> I thought this was all set to commit after commit 65, but I guess since it's
> been delayed enough that the cairo stuff landed, we have more stuffo deal with.

We could be nitpicky and say that the cairo stuff only affects GDK and commit the GTK+ patch as-is (it's ready and it might keep people from duplicating effort)


>  Regions are a bit of a pain, since we are conditionally compiling for
>  GTK+ 2 and GTK+ 3, so GdkRegion or cairo_region_t - I think I'd add a
>  private header file that #defined MetaRegion and meta_region_union(),
>  etc based on a <config.h> #define.

OK, will do.

 
> That is, take advantage of the fact that the cairo and GDK region API's are
> basically identical and compatible.

With one notable exception:
gdk_region_get_rectangles() is split into cairo_region_num_rectangles() and cairo_region_get_rectangle() ...

I'll try to figure something out which is not awful in the "non-native" case.
Comment 79 Florian Müllner 2010-07-01 15:52:58 UTC
Created attachment 165033 [details] [review]
Use cairo_region_t when building with gtk+-3.0

(In reply to comment #77)
>  Regions are a bit of a pain, since we are conditionally compiling for
>  GTK+ 2 and GTK+ 3, so GdkRegion or cairo_region_t - I think I'd add a
>  private header file that #defined MetaRegion and meta_region_union(),
>  etc based on a <config.h> #define.

Done. The AC_DEFINE depends on the GTK+-3.0 patch in bug 622303 (yeah, I know - laziness).


> That is, take advantage of the fact that the cairo and GDK region API's are
> basically identical and compatible.

Where the API is compatible, but not identical, I generally went with the cairo API, except where the difference is due to "cairo peculiarity" (create() instead of new(), some_type_t instead of SomeType).

The only incompatibility is the gdk_region_get_rectangles() <=> cairo_region_num_rectangles()/cairo_region_get_rectangle() change. I like the cairo version a tad bit more, but I can't think of an acceptable way of implementing it with (public) GDK API, so meta_region_get_rectangles() it is ...
Comment 80 Owen Taylor 2010-07-01 17:27:42 UTC
Review of attachment 165033 [details] [review]:

Looks good. A few minor comments below.

Also, you seem to be missing a Makefile.am change for the added region.h.

I don't really like adding region.h as src/region.h - I guess we put gtk-compat.h directly under src/, but it strikes me as "special" in a way that this header isn't. I think I'd:

 * Put it as include/region.h
 * Add it to mutter_SOURCES in src/Makefile.am, but *not* to libmutterinclude_HEADERS; it should not be installed.

We'll have a further problem if we add:

 meta_window_get_opaque_region()

to MetaWindow for use in compositor/mutter-window.c but we can deal with that when we come to it; probably with a:

 #ifndef MUTTER_COMPILATION
 typedef struct _MetaRegion MetaRegion
 #endif

dummy define in window.h.

::: configure.in
@@ +144,3 @@
        GTK_MIN_VERSION=2.90
        CANBERRA_GTK=libcanberra-gtk3
+       AC_DEFINE(HAVE_CAIRO_REGION,1,[Use cairo_region_t instead of GdkRegion])

This would look more straightforward if you called the #define USE_CAIRO_REGION - HAVE_CAIRO_REGION implies to me that we've checked for a cairo that has it.

Also, you can put spaces after the ',' here, and so should.

::: src/compositor/mutter-window.c
@@ +11,2 @@
 #include <clutter/x11/clutter-x11.h>
+#include <gdk/gdk.h>

Why this addition?

::: src/region.h
@@ +2,3 @@
+#define META_REGION_H
+
+#include <config.h>

<config.h> always has to be the very first include in a source file, or weird things can happen. (Forget the details, related to 64-bit offset_t as I recall.) So, I'd probably do this as something like:

 #ifndef PACKAGE_NAME
 #error "<config.h> must be included before meta-region.h"
 #endif

@@ +25,3 @@
+#define meta_region_intersect(r, other)      cairo_region_intersect(r, other)
+#define meta_region_contains_rectangle(r, rect) cairo_region_contains_rectangle(r, rect)
+#define meta_region_get_rectangles(r, rects, num)         \

This is a long and complicated function to do as a macro. I think it's probably better to define as a function, in util.c   (I think, could also be in boxes.c but that's really about *MetaRectangle*.)
Comment 81 Owen Taylor 2010-07-01 17:36:28 UTC
Review of attachment 164729 [details] [review]:

Looks good
Comment 82 Owen Taylor 2010-07-01 17:40:16 UTC
Review of attachment 164731 [details] [review]:

Looks right to me.

::: src/gdk-compat.h
@@ +16,3 @@
+#define gdk_window_get_back_pixmap(w,p,r)                           \
+  G_STMT_START {                                                    \
+    GdkWindowObject *priv = GDK_WINDOW_OBJECT (w);                  \

Sometimes we make attempts to call these variables something less likely to cause shadowing (and potentially compile warnings), like, say, gwgbp_priv. But since this is used in one place, and doesn't shadow, and this isn't a public header, it's not worth a reduction in readability. good like this.
Comment 83 Florian Müllner 2010-07-01 18:46:28 UTC
Created attachment 165045 [details] [review]
Use cairo_region_t when building with gtk+-3.0

(In reply to comment #80)
> We'll have a further problem if we add:
> 
>  meta_window_get_opaque_region()
> 
> to MetaWindow for use in compositor/mutter-window.c but we can deal with that
> when we come to it;

Not sure if I understand your concern here (well, maybe I just don't _want_ to understand the concern) - are you referring to exposing the (private) MetaRegion in a public header?

If so, we already came to it :-(
(in preview-widget.h)


> ::: src/compositor/mutter-window.c
> @@ +11,2 @@
>  #include <clutter/x11/clutter-x11.h>
> +#include <gdk/gdk.h>
> 
> Why this addition?

For GdkRectangle - gdk.h was included indirectly before (via mutter-window-private.h), but the patch changed that to only include gdk.h when using GdkRegion.

meta_region_get_rectangles() now requires gdk.h for the cairo case as well, so the include in region.h is now unconditional and I removed the change from the patch.
Comment 84 Owen Taylor 2010-07-01 19:16:28 UTC
(In reply to comment #83)
> Created an attachment (id=165045) [details] [review]
> Use cairo_region_t when building with gtk+-3.0
> 
> (In reply to comment #80)
> > We'll have a further problem if we add:
> > 
> >  meta_window_get_opaque_region()
> > 
> > to MetaWindow for use in compositor/mutter-window.c but we can deal with that
> > when we come to it;
> 
> Not sure if I understand your concern here (well, maybe I just don't _want_ to
> understand the concern) - are you referring to exposing the (private)
> MetaRegion in a public header?
> 
> If so, we already came to it :-(
> (in preview-widget.h)

Yeah, that was my concern... with the planned addition of meta_window_get_opaque_region() we need MetaRegion in a public header, because there is no separation between window.h for src/compositor/ and window.h as installed - we just have window-private.h which is for src/core/ and window.h for everything else.

> > ::: src/compositor/mutter-window.c
> > @@ +11,2 @@
> >  #include <clutter/x11/clutter-x11.h>
> > +#include <gdk/gdk.h>
> > 
> > Why this addition?
> 
> For GdkRectangle - gdk.h was included indirectly before (via
> mutter-window-private.h), but the patch changed that to only include gdk.h when
> using GdkRegion.
> 
> meta_region_get_rectangles() now requires gdk.h for the cairo case as well, so
> the include in region.h is now unconditional and I removed the change from the
> patch.

OK.
Comment 85 Nickolas Lloyd 2010-07-02 00:39:10 UTC
(In reply to comment #77)
> (In reply to comment #76)
> > (In reply to comment #75)
> > > jooc, why is it that almost half this patch was ripped off and pushed by
> > > somebody else?
> > 
> > I'd say because Claudio was unaware that this patch already existed?
> 
> Sorry for giving the commit approval for that. I had forgotten that this patch
> was still outstanding and was surprised that we still had deprecated usages in
> Mutter. Since the patches were correct I figured that it wasn't worth tracking
> down how they had been missed earlier.
> 
> I thought this was all set to commit after commit 65, but I guess since it's
> been delayed enough that the cairo stuff landed, we have more stuffo deal with.

Ya, sorry about that.  It was just kind of irritating to see.  In any case, if the patch is OK, could we get it pushed and just get it out of the way, or would it be better to wait for the GDK stuff to get sorted out first?
Comment 86 Florian Müllner 2010-07-02 03:08:05 UTC
Created attachment 165067 [details] [review]
Use cairo_region_t when building with gtk+-3.0

Changing meta_region_get_rectangles() from a macro to a function introduced a crasher due to a subtle difference in precedence:

cairo_region_get_rectangle (region, i, &(*rectangles[i]));
    should be
cairo_region_get_rectangle (region, i, &((*rectangles)[i]));


This is the (more readable!) interdiff:

diff --git a/src/core/util.c b/src/core/util.c
index cb752f0..507ffa7 100644
--- a/src/core/util.c
+++ b/src/core/util.c
@@ -898,7 +898,7 @@ meta_region_get_rectangles (MetaRegion    *region,
 
       *rectangles = g_new (cairo_rectangle_int_t, n);
       for (i = 0; i < n; i++)
-        cairo_region_get_rectangle (region, i, &(*rectangles[i]));
+        cairo_region_get_rectangle (region, i, *rectangles + i);
     }

Sorry for the noise.
Comment 87 Florian Müllner 2010-07-02 13:54:38 UTC
Attachment 164729 [details] pushed as c65a244 - Replace deprecated GDK symbols
Attachment 164731 [details] pushed as e267a63 - Add compatibility with GTK+ 2.18
Attachment 164964 [details] pushed as 42e786b - Remove deprecated Gtk+ symbols
Attachment 165067 [details] pushed as 7feeb72 - Use cairo_region_t when building with gtk+-3.0