GNOME Bugzilla – Bug 346927
Heuristics for fullscreening "legacy" apps
Last modified: 2006-08-21 18:11:35 UTC
Archives appear to be busted right now, but there's an interesting thread right now in metacity-devel-list about legacy apps (in particular, apps running under current versions of WINE) and fullscreen windows. The word "legacy" may not quite be fair here, as it appears to be caused by strangeness in the Windows API making it so that WINE apps will likely use funny manual methods to attempt to duplicate fullscreen functionality. It should be noted that this will likely also apply to some legacy apps not running WINE. It appears as though there are two areas in which our heuristics need to be improved: src/window.c:recalc_window_features() disables ability for apps to enter the fullscreen mode if the window is not resizable unless the minimum size hints are exactly the same size as the screen and the window is undecorated. While a decorated window with minimum size hints equal to the size of the screen inherently conflicts with fullscreen size (as can be viewed one of two ways -- either because decorations conflicts with fullscreen mode or such a window would be too large to be fullscreen), it appears that this is a method used by applications to attempt to manually fullscreen themselves instead of using the appropriate hints. It is likely that the majority (and maybe even all) apps with decorations and such a minimum size hint intend to be fullscreen so we should probably remove the decoration checking. src/stack.c:window_is_fullscreen_size() is used to try to treat certain windows as fullscreen despite not using the appropriate hints if we can detect that they have somehow manually tried to duplicate fullscreen functionality. These heuristics check both the size and position of the window, and require that the client window of the app be placed at the upper left corner of the workspace. For windows with decorations, that will not be possible as constraints will always force the decorations into the workarea. As noted above, we should probably treat apps as intending to be fullscreened even with decorations, so these checks should probably be based on size only. (Or, if they are based on position, then the outer window should be checked). Summary: * Remove decoration check in src/window.c:recalc_window_features() * Check window size only (not position) in src/stack.c:window_is_fullscreen_size()
Created attachment 69729 [details] [review] The patch which fixes the problem for me.
The problems appears to not be related to the heuristics Metacity uses to check whether a window may be fullscreened, but rather to a missing update of a window state after screen resolution change so that heuristics can do their work. I've used slightly old Metacity 2.10.0 sources, but hopefully the attached patch is still relevant.
Is the resolution change from xvidmode (a "viewport" change) or xrandr? If xrandr, we do handle the configure notify on the root window, so we could recalc features on all windows in that routine. I guess xvidmode lacks notification? but we most be notified somehow of the new screen size or the recalc would not do anything?
That's an XRandr notification and it leads to a ConfigureNotify on the root window. meta_screen_resize queues a move_resize event for all windows in that case, so it looks reasonable to simply add recalc_window_features to meta_window_move_resize_internal instead of exposing recalc_window_features outside of window.c. Attached window.Thief.diff does that and fixes Thief game. In order to fix fullscreening of Internet Explorer another patch is needed, I'm attaching window.IE.diff. Please let me know if any of the attached patches need further investigation or improvement.
Created attachment 70181 [details] [review] A fix for Thief game
Created attachment 70182 [details] [review] A fix for Internet Explorer
These both seem sane to me. There's a possible negative performance implication to having recalc_window_features there; it may mean that as a user is resizing a window, we keep setting X properties and things on the window. This may or may not actually matter though. It could be fixed if it happens by making recalc_window_features smarter.
Are you planning to commit the patches?
Elijah or Thomas probably will if they don't see a problem with them.
To avoid a performance hit probably it's better to move recalc_window_features to meta_window_client_message/_NET_WM_STATE_FULLSCREEN handling, some of client messages have that as well.
(In reply to comment #10) > To avoid a performance hit probably it's better to move recalc_window_features > to meta_window_client_message/_NET_WM_STATE_FULLSCREEN handling, some of > client messages have that as well. The places where recalc_window_features() are called in meta_window_client_message() is always _after_ making the relevant change to the window (because the change can alter further features of the window -- e.g. making the window be fullscreen disables shading and unshading). This proposal of yours would mean trying to do it _before_ as a check to say "do we really have the window features calculated correctly?" Calling it before the change seems to me like a workaround to a bug rather than a fix, and may likely result in other places needing the same kind of workaround. Also, it's worth noting that the fullscreen handling in meta_window_client_message() *does* call recalc_window_features() indirectly via the meta_window_(un)make_fullscreen functions. (In reply to comment #4) > That's an XRandr notification and it leads to a ConfigureNotify on the root > window. meta_screen_resize queues a move_resize event for all windows in > that case, so it looks reasonable to simply add recalc_window_features to > meta_window_move_resize_internal instead of exposing recalc_window_features > outside of window.c. Attached window.Thief.diff does that and fixes Thief > game. Why not just call recalc_window_features() on each window within meta_screen_resize(), as Havoc originally suggested? That way we avoid any possible performance problems by only calling it in this one case where it's needed (which also makes it clearer why the call is needed), as opposed to every path that calls meta_window_move_resize_internal(). > In order to fix fullscreening of Internet Explorer another patch is needed, > I'm attaching window.IE.diff. Looks good to me (other than a trivial update needed to apply to HEAD), as this is one of the two original suggested changes. So, I went ahead and committed it: 2006-08-04 Elijah Newren <newren gmail com> Patch from Dmitry Timoshkov to fix the heuristic for determining if windows can be made fullscreen (needed for WINE and possible also some legacy applications). Part of #346927.
> Why not just call recalc_window_features() on each window within > meta_screen_resize(), as Havoc originally suggested? That way we avoid any > possible performance problems by only calling it in this one case where it's > needed (which also makes it clearer why the call is needed), as opposed to > every path that calls meta_window_move_resize_internal(). I just overlooked that recalc_window_features() is already exported from window.c as meta_window_recalc_features(). Attached patch adds meta_window_recalc_features() to meta_screen_resize() and fixes the bug as well.
Created attachment 70243 [details] [review] Add meta_window_recalc_features() to meta_screen_resize()
2006-08-07 Elijah Newren <newren gmail com> * src/screen.c (meta_screen_resize_func): patch from Dmitry Timoshkov to make sure window features get recalculated when the screen is resized via XRandR. Part of #346927.
Since both of the patches have been committed, and they actually fix all known problems (for the Thief game, Firefox and IE), I think that the bug could be closed. If a new misbehaving application will be found it could be re-opened later.
I'm glad the WINE problems have been fixed (thanks for the patches!), but there was one other potential problem I noted while looking at this stuff. It might affect other apps and I think we should leave the bug opened until we've looked at it. That other item was: * Check window size only (not position) in src/stack.c:window_is_fullscreen_size()
I cannot fullscreen mplayer anymore in metacity 2.15.21. It still shows the panel (this is like bug 343115, except that that bug is about a specific app -- it now fails for many apps). Or just a nautilus/firefox window using the global metacity fullscreen keyboard shortcut. The firefox fullscreen shortcut (F11) does work though.
Olav: There were about 3-4 fullscreen bugs that I found in metacity (> 2.12.x, though one only affects 2.15.21 and newer). I've created a patch in bug 343115; could you test it out?
The remaining issues of this bug (pointed out in comments 16-18) have been addressed by the patch in bug 343115, so I'll go ahead and close. Thanks again for the fixes, Dmitry. :-)