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

Make the pf_reqs field of ContextBuilder public. #1002

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link
Contributor

Its type PixelFormatRequirements is already public with public fields.

Its type PixelFormatRequirements is already public with public fields.
@tomaka
Copy link
Contributor

tomaka commented Mar 23, 2018

This field was made private on purpose, although I don't remember why :-/

@jdm
Copy link
Contributor

jdm commented Nov 26, 2018

According to #608 it's because the members of PixelFormatRequirements were not considered stable that the time.

@jdm
Copy link
Contributor

jdm commented Nov 26, 2018

According to https://github.com/tomaka/glutin/blame/209ab925f5325c35b8c35020e2d9a85f519f84c3/src/lib.rs#L817 the struct members have not changed in 3 years.

@francesca64
Copy link
Member

@jdm thanks for investigating this!

@SimonSapin are you still interested in this?

@SimonSapin
Copy link
Contributor Author

Yes, this would allow fixing a bug in some code in WebRender’s Wrench, even though nothing happens to "exercise" that bug at the moment.

This was for servo/webrender#2529 where I forked some of Glutin’s src/api/egl/mod.rs in order to create an (E)GL context, but with statically-linked ANGLE. So this code receives ContextBuilder as a parameter, and needs to access its details.

The work-around was to use PixelFormatRequirements::default(), which was ok because the only user of this code happened not to use any ContextBuilder API that would change the pixel format requirements from the defaults.

Forking this EGL code would not be necessary in the first place if Glutin itself could support statically-linked EGL/ANGLE. I attempted that in #998, but it resulted in an awkward public API.

@francesca64
Copy link
Member

Okay, cool. I'm not opposed to this, so provided you add a CHANGELOG entry we can merge this.

@goddessfreya goddessfreya added this to the 0.20 milestone Mar 4, 2019
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.

None yet

5 participants