Created attachment 100597 [details] [review]
Patch removing stuff deprecated in GTK 2.12

This patch has already been reviewed by Snark in bug 361679.
It mainly addresses the GtkTooltips -> GtkTooltip migration and the deprecation of gtk_menu_item_remove_submenu.

I'm annoyed to see your patch bitrotting :-(

I thought maybe I could at least commit the fix for the gtk_menu_item_remove_submenu, since that involved just using a function which does exist in GTK+ 2.10 ; hélas, I had a look here : http://svn.gnome.org/viewvc/gtk%2B/tags/GTK_2_10_9/gtk/gtkmenuitem.c?view=markup

It's clear that in GTK+ 2.10, gtk_menu_item_set_submenu can't really be used to remove a submenu, as that will trigger warnings when trying to execute the gtk_menu_attach_to_widget line (reported as bug #531904).

Damien, when do we switch to GTK+ 2.12?

IMHO it's safe to.

J.

gtk+ version 2.12.11 is available on win32, according to :
http://www.gtk.org/download-windows.html

Reassigning to me, for the GtkTooltips deprecations.

GtkTooltips removed.

gtk_menu_item_remove_submenu() removed.

Though I did it manually because the patch was a bit older, there were no changes needed to the patch. Thanks for it.

Next things to do:
The combo boxes API has changed :(

Just for the record:

Unable to compile with GTK_DISABLE_DEPRECATED because of libgnomeui is not GTK-2.12 ready (it seems).

...

Jan :
(1) using gtk_menu_item_set_submenu (item, NULL) to remove the submenu doesn't work in gtk+ 2.10 afaik -- and this is the version we ask in configure.ac ;
(2) gtk_widget_set_tooltip_text is new in gtk+ 2.12, which doesn't work with 2.10, and this is the version we ask in configure.ac ;
(3) ubuntu edgy doesn't have gtk+ 2.12, only the newer has, so it's perhaps too early to switch to gtk+ 2.12.

Damien?

Julien,

to (1):
We can't bump the version request when there's still deprecated code, it's work in progress. Or should it go vice versa?

to (2):
What is Edgy based on? My personal indicator for "bleeding edge" is the *very conservative* Debian. The upcoming stable (Lenny) has 2.12.11 here. Though I have trouble to compile against libgnomeui here...

For the record: GNOME_DISABLE_DEPRECATED also exists... *doh* my bad

I tested around a bit. We don't use old GtkCombo* API.

The GNOME libraries compiled with GNOME_DISABLE_DEPRECATED lack the definition of _() and N_(). These were moved to libbonobo. So everywhere we #include <gnome.h> to have I18N, an #include <libbonobo.h> should fix it.

J.

Ok, we have go from mission control. I'll investigate.

Okay, changes summary:
- _() and N_() are done by libbonobo now (before: libgnome)
- GLib main loop API changed
- GTK menu item API changed
- GTK tooltips API changed

The code compiles fine with CPPFLAGS="-DG_DISABLE_DEPRECATED=1 -DGTK_DISABLE_DEPRECATED=1 -DGNOME_DISABLE_DEPRECATED=1", except:
In file included from /usr/include/orbit-2.0/orbit/dynamic/dynamic.h:4,
                 from /usr/include/orbit-2.0/orbit/orbit.h:19,
                 from /usr/include/orbit-2.0/orbit/orb-core/orbit-interface.h:738,
                 from /usr/include/bonobo-activation-2.0/bonobo-activation/Bonobo_Unknown.h:78,
                 from /usr/include/bonobo-activation-2.0/bonobo-activation/bonobo-activation.h:27,
                 from /usr/include/libbonobo-2.0/bonobo/bonobo-object.h:14,
                 from /usr/include/libbonobo-2.0/bonobo/bonobo-types.h:16,
                 from /usr/include/libbonobo-2.0/libbonobo.h:18,
                 from gui/statusicon.cpp:59:
/usr/include/orbit-2.0/orbit/dynamic/dynamic-defs.h:673: warning: ‘CORBA_TypeCode_struct* DynamicAny_DynAny_type(DynamicAny_DynAny_type*, CORBA_Environment*)’ hides constructor for ‘struct DynamicAny_DynAny_type’

But that shouldn't hurt (us).

Changes committed, I hope everything works well. Closing.

J.

Notice that _() and N_() are gettext, and hence isn't really gnome-specific.

gettext is used when !HAVE_GNOME, Gnome (before) and Bonobo (now) have replacements for it. Though I don't know if it's safe to assume we have Bonobo when we HAVE_GNOME...

J.

I think we have a few #if GTK_HAVE_VERSION (...) in the code, which should probably be removed now we assume a higher version.

Reopening, as it irks me to discuss a not-so-FIXED bug.

Full ACK - what would I do without you!

Cleaned the code from version checks below 2.12.0 (still found some 2.8 one hehe), bumped version check in configure.ac.

Leaving open for comments. Also having a look for versions of supporting libraries like GLib.

J.

Just a couple of notes.

not sure what we use from libgnome but its in the gnome kill list 
http://live.gnome.org/LibgnomeMustDie

Ekiga is also on the gio port todo list but I don't see any gnome_vfs calls in the current svn code, so I suspect it doesn't any more and someone in the know just has to update the gnome wiki.
http://live.gnome.org/GioPort

This is also worth a look.
http://live.gnome.org/ProjectRidley

Yea, I know about massive GNOME changes, but it's not a good time (yet) to start this IMHO.

J.

(In reply to comment #20)

> Ekiga is also on the gio port todo list but I don't see any gnome_vfs calls in
> the current svn code, so I suspect it doesn't any more and someone in the know
> just has to update the gnome wiki.
> http://live.gnome.org/GioPort

AFAIR Ekiga just uses the GnomeVFS backend for the open dialog. It was waiting for the proper GIO backend to be integrated into Gtk+.

This all should be finished in Ekiga 3.2 IMHO.

Currently (heading for 3.0) there's too much other hassle.

Created attachment 122455 [details] [review]
Kill remaining bits of libgnome/gnomevfs

This drops the dependency against libgnome when at least gtk 2.13 is available

(In reply to comment #24)
> Created an attachment (id=122455) [edit]
> Kill remaining bits of libgnome/gnomevfs
> 
> This drops the dependency against libgnome when at least gtk 2.13 is available
> 

Seems that the patch uses this to check the GTK version:
#if GTK_MAJOR_VERSION > 2 || GTK_MINOR_VERSION >= 13

This can't work. First, the major is checked with '>' instead of '>=', and the condition used should use a logical and (&&) instead of logical or (||).

To avoid this kind of problems, use GTK_CHECK_VERSION() instead. I have not enough knowledge on the other aspects of the patch to review them, so I invite others reviewers to add some comments.

I tried using GTK_CHECK_VERSION but it didn't seem to work in an #if statement...

Also, this works just fine because either you use a GTK_MAJOR_VERSION > 2 (say, 3.0.0), or you use GTK_MINOR_VERSION >= 13 (for 2.13.1). Gtk 1.X are not supported by Ekiga anyway.

Sure, but the semantics are a bit broken, especially for the GTK_VERSION_MINOR check. 

GTK_MAJOR_VERSION >= 2 && GTK_MINOR_VERSION >= 13 is clearer and cleaner.
And last time I used GTK_CHECK_VERSION in a #if, it worked. Please try again to use it, if it doesn't work, there's a bug somewhere...

> GTK_MAJOR_VERSION >= 2 && GTK_MINOR_VERSION >= 13 is clearer and cleaner.

... and wrong: it won't work for 3.0 for instance.


> Please try again to use it, if it doesn't work, there's a bug somewhere...

Will do.

toolbox-gtk.c:47:22: error: missing binary operator before token "("

The line is:

#if GTK_CHECK_VERSION(2, 13, 1)

Okay I found the bug. Now it works.

Commited the fixed version with the agreement of Damien Sandras.

(In reply to comment #28)
> > GTK_MAJOR_VERSION >= 2 && GTK_MINOR_VERSION >= 13 is clearer and cleaner.
> 
> ... and wrong: it won't work for 3.0 for instance.

Oh, I'm sorry, you're right. Should have been
GTK_MAJOR_VERSION > 2 || (GTK_MAJOR_VERSION == 2 && GTK_MINOR_VERSION >= 13)

But it's great if you found how to make GTK_CHECK_VERSION do the work for you.