I'm not fogetting this bug, but I couldn't achieve getting a full GNOME 2.16 desktop with jhbuild, to allow me to test GTK 2.10 (required by GtkStatusIcon). So i'm spending some time in preparing my Mandriva 2006 -> 2007 (which uses GNOME 2.16) migration, but I also have to switch from XFS to ext3 before, which is more time consuming (sort data, do some backups, etc.)... Once I have this all (by mid-november I think), I'll be ready to test my first modifications on this.

Ok, i'm back in business :-) for this...

So how far have you gone ?

Normally it should be as easy as writing a new implementation of the GmTray api... with just some little platform-specific code for the tray menu.

The problem I have is that i'm not sure writin an imprementation of GmTray is the best way to go. It's surely the simplest, but I'm not sure if it's the most efficient.

Currently, GmStatusIcon uses GmTray wich will in turn use GtkStatusIcon.
GmStatusIcon manipulates around a GtkWidget. But GtkStatusIcon is not derived from GtkWidget, but from GObject:

http://developer.gnome.org/doc/API/2.0/gtk/GtkStatusIcon.html#id2870112
"Making it a widget would be impractical, since the system tray on Win32 doesn't allow to embed arbitrary widgets."

So, as I understand it, if I only create a GmTray implementation, I would end up creating a fake widget, for GmStatusicon, while it's unnecessary. I'd prefer to rewrite GmStatusIcon

Could you please confirm that ?

Another problem I encounter is that GtkStatusIcon has no callback for middle click management, which seems to be used by ekiga to display the adress book...

If you check the code, you'll see the GtkStatusIcon is a dummy GtkLabel, to which I add the necessary data, mostly to use the refcounting of said GtkLabel to properly dispose of this data when it isn't needed anymore.

GmTray itself is a simple structure, as you'll see from gmtray-internal.h.

As proof as this, the fact that "the system tray on Win32
doesn't allow to embed arbitrary widgets" didn't prevent me from implementing a GmTray for this platform!

Now the question is more : when does gtk+ 2.10 hit unstable, so we can finally get rid of as much code as possible!?

You skipped one problem: the middle click management is not supported in GTK 2.10 by GtkStatusIcon. So popping the adressbook will first be unsupported, until GTK is patched. It's an easy patch, but i don't know if this can be part of a future maintenance release.

Also, the icons used for notification are 16x16. GtkStatusIcon stretches them to fit the available size. Maybe we could behave like this for the choice of the icon ?
http://wayofthemonkey.com/index.php?date=2007-02-13&month=02&year=2007

Created attachment 82640 [details] [review]
First try, with no backend selection

Here is a rough first implementation. The Makefile.am just forgets the other implementations (win32 and x11). I can't test that on win32, but it works well for me.

3 issues, however:
* The icon size is altered (stretched)
* The middle click to show the address book can't be handled (GtkStatusIcon limitation)
* I need something in the Makefile.am to tell to choose either the GtkStatusIcon implementation if GTK2.10 is available, or one of the win32 or x11 ones. But I don't know how to do it (still not experimented enough with autotools).

+  gtk_menu_popup (menu, 
+		  NULL, NULL, NULL, NULL,
+		  button, 
+		  activate_time);

You should use gtk_status_icon_position_menu and the status icon as user-data for the menu positioning function here (3rd and 4th NULL, resp.).

As for the Makefile/configure thing, do this:

in configure:

PKG_CHECK_EXISTS([gtk+-2.0 >= 2.10],[have_gtk_2_10=yes LIBNOTIFY_REQUIRED=0.4.3],[have_gtk_2_10=no])
AM_CONDITIONAL([HAVE_GTK_2_10],[test "$have_gtk_2_10" = "yes"]

and in Makefile.am:

foo_SOURCES = ....

if !HAVE_GTK_2_10
foo_SOURCES += eggtrayicon.c eggtrayicon.h ....
endif

and in C/C++ sources, you can use 

#include <gtk/gtkversion.h>

#if GTK_CHECK_VERSION (2, 10, 0)
...
#endif

Thanks a lot for your help :-)
Changes applied, here gose the updated patch...

Created attachment 82906 [details] [review]
Second try, with backend selection

Needs some testing, but thanks to chpe, should be ok. Works for me on Linux, GTK 2.10.

Created attachment 82907 [details] [review]
Third try, corrects a typo in error message

Ooops... same as previous one, just corrects a typo in the error message.

Oh, I didn't use the LIBNOTIFY_REQUIRED=0.4.3 statement, because I thought at first that it was just due to some copy/paste, but maybe I'm missing something. Is this check really mandatory, and if it is, could you tell me why ? Thanks again Christian.

That's just because I copied it from some configure that did use libnotify. It's not needed unless ekiga uses libnotify (in which case the requirement is >= 0.4.3 for use with gtkstatusicon).

About the icon size : would it be possible to have svg icons ?

Maybe asking on the tango mailing list...

(In reply to comment #7)
> Also, the icons used for notification are 16x16. GtkStatusIcon stretches them
> to fit the available size. Maybe we could behave like this for the choice of
> the icon ?
> http://wayofthemonkey.com/index.php?date=2007-02-13&month=02&year=2007
> 

GtkStatusIcon isn't fixed to 16x16. Since you use gtk_status_icon_new_from_stock, it will automatically adapt to size changes, and choose the closest fitting available icon. So all you have to do is to make sure that your stock icon has the necessary sizes available. (I take it you're registering your own stock icon here.)

GtkStatusIcon is not the problem, sure (but I didn't know it when I wrote that comment). The problem is that we are missing SVG icons (or big png icons). ATM, most of the icons we have are 16x16, including the ones used for the status icon. Chosing the fittest icon for now will be useless: we just don't have that choice.

[liberforce@donald pixmaps]$ identify -format "%hx%w : %f\n" status*
16x16 : status-auto-answer-16.png
16x16 : status-available-16.png
16x16 : status-do-not-disturb-16.png
16x16 : status-forward-16.png
16x16 : status-in-a-call-16.png
16x16 : status-offline-16.png
16x16 : status-ringing-16.png

Ok, so for the icon problem, I this that the current situations with do fine for now. We are not currently even following strict guidelines about the icons, as pointed by Vinicius Depizzol on the Tango mailing list.
http://lists.freedesktop.org/archives/tango-artists/2007-March/001072.html

So this should be a separate bug, which is in fact related to GNOME Goal #2
http://live.gnome.org/GnomeGoals/AppIcon

The remaining problem is the middle click that does not open the address book, which is GTK bug 409435, and wich I don't find blocking too.

So maybe my patch can now be reviewed...

I'll try to have a look real soon ; notice that I just installed gtk+ 2.10 today (it cost me my dear jamendo-enabled rhythmbox)...

Well, you can try a minimal jhbuild environment for GNOME 2.18 to test it. That way you don't need to touch to your system. That's how I test GNOME software...

I tried to read it -- didn't see anything wrong, but it isn't really readable.

Could you split it ? One patch file to modify configure.ac/Makefile.am, and the gmtray-gtk.c source file... After all, it's a new file, so reading it as a diff against another implementation isn't ideal.

Yeah, I should have done a simple copy instead of an svn copy. Gonna take care of this.

Created attachment 85935 [details] [review]
Clearer patch

The gmtray-gtk.c file is now a new file in the patch, and no more a diff against another file.

+  gtk_menu_popup (menu, 
+		  NULL, NULL, NULL, NULL,
+		  button, 
+		  activate_time);

Still missing the right menu positioning function.

Argh ! It was in the previous patch, but I had some problems in applying it, and had to use an older file...
+  gtk_menu_popup (menu, 
+		  NULL, 
+		  NULL, 
+		  gtk_status_icon_position_menu, 
+		  tray->specific->status_icon,
+		  button, 
+		  activate_time);

Final patch is for tomorrow :-) , I promise

Created attachment 86137 [details] [review]
Fifth try, with popup menu callback

Here it is... Thanks you Christian, to have pointed that out. Hope this is my last try :-)

It looks pretty good ; minor nitpicks :
- the copyright at the beginning of gmtray-gtk.c should now be 2000-2007 ;
- later down you write both of us as authors, and again a copyright for Damien, when in fact the copyright should be yours since you wrote the file probably for 2006-2007 now ;
- I don't think I wrote anything in that file, so I should probably be removed.

I'll commit if you tell me you're okay with those little changes. :-)

Well in fact I used as a template the x11 tray implementation of ekiga. There is has your name, so I left your name there. I let you decide what to change about this, your modifications are ok for me.

It's in! Thanks :-)

Now, let's see how long it will take before debian unstable has gtk+ 2.10 and Kilian's packages use it :-)