GNOME Bugzilla – Bug 310381
Fix active_task checking for group/app modal (i.e. "group transient") windows
Last modified: 2018-01-24 13:28:59 UTC
Please describe the problem: xmms and beep-media-player have multiple windows (player, playlist, equalizer, ...). each window has the flag WNCK_WINDOW_STATE_SKIP_TASKLIST set except the main window (the player itself). So only the player has a corresponding task in tasklist. The problem is when xmms become active, the callback «wnck_tasklist_active_window_changed» is runned for each window composing xmms ! first time «wnck_screen_get_active_window» says that it's the player witch is the active window, that's cool because the player has a corresponding task witch become active. but after that, the callback is runned a second time and then «wnck_screen_get_active_window» returns the playlist as active window, witch isn't cool at all because there is no corresponding task and then active_window become NULL ! that's not dramatic for now, but it's more visible when applying the patch in bug #309956. Steps to reproduce: 1. lunch xmms 2. click on its task on the window-list applet 3. you see ? the button isn't pushed down ! Actual results: Expected results: Does this happen every time? yes Other information:
is it a bug or a feacture ? I done some tests, this bug isn't only for xmms/b-m-p but also for all multi-window programs. For example: lunch gedit then open the filechoozer window... then when clicking on the gedit's button on the panel you see that the button isn't pushed down ! I think solving that (if it's not a feacture) may be very hard. Any comment ?
It's a bug. wnck_tasklist_active_window_changed() should not only check the hash for the active window but also for its ancestors. E.g. in the gedit case the open file window is a transient of the gedit window, so when libwnck notices that there is no task for the open file window it should then check if there's a task for the gedit window. If someone would like to look into fixing this, a basic version could be done using some kind of wnck_window_get_ancestor_of helper function (see the wnck_window*transient* functions for help though those do the opposite), but a fully correct version will be complicated a little bit by group modal dialogs (see the _NET_WM_STATE_MODAL section of http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html#id2522991) requiring checking a group of windows and their ancestors as well...
Created attachment 49315 [details] [review] works !!! it was not so hard. is it the right way to fix the bug ?
Created attachment 49317 [details] [review] same patch but against CVS
Looks pretty good. A few minor comments: - You're doing extra unnecessary checking in wnck_tasklist_active_window_changed(). Note that the g_hash_table_lookup() call that precedes yours does not check whether the active_window is NULL or not. - You should also handle the case of window A which is transient for window B, which is transient for window C, and where window C is the only one that has a corresponding task. It shouldn't be too big a change from the current code. - I'd prefer that wnck_window_get_transient() returned a WnckWindow* rather than the xid (i.e. the gulong it currently returns) of the window. - Since you're not tackling group modal windows, please add a FIXME in wnck_tasklist_active_window_changed() about it to help us remember to get back to it.
Created attachment 49321 [details] [review] better ? oki, thanks for comments ! is it better now ?
Yes, though wnck_tasklist_active_window_changed() still needs a little more work. - It is possible to have both a window and its transient shown in the tasklist, so you need to do the g_hash_table_lookup() for each active window found and quit as soon as you find an active task. - You don't need both active_window and tmp_window (especially since you merely copy tmp_window to active_window and don't do anything else with it). - You still didn't add a FIXME about group modal windows (though if you want me to commit the patch I can do that for you, I guess, especially since I should mention that there's lots of other places in libwnck that need fixing for group modal windows)
Created attachment 49324 [details] [review] yet another patch getting better... any comment ?
Looks great! Do you have a CVS account, or will you need me to commit it for you?
*** Bug 117488 has been marked as a duplicate of this bug. ***
I've no CVS account, this will be my first commited patch... tanks.
Would you like to make a ChangeLog entry and then I'll commit?
2005-07-18 Xavier Claessens <x_claessens@skynet.be> look for the right task when transient window become active (bug #310381) * libwnck/tasklist.c (wnck_tasklist_active_window_changed): get parent window until we have a correcponding task. * libwnck/window.{h,c} : add function wnck_window_get_transient() returns the parent window of a transient.
Committed, thanks. I'll leave the bug open for the group transiency issue that occurs with group modal windows mentioned in comment 2.
Hmm, it seems comment 2 is no longer relevant : I've backported this patch (with other fixes for transient activation windows) to libwnck 2.10.3 and the file selector issue is fixed by this patch.
Frederic: Most of comment 2 isn't relevant because the patch was committed, as noted in comment 14. The part of comment 2 about group modal windows (which gtk+ unfortunately makes really difficult to implement so you may not be aware of many examples) is still valid. Also, note that 'group' in 'group modal' has a totally different meaning than 'group' in 'grouped windows', where the former is defined in the EWMH and the latter is just a way to lump windows (including from separate applications) together in the tasklist.
If anybody knows of group modal dialogs in real applications, I'm sure this would help me understand better the bug and fix it :-)
I could try to dig up the cases (nautilus used to do it for one window, but I think they removed it due to nasty bugs in metacity; eclipse probably also has workarounds for metacity not behaving well with such windows). However, I really need to fix this in metacity as well, and the fix to libwnck will probably be a subset of the code to fix this in metacity. If you still want to fix it, I'll go ahead and dig up the testcases, but you should feel free to leave this one to me. :-)
I'll take the lazy way for now and wait for you to do all the hard work ;-)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libwnck/issues/48.