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

Update to winit 0.28 #1738

Merged
merged 13 commits into from
May 25, 2023
Merged

Update to winit 0.28 #1738

merged 13 commits into from
May 25, 2023

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented Feb 28, 2023

This updates Iced to work with a rebased version of Iced's winit fork to latest winit master (nominally 0.28)

This depends on:

This PR will need to be updated with the new dependency versions once those have been merged.

The only code change in this PR is due winit now having a Window::set_window_level(WindowLevel) method rather then Window::set_always_on_top(bool). WindowLevel as defined as:

pub enum WindowLevel {
    AlwaysOnBottom,
    Normal,
    AlwaysOnTop,
}

This PR just maps the boolean from Iced's existing API to the correct value in the new enum. It could alternatively expose this new enum to Iced users if that's considered desirable.

@BBaoVanC
Copy link

BBaoVanC commented Feb 28, 2023

Doesn't seem to compile:

error[E0432]: unresolved import `winit::platform::unix`
   --> winit/src/settings.rs:170:36
    |
170 |             use ::winit::platform::unix::WindowBuilderExtUnix;
    |                                    ^^^^ could not find `unix` in `platform`

error[E0599]: no method named `with_name` found for struct `WindowBuilder` in the current scope
   --> winit/src/settings.rs:173:49
    |
173 |                 window_builder = window_builder.with_name(id.clone(), id);
    |                                                 ^^^^^^^^^
    |
    = help: items from traits can only be used if the trait is in scope
help: the following traits are implemented but not in scope; perhaps add a `use` for one of them:
    |
2   | use winit::platform::wayland::WindowBuilderExtWayland;
    |
2   | use winit::platform::x11::WindowBuilderExtX11;
    |
help: there is a method with a similar name
    |
173 |                 window_builder = window_builder.with_theme(id.clone(), id);
    |                                                 ~~~~~~~~~~

Is there something I need to change? I really need to use this because the old version of winit has some conflicting dependencies with another crate I have in my project.

In my project, I only get the "could not find unix in `platform" error, the other ones I only get when cloning and building (not as a dependency).

@nicoburns
Copy link
Contributor Author

@hecrj Could I get CI approved on this? This builds on macOS, but clearly there are issues on other platforms.

@BBaoVanC The thing to do would be to look at the winit changelog between 0.27 and 0.28 and work out what needs to be updated here to make this compile (there's no trick here - the error shown needs to be fixed).

@nicoburns
Copy link
Contributor Author

nicoburns commented Feb 28, 2023

Ok, this is currently blocked on updating glutin. That's going to be a bit difficult for me to take on because:

  • Glutin 0.30 is a complete rewrite with a different API to 0.29
  • I know nothing about OpenGL
  • I don't even have a linux machine to test on atm

The good news is that glutin 0.30 looks like has support for srgb (see: https://docs.rs/glutin/latest/glutin/surface/struct.SurfaceAttributesBuilder.html#method.with_srgb), so I suspect it may be possible to drop Iced's patch for that.

Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

We should wait until the advanced-text changes land. iced_glutin does not exist in that branch.

@BBaoVanC
Copy link

BBaoVanC commented Mar 1, 2023

For documentation, I made a branch on my fork to use as a dependency, and after merging both advanced-text and this PR, I think it's getting a lot closer:

error[E0277]: the trait bound `iced_graphics::Renderer<iced_wgpu::Backend, <A as application::Application>::Theme>: iced_winit::Renderer` is not satisfied
   --> /Users/bbaovanc/.local/share/cargo/registry/src/github.com-1ecc6299db9ec823/iced-0.8.0/src/application.rs:235:9
    |
235 | impl<A> crate::runtime::Application for Instance<A>
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `iced_winit::Renderer` is not implemented for `iced_graphics::Renderer<iced_wgpu::Backend, <A as application::Application>::Theme>`
    |
    = help: the following other types implement trait `iced_winit::Renderer`:
              iced_graphics::renderer::Renderer<B, T>
              iced_winit::renderer::Null

(error seems to happen on this PR regardless, and I don't get the other ones anymore, although maybe I changed something)

@hecrj hecrj added this to the 0.10.0 milestone Apr 12, 2023
@nicoburns
Copy link
Contributor Author

Rebased on top of latest master. This may fix the issues as glutin has now been removed, but we'll have to see once CI runs.

@nicoburns
Copy link
Contributor Author

Ok, this (when combined with iced-rs/winit#9) is now passing CI and the tour example runs locally on my Mac (I am currently not able to test actually running on other platforms).

@hecrj hecrj added improvement An internal improvement feature New feature or request shell labels May 25, 2023
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Awesome! Looks good.

Thanks for taking care of this 🥳

@hecrj hecrj enabled auto-merge May 25, 2023 21:21
@hecrj hecrj disabled auto-merge May 25, 2023 21:21
@hecrj hecrj enabled auto-merge May 25, 2023 21:21
@hecrj hecrj merged commit c61a4cc into iced-rs:master May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request improvement An internal improvement shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants