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

examples: Upgrade to winit 0.25 #487

Merged
merged 4 commits into from
Dec 20, 2021
Merged

examples: Upgrade to winit 0.25 #487

merged 4 commits into from
Dec 20, 2021

Conversation

pingpongcat
Copy link
Contributor

Current examples are based on old version of winint that was causing thread panic on my system, this is well know issue that was already fixed by winint team. I updated examples to use most up to date 0.25.0 version

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

examples/src/lib.rs Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Collaborator

Note that there have been quite a few attempts at fixing this before, see for example #356 and #481 and all the linked issues/PRs. The main problem is that they all try to refactor the entire example to clean up resources inside the event loop because fn run() doesn't return, leading to massive hard-to-review PRs that just get procrastinated.

This PR on the other hand takes the "smart" approach of using EventLoopExtRunReturn which is supported on every platform except iOS. At this point I'm inclined to just accept that and get rid of this issue for the majority of users, and hope someone (not me) will get around to reviewing and accepting the refactor to re-enable that platform once again.

examples/src/lib.rs Outdated Show resolved Hide resolved
@hYdos
Copy link

hYdos commented Dec 14, 2021

I'm able to open my PR again if you want to use #481

@MarijnS95 MarijnS95 changed the title winit v0.25.0 examples: Upgrade to winit 0.25 Dec 20, 2021
@Ralith Ralith merged commit b56f39e into ash-rs:master Dec 20, 2021
@MarijnS95
Copy link
Collaborator

MarijnS95 commented Dec 20, 2021

@Ralith I noticed you don't seem to (usually?) squash merge requests - appreciated for my PRs which are generally multiple commits that all have their own purpose, though I create separate PRs for each of them now - but might have been desired for this PR otherwise I'd have squashed the whole loose bunch before force-pushing :D

EDIT: It also doesn't add the PR number to the final, squashed commit (since there is none 😅).

@Ralith
Copy link
Collaborator

Ralith commented Dec 21, 2021

Whoops, my bad! I had a roll going with all your PRs and didn't look closely enough here. I'm a big fan of the rebase/rewrite oriented workflow in general, since it optimizes for readability of the final product, which is what matters most.

@MarijnS95
Copy link
Collaborator

@Ralith I'm a little surprised since for me "squash and merge" is shown as default for this repository (and I don't recall that being a per user, per repository setting). We can probably disable the other options too now though.

@Ralith
Copy link
Collaborator

Ralith commented Dec 21, 2021

I think it's per-user per-repo, and I probably used it on some of my own PRs in the past.

@MarijnS95
Copy link
Collaborator

Looks it simply remembers the last merge method that was used, indeed. I don't have access to a settings page for this repo so there's no way for me to configure it.

@Ralith
Copy link
Collaborator

Ralith commented Dec 21, 2021

Something to hassle @MaikKlein about, I suppose. In the mean time I'll exercise extra caution.

@MaikKlein
Copy link
Member

MaikKlein commented Dec 22, 2021

I don't have access to a settings page for this repo so there's no way for me to configure it.

I'll have a look.

Something to hassle @MaikKlein about, I suppose. In the mean time I'll exercise extra caution.

Personally I prefer squash and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants