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

Add a way to retrieve the EGL display #1082

Merged
merged 2 commits into from Jan 16, 2019

Conversation

@ceyusa
Copy link
Contributor

commented Nov 8, 2018

to say EGLDisplay or XDisplay

@ceyusa ceyusa force-pushed the ceyusa:native-display branch 3 times, most recently from de9ce42 to 576a0f0 Nov 8, 2018

@ceyusa ceyusa force-pushed the ceyusa:native-display branch from 576a0f0 to b3e4c82 Nov 19, 2018

@jdm

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

@francesca64 Are you the right person to review these changes?

@jdm

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

Note: this is addressing #1078.

@francesca64

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2018

@jdm I'm the right person, but there are currently a few other things I need to review, so it can be hard to find time.

@ceyusa thanks for the PR!

Window::native_display follows the style of the platform_display and platform_window methods that were deprecated in glutin/winit a while ago, and were recently removed. The convention we've moved to is using WindowExt methods per-platform, each with their own documentation and descriptive name (native_display isn't really an acceptable name, simply because it's so similar to the name of a method that was recently removed).

I'm also not a fan of having external types exposed, since it means that dependency upgrades could be breaking changes. We use void pointers (*mut std::os::raw::c_void) to get around this issue (see #939 for discussion).

Having macOS and iOS return 0 isn't idiomatic, and is very surprising/dangerous. My recommendation is to just create WindowExt methods (probably get_egl_display) for the platforms that can have an EGLDisplay. You can use an Option, and document on Windows that None indicates WGL, and on X11 that None indicates GLX (in which case, the XDisplay can be retrieved via WindowExt::get_xlib_display; also including a note that Wayland always uses EGL).

@ceyusa

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Thanks a lot for the review! :)

My recommendation is to just create WindowExt methods (probably get_egl_display) for the platforms that can have an EGLDisplay.

If I understand correctly, this method should be in in winit, not int glutin. Do I understand correctly?

@ceyusa

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Do I understand correctly?

Nope. I did not :) there's no EGL code in winit...

@ceyusa

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

Sorry, I'm lost. I'm staring at the code and the only way that I see is creating a GlContextExt trait with the get_egl_display() method, implemented by the platform's GlContext.

Otherwise, I should add the method in winit's trait and implement them in glutin, which looks odd to me.

@francesca64

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2018

Right, sorry. I think GlContextExt makes sense for this.

@ceyusa ceyusa force-pushed the ceyusa:native-display branch 4 times, most recently from bab98ec to 22d2f75 Dec 5, 2018

@ceyusa

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

\o/ all checks passed

@francesca64 whenever you can :)

@ceyusa ceyusa changed the title Add a way to retrieve the native display Add a way to retrieve the EGL display Dec 5, 2018

@francesca64

This comment has been minimized.

Copy link
Collaborator

commented Jan 5, 2019

Sorry for the delay! This looks good to me, but it would be great if you could add a CHANGELOG entry. Then we'll be good to merge.

@ceyusa ceyusa force-pushed the ceyusa:native-display branch from 22d2f75 to b43ac74 Jan 8, 2019

@ceyusa

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Sorry for the delay! This looks good to me, but it would be great if you could add a CHANGELOG entry. Then we'll be good to merge.

Done :)

@francesca64 francesca64 merged commit c16f997 into rust-windowing:master Jan 16, 2019

5 checks passed

ci/circleci: android-test Your tests passed on CircleCI!
Details
ci/circleci: asmjs-test Your tests passed on CircleCI!
Details
ci/circleci: wasm-test Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.