[DEV] tray [was: amsn and fluxbox]

[kjir]

Allright I fixed the issue, the patch can be found here.

I can't visualize the tray icon in XFCE, but Fluxbox, Stalonetray now work. Can somebody check if it still works with GNOME and KDE? (I don't have those installed...)

Fixing XFCE is the last thing left to do, as far as I am concerned, so we can have a tray icon working in most cases

[kjir]

Fixed the XFCE issue too. Now it should work everywhere!

[kakaroto]

Hello kjir,
I took a quick look at your patch and I don't understand everything from it.. you removed a lot of stuff, was it all unnecessary ? You also changed a False into True, care to tell us why and what will it affect? was it really supposed to be True ?
I'm concerned about this patch because :
1 - it's not as simple as your first one
2 - I don't understand it
3 - I don't have gnome or kde to test it with...

I have just installed stalonetray and it seems to work fine already with it... I also installed trayer and docker, and it works with both of them too... Could you test this on the other desktops and confirm it doesn't break gnome/kde support ?
Once you explained all your changes and confirmed that it didn't break anything, I'll apply the patch.

Thanks!

[kjir]

I guessed that this one wouldn't be as simple as the first one, so I'll break it down to you and explain (WARNING: This will be a long post):

Code:

Index: utils/linux/traydock/libtray.c
===================================================================
--- utils/linux/traydock/libtray.c (revision 10522)
+++ utils/linux/traydock/libtray.c (working copy)
@@ -81,6 +81,14 @@
 //static int tooltip=0;
 Tcl_Interp * globalinterp;
 
+static Window GetParentWindow(Window w) {
+ Window root, parent, *children;
+ unsigned int n;
+ XQueryTree(display, w, &root, &parent, &children, &n);
+ XFree(children);
+ return parent;
+}
+
 /* Set embed information */
 static void
 xembed_set_info (Tk_Window window, unsigned long  flags)
@@ -161,30 +169,24 @@
 DockIcon(ClientData clientData)
 {
 
- Window root, parent, *children;
- unsigned int n, ret, atom;
+ Window parent;
+ unsigned int ret, atom;
  TrayIcon *icon= clientData;
  char* wm_name;
 
- Tk_MapWindow(icon->win);
+    Tk_MakeWindowExist(icon->win);
 
- XQueryTree(display, Tk_WindowId(icon->win), &root, &parent, &children, &n);
- XFree(children);
-
  Tk_SetWindowBackgroundPixmap(icon->win, ParentRelative);
- XSetWindowBackgroundPixmap(display, parent, ParentRelative);
 
  xembed_set_info(icon->win,XEMBED_MAPPED);
 
- Tk_UnmapWindow(icon->win);
-
- if ( (atom = XInternAtom(display, "_NET_SYSTEM_TRAY_OPCODE", True )) == None ) {
+ if ( (atom = XInternAtom(display, "_NET_SYSTEM_TRAY_OPCODE", False )) == None ) {
  wm_name = get_wm_name();
  if (wm_name != NULL && !strcmp(wm_name, "KWin")) {
  atom = XInternAtom(display, "_KDE_NET_WM_SYSTEM_TRAY_WINDOW_FOR", False);
+ parent = GetParentWindow(Tk_WindowId(icon->win));
  ret = XChangeProperty(display, parent, atom,
  XA_WINDOW, 32, PropModeReplace, (unsigned char *)&parent, 1);
- Tk_MapWindow(icon->win);
  }
  XFree(wm_name);
  } else {

Here I created a new function to get the parent window. This is why you see a lot of deletions, but I mostly moved code in the function; what I removed/modified is:

Code:

-   Tk_MapWindow(icon->win);
+    Tk_MakeWindowExist(icon->win);

Here instead of mapping the window I only make it exist. This is because XFCE doesn't seems to accept changes to the size of the window after it has been mapped, so instead of Mapping (showing) the XWindow, I only make sure that Tk creates the XWindow unmapped (because Tk caches Window creations)

Code:

   Tk_SetWindowBackgroundPixmap(icon->win, ParentRelative);
-   XSetWindowBackgroundPixmap(display, parent, ParentRelative);

This is because the first function call makes the exact same thing and it is a Tk Function, so it is preferable to the direct X call.

Code:

-   Tk_UnmapWindow(icon->win);
-

Window is not mapped so useless to Unmap it. And besides this is what it prevented from showing up in stalonetray.

Code:

-   if ( (atom = XInternAtom(display, "_NET_SYSTEM_TRAY_OPCODE", True )) == None ) {
+   if ( (atom = XInternAtom(display, "_NET_SYSTEM_TRAY_OPCODE", False )) == None ) {
       wm_name = get_wm_name();

Here the change from True to False means "if there is no such Atom, try to create it". This is according to the systemtray specification and it is necessary for Fluxbox: if aMSN is the first app you launch, it won't go into the system tray if you don't create the "_NET_SYSTEM_TRAY_OPCODE" atom.
This if will most likely always end up in the else branch, so this is why it MAY change behaviour on KDE. I'll go get my laptop and check it out

Code:

         ret = XChangeProperty(display, parent, atom,
             XA_WINDOW, 32, PropModeReplace, (unsigned char *)&parent, 1);
-         Tk_MapWindow(icon->win);
       }

Same reason as above

Code:

@@ -245,6 +246,8 @@
  if (heightImg > h)
  heightImg = h;
 
+ if( !Tk_IsMapped(icon->win) )
+ Tk_MapWindow(icon->win);
  Tk_RedrawImage(icon->pixmap, 0, 0, widthImg, heightImg, Tk_WindowId(icon->win), (w-widthImg)/2 , (h-heightImg)/2 );
 
 }

Since we do not map the window in the above code anymore (because XFCE doesn't like it), we do it now, otherwise the icon won't show up in stalonetray (and possibly other trays)

Code:

@@ -351,6 +354,12 @@
  XSizeHints *hint;
  char cmdBuffer[1024];
 
+ /* systemtray was not available in Init */
+ if (systemtray==0) {
+ Tcl_AppendResult (interp, "cannot create a tray icon without a system tray", (char *) NULL);
+ return TCL_ERROR;
+ }
+
  /* Get memory for trayicon data and zero it*/
  icon = (TrayIcon *) malloc(sizeof(TrayIcon));
  memset((void *) icon, 0, (sizeof(TrayIcon)));
@@ -358,18 +367,11 @@
 
  mainw=Tk_MainWindow(interp);
 
- /* systemtray was not available in Init */
- if (systemtray==0) {
- Tcl_AppendResult (interp, "cannot create a tray icon without a system tray", (char *) NULL);
- return TCL_ERROR;
- }
-
  /* Get the first argument string (object name) and check it */
  arg=Tcl_GetStringFromObj(objv[1],(int *) &length);
  //printf("Arg: %s\n",arg);
- if (strncmp(arg,".",1)) {
- Tcl_AppendResult (interp, "bad path name: ",
- Tcl_GetStringFromObj(objv[1],(int *) &length) , (char *) NULL);
+ if (length >= 1 && arg[0] != '.') {
+ Tcl_AppendResult (interp, "bad path name: ", arg , (char *) NULL);
  return TCL_ERROR;
  }
 

Here I moved the check on systemtray before the memory allocations to prevent some memory leak. The other modification is equivalent code, just a little more readable.

Code:

@@ -431,10 +433,12 @@
  if (pixmap != NULL) {
  /* Create the window */
  icon->win=Tk_CreateWindowFromPath(interp,mainw,
- Tcl_GetStringFromObj(objv[1],(int *) &length),"");
+ Tcl_GetStringFromObj(objv[1],(int *) &length),NULL);

Here I substituted "" with NULL so that instead of creating a topLevel window, we create a window inside the main window. This is because if we create a topLevel window the window manager decorations will appear in some systems (i.e. Flux or Openbox + Stalonetray). I couldn't find any difference in the main window, if you spot something let me know and I'll find some other solution.

Code:

 
  DockIcon((ClientData)icon);
 
+ Tk_GeometryRequest( icon->win, 24, 24);
+
  icon->pixmap=Tk_GetImage(interp,icon->win,pixmap,ImageChangedProc, (ClientData)icon);
 
  /* Create callback function for event handling */
@@ -444,9 +448,11 @@
 
  /* Set default icon size hint */
  hint = XAllocSizeHints();
- hint->flags |=PMinSize;
- hint->min_width=24;
- hint->min_height=24;
+ hint->flags |=PMinSize|PMaxSize;
+ hint->min_width=16;
+ hint->max_width=64;
+ hint->min_height=16;
+ hint->max_height=64;
 
  XSetWMNormalHints(display,Tk_WindowId(icon->win),hint);
  XFree(hint);

Here is set the initial window size to 24x24 and I set the XWMNormalHints to a minimum and maximum size (these values should be reasonable, but we'll always be in time to change them should the necessity arise!)

If you still have other concerns I'll gladly try to address them, meanwhile I'll try to test this on Gnome and KDE. If you want me to comment more the code I can do that too

[kjir]

Ok, tested with Gnome, KDE 3.5 and KDE 4: It works!

[billiob]

That looks great (even if i can not test it).

[kakaroto]

I guessed that this one wouldn't be as simple as the first one, so I'll break it down to you and explain (WARNING: This will be a long post):

Thanks, that's appreciated!

[...]
what I removed/modified is:

Code:

-   Tk_MapWindow(icon->win);
+    Tk_MakeWindowExist(icon->win);

Here instead of mapping the window I only make it exist. This is because XFCE doesn't seems to accept changes to the size of the window after it has been mapped, so instead of Mapping (showing) the XWindow, I only make sure that Tk creates the XWindow unmapped (because Tk caches Window creations)

ok this is cool!

[...]

Code:

-   Tk_UnmapWindow(icon->win);
-

Window is not mapped so useless to Unmap it. And besides this is what it prevented from showing up in stalonetray.

makes sense.. but I was trying stalonetray and it was working.. so I wonder why you say it didn't work for stalone tray because it worked here for me!

Code:

-   if ( (atom = XInternAtom(display, "_NET_SYSTEM_TRAY_OPCODE", True )) == None ) {
+   if ( (atom = XInternAtom(display, "_NET_SYSTEM_TRAY_OPCODE", False )) == None ) {
       wm_name = get_wm_name();

Here the change from True to False means "if there is no such Atom, try to create it". This is according to the systemtray specification and it is necessary for Fluxbox: if aMSN is the first app you launch, it won't go into the system tray if you don't create the "_NET_SYSTEM_TRAY_OPCODE" atom.
This if will most likely always end up in the else branch, so this is why it MAY change behaviour on KDE. I'll go get my laptop and check it out

ok.. here I'm not as convinced.. I took a look at Xlib's man page for XInternalAtom and the True/False change was on 'only_if_exists' which means that yes, it will always create the atom for us, so the if will always branch to the else.. so the check for KDE (old versoins of KDE) will fail! And yes you have tested and made sure it still works for KDE, but that's because that part of the code is specifically for KDE 2 and KDE 3.1- which are pretty old but we still need to check for it.
Read these threads to know more : http://www.amsn-project.net/forums/viewtopic.php?t=4665
http://bugs.kde.org/show_bug.cgi?id=155730

Humm.. I also just noticed that we don't do anything if 'systemtray' is false, and we set that in the Init function of the lib.. which itself checks for the atom _NET_SYSTEM_TRAY_Sn, which is what was suggested to check to know if XDG tray is supported by KDE according to the bugs.kde.org link... so I'm guessing that maybe on KDE 2 and 3.1-, libtray won't even call that code because it will always fail with systemtray==false... so maybe that whole code is useless anyway.. in which case, it needs to be deleted..
I'd like to know what you think about this... thx!

[...]

Code:

[...] /* Get the first argument string (object name) and check it */
  arg=Tcl_GetStringFromObj(objv[1],(int *) &length);
  //printf("Arg: %s\n",arg);
- if (strncmp(arg,".",1)) {
- Tcl_AppendResult (interp, "bad path name: ",
- Tcl_GetStringFromObj(objv[1],(int *) &length) , (char *) NULL);
+ if (length >= 1 && arg[0] != '.') {
+ Tcl_AppendResult (interp, "bad path name: ", arg , (char *) NULL);
  return TCL_ERROR;
  }
 

Here I moved the check on systemtray before the memory allocations to prevent some memory leak. The other modification is equivalent code, just a little more readable.

the check is bad... if the arg is "" it won't fail... o we need it to also fail for length < 1...
The code we had before wasn't great either, I now changed it into :

Code:

       if (arg == NULL || length < 1 || strncmp(arg, ".", 1)) {

Code:

@@ -431,10 +433,12 @@
  if (pixmap != NULL) {
  /* Create the window */
  icon->win=Tk_CreateWindowFromPath(interp,mainw,
- Tcl_GetStringFromObj(objv[1],(int *) &length),"");
+ Tcl_GetStringFromObj(objv[1],(int *) &length),NULL);

Here I substituted "" with NULL so that instead of creating a topLevel window, we create a window inside the main window. This is because if we create a topLevel window the window manager decorations will appear in some systems (i.e. Flux or Openbox + Stalonetray). I couldn't find any difference in the main window, if you spot something let me know and I'll find some other solution.

well, this is good, that must be why some systems get a real window with decorations... this way we can avoid that, and no, it won't change anything in the main window of amsn because we dont "pack" that window, so it won't appear, which is cool. I just never thought we could have an internal window work as a tray icon.. I always thought it must be a toplevel window.. but I'm guessing the rest of the code in libtray actually reparents the window, right ?

Code:

 
  DockIcon((ClientData)icon);
 
+ Tk_GeometryRequest( icon->win, 24, 24);
+
  icon->pixmap=Tk_GetImage(interp,icon->win,pixmap,ImageChangedProc, (ClientData)icon);
 
  /* Create callback function for event handling */
@@ -444,9 +448,11 @@
 
  /* Set default icon size hint */
  hint = XAllocSizeHints();
- hint->flags |=PMinSize;
- hint->min_width=24;
- hint->min_height=24;
+ hint->flags |=PMinSize|PMaxSize;
+ hint->min_width=16;
+ hint->max_width=64;
+ hint->min_height=16;
+ hint->max_height=64;
 
  XSetWMNormalHints(display,Tk_WindowId(icon->win),hint);
  XFree(hint);

Here is set the initial window size to 24x24 and I set the XWMNormalHints to a minimum and maximum size (these values should be reasonable, but we'll always be in time to change them should the necessity arise!)

If you still have other concerns I'll gladly try to address them, meanwhile I'll try to test this on Gnome and KDE. If you want me to comment more the code I can do that too

It doesn't look too good.. try launching 'trayer' and try that code.. the tray becomes 16x16 so the image gets squashed which is bad... I think the minimum should be 24/24... or maybe the GeometryRequest should be done after setting the min/max .. I don't know, but in any case, I had to reset it to 24x24 so it works good now here...

No need to comment the code more, it's more like commenting the changes, which you have just done brilliantly, thanks!

[kjir]

[...]

Code:

-   Tk_UnmapWindow(icon->win);
-

Window is not mapped so useless to Unmap it. And besides this is what it prevented from showing up in stalonetray.

makes sense.. but I was trying stalonetray and it was working.. so I wonder why you say it didn't work for stalone tray because it worked here for me!

I will check again, what's your window manager?

[...]
ok.. here I'm not as convinced.. I took a look at Xlib's man page for XInternalAtom and the True/False change was on 'only_if_exists' which means that yes, it will always create the atom for us, so the if will always branch to the else.. so the check for KDE (old versoins of KDE) will fail! And yes you have tested and made sure it still works for KDE, but that's because that part of the code is specifically for KDE 2 and KDE 3.1- which are pretty old but we still need to check for it.
Read these threads to know more : http://www.amsn-project.net/forums/viewtopic.php?t=4665
http://bugs.kde.org/show_bug.cgi?id=155730

Humm.. I also just noticed that we don't do anything if 'systemtray' is false, and we set that in the Init function of the lib.. which itself checks for the atom _NET_SYSTEM_TRAY_Sn, which is what was suggested to check to know if XDG tray is supported by KDE according to the bugs.kde.org link... so I'm guessing that maybe on KDE 2 and 3.1-, libtray won't even call that code because it will always fail with systemtray==false... so maybe that whole code is useless anyway.. in which case, it needs to be deleted..
I'd like to know what you think about this... thx!

Was it working to this day with those versions of KDE? These are very old versions of KDE, which means that such a system probably doesn't have suitable tcl/tk versions and probably has other problems too, is it a good idea to try to still support such systems? Do we have news of any user that has such a legacy system? And what about the other DEs prior to XDG support? (Namely GNOME)

Generally I am for scrapping legacy code, unless there is a good reason to mantain such a distant backwards compatibility. But this is not my project, so if you say that we need this legacy code, I can find a workaround to make it work (even though I wouldn't be able to test it)

[...]
the check is bad... if the arg is "" it won't fail... o we need it to also fail for length < 1...
The code we had before wasn't great either, I now changed it into :

Code:

       if (arg == NULL || length < 1 || strncmp(arg, ".", 1)) {

You're right, luckily you caught that bug!

[...]
well, this is good, that must be why some systems get a real window with decorations... this way we can avoid that, and no, it won't change anything in the main window of amsn because we dont "pack" that window, so it won't appear, which is cool. I just never thought we could have an internal window work as a tray icon.. I always thought it must be a toplevel window.. but I'm guessing the rest of the code in libtray actually reparents the window, right ?

The reparenting is done by the system tray (or should be) according to the systemtray specifications

It doesn't look too good.. try launching 'trayer' and try that code.. the tray becomes 16x16 so the image gets squashed which is bad... I think the minimum should be 24/24... or maybe the GeometryRequest should be done after setting the min/max .. I don't know, but in any case, I had to reset it to 24x24 so it works good now here...

Well according to the systemtray specification we should not even do that, the icon should be freely resizable, or at least this is what I am to understand from the following sentence:

Tray icons may be assigned any size by the system tray, and should do their best to cope with any size effectively.

No need to comment the code more, it's more like commenting the changes, which you have just done brilliantly, thanks!

Ok fine!

[kjir]

I made some quick tests:
I verified that the Tk_MapWindow is not needed anymore. The icon gets displayed anyway (because the window is reparented). A call to Tk_UnmapWindow hides the icon, though. I didn't verify this with fluxbox, but I think that it will behave the same way (tomorrow I'll check).

I installed trayer and it is true that it downsizes the icon to the minimum width/height specified. This is a wrong behaviour on the side of trayer because it ignores the base_width/base_height settings (I tried to specify those, but without success). Setting min_width/min_height to 24 is a workaround, but it means that the icon would never be any smaller even if requested, so we may receive some "aMSN icon too big" bugs. The deluge tray icon works fine with trayer (deluge is written in pygtk), so there is some way of making this right without losing the ability to resize to 16x16 if requested, maybe in the coming days I could have a look and see if I can find a solution.

I'll leave to you the honour of choosing the best solution to the problems that arose from this discussion, if you need more confrontation just ask!

[kakaroto]

I will check again, what's your window manager?

E17, and I launched stalonetray simply by typing "stalonetray" on a terminal...

Was it working to this day with those versions of KDE? These are very old versions of KDE, which means that such a system probably doesn't have suitable tcl/tk versions and probably has other problems too, is it a good idea to try to still support such systems? Do we have news of any user that has such a legacy system? And what about the other DEs prior to XDG support? (Namely GNOME)

Generally I am for scrapping legacy code, unless there is a good reason to mantain such a distant backwards compatibility. But this is not my project, so if you say that we need this legacy code, I can find a workaround to make it work (even though I wouldn't be able to test it)

well, as I said, it looks like for that kind of legacy system, the Libtray_Init would have found that there is no XDG compatible systray and would just refuse to create any systray :

Code:

       /* systemtray was not available in Init */
        if (systemtray==0) {
                Tcl_AppendResult (interp, "cannot create a tray icon without a system tray", (char *) NULL);
                return TCL_ERROR;
        }

So I'm guessing that code was just dead code anyways... so yes, I would vote for deleting that legacy code...
Especially considering the fact that noone has ever complained about it, so we're all good! The thing is we try to support legacy systems as much as possible, we have a lot of win95 users for example, and I don't know about the linux dudes... but we thought "hey, the code is there, we can just leave it there" since we had nothing to loose by doing so. Now I realize that it wasn't used and noone complained which is proof enough that we can safely remove it now!
That would mean the 'atom = XInternAtom' would not need to be an in 'if' anymore, and we can completely remove the code for checking KWin, as well as removing the whole GetParentWindow and get_wm_name functions since they are not used anywhere else anymore.

Well according to the systemtray specification we should not even do that, the icon should be freely resizable, or at least this is what I am to understand from the following sentence:

Tray icons may be assigned any size by the system tray, and should do their best to cope with any size effectively.

humm.. well, I don't understand then why it would show me a tiny icon in trayer, which I launched like this :

Code:

trayer --edge bottom --align right --distance 41 --widthtype request --heighttype request &

[kjir]

So I'm guessing that code was just dead code anyways... so yes, I would vote for deleting that legacy code...
Especially considering the fact that noone has ever complained about it, so we're all good! The thing is we try to support legacy systems as much as possible, we have a lot of win95 users for example, and I don't know about the linux dudes... but we thought "hey, the code is there, we can just leave it there" since we had nothing to loose by doing so. Now I realize that it wasn't used and noone complained which is proof enough that we can safely remove it now!
That would mean the 'atom = XInternAtom' would not need to be an in 'if' anymore, and we can completely remove the code for checking KWin, as well as removing the whole GetParentWindow and get_wm_name functions since they are not used anywhere else anymore.

Yeah that is quite a cleanup, I love reductions of code, fewer bugs The day a KDE 2 user is coming up and complaining we can always look back into SVN history and reimplement it. And besides this is a secondary feature, aMSN can be used also without a tray icon.

humm.. well, I don't understand then why it would show me a tiny icon in trayer, which I launched like this :

Code:

trayer --edge bottom --align right --distance 41 --widthtype request --heighttype request &

I have the same behaviour, I think it is a bug in the trayer app, but as I said I might find another solution to the problem.

[kakaroto]

1 - hehe, yeah, the day that guy comes, I can tell him to use amsn 0.97 and still with it :p and yeah, the tray is secondary, I've never ever had a tray on any of my linux systems until yesterday when I decided to try stalonetray and trayer :p

2 - ok.. if you could fix that, it would be nice! In the meantime, I'll commit the changes that we've just discussed...

oh and by the way, this is like the 3rd message I think where I want to say something and I forget, hehe.
I noticed that if I launch trayer, I automatically get a few tray icons in there, but amsn's tray doesn't show... if I start amsn after trayer, then it shows.. then if I launch stalonetray, the tray icons that were in trayer get unmapped and they get mapped in stalonetray instead.. but amsn's tray icon stays in trayer.... I'm guessing we should be listening to a 'tray changed/available' message or something so we can reparent our tray icon, or so we can create our tray icon if a new tray is available.
Do you have any idea how to do that ? I'm just asking 'cause it would be nice to be more 'complete', but I'm not asking you to do it if you don't want to/don't have time to.

anyways, thx again for your hard work! We could use a guy like you on the team

[kjir]

Yes, this behaviour is because we don't implement the part of the specification for monitoring systray changes, if we don't find one we just give up. If we find one, we stick to that one, no matter what. That would actually be a nice feature to have to be more 'complete', as you say (or more compliant ), it shouldn't be too hard either.

I love coding and fixing problems (especially mine!), and I think that that's the reason why there is such a thing as open source. I can't make any promises as to WHEN I can fix problems, but as you can see when the code is clean and not bloated it's easier to get things done.
Thank you for your collaboration, it's a lot harder when you don't have any help from the mantainers. I'll gladly help to resolve some more bugs in the future, especially if you're missing C developers and need help on that.

[trv]

I'm using openbox (as a standalone desktop, not in gnome).
I'm also using a cool application called pypanel. It's sort of a extreme lightweight gnome-panel replacement, absolutely perfect.
It also features an icon tray.

Well, the problem (obviously) is that amsn's icon is not there. Opera,pidgin,xchat etc display properly.

I'm using latest svn (r10573).

Any thoughts? I thought it should now work everywhere

[kakaroto]

well, everywhere is relative.. obviously, you're using something crappy developped by some crappy developper who doesn't know how to implement his stuff correctly... (sorry to the developper, j/k)
in any case, we'll look into it :p