Move fullscreen modes to not touch physical resolutions #270

pedrocr · 2017-08-29T16:36:56Z

The previous XF86 resolution switching was broken and everything seems to have moved on to xrandr. Use that instead while cleaning up the code a bit as well. Right now everything seems to work fine. The only bug I've noticed is that at least in Unity after the resolution change the top bar is still there. Doing an alt-tab from and to the app again makes _NET_WM_STATE_FULLSCREEN take effect. Don't know how to fix that right now but this is already much better than what was before.

Ralith · 2017-08-29T16:53:44Z

Has this been tested on a multi-monitor system? XDisplayWidth/Height are definitely the wrong function here, and I suspect XRRSizes is inappropriate as well. Under XRandR, the root window spans all outputs; there is only one native X11 "screen" no matter how many monitors you have. The xrandr command line utility should contain examples of correct use, though it's not easy to make sense of.

In particular, we should be working in terms of XRandR "outputs," not X11 screens. MonitorId should be adjusted to account for this. XRRGetOutputInfo provides access to the current set of output states.

pedrocr · 2017-08-29T18:51:43Z

Haven't tested on multiscreen yet but I have a few external screens I can test this with. I assumed MonitorId was all we needed given all the functions take that as input but maybe it's not so simple.

pedrocr · 2017-08-30T23:01:59Z

This now has the basics needed for proper multiscreen and the resolution changes seem to work. I still need to place the window in the correct place and figure out why in some cases it will segfault.

pedrocr · 2017-08-31T11:29:33Z

@tomaka after discussions on IRC with @Ralith and looking at Wayland it seems much cleaner to just never change the physical hardware resolution. With LCD panels there's no actual resolution change and GPU scaling is cheap enough that apps should just scale to whatever the physical resolution is and not do any mode switching. So the proposed semantics are:

FullScreen::None: make the window normal with decorations at whatever size has been set
FullScreen::CurrentMonitor: make it fullscreen in whatever monitor it happens to be in right now
FullScreen::SpecificMonitor(MonitorId): make it fullscreen in a specific monitor specified by the app

The X11 code for this ends up very clean and should even make it easy to support weird setups like videowalls and MST monitors.

pedrocr · 2017-08-31T11:51:13Z

Implemented XRandR 1.5 now so even MST/videowalls should work fine.

tomaka · 2017-08-31T15:12:54Z

src/lib.rs

    None,
-    Windowed,
-    Exclusive(MonitorId),
+    /// Set fullscreen in whatever monitor the window happens to be right now


I think that's a bad design. The user should just have a way to retrieve the monitor of the window instead.

In that case we can just get rid of the enum and do .set_/.with_fullscreen(Option<MonitorId>) and do a Window::get_monitor(). How does that sound?

I guess it's good.

pedrocr · 2017-09-03T14:55:30Z

I've implemented the interface as discussed but I'm not too sure of it. w.set_fullscreen(Some(w.get_current_monitor())) ends up much slower than w.set_fullscreen(FullScreen::CurrentMonitor) because the first, at least on X11, needs to enumerate monitors and check the window position and dimensions against it. The second, again on X11, just needs to set a window manager hint and it does everything for us, possibly with more info than we have.

pedrocr · 2017-09-03T18:01:40Z

The basics should be done and the remaining issues are:

Not all platforms implement all things. Implementations for get_current_monitor() and set_fullscreen() are missing in most non-X11 platforms. The code compiles and is no worse than before though.
with_fullscreen() implementations (particularly Windows) should stop fiddling with resolutions and just set the window fullscreen at the current resolution in the given monitor
It may make sense to go back to .set_fullscreen(FullScreen::None/CurrentMonitor/Monitor<MonitorId>) as at least on X11 .set_fullscreen(FullScreen::CurrentMonitor) can be implemented much faster than .set_fullscreen(Some(window.get_current_monitor()))

pedrocr · 2017-09-05T22:23:50Z

@tomaka From my point of view this is ready to merge. Let me know if you want something else.

tomaka · 2017-09-06T06:45:47Z

src/platform/linux/x11/xdisplay.rs

-            xinput2: xinput2,
-            xlib_xcb: xlib_xcb,
-            display: display,
+            xlib,


Let's keep compatibility with older Rust versions if we can. Please don't use that new syntax.

tomaka · 2017-09-06T06:46:31Z

src/platform/windows/window.rs

    }

    #[inline]
-    pub fn set_fullscreen(&self, state: FullScreenState) {
+    pub fn set_fullscreen(&self, _monitor: Option<RootMonitorId>) {


Should contain unimplemented!().

tomaka · 2017-09-06T06:46:34Z

src/platform/windows/window.rs

@@ -280,11 +279,16 @@ impl Window {
    }

    #[inline]
-    pub fn set_maximized(&self, maximized: bool) {
+    pub fn set_maximized(&self, _maximized: bool) {


Should contain unimplemented!().

I didn't set these as unimplemented because they're safe as no-ops. No need to panic the app just because we can't set it as maximized/fullscreen.

Using no-ops means that these functions will likely be left unimplemented for years.

Ok, done now

tomaka · 2017-09-06T06:47:14Z

src/platform/macos/window.rs

    }

    #[inline]
-    pub fn set_fullscreen(&self, state: FullScreenState) {
+    pub fn set_fullscreen(&self, _monitor: Option<RootMonitorId>) {


Should contain unimplemented!().

tomaka · 2017-09-06T06:47:16Z

src/platform/macos/window.rs

@@ -636,11 +636,16 @@ impl Window {
    }

    #[inline]
-    pub fn set_maximized(&self, maximized: bool) {
+    pub fn set_maximized(&self, _maximized: bool) {


Should contain unimplemented!().

tomaka · 2017-09-06T06:47:59Z

src/platform/linux/x11/monitor.rs


 use super::XConnection;

 #[derive(Clone)]
-pub struct MonitorId(pub Arc<XConnection>, pub u32);
+pub struct MonitorId {


Wrong indentation

tomaka · 2017-09-06T06:48:51Z

src/platform/android/mod.rs

+
+    #[inline]
+    pub fn get_current_monitor(&self) -> RootMonitorId {
+        unimplemented!()


Android provides only one dummy monitor. This function should return RootMonitorId(MonitorId).

tomaka · 2017-09-06T06:48:58Z

src/platform/ios/mod.rs

+
+    #[inline]
+    pub fn get_current_monitor(&self) -> RootMonitorId {
+        unimplemented!()


iOS provides only one dummy monitor. This function should return RootMonitorId(MonitorId).

tomaka · 2017-09-06T06:49:07Z

src/platform/ios/mod.rs

+
+    #[inline]
+    pub fn get_position(&self) -> (u32, u32) {
+        unimplemented!()


tomaka · 2017-09-06T06:49:11Z

src/platform/android/mod.rs

+
+    #[inline]
+    pub fn get_position(&self) -> (u32, u32) {
+        unimplemented!()


The previous XF86 resolution switching was broken and everything seems to have moved on to xrandr. Use that instead while cleaning up the code a bit as well.

Wayland has made the decision that apps shouldn't change screen resolutions and just take the screens as they've been setup. In the modern world where GPU scaling is cheap and LCD panels are scaling anyway it makes no sense to make "physical" resolution changes when software should be taking care of it. This massively simplifies the code and makes it easier to extend to more niche setups like MST and videowalls.

Moving to just having two states None and Some(MonitorId) and then being able to set full screen in the current monitor with something like: window.set_fullscreen(Some(window.current_monitor()));

Do it by iterating over the available monitors and finding which has the biggest overlap with the window. For this MonitorId needs a new get_position() that needs to be implemented for all platforms.

Since we're no longer using XF86 there's no need to keep the package around for CI.

On Android and iOS we can assume single screen apps that are already fullscreen and maximized so there are a few methods that are implemented by just returning a fixed value or not doing anything.

These would be safe as no-ops but we should make it explicit so there is more of an incentive to actually implement them.

tomaka · 2017-09-07T08:34:22Z

Thanks for all the trouble

pedrocr · 2017-09-07T09:14:24Z

No trouble, hope it helps. Do ping me if you need help with any of this code or with anything on X11.

pedrocr force-pushed the maximization branch 2 times, most recently from bbb92fd to c4bf208 Compare August 29, 2017 16:43

pedrocr force-pushed the maximization branch 2 times, most recently from 54ea27a to a0d929f Compare August 30, 2017 23:35

pedrocr changed the title ~~Fix X11 screen resolution change using XrandR~~ Move fullscreen modes to not touch physical resolutions Aug 31, 2017

tomaka suggested changes Aug 31, 2017

View reviewed changes

pedrocr force-pushed the maximization branch 2 times, most recently from 6b7bd9a to fbbd347 Compare September 3, 2017 15:47

pedrocr force-pushed the maximization branch 7 times, most recently from ac29dd8 to c824e14 Compare September 5, 2017 20:27

tomaka suggested changes Sep 6, 2017

View reviewed changes

pedrocr force-pushed the maximization branch from 20be37c to 3b422c1 Compare September 6, 2017 11:38

tomaka approved these changes Sep 6, 2017

View reviewed changes

pedrocr force-pushed the maximization branch from 3b422c1 to 4198011 Compare September 6, 2017 11:48

pedrocr added 3 commits September 6, 2017 17:37

Fix X11 screen resolution change using XrandR

ac24c21

The previous XF86 resolution switching was broken and everything seems to have moved on to xrandr. Use that instead while cleaning up the code a bit as well.

Use XRandR for actual multiscreen support in X11

3093fb9

Use actual monitor names in X11

a254e8a

pedrocr added 17 commits September 6, 2017 17:37

Get rid of ptr::read usage in X11

a5a4048

Use a bog standard Vec instead of VecDeque

d78eb9c

Rename fullscreen options to match new semantics

3cc9027

Implement XRandR 1.5 support

0c5616d

Get rid of the FullScreen enum

2a2231e

Moving to just having two states None and Some(MonitorId) and then being able to set full screen in the current monitor with something like: window.set_fullscreen(Some(window.current_monitor()));

Implement Window::get_current_monitor()

7e057e1

Do it by iterating over the available monitors and finding which has the biggest overlap with the window. For this MonitorId needs a new get_position() that needs to be implemented for all platforms.

Add unimplemented get_position() to all MonitorId

c26a54f

Make get_current_monitor() platform specific

16ba280

Add unimplemented get_current_monitor() to all

bd283b8

Implement proper primary monitor selection in X11

7e495f4

Shut up some warnings

4bc6ef5

Remove libxxf86vm package from travis

3f42a09

Since we're no longer using XF86 there's no need to keep the package around for CI.

Don't use new struct syntax

97198ad

Fix indentation

eed355b

Adjust Android/iOS fullscreen/maximized

02d331b

On Android and iOS we can assume single screen apps that are already fullscreen and maximized so there are a few methods that are implemented by just returning a fixed value or not doing anything.

Mark OSX/Win fullscreen/maximized unimplemented()!

47a65db

These would be safe as no-ops but we should make it explicit so there is more of an incentive to actually implement them.

pedrocr force-pushed the maximization branch from 4198011 to 47a65db Compare September 6, 2017 16:41

tomaka merged commit 59c33d2 into rust-windowing:master Sep 7, 2017

Ralith mentioned this pull request Sep 21, 2017

Dependency on XFree86-VidModeExtension fails with remote X, Wayland #291

Closed

This was referenced Apr 11, 2018

Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for windows #457

Merged

Expected behavior of fullscreen and maximized in MacOS #460

Closed

This was referenced May 16, 2018

Fullscreen support on ARM Linux (Ubuntu) rust-windowing/glutin#829

Closed

Linux Fullscreen fails with "Could not find a suitable graphics mode" rust-windowing/glutin#582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move fullscreen modes to not touch physical resolutions #270

Move fullscreen modes to not touch physical resolutions #270

pedrocr commented Aug 29, 2017

Ralith commented Aug 29, 2017

pedrocr commented Aug 29, 2017

pedrocr commented Aug 30, 2017

pedrocr commented Aug 31, 2017

pedrocr commented Aug 31, 2017

tomaka Aug 31, 2017

pedrocr Aug 31, 2017

tomaka Sep 1, 2017

pedrocr commented Sep 3, 2017

pedrocr commented Sep 3, 2017

pedrocr commented Sep 5, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

pedrocr Sep 6, 2017

tomaka Sep 6, 2017

pedrocr Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka Sep 6, 2017

tomaka commented Sep 7, 2017

pedrocr commented Sep 7, 2017

Move fullscreen modes to not touch physical resolutions #270

Move fullscreen modes to not touch physical resolutions #270

Conversation

pedrocr commented Aug 29, 2017

Ralith commented Aug 29, 2017

pedrocr commented Aug 29, 2017

pedrocr commented Aug 30, 2017

pedrocr commented Aug 31, 2017

pedrocr commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrocr commented Sep 3, 2017

pedrocr commented Sep 3, 2017

pedrocr commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka commented Sep 7, 2017

pedrocr commented Sep 7, 2017