Add exclusive fullscreen mode

[ghost]

Resolves #717. This uses CGConfigureDisplayWithDisplayMode on macOS for now, but I think what I'm going to end up doing is move to use enterFullScreenMode instead, since that seems to be the preferred way to do fullscreen on macOS. Implementing this on X11 should be fairly straight-forward. I'm not sure how to do this on Wayland however.

• Tested on all platforms changed
  • Windows
  • macOS
  • X11
  • Wayland
  • iOS
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

[elinorbgr]

I'm not sure how to do this on Wayland however.

That'll be pretty simple actually: the wayland server will never give any app direct control over the monitor, hence "exclusive fullscreen" does not exist, only regular fullscreen.

[Osspial]

Thanks for looking into this and implementing it!

I've tested this on my machine, and have left comments where I noticed bugs/potential bugs.

[Osspial]

I realize this specific feedback would've been better-placed in the original PR, but it's only occurred to me now: is it possible to add a function to Monitor to retrieve the currently-active video mode? That way, applications can launch in fullscreen with the sensible default of whatever fullscreen settings the monitor currently has, instead of having to guess which mode to use.

[ghost]

Shouldn't be a problem, but I'm inclined to do this in a separate pull request since I will have to implement it for all four platforms. I think the borderless mode would suffice for a sensible default in any case.

[ghost]

The macOS implementation is starting to look pretty good. The only remaining issue there is that upon exiting exclusive fullscreen and re-entering it, the video mode change seems to fail. Perhaps the list of video modes is invalidated upon capturing the display.

With these new fullscreen modes, I'm inclined to deprecate set_simple_fullscreen because that fullscreen behaviour seems to me very strange and unidiomatic, and most certainly not what the user is going to expect. I won't touch upon that in this pull request however, and will leave that open for discussion.

[ghost]

I've addressed most of your notes regarding the Windows implementation. I still have to fix the case where the video mode is changed while already in exclusive fullscreen mode. I realized that this hasn't been tested on the macOS implementation either, so I will have to create a test case for this. As for sorting the video modes, I've implemented this for Windows. It may be better to do this on the public API level (so that it's consistent across platforms and that the user can take advantage of the Ord trait implementation). I'll take a look at this tomorrow.

I've marked exclusive fullscreen unimplemented!() on iOS for now, as I can't seem to be able to detect external displays consistently with the simulator (and they report only a single video mode). I would have to test this on a physical device, which I would need to go out and buy some cables for..

Once I've worked out the remaining kinks, all that remains is the X11 implementation. I think I may have to end up doing a proper Linux install for this, as the virtual machine video driver only reports a single video mode. I'll also have to mark this unimplemented!() for Wayland. Or perhaps we want to modify the API so that set_fullscreen returns Result, with it always being an Err on Wayland?

While writing this, I was reminded that we could potentially have set_fullscreen called on a window when a different window may already be in fullscreen mode.. I think it might be good for us to track this and disallow set_fullscreen if there already is a fullscreen window. It'd be good to return an Err for this reason as well.

[felixrabe]

Q project people: Could we merge this in a few smaller commits than just one mega-commit if accepted?

Q everyone: Maybe split this up into smaller chunks merged in separate PRs or something? (If it is not too much trouble.)

Why I suggest this: ... Edit: Moved longer off-topic comment to issue #970 to discuss future PR merge strategies.

[ghost]

The macOS and Windows implementations have had all issues addressed and should be ready for final review. I will still have to do the implementation for X11.

I don't think splitting this into multiple pull requests makes any sense. This just adds friction for no benefit, as they will all be merged in anyway. I could squash these commits into a single commit per backend, and we could merge those commits as is without squashing again, but I don't think that's hardly worth the effort. I would suggest squash-merging this as usual in a single commit. If you wish to only inspect a single backend in the commit, you can do so easily by ignoring files in the other folders.

[ghost]

It doesn't look like core-foundation-rs is actively maintained, so instead of waiting for a review for servo/core-foundation-rs/pull/318, I might have to pull this functionality into our ffi.rs for macOS.

With the exception of that, this should be all done! Implemented on all supported platforms (i.e. macOS, Windows and X11), with the exception of iOS, but I feel like it's quite a niche use-case there anyway.

[ghost]

At the start sometimes it landed on the wrong window. Not sure if this is caused by this PR or #978. :/

I haven't touched the window positioning code, so this is most likely the same issue.

[Osspial]

I'm getting a protocol error upon running the fullscreen example on Weston in the master branch: zxdg_shell_v6@13: xdg_surface buffer does not match the configured state. It's not caused by the changes in this pull request.

My guess is that this should get fixed by #981.

[elinorbgr]

This is pretty likely that #981 will solve the protocol error indeed.

[ghost]

I will be leaving on a two week long trip in a few days and will be unable to make any further changes for quite some time, so I would like to get this merged before I leave. Do we have a consensus on the remaining review notes, or do you feel strongly that these ought to be changed? I will change these if that's what it takes to get this merged, but I still feel like long-term maintainability would suffer from these changes.

[Osspial]

I'm still running into the window in wrong position problem in my latest tests (see video here), so I suspect my initial diagnosis for that wasn't the correct one. If you're unable to reproduce this, I could look into it on my machine, then just push directly to the branch and merge it once I've figured out what's going on.

[ghost]

I'm not able to reproduce this on my system. I suspect that may have to do with the virtual layout in which your monitors are configured. Thank you for offering to look into this! It would be helpful. Feel free to commit directly to this branch. I also have an opportunity tomorrow to make changes, and maybe a few hours on Friday, but then I'll be unreachable until the end of July or so.

[Osspial]

Alright, this turned out to be one hell of a bug to track down.

It looks like the underlying cause was that sometimes, the call to ChangeDisplaySettingsExW would hang for such a long time that Windows would erroneously think our program had frozen, taking away control over the Winit window*. When that function finally returned, we'd still lack control over our window, causing SetWindowPos to fail and the window not getting moved to the proper position.

If you're interested in reproducing this bug, just apply this patch, run the fullscreen example in exclusive mode, and furiously click on the window before it properly gets fullscreened:

diff --git a/src/platform_impl/windows/window.rs b/src/platform_impl/windows/window.rs
index 756fc5ad..b8362e1f 100644
--- a/src/platform_impl/windows/window.rs
+++ b/src/platform_impl/windows/window.rs
@@ -480,6 +480,7 @@ impl Window {
                     let mut native_video_mode = video_mode.video_mode.native_video_mode.clone();

                     let res = unsafe {
+                        std::thread::sleep_ms(6000);
                         winuser::ChangeDisplaySettingsExW(
                             display_name.as_ptr(),
                             &mut native_video_mode,

The furious clicking is necessary to make Windows detect a freeze in our program. I'm not sure what steps you need to take to reproduce this without that patch, since this seems to be caused by various timing flukes outside of our control, but after inspecting my ETW logs I'm confident that this is the correct diagnosis.

Fortunately, the solution is pretty simple - just insert a call to PeekMessageW after ChangeDisplaySettingsExW to inform Windows that our program is still running properly. I've pushed that change to this branch.

^{* This is the same infrastructure that causes the window to gray out and (Not Responding) to appear in the title bar. Funnily enough, windbg disables that behavior in debugged applications, making this bug impossible to reproduce when running in a debugger. :/}

[ghost]

Thank you for tracking this down! I just got back home from my trip last evening. It's interesting that ChangeDisplaySettingsExW would take such a long time to complete. This didn't occur on my system. I wonder if that is dependent on the display and drivers being used.

[Osspial]

It's interesting that ChangeDisplaySettingsExW would take such a long time to complete. This didn't occur on my system. I wonder if that is dependent on the display and drivers being used.

@aleksijuvani I did a bit more investigation, and I don't think I was entirely correct with my original assessment! ChangeDisplaySettingsExW takes some time to execute, but that only makes the problem more visible instead of directly causing it - the root cause is that we ask for the monitor between creating the EventLoop and creating the Window. If the user takes a while to enter their desired monitor configuration, the program freeze handler kicks in and the SetWindowPos call doesn't work. Incidentally, that also means it's possible to reproduce this bug on master without exclusive fullscreen - just wait five or more seconds before entering the desired monitor configuration

That's why I was able to reproduce it, but you weren't - whenever the display selection interaction happened, I took a good amount of time to select the correct display and configuration, which took long enough that Windows thought the program had frozen. I'm assuming you didn't do that, so you never saw the bug.

[ghost]

I'm still not able to reproduce this even after waiting quite a bit before applying the desired display configuration. Which Windows version are you running on? I'm on Windows 10 version 1809. It seems odd to me that Windows would outright ignore API calls if it thinks that the window is frozen. Surely it should continue to allow the window to be manipulated?

[ghost]

I neglected to do this before, but I've just now also tested reproducing this by inserting the std::thread::sleep_ms call, and this does allow me to reproduce the issue. Either way, introducing the PeekMessageW call seems to fix the issue. Shall we go ahead with merging this?

[Osspial]

@aleksijuvani I've merged this now. Thank you so much for implementing this everywhere!