Ported glutin to winit

[ozkriff]

For #813

Requires rust-windowing/winit#23

[ozkriff]

Wayland is still broken (tested with sway):

thread 'main' panicked at 'Version 5 of seat interface is needed by glutin.', /home/vlad/.config/cargo/git/checkouts/winit-ca99edec3a8d9e90/port_glutin_to_winit/src/api/wayland/context.rs:129
note: Run with `RUST_BACKTRACE=1` for a backtrace.
[wayland-client error] A handler for wl_registry panicked, aborting.error: Process didn't exit successfully: `target/debug/examples/fullscreen` (signal: 6, SIGABRT: process abort signal)

Have not touched ios and emscripten backends.

win, linux/x11, osx, android backends work but I haven't tested them extensively.

[ozkriff]

r? :)

[elinorbgr]

The wayland error is expected, it appears that sway is a little behind in its versions of the core wayland protocol.

It'd be good to relax this restriction, but this is a 100% winit issue anyway, so not relevant for this PR.

[elinorbgr]

Other than that, LGTM for the wayland part (expect for the TODO, but this is EGL stuff and I know nothing about that)

[elinorbgr]

As an afterthought, I don't know how it is on other platforms, but on a resized event, for wayland, the EGL won't be automatically resized, and that's it which defines the actual size of the window. This is the reason for this particular line in the previous backend : https://github.com/tomaka/glutin/blob/d87fa4f6b749f67eb37300cb9dc58cf31c3ff5b5/src/api/wayland/window.rs#L79

No sure how it should be implemented now, though.

EDIT: likewise, set_inner_size() method in Window should resize the EGL surface as well

[tomaka]

Some initial review:

• I think the winit::Window should be stored in the structs in the platform directory, instead of being stored in the Window at the root of the library. This will be required for emscripten to work (as emscripten has no winit window), and would solve @vberger's problem by adding a small code in linux's poll_events() and next_events(). The winit builder can remain in the root WindowBuilder and can be passed by value (instead of by reference) to the code in platform.
• The content of api/win32 should be merged with platform/win32 (and api/win32 should disappear).
• Similarly, api/x11 and api/wayland should be merged with platform/linux. This is probably trickier, however.

There's no justified reason anymore to have api/foo except when foo is related to OpenGL.

I think it looks mostly good. Other than making it possible to use winit with gfx, it will also considerably clean up glutin.

I'll review again once these three things are resolved.

[ozkriff]

@tomaka Done

[tomaka]

Looks good. There are some things that could be improved now that the organization has changed, but it's ok for now.

r? @vberger for the wayland stuff?

[ozkriff]

I have no working wayland test zone so wayland's resizing stuff is still not fixed :(

[tomaka]

My idea was that now that poll_events and wait_events are back in src/platform/linux, it should be possible to put some wayland-specific code in there.

[ozkriff]

Ok, I'll ask @Vinatorul if he can look into it

[Vinatorul]

@ozkriff sure, I'll check tomorrow

[elinorbgr]

The idea would be to make a light wrapper around the *EventsIterators from winit to call EGLSurface::resize(..) whenever a Resize event appears, before forwarding it to the user.

Also, EGLSurface::resize(..) needs to be called during set_inner_size(..) as well.

[Vinatorul]

I've done this, please check WIP_wayland_port_glutin_to_winit, but I can't test it.
It appears that with new wayland_client or with last changes winit somehow creates window with x11 display-server. I'm using gnome-shell-wayland in virtual terminal.

[elinorbgr]

I tried running it yesterday on weston, and the window example segfaulted somewhere in libEGL and libwayland. I'll try to get a proper stacktrace, to figure out if the bug is from wayland-client, winit, or your PR.

[elinorbgr]

Oh wait, nvm, I hadn't checked out the proper branch

[elinorbgr]

(I'll test it this evening, my current computer is not wayland-egl compatible)

However @Vinatorul : winit should only fallback to X11 if creation of a wayland context failed, so this might be a bug with wayland-client. Could you try to run the simple_client example from wayland-client and open an issue on wayland-client about why the creation failed ?

[Vinatorul]

@vberger I've run simple_client example and it seems working:

Globals advertized by server:
   1 : wl_drm (version 2)
   2 : wl_compositor (version 3)
   3 : wl_shm (version 1)
   4 : wl_output (version 2)
   5 : wl_data_device_manager (version 2)
   6 : xdg_shell (version 1)
   7 : wl_shell (version 1)
   8 : gtk_shell (version 2)
   9 : wl_subcompositor (version 1)
  10 : _wl_pointer_gestures (version 1)
  11 : wl_seat (version 4)

[elinorbgr]

Well, I've tested your code on weston, seems to work. There are just two things:

• For the window to appear, something must be drawn. That means that no window will appear before the first call to swap_buffers is done. This was already the case before, I think. But it might be good to just draw a black content and swap buffers once during window creation, to mimic other platforms. (For example, the window example does not do that, and remains stuck because the window won't appear before the first event is received, and this event is never received because the window does not yet exist!)
• For some reason, the borders at start are not the right size, but this is probably winit's fault, as you use get_inner_size from winit. I'll look into that.

[elinorbgr]

Other than that, looks good 😄

[elinorbgr]

The border size issue was indeed from winit, see rust-windowing/winit#30

[ozkriff]

For the window to appear, something must be drawn...
This was already the case before

@vberger Hmm, I'm not sure this should be done in this PR

[elinorbgr]

That's probably out of scope for this PR indeed, as it was already the previous behaviour. It's just that I re-discovered it while testing your code.

[ozkriff]

So, are we ready for merging? :)

[Jascha-N]

WindowBuilder is no longer Clone. Any reason for this?

[tomaka]

@Jascha-N No, probably a mistake.