GNOME Bugzilla – Bug 582639
should not move/resize override redirect windows
Last modified: 2009-06-30 13:49:01 UTC
commit 6849735e9dae194358941f861140de4a30f80b2d changed the way override redirect windows are handled. Specifically, the following was removed from meta_window_new_with_attrs: if (attrs->override_redirect) { meta_verbose ("Deciding not to manage override_redirect window 0x%lx\n", xwindow); return NULL; } Which results in meta_window_move_resize_internal being called on override redirect windows. These are queued from a variety of places in the code as well as it being called explicitly from meta_window_new_with_attrs. I'm not sure it makes sense to (or perhaps violates the contract of OR if we) move resize OR windows at all. But the specific problem I've noticed is when we apply the constraints to OR windows. The observed problem... gnome-screensaver currently creates a toplevel window for each gdk monitor. We take steps to avoid overlapping screensaver windows. In the mirrored screens case this results in an empty (1x1) window on one of the monitors. gnome-screensaver, perhaps erroneously, sets the fullscreen hint on these windows (my vague recollection is that this was to work around a compiz bug a while back). This is not a problem for metacity because it does not manage these windows at all. However, now that mutter does it tries to apply constraints to this window. One of these constraints resizes the window to full screen size due to the presence of the hint.
Created attachment 134655 [details] [review] bail out of move_resize_internal for OR windows Since there are so many ways that move_resize_internal is called here is a patch that just bails out of the function when it is called for an OR window. Otherwise, we could play wack-a-mole and put conditionals in all the calling functions. This fixes the overlapping screensaver windows problem for me.
Regardless of the hint, we should not resize OR windows, so the patch makes good sense to me.
*** Bug 581145 has been marked as a duplicate of this bug. ***
The argument for minimalism here - just adding a late check at the end - has some appeal. On the other hand, it makes it pretty confusing as to what is really going on, if we update member variables, read and set properties, debug spew, and then at the very end don't do anything. It also misses an opportunity to track down other things we might be doing inappropriately with override-redirect windows, e.g.: using them in edge-resistance computations when resizing, or saving them in the session. I'll attach a set of patches that implement a four-pronged approach: 1) Remove override-redirect windows from meta_display_list_windows() and similar. (I did thorough analysis of all code paths to make sure that the callers didn't expect override-redirect windows, and added meta_display_list_all_windows() for the two cases that did want it.) 2) Skip most property and client message handling for override-redirect windows. This removes many code paths that could cause us to try and do things to an override-redirect window (a long with improiving efficiency) 3) Add selected if (!override_redirect) checks 4) Add g_return_if_fail() at the leaf functions so we find the places where 1-3 weren't comprehensive enough. The patch set here covers everything but stacking; I'll put my patches for stacking in a separate bug report. The two sets are pretty much independent, but I haven't tested them independently, so there may be some implicit dependnecies between them.
Created attachment 136719 [details] [review] Add checks against inappropriate changes to override-redirect window Add g_return_if_fail() to check that window-management functions like meta_window_maximize() aren't called on override-redirect windows. This reveals that were were "unminimizing" override-redirect windows when adding them; avoid doing that.
Created attachment 136720 [details] [review] Don't read most properties for override-redirect windows Skipping handling of properties for override redirect windows has two advantages: first it reduces the amount of work needed to get an override-redirect window (menu, tooltip, drag icon) onto the screen. But more importantly, it reduces the number of code-paths for an override-redirect to get into some code portion where it isn't expected. * Integrate the list of properties we load initially with the list of property hooks; this avoids having two separate lists that we have to keep in sync. * Add a flag to MetaWindowPropHooks to indicate whether the property should be handled for override-redirect windows; currently we load a) properties that identify the window - useful for debugging purposes b) WM_TRANSIENT_FOR (could be used to associate menus with toplevels.) * For properties that aren't always loaded through window-props.c, add !window->override checks to places that trigger loading, and add g_return_if_fail(!window->override) to the load functions as a double-check.
Created attachment 136721 [details] [review] meta_display_list_windows: Exclude override-redirect Don't include override-redirect windows in the list return by meta_display_list_windows(), since we almost never want to handle them when considering "all window" for the display. Add a separate meta_display_list_all_windows() that includes override-redirect windows.
Created attachment 136722 [details] [review] meta_screen_foreach_window(): Skip override-redirect windows Don't include override-redirect windows when iterating the windows in the screen. We don't need them for any of the current uses: - Queueing redraws and resizes on managed windows - Checking which windows should be added to a new workspace
Created attachment 136723 [details] [review] Ignore client messages sent to override-redirect windows If someone asks us to close, maximize, etc, an override-redirect window, just ignore the request.
Created attachment 136724 [details] [review] Don't add override-redirect windows to workspaces Normally a window that is "on all workspaces", is also on a particular workspace (to deal with being unstuck.) This is pointless for override-redirect windows.
Created attachment 136725 [details] [review] Avoid moving and resizing override-redirect windows Override-redirect windows should not be moved or resized by the window manager. - Mark override-redirect windows as already placed to avoid placing them when first shown. - Don't move-resize newly created override-redirect MetaWindow - Don't queue a resize on override-redirect windows when reading their WM_TRANSIENT_FOR hint. - Add g_return_if_fail (!window->override_redirect) to catch unexpected code paths that might result in override-redirect windows being moved or resized.
I've pushed the patches from: Bug 582639 – should not move/resize override redirect windows Bug 585984 – Fix stacking for override-redirect windows along with some other separately filed bug fixes that these patches depend upon to the 'override-redirect-exclusion' branch in the Mutter repository.
(In reply to comment #5) > Created an attachment (id=136719) [edit] > Add checks against inappropriate changes to override-redirect window Looks good. (In reply to comment #6) > Created an attachment (id=136720) [edit] > Don't read most properties for override-redirect windows > * Add a flag to MetaWindowPropHooks to indicate whether the > property should be handled for override-redirect windows; > currently we load... > ... b) WM_TRANSIENT_FOR (could be > used to associate menus with toplevels.) Why would mutter need to know this more than regular metacity would? (Likewise with WM_PROTOCOLS.) + * where disappearance of a previously value is significant. missing a word (In reply to comment #7) > Created an attachment (id=136721) [edit] > meta_display_list_windows: Exclude override-redirect > > Don't include override-redirect windows in the list return by > meta_display_list_windows(), since we almost never want to handle > them when considering "all window" for the display. Add a separate > meta_display_list_all_windows() that includes override-redirect > windows. meta_display_list_all_windows() needs a better name. The commit message is nearly surreal. ("Add a new meta_display_list_all_windows() method, which should almost never be used by code that wants to list all windows.") You can simplify list_windows() a bunch by using a GHashTableIter instead of g_hash_table_foreach. (In reply to comment #8) > Created an attachment (id=136722) [edit] > meta_screen_foreach_window(): Skip override-redirect windows (In reply to comment #9) > Created an attachment (id=136723) [edit] > Ignore client messages sent to override-redirect windows (In reply to comment #10) > Created an attachment (id=136724) [edit] > Don't add override-redirect windows to workspaces These all look good. (In reply to comment #11) > Created an attachment (id=136725) [edit] > Avoid moving and resizing override-redirect windows Generally good. This hurt my head: + g_return_if_fail (!((queuebits & META_QUEUE_MOVE_RESIZE) != 0 && window->override_redirect)); Applying DeMorgan's law and swapping the terms gives: g_return_if_fail (!window->override_redirect || (queuebits & META_QUEUE_MOVE_RESIZE) == 0); which I think is a little clearer and looks more like the other g_return_if_fails too.
Thanks for the reviews, Dan. > (In reply to comment #6) > > Created an attachment (id=136720) [edit] > > Don't read most properties for override-redirect windows > > > * Add a flag to MetaWindowPropHooks to indicate whether the > > property should be handled for override-redirect windows; > > currently we load... > > ... b) WM_TRANSIENT_FOR (could be > > used to associate menus with toplevels.) > > Why would mutter need to know this more than regular metacity would? (Likewise > with WM_PROTOCOLS.) In both cases it was "future proofing", and nothing used currently: WM_TRANSIENT_FOR - IIRC, Sun's Looking Glass used WM_TRANSIENT_FOR to group menus with windows when transforming a window. WM_PROTOCOLS - might have future protocols for synchronization of drawing for composited windows, this would be an appropriate place to advertise them. But certainly it might well be cleaner to just omit them for now, and add them back later when we need them. > (In reply to comment #7) > > Created an attachment (id=136721) [edit] > > meta_display_list_windows: Exclude override-redirect > > > > Don't include override-redirect windows in the list return by > > meta_display_list_windows(), since we almost never want to handle > > them when considering "all window" for the display. Add a separate > > meta_display_list_all_windows() that includes override-redirect > > windows. > > meta_display_list_all_windows() needs a better name. The commit message is > nearly surreal. ("Add a new meta_display_list_all_windows() method, which > should almost never be used by code that wants to list all windows.") The only way I thought of doing it better was: typedef enum { META_LIST_DEFAULT = 0, META_LIST_INCLUDE_OVERRIDE_REDIRECT = 1 << 0 } MetaListWindowsFlags; then add that to meta_display_list_windows(). Which I was reluctant to do because it woudl require me to change all callers. But probably should do anyways. meta_display_list_windows_including_override_redirect() is a non-starter of a function name for me. > You can simplify list_windows() a bunch by using a GHashTableIter instead of > g_hash_table_foreach. Yeah. GHashTableIter didn't exist of course when that code was written. > (In reply to comment #11) > > Created an attachment (id=136725) [edit] > > Avoid moving and resizing override-redirect windows > > Generally good. This hurt my head: > > + g_return_if_fail (!((queuebits & META_QUEUE_MOVE_RESIZE) != 0 && > window->override_redirect)); > > Applying DeMorgan's law and swapping the terms gives: > > g_return_if_fail (!window->override_redirect || (queuebits & > META_QUEUE_MOVE_RESIZE) == 0); > > which I think is a little clearer and looks more like the other > g_return_if_fails too.> Sure, I was thinking: "If we are queuing a move resize, then the window must not be override redirect" But you are right that the contrapositive "If the window is an override-redirect window, then we must not be queueing a resize" does produce a more decipherable result C expression. I'll clean these points up when rebasing to merge into master.
Pushed to master with suggested changes