Skip to content

Update to new winit 0.6 EventsLoop API#864

Merged
tomaka merged 15 commits into
rust-windowing:masterfrom
mitchmindtree:winit_api_update
Apr 23, 2017
Merged

Update to new winit 0.6 EventsLoop API#864
tomaka merged 15 commits into
rust-windowing:masterfrom
mitchmindtree:winit_api_update

Conversation

@mitchmindtree
Copy link
Copy Markdown
Contributor

This updates glutin's API to reflect winit's new API introduced in rust-windowing/winit#126, recently published under version 0.6.0. Please see the linked PR for further details on the new API.

You can also see rust-windowing/winit#132 to find out how this new version of winit solves many old macos bugs.

So far, this PR updates the API, macos backend and all of the examples to the new winit API. I haven't updated any of the other backends yet.

If I get time soon, I'll look into making a gen_api_transition! macro similar to the one used in winit. If anyone else would like to tackle this in the mean time feel free to fork this PR and get it done!

bors-servo pushed a commit to servo/webrender that referenced this pull request Mar 14, 2017
Use gl::Gl trait object in webrender

The pull request is preparation of #771. It just replace gleam::gl function calls with function calls to gleam::gl::Gl trait object.

API between winit and glutin is broken now. Then the pull request does not update glutin. The glutin still refers to old cgl and winit. It causes old gleam usage within them. Fixing the break is handled by rust-windowing/glutin#864.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/965)
<!-- Reviewable:end -->
@mitchmindtree
Copy link
Copy Markdown
Contributor Author

I looked into creating a gen_api_transition! macro for this, however it seems like it would be quite a bit more involved than what was necessary for winit's one, as this transition requires both removing the old iterator/proxy APIs while also wrapping the new EventsLoop API.

I decided that it would probably be easier to setup windows and linux VMs and try to manually update their glutin APIs than it would be to create some kind of transition macro that would be removed shortly after anyway.

@vberger, I briefly started transitioning the linux backend, however I noticed you recently re-organised the linux API in winit and it seems as though you might already have some plans on how to re-organise the glutin linux API accordingly as well? E.g. I notice that the glutin/src/platform/linux/x11.rs module currently imports winit::os::unix::x11::XConnection - I tried fixing this but noticed that it no longer seems to be publicly exported by winit at all, so I suspect you might have some other way of updating it in mind? Would you possibly be interested in taking care of the linux transition and doing a PR against this branch? I feel like I could keep having a go, however it seems you are much more familiar and could probably do it in much less time. That way I can have a go at updating the windows API in the meantime and we can hopefully get this out quicker. No worries though if you're busy with other things 👍

@elinorbgr
Copy link
Copy Markdown
Contributor

elinorbgr commented Mar 15, 2017

Hmm.

Regarding XConnection, indeed I removed it... This is a mistake from my side. (But I don't understand why this seems to have already dissapeared in glutin 0.6.1, while my PR was merge after 0.6.1 was released... ?) (nvm, looks like I can't read a git history)

Anyway, I'll try to give a got at updating the unix backend of glutin. However I'm mostly familiar with wayland, not x11, so I cannot promise anything, and this will probably need a detailed review.

@mitchmindtree
Copy link
Copy Markdown
Contributor Author

mitchmindtree commented Mar 17, 2017

Ok, I think this might be just about ready to go!

@vberger has updated the x11 and wayland backends and made a fix to the multiwindow example which got it working nicely again. I tested his changes on wayland in my VM and the examples seem to run fine.

I just updated the windows backend using a VM to test compilation, however I couldn't run any of the examples due to the poor opengl support for win10 in my VM. There was not too much to change and I'm fairly sure it should behave the same, however it would be nice if someone could test this.

I also had a go at updating the android backend as there seemed to be hardly anything to change. Again, I could not run this myself for testing, so it would be great if someone else could do so.

I have not updated the ios or emscripten backends as I'm not sure how to test for either of them and it looks like neither of them have been updated to use winit at all yet.

@tomaka would you mind testing the windows build when you get a chance? Maybe also the android build if you have a setup available for doing so? If you don't have time, perhaps you could ping someone who might?

Currently in the macOS backend the `Context` associated with each
`Window` is never removed from the map once it is added until the
`EventsLoop` is `Drop`ped. This commit refactors the backend to ensure
each `Window`'s `Context` is removed from the map immediately upon
either `Closed` or `Drop`.
@mitchmindtree
Copy link
Copy Markdown
Contributor Author

I just realised that the context associated with each Window in the macOS backend was never being removed from the map owned by the EventsLoop. I've added a commit that ensures that when a Window is Closed or Dropped the associated Context is also removed from the context map.

@mitchmindtree
Copy link
Copy Markdown
Contributor Author

@vberger it seems like wayland-window 0.5.1 was published with some breaking changes. Travis is getting lots of errors that look like this:

error[E0425]: cannot find function `wl_display_connect` in this scope

  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/wayland-client-0.9.0/src/display.rs:52:61

   |

52 |     let ptr = unsafe { ffi_dispatch!(WAYLAND_CLIENT_HANDLE, wl_display_connect, ::std::ptr::null()) };

   |                                                             ^^^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function `wl_display_flush` in this scope

  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/wayland-client-0.9.0/src/display.rs:70:65

   |

70 |         let ret = unsafe { ffi_dispatch!(WAYLAND_CLIENT_HANDLE, wl_display_flush, self.ptr() as *mut _) };

   |                                                                 ^^^^^^^^^^^^^^^^ not found in this scope

error[E0425]: cannot find function `wl_display_create_queue` in this scope

  --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/wayland-client-0.9.0/src/display.rs:78:65

   |

78 |         let evq = unsafe { ffi_dispatch!(WAYLAND_CLIENT_HANDLE, wl_display_create_queue, self.ptr() as *mut _) };

   |       

See travis' build log for more details.

@elinorbgr
Copy link
Copy Markdown
Contributor

Okay, I'm not very troubled at how cargo handles dependencies.

Why does it pull wayland-client 0.9, while winit and glutin require 0.8, and wayland-window allows both ?

Anyway, I'll yank wayland-window 0.5.1 (and wayland-kbd 0.8.1 which probably has the same issue)...

@mitchmindtree
Copy link
Copy Markdown
Contributor Author

@vberger cargo always uses the most recent version allowed for each dependency, so although it does use the 0.8 version of wayland-client included in the Cargo.toml, it was also using version 0.9 via wayland-window version 5.1. Cargo allows for multiple versions of the same crate to be used at once as long as there are no direct conflicts in code (e.g. a version 0.8 implementation of a trait does not satisfy a version 0.9 trait declaration, even if the signatures are identical) and as long as the library does not have any non-rust dependencies (I believe cargo can only link non-rust libraries once. This is why it's such a big problem when a library like libc publishes a new version, as all crates that depend on it must be using the same version).

Okay, I'm not very troubled at how cargo handles dependencies.

No worries! I think that the tooling around cargo could be greatly improved and more aware of the semantic versioning. E.g. if a user attempts to publish a new tiny version when the change is potentially breaking, it would be nice if cargo at least emitted a warning with a small note about how the change could break downstream crates. I think we'd avoid lots of accidental breakage like this. I kind of wish the semantic versioning was enforced even if conservatively, but that ship sailed long ago.

@tomaka do you mind running the travis linux build again?

@mitchmindtree mitchmindtree changed the title [WIP] Update to new winit 0.6 EventsLoop API Update to new winit 0.6 EventsLoop API Mar 21, 2017
@mitchmindtree
Copy link
Copy Markdown
Contributor Author

@tomaka this is ready for review next time you get a chance 🙇

@ebkalderon
Copy link
Copy Markdown
Contributor

ebkalderon commented Apr 15, 2017

What is the status of this pull request? This update is very important to projects that are looking to utilize the latest version of winit.

@ishitatsuyuki
Copy link
Copy Markdown

Here's another weekly ping.

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.

5 participants