-
Notifications
You must be signed in to change notification settings - Fork 921
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
Implement set_maximized, get_current_monitor, set_fullscreen and set_decorations for MacOS #465
Conversation
I installed High Sierra in preparation for this! I'll review it as soon as I get a chance. |
This implementation is quite straight forward, which is just calling However,
|
servo/core-foundation-rs#204 will give us some more options (literally), though nothing that it looks like we presently need. |
src/platform/macos/window.rs
Outdated
win_attribs: RefCell<WindowAttributes>, | ||
standard_frame: Cell<Option<NSRect>>, | ||
|
||
handle_with_fullscreen: bool, |
There was a problem hiding this comment.
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.
src/platform/macos/window.rs
Outdated
// 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); |
There was a problem hiding this comment.
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.
src/platform/macos/window.rs
Outdated
/// 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) { |
There was a problem hiding this comment.
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.)
src/platform/macos/window.rs
Outdated
|
||
use super::events_loop::Shared; | ||
use super::events_loop::{EventsLoop,Shared}; |
There was a problem hiding this comment.
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>, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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??
There was a problem hiding this comment.
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.
src/platform/macos/window.rs
Outdated
return; | ||
} | ||
(Some(_), Some(_)) => { | ||
return; |
There was a problem hiding this comment.
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!()
.
src/platform/macos/window.rs
Outdated
let win_attribs = state.win_attribs.borrow_mut(); | ||
|
||
let current = win_attribs.fullscreen.clone(); | ||
match (current.clone(), monitor.clone()) { |
There was a problem hiding this comment.
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.
src/platform/macos/window.rs
Outdated
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}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I want to use this but it is absent in current CoreFoundation binding which is c api which i cannot use
Indeed, we would make another PR to address this too.
We do have some comment about this issue in WindowExt, but i don't know it is enough to indicate that. |
Ok, seem to be we cannot use |
These bugs should be fixed. |
@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. |
src/platform/macos/window.rs
Outdated
} | ||
|
||
// Make key have to be after set fullscreen | ||
// to prevent normal zie window brefly appears |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zie -> size
src/platform/macos/window.rs
Outdated
afterDelay: 0.5 | ||
]; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else {
Thanks, and addressing the rest in a later PR seems reasonable. Two more nitpicks and we should be good to merge. |
Fixed, and thank you so much. |
No, i just tested it and it only take 1 try. Could you mind to detail step to reproduce it? |
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:
As you can see, the first F press never generates a
|
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? |
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? |
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? |
@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. |
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? |
Alright, I agree that we should wait for @swiftcoder. Just please make some note in #468 in case tomaka swoops in and merges it. |
I just closed it, I'll reopen once we figure this out. I can also try the filtering thing I mentioned real quick. |
i just tried a really naive filtering:
and that doesn't appear to fix anything, so I'll wait for @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 |
Derp, you're right. Changing that to It also appears to be causing some weird issues with the relatively new WindowBuilder I think it makes the most sense to back that change out for now. |
…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>
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