Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for MacOS #465

Merged
merged 11 commits into from
Apr 17, 2018

Conversation

edwin0cheng
Copy link
Contributor

@edwin0cheng edwin0cheng commented Apr 14, 2018

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
Copy link
Member

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

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 15, 2018

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
Copy link
Member

  • 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.

win_attribs: RefCell<WindowAttributes>,
standard_frame: Cell<Option<NSRect>>,

handle_with_fullscreen: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// We set a normal style to it temporary.
// It will clean up at window_did_exit_fullscreen.
if current.is_none() && !win_attribs.decorations {
state.window.setStyleMask_(NSWindowStyleMask::NSTitledWindowMask|NSWindowStyleMask::NSResizableWindowMask);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long.

/// due to being in the midst of handling some other animation or user gesture.
/// This method indicates that there was an error, and you should clean up any
/// work you may have done to prepare to enter full-screen mode.
extern "C" fn window_did_fail_to_enter_fullscreen(this: &Object, _: Sel, _: id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


use super::events_loop::Shared;
use super::events_loop::{EventsLoop,Shared};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -33,6 +35,77 @@ struct DelegateState {
view: IdRef,
window: IdRef,
shared: Weak<Shared>,

win_attribs: RefCell<WindowAttributes>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to be using cells in async callbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood how the delegate methods work.

return;
}
(Some(_), Some(_)) => {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let win_attribs = state.win_attribs.borrow_mut();

let current = win_attribs.fullscreen.clone();
match (current.clone(), monitor.clone()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these clones are necessary.

use cocoa::appkit::{self, NSApplication, NSColor, NSView, NSWindow, NSWindowStyleMask, NSWindowButton};
use cocoa::foundation::{NSDictionary, NSPoint, NSRect, NSSize, NSString};
use cocoa::appkit::{self, NSApplication, NSColor, NSScreen, NSView, NSWindow, NSWindowButton,
NSWindowStyleMask};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 17, 2018

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
Copy link
Contributor Author

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

@edwin0cheng
Copy link
Contributor Author

edwin0cheng commented Apr 17, 2018

  • 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
Copy link
Contributor Author

edwin0cheng commented Apr 17, 2018

@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.

}

// Make key have to be after set fullscreen
// to prevent normal zie window brefly appears
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zie -> size

afterDelay: 0.5
];
}
else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else {

@francesca64
Copy link
Member

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

@edwin0cheng
Copy link
Contributor Author

Fixed, and thank you so much.

@francesca64 francesca64 merged commit 0474dc9 into rust-windowing:master Apr 17, 2018
@edwin0cheng edwin0cheng deleted the macos-fullscreen branch April 17, 2018 18:09
@francesca64
Copy link
Member

francesca64 commented Apr 17, 2018

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
Copy link
Contributor Author

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

@francesca64
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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 edwin0cheng restored the macos-fullscreen branch April 17, 2018 20:34
@edwin0cheng
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

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 sodiumjoe mentioned this pull request Apr 17, 2018
@sodiumjoe
Copy link
Contributor

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

@sodiumjoe
Copy link
Contributor

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
Copy link
Contributor

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.

francesca64 added a commit that referenced this pull request Apr 18, 2018
… and set_decorations for MacOS (#465)"

This reverts commit 0474dc9.
@sodiumjoe
Copy link
Contributor

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.

@edwin0cheng edwin0cheng deleted the macos-fullscreen branch May 2, 2018 17:22
tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022
…ples/canvas_webgl_minimal/www/dns-packet-1.3.4, r=jdm

Bump dns-packet from 1.3.1 to 1.3.4 in /examples/canvas_webgl_minimal/www

Bumps [dns-packet](https://github.com/mafintosh/dns-packet) from 1.3.1 to 1.3.4.
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/mafintosh/dns-packet/commit/ebdf849da5dc0d96836e87628349776c623c5be7"><code>ebdf849</code></a> 1.3.4</li>
<li><a href="https://github.com/mafintosh/dns-packet/commit/ac578722f2707310b841b65aae61d6332f8882a1"><code>ac57872</code></a> move all allocUnsafes to allocs for easier maintenance</li>
<li><a href="https://github.com/mafintosh/dns-packet/commit/c64c9507e51532c9e9a3cbefa146a134ecc025fd"><code>c64c950</code></a> 1.3.3</li>
<li><a href="https://github.com/mafintosh/dns-packet/commit/0598ba19d18da4568b32415e60a9629061b3c45c"><code>0598ba1</code></a> fix .. in encodingLength</li>
<li><a href="https://github.com/mafintosh/dns-packet/commit/010aedb33c1ee8c3f558db5249c1d46e2bd7a101"><code>010aedb</code></a> 1.3.2</li>
<li><a href="https://github.com/mafintosh/dns-packet/commit/0d0d593f8df4e2712c43957a6c62e95047f12b2d"><code>0d0d593</code></a> backport encodingLength fix to v1</li>
<li>See full diff in <a href="https://github.com/mafintosh/dns-packet/compare/v1.3.1...v1.3.4">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=dns-packet&package-manager=npm_and_yarn&previous-version=1.3.1&new-version=1.3.4)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language

You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/servo/pathfinder/network/alerts).

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants