Bug 1152 - OpenBox sometimes doesn't reparent unmanaged windows back to root
Status: CLOSED FIXED
Alias: None
Product: Openbox
Classification: Unclassified
Component: general
Version: unspecified
Hardware: PC Linux
: P2 normal
Assignee: Mikachu
QA Contact:
URL:
Depends on:
Blocks:
 
Reported: 2004-02-08 09:38 EST by Lubos Lunak
Modified: 2007-03-18 11:55:09 EDT
0 users

See Also:


Attachments
Check the parent of the ReparentNotify event (2.03 KB, patch)
2004-02-24 12:34 EST, Dana Jansens
Attempt #2. Checking only a single event in the queue is always bad logic :D (2.20 KB, patch)
2004-02-24 12:42 EST, Dana Jansens

Description Lubos Lunak 2004-02-08 09:38:43 EST
How to reproduce: Install KDE3.2 :), go to http://ktown.kde.org/~seli/kdewm, 
install the kdetrayproxy utility from that page, and run OpenBox-3.1 as 
described there. Run some systray KDE applications (keyboard layout switcher, 
Klipper,...). Restart Kicker (the KDE panel) using 'dcop kicker Panel 
restart'. Sometimes some of the systray icons won't be docked back, because 
Kicker releases the systray windows, OpenBox temporarily manages them, 
kdetrayproxy tries to take them over from the WM, but OpenBox doesn't reparent 
them back to the root window, resulting in the window being destroyed as a 
child of its OpenBox frame. 
 
The relevant code seems to be in frame.c , frame_release_client(), the 
condition checking for ReparentNotify. In this specific case, OpenBox gets 
first MapRequest for the window, kdetrayproxy detects new window in 
_NET_CLIENT_LIST and does withdrawn of the window (i.e. XUnmapWindow() + the 
matching root window message), and UnmapNotify is processed by OpenBox before 
it processes its own ReparentNotify caused by reparenting the window in the 
frame. You can easily check by comparing the parent field in the event with 
the frame window. As a result of this, OpenBox doesn't reparent the window 
back, and eventually the window is destroyed. 
 
I personally don't understand at all why the check is there. ICCCM says that 
in order to withdrawn a window, the client should do XUnmapWindow()+the root 
window message, so I don't see why you should try to handle clients 
reparenting managed windows elsewhere, as that's clearly ICCCM violation. I 
case that's a workaround for some broken applications, I suggest at least 
first filtering out events caused by OpenBox itself. 
 
BTW, I believe the same problem could be reproduced also by simply doing 
'XMapWindow(); XSync(); some_delay(); XUnmapWindow()', so I wonder you've 
never run into this problem. E.g. the progress dialogs in KDE probably could 
trigger this problem as well (KWin used to have a similar problem in the 
KDE3.2beta stage).
Comment 1 Mikachu 2004-02-23 18:11:53 EST
would you like to supply a patch perhaps?
Comment 2 Lubos Lunak 2004-02-24 03:25:43 EST
As I said, I don't understand why that piece of code is there at all, so my patch  
would be dumping that whole if() part (i.e. with current CVS removing lines  
511-523,528 from frame.c). But I don't understand OpenBox source, so I don't know  
why exactly that code is there.  
(BTW, in case you'll have some problems with OpenBox+KDE3.2, feel free to mail  
kwin@kde.org or me directly, I'll be glad to help.)  
  
Comment 3 Mikachu 2004-02-24 11:09:52 EST
just a side note, maybe you could try and not use the exact name 'kdetrayproxy'
as a utility included with openbox has that very name and probably predates yours
Comment 4 Dana Jansens 2004-02-24 12:33:23 EST
This part of the code is a little hazy for me now.. I think the reparent check 
has to do with supporting WMaker dock apps or somesuch.

Anyhow, your suggested fix makes sense to me. The code works otherwise as it is. 
And I do remember having to debug it all to get it working. So I would suggest 
fixing it as you stated, checking the parent in the event.

I will attach a patch, can you test it for us Lubos? That would be much 
appreciated.
Comment 5 Dana Jansens 2004-02-24 12:34:46 EST
Created attachment 205 [details]
Check the parent of the ReparentNotify event
Comment 6 Dana Jansens 2004-02-24 12:42:26 EST
Created attachment 206 [details]
Attempt #2. Checking only a single event in the queue is always bad logic :D
Comment 7 Lubos Lunak 2004-02-24 13:19:32 EST
Seems to work fine, thanks. 
 
Comment 8 Mikachu 2004-03-21 08:49:57 EST
forgot to close this earlier