After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 346927 - Heuristics for fullscreening "legacy" apps
Heuristics for fullscreening "legacy" apps
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: general
trunk
Other Linux
: Normal normal
: ---
Assigned To: Metacity maintainers list
Metacity maintainers list
Depends on:
Blocks:
 
 
Reported: 2006-07-07 17:08 UTC by Elijah Newren
Modified: 2006-08-21 18:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The patch which fixes the problem for me. (603 bytes, patch)
2006-07-27 13:48 UTC, Dmitry Timoshkov
needs-work Details | Review
A fix for Thief game (367 bytes, patch)
2006-08-04 06:00 UTC, Dmitry Timoshkov
needs-work Details | Review
A fix for Internet Explorer (584 bytes, patch)
2006-08-04 06:01 UTC, Dmitry Timoshkov
committed Details | Review
Add meta_window_recalc_features() to meta_screen_resize() (312 bytes, patch)
2006-08-05 05:31 UTC, Dmitry Timoshkov
committed Details | Review

Description Elijah Newren 2006-07-07 17:08:30 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()
Comment 1 Dmitry Timoshkov 2006-07-27 13:48:27 UTC
Created attachment 69729 [details] [review]
The patch which fixes the problem for me.
Comment 2 Dmitry Timoshkov 2006-07-27 13:49:22 UTC
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.
Comment 3 Havoc Pennington 2006-07-27 15:26:48 UTC
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?

Comment 4 Dmitry Timoshkov 2006-08-04 05:59:22 UTC
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.
Comment 5 Dmitry Timoshkov 2006-08-04 06:00:45 UTC
Created attachment 70181 [details] [review]
A fix for Thief game
Comment 6 Dmitry Timoshkov 2006-08-04 06:01:31 UTC
Created attachment 70182 [details] [review]
A fix for Internet Explorer
Comment 7 Havoc Pennington 2006-08-04 14:02:12 UTC
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.

Comment 8 Dmitry Timoshkov 2006-08-04 14:55:53 UTC
Are you planning to commit the patches?
Comment 9 Havoc Pennington 2006-08-04 15:17:52 UTC
Elijah or Thomas probably will if they don't see a problem with them.
Comment 10 Dmitry Timoshkov 2006-08-04 15:26:52 UTC
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.
Comment 11 Elijah Newren 2006-08-04 22:37:47 UTC
(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.
Comment 12 Dmitry Timoshkov 2006-08-05 05:30:25 UTC
> 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.
Comment 13 Dmitry Timoshkov 2006-08-05 05:31:08 UTC
Created attachment 70243 [details] [review]
Add meta_window_recalc_features() to meta_screen_resize()
Comment 14 Elijah Newren 2006-08-07 17:17:14 UTC
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.
Comment 15 Dmitry Timoshkov 2006-08-08 03:08:32 UTC
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.
Comment 16 Elijah Newren 2006-08-08 03:50:34 UTC
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()
Comment 17 Olav Vitters 2006-08-19 10:29:56 UTC
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.
Comment 18 Elijah Newren 2006-08-20 02:21:24 UTC
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?
Comment 19 Elijah Newren 2006-08-21 18:11:35 UTC
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.  :-)