GNOME Bugzilla – Bug 338013
[PATCH] support for group-transient and pathologically-transient windows
Last modified: 2018-01-24 13:31:41 UTC
Please describe the problem: Internally, libwnck has had problems dealing with group-transient windows (which set their WM_TRANSIENT_FOR hint to point to the root) and with pathological cases such as "A is a transient for B and B is a transient for A". In particular, transiency cycles would lead to infinite loops. Bug 332493 was resolved with a fix for transiency cycles that worked provided no window outside the cycle marked itself transient for one of the windows in the cycle. Steps to reproduce: [While trying to package a test program a while back I accidentally overwrote the most important source file with a tarball. :-P I can provide an x86 binary version or try to rewrite it upon request.] Actual results: Expected results: Does this happen every time? Other information: From bug 332493: This patch amost completely solves the transiency problems in wnck. It's not quite EWMH compliant since setting WM_TRANSIENT_FOR to None will result in a window which isn't transient. (_wnck_atom_get doesn't really provide a way to tell whether the hint is set to None or unset.) There might be one other exception I haven't thought about much: an application closing a transient window's parent without changing the WM_TRANSIENT_FOR hint. I actually haven't coaxed a program into doing that... and I doubt it'd cause a crash. Central idea of the patch: window.c will do the necessary bookkeeping to ensure that parent-chasing loops behave correctly. Rather than returning whatever nonsense an application reports, wnck_window_get_transient() will return the parent app window if its argument has one and NULL otherwise. The return value is guaranteed not to cycle. If an application sets up a WM_TRANSIENT_FOR cycle, of length one or otherwise, all windows in the cycle will be considered group transient. (A transient for a group-transient window, however, isn't automatically group-transient.) This patch will change wnck's behavior wrt transients. For completeness (and to help you decide whether the patch is acceptable): 1. wnck_window_get_transient() will return NULL if argument is group-modal. Reason: it's not actually transient for an *app* window. In addition, setting TRANSIENT_FOR to root or None should give the same group-modal behavior (even if, as I mentioned, that's not yet true). 2. wnck_window_or_transient_needs_attention() will stick to the old behavior: if a window in the last_transient chain needs attention, it'll return true. If some other transient needs attention, it will be ignored. IMO this behavior is wrong and /any/ (non-group) transient with the NEEDS_ATTENTION hint should count, but that'd be a different patch anyway. Reason group transients shouldn't count: the main use I've seen for the function is to ensure that a tasklist/selector makes the right items flash. Since wnck doesn't mark group transient windows as SKIP_TASKBAR, we'd want group transient windows themselves to flash if they need attention. 3. (Random) wnck_tasklist_activate_task_window() will now call wnck_window_unminimize() instead of wnck_window_activate_transient() if it's activating a minimized window. AFAICT that captures the intent more correctly and ultimately does the same thing. I probably shouldn't have done that, but I was trying to cut down on explicit mentions of transiency anyway. Overall, it seems that libwnck's best bet when faced with group transiency is to ignore it. However, these code changes should make it good at *identifying* it. I imagine the corresponding changes in metacity would be more interesting. Dan: I did not tell wnck to emit a warning, but it's easy to insert one at any point in update_transient_for() when ...->priv->transient_for_cycle is set to TRUE.
Created attachment 63195 [details] [review] Patch to eliminate transiency-related infinite loops and support group-transient windows
Sorry for taking so long to reply... (In reply to comment #0) > It's not quite EWMH compliant since setting WM_TRANSIENT_FOR to None will > result in a window which isn't transient. (_wnck_atom_get doesn't really > provide a way to tell whether the hint is set to None or unset.) _wnck_atom_get() doesn't try to obtain the value of the hint at all; it merely returns the Atom (a 32 bit integer) corresponding to the given property. In the case of "WM_TRANSIENT_FOR" the value seems to be 68 (as given in /usr/X11R6/include/X11/Xatom.h on this machine). It is _wnck_get_window() which takes this Atom value of 68 and the relevant window and queries the X server to find out the value of the property for that Atom of that window. And _wnck_get_window() can distinguish between whether the hint is set to None or unset. > If an application sets up a WM_TRANSIENT_FOR cycle, of length one or > otherwise, all windows in the cycle will be considered group transient. Circularity like this just doesn't make much sense to me -- in the case of transient-to-group it should only mean transient to members of the same group mapped before that window. Of course, this may be related to the fact that you made group-transient basically not mean anything and so you just decided to use that, but real group-transiency should mean something. > 1. wnck_window_get_transient() will return NULL if argument is group-modal. > Reason: it's not actually transient for an *app* window. Yes, but the meaning is that it is transient to *all* app windows mapped before it. I'm not sure if we just want to pick off the first ancestor or leave this function as is and write some other abstraction to be smarter about walking up transients, but we do need to do something with group-modal windows. I know you were just trying to fix the circular transiency issue, but that's only one piece of the pie I'm worried about. > 2. wnck_window_or_transient_needs_attention() will stick to the old behavior: > if a window in the last_transient chain needs attention, it'll return > true. If some other transient needs attention, it will be ignored. > IMO this behavior is wrong and /any/ (non-group) transient with the > NEEDS_ATTENTION hint should count, but that'd be a different patch anyway. Yeah, wnck_window_or_transient_needs_attention needs to be rewritten. Anyway, group-transients should definitely count *if* they don't have a window in the taskbar. And the same should be true for normal transients -- they should count if and only if they don't have a window in the taskbar. > Since wnck doesn't mark group transient windows as SKIP_TASKBAR, > we'd want group transient windows themselves to flash if they need > attention. libwnck doesn't manually override the app in the case of group transient windows to mark them as skip_taskbar, but the application itself (or maybe even the window manager) may have marked it as such and we use the value we read in from the _NET_WM_STATE hints. So, group-transient windows do not necessarily have a window in the taskbar of their own. > 3. (Random) wnck_tasklist_activate_task_window() will now call > wnck_window_unminimize() instead of wnck_window_activate_transient() > if it's activating a minimized window. AFAICT that captures the intent > more correctly and ultimately does the same thing. That's actually incorrect; we want the transient activated instead of the main window. (Well, that's actually incorrect too. We'd prefer to focus the most recently used window of the group of windows corresponding to that taskbar button, but that's something we haven't gotten around to implementing. Closely related to bug 338660) > Overall, it seems that libwnck's best bet when faced with group transiency is > to ignore it. I think we really do need to make use of it. I think adding a tree structure to track transient relationships and nuking ugly code like find_last_transient_for (in favor of walking up or down the tree) is probably the way to go. It probably means rewriting some other sections of code and adding some more infrastructure, but such is life, I guess. > Dan: I did not tell wnck to emit a warning We should probably do so since the app is broken. As you pointed out, though, that's a trivial change. :-) Thanks for working on this! And thanks even more for your patience...
-- 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/58.