GNOME Bugzilla – Bug 691856
Add support for _NET_WM_FULLSCREEN_MONITORS
Last modified: 2013-01-25 12:21:35 UTC
Most window managers support _NET_WM_STATE_FULLSCREEN but (rightfully) restrain the fullscreen window to a single monitor in a multi-head setup. There are cases where an application may want a fullscreen window to span across multiple monitors (e.g. a movie or presentation application, or even an on-screen display window). EWMH provides a mechanism for this by the use of _NET_WM_FULLSCREEN_MONITORS [1] which is supported by most modern window managers (mutter/metacity, Compiz, kwin, xfwm4, etc.), but gtk+/gdk is lacking support for using this mechanism. [1] http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#id2579022
Created attachment 233595 [details] [review] PATCH 1/3: gdk: add gdk_window_set_fullscreen_on_monitors()
Created attachment 233596 [details] [review] PATCH 2/3: x11: implement gdk_x11_window_set_fullscreen_on_monitors() Note that on X11, the EWMH spec for _NET_WM_FULLSCREEN_MONITORS has 2 limitations: 1. This is not a hint, but a client message sent to the root window to specify the monitors of an already mapped, fullscreen window so this will work on mapped toplevels only. 2. The indices used to specify the edge windows are from the Xinerama list of monitors, which does not necessarily matches the GdkSCreen list of monitors, so the indices given need to be translated back into the position of the given monitors in the Xinerama list of monitors (eeeks!)
Created attachment 233597 [details] [review] PATCH 3/3: tests: add a test for gdk_window_set_fullscreen_on_monitors()
Created attachment 233752 [details] [review] [PATCH 1/3] gdk: add gdk_window_set_fullscreen_mode() Reworked the patches following our discussion on irc. - Define an enum GdkFullscreenMode as follow: GDK_FULLSCREEN_ON_CURRENT_MONITOR: Fullscreen on current monitor only. GDK_FULLSCREEN_ON_ALL_MONITORS: Span across all monitors when fullscreen to indicates which monitor (in a multi-head setup) a window should span over when in fullscreen mode. - Added an new API gdk_window_set_fullscreen_mode() so set the fullscreen mode as defined above.
Created attachment 233753 [details] [review] [PATCH 2/3] x11: implement gdk_window_set_fullscreen_mode() in X11 backend
Created attachment 233754 [details] [review] [PATCH 3/3] tests: add a test for fullscreen mode
Created attachment 233997 [details] [review] patch 1/3: gdk: add gdk_window_set_fullscreen_mode() and gdk_window_get_fullscreen_mode() Fullscreen mode is now stored in GdkWindow private so that the API can implement a setter and a getter for fullscreen mode.
Created attachment 233998 [details] [review] patch 2/3: x11: implement gdk_window_apply_fullscreen_mode() Since fullscreen mode is no stored in the GdkWindow struct, we can safely apply the mode on unmap/remap so that the behavior is now consistent.
Created attachment 233999 [details] [review] patch 3/3: tests: add a test for fullscreen mode
Review of attachment 233997 [details] [review]: I'm not entirely sure that 'fullscreen on all monitors' can be implemented by the various backends, outside of the x11 one. ::: gdk/gdkwindow.c @@ +10722,3 @@ + + window->fullscreen_mode = mode; + GDK_WINDOW_IMPL_GET_CLASS (window->impl)->apply_fullscreen_mode (window); you should add a NULL check, in case this function cannot be implemented by any other backend. also, since patch sets should not break between commits, and you introduce the x11 implementation later on. @@ +10740,3 @@ + g_return_val_if_fail (GDK_IS_WINDOW (window), GDK_FULLSCREEN_ON_CURRENT_MONITOR); + + return (window->fullscreen_mode); no need to use the parentheses here. ::: gdk/gdkwindow.h @@ +323,3 @@ + * + * Indicates which monitor (in a multi-head setup) a window should span over + * when in fullscreen mode. missing Since: annotation. @@ +788,3 @@ void gdk_window_unmaximize (GdkWindow *window); void gdk_window_fullscreen (GdkWindow *window); +void gdk_window_set_fullscreen_mode (GdkWindow *window, missing GDK_AVAILABLE_IN_* annotation.
Review of attachment 233998 [details] [review]: apart from "ugh, xinerama", my question is: what happens if the XINERAMA extension is not present, but the XRandR one is? does XRandR fulfil the XINERAMA contract as well? given that XINERAMA is only used on legacy/crappy X servers (*cough* Solaris *cough*) or by proprietary X11 drivers (*cough* nvidia *cough*), or by crappy multi-output set ups, what's our story for everybody doing things correctly? ::: gdk/x11/gdkscreen-x11.c @@ +885,3 @@ + +gint +_gdk_x11_screen_get_xinerama_indice (GdkScreen *screen, get_xinerama_index(). ::: gdk/x11/gdkscreen-x11.h @@ +114,3 @@ void _gdk_x11_screen_process_owner_change (GdkScreen *screen, XEvent *event); +gint _gdk_x11_screen_get_xinerama_indice (GdkScreen *screen, get_xinerama_index().
Review of attachment 233999 [details] [review]: looks okay to me, obviously blocking on the previous two patches.
(In reply to comment #11) > apart from "ugh, xinerama", my question is: what happens if the XINERAMA > extension is not present, but the XRandR one is? does XRandR fulfil the > XINERAMA contract as well? Using Xinerama is from the spec itself [1] which states the "indices are from the set returned by the Xinerama extension" It's just a matter of an agreement between the application and the window manager to identify the monitors (but the choice of Xinerama for that, is well, not the best imho - still it's the spec as it is for now). My take is that if we were to redo that spec now, we would most likely not decide on using indices (for a start) from the Xinerama (definitely, not Xinerama) list of monitors. > given that XINERAMA is only used on legacy/crappy X > servers (*cough* Solaris *cough*) or by proprietary X11 drivers (*cough* nvidia > *cough*), You can still use the Xinerama API even on modern Xservers, and you get the list of monitors. What matters is that the indices match between the Window manager and the application, and mutter/metacity use the xinerama indices [2] as returned by Xinerama as well [3]. > or by crappy multi-output set ups, what's our story for everybody > doing things correctly? If Xinerama is not present, the behavior would be undefined in that case so (ie if XineramaIsActive() is FALSE) the code would return without doing anything. [1] http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#id2579022 [2] http://git.gnome.org/browse/mutter/tree/src/core/constraints.c#n414 [3] http://git.gnome.org/browse/mutter/tree/src/core/screen.c#n484
Created attachment 234004 [details] [review] patch 1/3: gdk: add gdk_window_set_fullscreen_mode() and gdk_window_get_fullscreen_mode() Updated patch after review comment #10
Created attachment 234006 [details] [review] patch 2/3: x11: implement gdk_window_apply_fullscreen_mode() Updated patch after comment #11 > +gint _gdk_x11_screen_get_xinerama_indice (GdkScreen *screen, > > get_xinerama_index(). I wasn't sure if you meant s/_gdk_x11_screen_get_xinerama_indice/get_xinerama_index/ here or just s/_gdk_x11_screen_get_xinerama_indice/_gdk_x11_screen_get_xinerama_index/ so I chose the latter (since all other exported funcs are prefixed with _gdk_x11_screen* in gdkscreen-x11.h) > apart from "ugh, xinerama", my question is: what happens if the XINERAMA > extension is not present, but the XRandR one is? To clarify what I wrote in comment #13 if Xinerama is not present the xinerama_matches hash would be empty and _gdk_x11_screen_get_xinerama_index() would return -1 so that g_return_if_fail (xinerama_monitor_* >= 0); would catch that in gdk_x11_window_apply_fullscreen_mode() I wonder, is it OK to use g_return_if_fail() in this case here?
(In reply to comment #13) > (In reply to comment #11) > > apart from "ugh, xinerama", my question is: what happens if the XINERAMA > > extension is not present, but the XRandR one is? does XRandR fulfil the > > XINERAMA contract as well? > > Using Xinerama is from the spec itself [1] which states the "indices are from > the set returned by the Xinerama extension" yes, I know. but we can either amend the spec, or write a better spec. > My take is that if we were to redo that spec now, we would most likely not > decide on using indices (for a start) from the Xinerama (definitely, not > Xinerama) list of monitors. that would be a good point to have the spec amended, and use proper geometry, instead of indices that can be reassigned. > > or by crappy multi-output set ups, what's our story for everybody > > doing things correctly? > > If Xinerama is not present, the behavior would be undefined in that case so (ie > if XineramaIsActive() is FALSE) the code would return without doing anything. I guess this would be a good thing to note in the documentation.
(In reply to comment #15) > Created an attachment (id=234006) [details] [review] > patch 2 [details]/3: x11: implement gdk_window_apply_fullscreen_mode() > > Updated patch after comment #11 > > > +gint _gdk_x11_screen_get_xinerama_indice (GdkScreen *screen, > > > > get_xinerama_index(). > > I wasn't sure if you meant > s/_gdk_x11_screen_get_xinerama_indice/get_xinerama_index/ here or just > s/_gdk_x11_screen_get_xinerama_indice/_gdk_x11_screen_get_xinerama_index/ so I > chose the latter (since all other exported funcs are prefixed with > _gdk_x11_screen* in gdkscreen-x11.h) sorry, I was a bit too cryptic. I meant that "get_xinerama_indice" is not English. :-) the naming should be: _gdk_x11_screen_get_xinerama_index (...) > I wonder, is it OK to use g_return_if_fail() in this case here? internal functions should not use g_return_if_fail(): they should either assert() if the internal state is mandatory, or warn and return. g_return_if_fail() can be disabled (usually in stable releases) and compiled out, so it's not a good thing to use to ensure internal state. assertions can be disabled as well, but it's rare that people actually do that.
(In reply to comment #17) > (In reply to comment #15) > > I wonder, is it OK to use g_return_if_fail() in this case here? > > internal functions should not use g_return_if_fail(): they should either > assert() if the internal state is mandatory, or warn and return. > I like using g_return_if_fail() in internal APIs because it means that (a) I get the same behavior as in public APIs and (b) should I ever choose to make that function a public API, I don't have to go and change everything. And to be absolutely clear about this: g_return_if_fail() is essentially an assertion. Hitting it is a programmer error and it is expected that the application/library can choose to crash instead.
Created attachment 234007 [details] [review] patch 1/3: gdk: add gdk_window_set_fullscreen_mode() and gdk_window_get_fullscreen_mode() (In reply to comment #16) > [...] > that would be a good point to have the spec amended, and use proper geometry, > instead of indices that can be reassigned. Yes, but that would be outside of the scope of this bug & patchset. > > > or by crappy multi-output set ups, what's our story for everybody > > > doing things correctly? > > > > If Xinerama is not present, the behavior would be undefined in that case so (ie > > if XineramaIsActive() is FALSE) the code would return without doing anything. > > I guess this would be a good thing to note in the documentation. Added. New patch attached.
Created attachment 234008 [details] [review] patch 2/3: x11: implement gdk_window_apply_fullscreen_mode() (In reply to comment #17) > [...] > > I wonder, is it OK to use g_return_if_fail() in this case here? > > internal functions should not use g_return_if_fail(): they should either > assert() if the internal state is mandatory, or warn and return. > g_return_if_fail() can be disabled (usually in stable releases) and compiled > out, so it's not a good thing to use to ensure internal state. assertions can > be disabled as well, but it's rare that people actually do that. Fixed. New patch attached.
Wait, patch #234007 breaks gtk layouts. Working on it.
Created attachment 234115 [details] [review] patch 1/3: gdk: add gdk_window_set_fullscreen_mode() and gdk_window_get_fullscreen_mode() - Forgot to add gdk_window_get_fullscreen_mode() in gdkwindow.h - Set the mode only if different from the current mode, that will help keeping consistent with current implementation (ie as long as gdk_window_set_fullscreen_mode() is not used to change the mode, there should be no difference).
Created attachment 234117 [details] [review] patch 2/3: x11: implement gdk_window_apply_fullscreen_mode() Use largest values to unset _NET_WM_FULLSCREEN_MONITORS. While this is not a documented mechanism, there is actually no documented way to unset _NET_WM_FULLSCREEN_MONITORS once set and this works with most window managers out there, successfully tested on mutter/metacity, Compiz and xfwm4. Notable exception is KWin but _NET_WM_FULLSCREEN_MONITORS does not seem to work well on KWin (at least in my own tests here) so that doesn't break or make things worse on KWin/KDE either. Beside the code itself will not use _NET_WM_FULLSCREEN_MONITORS as long as the mode remains GDK_FULLSCREEN_ON_CURRENT_MONITOR (the default) so this should not break backward compat between GTK+ apps and KWin (thinking of the Firefox and google-chrome users under KDE here).
Created attachment 234118 [details] [review] patch 3/3: tests: add a test for fullscreen mode Clean-up a couple of extraneous white spaces.
Created attachment 234219 [details] [review] patch 2/3: x11: implement gdk_window_apply_fullscreen_mode() Sends the "_NET_WM_FULLSCREEN_MONITORS" message when changing the window state to fullscreen, so that the monitor indices remain accurate even if the XRandR layout has changed since the fullscreen mode was set.
Created attachment 234368 [details] [review] patch 2/3: x11: implement gdk_window_apply_fullscreen_mode() Update the comment about the undocumented, non-standard use of invalid monitor indices to reset the "_NET_WM_FULLSCREEN_MONITORS" property. I think it remains the most reliable way to achieve this. It was suggested to unmap/remap the window to clear the property, but that causes all sort of other problems as we want to remove just that property and keep all the others. Beside calling gdk_window_x11_hide()/gdk_window_x11_show() quickly is a row ends up with a all-blank, empty gtk window which is not even fullscreen anymore. Using invalid indices values to clear the property works fine with the test application, as successfully tested on mutter/metacity, kwin, compiz and xfwm4. Fluxbox does not seem support "_NET_WM_FULLSCREEN_MONITORS" so this is irrelevant with Fluxbox. At last, this (non documented) mechanism is unlikely to be an issue anyway in most of the cases as it's used only for transitionning back from "all monitors" to "current monitor" mode. Applications who don't change the default mode won't ever trigger this mechanism.
All 3 patches committed and pushed to git master, as instructed by Benjamin on irc.