Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for MacOS

[edwin0cheng]

This PR implemented following functions in MacOS:

• Window::get_current_monitor
• Window::set_fullscreen
• Window::set_maximized
• Window::set_decorations
• WindowBuilder::with_maximized

And the maximized and fullscreen behaviors are following safari's conventions. (See #460).

This PR should f-ixed #300, #72, #66, #129, and Part of #252

[francesca64]

I installed High Sierra in preparation for this! I'll review it as soon as I get a chance.

[edwin0cheng]

This implementation is quite straight forward, which is just calling NSWindow::toggleFullscreen: for fullscreen and NSWindow::zoom for maximized.

However, set_decoration and with_fullscreen are little bit tricky:

• Calling toggleFullscreen: and zoom: on a non-resizable window (i.e set_decoration(false)) in MacOS is no-op by default, i have to do it manually (use setFrame for zoom) or set it to resizable tempority and restore it back. (for toggleFullScreen
• Normally, the implementation of with_fullscreen is just calling toggleFullscreen after window initialization. However, when a winit app is launched from a fullscreen app (e.g. from a FullScreen VS Code Terminal), it creates a new virtual desktop and a transition animation, which take one sec and cannot be disabled. In this animation time, all toggleFullscreen events will be failed. The PR just makes another toggleFullscreen call with delay again until it succeeds.

[francesca64]

• When using with_fullscreen, the window briefly appears at normal size before becoming fullscreen. Setting fullscreen prior to making the window key fixes this.
• This currently disregards whatever monitor is passed for setting fullscreen, always using whichever monitor has the keyboard focus. enterFullScreenMode should work for this, but it appears to be absent from core-foundation.
• Similarly, get_current_monitor always returns the currently focused monitor, rather than the monitor the window is on. CGGetDisplaysWithRect could be used to find the monitor that the majority of the window is on.
• set_decorations doesn't respect the more granular style settings provided by WindowBuilderExt. The best way to handle this isn't obvious, as "decorated" isn't a binary state in this case.
• set_fullscreen similarly has no awareness of the extra attributes from WindowBuilderExt, i.e. using with_titlebar_hidden makes entering fullscreen impossible.
• The titlebar popping in when switching to fullscreen from an undecorated window can be made less jarring by setting the titlebar to transparent, and the title and buttons to hidden, though that small visual improvement might not be worth the added complexity.
• Using the button to set maximization causes the wrong maximization state to be restored when exiting fullscreen.

servo/core-foundation-rs#204 will give us some more options (literally), though nothing that it looks like we presently need.

[francesca64]

It would be good to have a comment explaining what handle_with_fullscreen represents.

[francesca64]

This line is too long.

[francesca64]

When handle_with_fullscreen is false, this should restore the window state to what it was prior to attempting to enter fullscreen.

(Also, none of the other callbacks explicitly specified "C". It doesn't make a difference, since C is the default, but consistency is nice.)

[francesca64]

There should be a space after the comma (also applies to your cell import).

[francesca64]

Is it safe to be using cells in async callbacks?

[edwin0cheng]

It should be not used in an async callback, or do i miss something??

[francesca64]

Sorry, I misunderstood how the delegate methods work.

[francesca64]

When the two monitors aren't the same, this should be unimplemented!().

[francesca64]

None of these clones are necessary.

[francesca64]

Indentation

[edwin0cheng]

This currently disregards whatever monitor is passed for setting fullscreen, always using whichever monitor has the keyboard focus. enterFullScreenMode should work for this, but it appears to be absent from core-foundation.

We can use msg_send! macro to send the selector to use it, i will give it a try. And we could expose the fullscreen options in WindowExt and WindowBuilderExt, but i think i would recommend to create another PR to address it.

Similarly, get_current_monitor always returns the currently focused monitor, rather than the monitor the window is on. CGGetDisplaysWithRect could be used to find the monitor that the majority of the window is on.

I want to use this but it is absent in current CoreFoundation binding which is c api which i cannot use msg_send!.

The titlebar popping in when switching to fullscreen from an undecorated window can be made less jarring by setting the titlebar to transparent, and the title and buttons to hidden, though that small visual improvement might not be worth the added complexity.

Indeed, we would make another PR to address this too.

set_decorations doesn't respect the more granular style settings provided by WindowBuilderExt. The best way to handle this isn't obvious, as "decorated" isn't a binary state in this case.

We do have some comment about this issue in WindowExt, but i don't know it is enough to indicate that.

[edwin0cheng]

Ok, seem to be we cannot use enterFullScreenMode as all keys of NSViewFullScreenModeOptionKey are not bound in rust cocoa library.

[edwin0cheng]

• When using with_fullscreen, the window briefly appears at normal size before becoming fullscreen. Setting fullscreen prior to making the window key fixes this.
• set_fullscreen similarly has no awareness of the extra attributes from WindowBuilderExt, i.e. using with_titlebar_hidden makes entering fullscreen impossible.
• Using the button to set maximization causes the wrong maximization state to be restored when exiting fullscreen.

These bugs should be fixed.

[edwin0cheng]

@francesca64 That's it, except for the points i mentioned in previous comments, we need to wait upstream cocoa crate to add corresponding binding. And i think we could add another PRs for these improvement.

[francesca64]

zie -> size

[francesca64]

} else {

[francesca64]

Thanks, and addressing the rest in a later PR seems reasonable. Two more nitpicks and we should be good to merge.

[edwin0cheng]

Fixed, and thank you so much.

[francesca64]

Are you noticing that when using with_fullscreen, it now takes two tries to succeed in exiting fullscreen? Things worked fine as of 8842875, and the problem starts in 1a808b0.

[edwin0cheng]

No, i just tested it and it only take 1 try. Could you mind to detail step to reproduce it?

[francesca64]

Okay, I've tried a bit more, and I think I know what's happening. I run the fullscreen example, press F twice, then quit, and I get this event log:

WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: Focused(true) }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: HiDPIFactorChanged(1.0) }
WindowEvent { window_id: _, event: ReceivedCharacter('f') }
WindowEvent { window_id: _, event: KeyboardInput { device_id: _, input: KeyboardInput { scancode: 3, state: Released, virtual_keycode: Some(F), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }
WindowEvent { window_id: _, event: KeyboardInput { device_id: _, input: KeyboardInput { scancode: 3, state: Pressed, virtual_keycode: Some(F), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }
WindowEvent { window_id: _, event: ReceivedCharacter('f') }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: HiDPIFactorChanged(1.0) }
WindowEvent { window_id: _, event: Resized(2560, 1315) }
WindowEvent { window_id: _, event: ReceivedCharacter('\u{1b}') }
WindowEvent { window_id: _, event: KeyboardInput { device_id: _, input: KeyboardInput { scancode: 53, state: Released, virtual_keycode: Some(Escape), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }

As you can see, the first F press never generates a Pressed event! Besides pressing F a second time to get it to work, I can also just click and then press F - the key is that the first event is being ignored. If you've been keeping up on the recent PRs, this sounds like the result of #466. If I drop that commit, then I get the expected event log:

WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: Focused(true) }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: HiDPIFactorChanged(1.0) }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: HiDPIFactorChanged(1.0) }
WindowEvent { window_id: _, event: KeyboardInput { device_id: _, input: KeyboardInput { scancode: 3, state: Pressed, virtual_keycode: Some(F), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }
WindowEvent { window_id: _, event: ReceivedCharacter('f') }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: Resized(2560, 1440) }
WindowEvent { window_id: _, event: HiDPIFactorChanged(1.0) }
WindowEvent { window_id: _, event: KeyboardInput { device_id: _, input: KeyboardInput { scancode: 3, state: Released, virtual_keycode: Some(F), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }
WindowEvent { window_id: _, event: Resized(2560, 1315) }
WindowEvent { window_id: _, event: KeyboardInput { device_id: _, input: KeyboardInput { scancode: 53, state: Pressed, virtual_keycode: Some(Escape), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } } } }

@sodiumjoe @swiftcoder

[sodiumjoe]

Interesting, that does seem like the likely culprit. I guess since full screen doesn't invoke the modal tracking loop, so it doesn't eat the MouseDown event?

[sodiumjoe]

I'm actually seeing other weird downstream effects related to the hidpifactorchanged event that goes away once i revert that commit. we may want to revert and rethink that?

[edwin0cheng]

I checked my code, and didn't think of some special problem. However, maybe i dont know how the discard event work. How do you make sure if window_did_resize called, and the next event must be the event you want discard?

[francesca64]

@edwin0cheng I don't believe this is your fault, but rather an unforeseen interaction between this and the other PR. We didn't notice it before because your branch is based prior to that PR being merged (which is also why I ended up thinking it was a problem in one of your later commits, since switching back to your branch to test that meant the issue was no longer present). The moral of the story is that we should always rebase before testing. But anyway, I don't think you need to take any further action.

@sodiumjoe oh geeze, but I guess it's not surprising; #466 is admittedly a hack. The other moral of the story is that we should have some sort of test suite. I guess it should be reverted, and them maybe we should glob fixing it into a larger "what are we going to do about the fundamental conflicts between Cocoa and winit's design" issue? Assuming I'm correct in interpreting that the disparity is the reason we have to deal with this.

[sodiumjoe]

I'd be interested to hear @swiftcoder 's opinion. It seems like we could keep the hack contained by checking the event type here and only discarding MouseUp events?

[francesca64]

Alright, I agree that we should wait for @swiftcoder. Just please make some note in #468 in case tomaka swoops in and merges it.

[sodiumjoe]

I just closed it, I'll reopen once we figure this out. I can also try the filtering thing I mentioned real quick.

[sodiumjoe]

i just tried a really naive filtering:

                match event {
                    // Call the user's callback.
                    Some(event) => match event {
                        Event::WindowEvent { event: WindowEvent::MouseInput { state: ElementState::Released, .. }, .. } => {
                            if !self.shared.should_discard_next_event() {
                                self.shared.user_callback.call_with_event(event);
                            }
                        },
                        _ => self.shared.user_callback.call_with_event(event),                    },
                    None => break,
                }

and that doesn't appear to fix anything, so I'll wait for @swiftcoder

[swiftcoder]

Your filtering would work, but you need to restrict to pressed events, not release.

That said, we are now layering a hack on top of a hack. I hadn't anticipated that windowDidResize would be invoked in other cases, but it makes sense. Maybe need a better approach for the key presses... Feel free to back out my patch in the meantime, I'll have a think this evening about a more comprehensive solution.

[sodiumjoe]

Derp, you're right. Changing that to Pressed seems to fix most of the issues, though there are still some weird artifacts in my branch of alacritty that's using the HiDpiFactorChanged event.

It also appears to be causing some weird issues with the relatively new WindowBuilder .with_titlebar_transparent(true) and .with_fullsize_content_view(true), which seems really weird.

I think it makes the most sense to back that change out for now.