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.

Sign up for GitHub

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

Jump to bottom

Child window #96

Merged
merged 6 commits into from
Dec 2, 2016
Merged

Child window #96

merged 6 commits into from
Dec 2, 2016

Conversation

k-brac
Copy link
Contributor

@k-brac k-brac commented Nov 28, 2016

Here is my attempt to implement #93 for windows.

I am not sure it is the way to go (I changed some members visibility).

I have added an example as a proof of concept.

@tomaka
Copy link
Contributor

tomaka commented Nov 28, 2016 •
edited
Loading

I don't think WindowProxy should be exposed. It's just an implementation detail.
What about passing directly a HWND to with_parent_window?

@k-brac
Copy link
Contributor Author

k-brac commented Nov 29, 2016

I thought it would be more convenient to use WindowProxy because it implements the send and sync traits.
But it seems that I was wrong, and I think it is cleaner now

tomaka
tomaka reviewed Nov 29, 2016
View reviewed changes
src/platform/windows/init.rs
let window = window.clone();

let attribs = pl_attribs.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not pass the attributes by reference to init instead of cloning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing by reference causes a lifetime error that I don't know how to fix. Since we are cloning WindowAttributes I did the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right I forgot we were spawning a thread, so nevermind.

@tomaka
Copy link
Contributor

tomaka commented Nov 29, 2016 •
edited
Loading

I also think that the example is maybe a bit too complicated.
It's nice to have some sort of code to test child windows, but it's really not the kind of code that people should be looking at.

This is just a remark. I'm not sure what to do between removing the example altogether or keeping it.

@k-brac
Copy link
Contributor Author

k-brac commented Nov 29, 2016

I agree with you about the example.
I left it because it is my test case and I thought it could be useful when implementing this feature for other platform.
Once there is something correct for windows I may try to do it for mac os too

@tomaka
Copy link
Contributor

tomaka commented Dec 2, 2016

Oh sorry, I forgot about this PR. I wanted to wait for travis/appveyor to pass before merging, but apparently Cargo is still broken sigh.

@tomaka tomaka merged commit 1ee9136 into rust-windowing:master Dec 2, 2016
@k-brac k-brac deleted the child_window branch December 2, 2016 14:37
jrmuizel pushed a commit to jrmuizel/winit that referenced this pull request Mar 29, 2017
Auto merge of rust-windowing#96 - pcwalton:mac-proper-activation, r=m…
a8f834a 
…etajack

cocoa: Activate the `NSApplication` instance after the event loop is set up, and use `orderFrontRegardless` to more reliably pop up the main window.

This makes the menus work right away when the app is started.

See e.g. http://czak.pl/2015/09/23/single-file-cocoa-app-with-swift.html

r? @metajack

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/96)
<!-- Reviewable:end -->
@francesca64 francesca64 mentioned this pull request May 6, 2018
Closed
@t-sin t-sin mentioned this pull request Dec 15, 2021
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomaka tomaka tomaka left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k-brac @tomaka @baubry-arturia