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

Changed EventLoop to give a result instead of panicking #2165

Closed
wants to merge 24 commits into from

Conversation

lemonlambda
Copy link

@lemonlambda lemonlambda commented Jan 28, 2022

  • Tested on all platforms changed (Tested control_flow and cursor_grab on linux)
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

I wanted to because if creating a EventLoop fails because not on main thread then you could match that and do EventLoop::new_any_thread() on linux you can at least.

@filnet
Copy link
Contributor

filnet commented Jan 28, 2022

You can test if you are on the main thread yourself and call the appropriate new function (when on linux).

Copy link
Contributor

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Seems sensible to me — I already have to handle errors from other things constructed alongside the EventLoop.

@@ -136,7 +136,7 @@ impl EventLoop<()> {
/// ## Platform-specific
///
/// - **iOS:** Can only be called on the main thread.
pub fn new() -> EventLoop<()> {
pub fn new() -> Result<EventLoop<()>, String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Check doc on this method

Copy link
Author

@lemonlambda lemonlambda Jan 29, 2022

Choose a reason for hiding this comment

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

I am guessing you mean change the docs for this one, and the example

@lemonlambda
Copy link
Author

lemonlambda commented Jan 30, 2022

You can test if you are on the main thread yourself and call the appropriate new function (when on linux).

To me this just feels like trying to get around the issue of panicking instead of just making it return a result instead of panicking. Which returning a result feels cleaner to me.

@msiglreith msiglreith added C - needs discussion Direction must be ironed out S - api Design and usability labels Jan 30, 2022
@madsmtm
Copy link
Member

madsmtm commented Jan 31, 2022

Let's merge #2137 first, that has a fair implication on this

@lemonlambda
Copy link
Author

Let's merge #2137 first, that has a fair implication on this

#2137 Has been merged do you think now this has a chance?

@madsmtm
Copy link
Member

madsmtm commented Feb 19, 2022

Yeah, I think it makes sense to do this change (but please rebase first).

However, since it is probably quite rare that people will be needing this, maybe we should consider EventLoopBuilder::try_build/EventLoop::try_new?

@lemonlambda
Copy link
Author

I have fixed the conflicts in the different branches, and it works fine from testing custom_events and control_flow

@madsmtm
Copy link
Member

madsmtm commented Feb 20, 2022

Could you put those changes in this branch (by using either git rebase or git reset --hard, and then git push --force-with-lease afterwards)

@lemonlambda
Copy link
Author

Could you put those changes in this branch (by using either git rebase or git reset --hard, and then git push --force-with-lease afterwards)

I have done this now

examples/custom_events.rs Outdated Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
src/platform_impl/linux/mod.rs Outdated Show resolved Hide resolved
@lemonlambda
Copy link
Author

For some reason I was on the wrong branch, now I am on the right branch

src/event_loop.rs Outdated Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
@lemonlambda
Copy link
Author

I have made those changes

@lemonlambda
Copy link
Author

@madsmtm Any changed I need to do now or is it good?

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Sorry, I don't have much time lately.

But CI doesn't pass, so that's at least missing.

@lemonlambda
Copy link
Author

Sorry, I don't have much time lately.

But CI doesn't pass, so that's at least missing.

Cl?

@maroider
Copy link
Member

CI?

Continuous Integration.

In our case, we make sure everything is formatted correctly and compiles on all platforms (there's also a test suite, but I believe that one is relatively small).

You can see the things that failed at the bottom of a PR's page.

image

We need all of these checks to pass in order to merge.

@lemonlambda
Copy link
Author

Why are they failing?

@maroider
Copy link
Member

maroider commented Mar 3, 2022

You can click on the "details" button to find out

@lemonlambda
Copy link
Author

You can click on the "details" button to find out

it looks like cthulu is speaking in my ears

@maroider
Copy link
Member

maroider commented Mar 3, 2022

Some of winit's public structs (like EventLoop and Window) are shims to a platform-specific struct. Thus, I can without looking too closely at the specific error messages be fairly confident that most of the errors are type mismatch errors stemming from a change in the public API without a corresponding change on every backend.

@lemonlambda
Copy link
Author

Why isn't it telling me in my IDE there are these type errors

@maroider
Copy link
Member

maroider commented Mar 4, 2022

Because your IDE only runs checks for a single target triple (presumably i686-unknown-linux-gnu, since that's the only triple I see where CI passes), while we try to run checks for every supported target triple. If you have the appropriate targets installed through rustup, you can check for these errors locally with cargo check --target=<target-triple>. Your IDE should also have some way of configuring the target triple used when running checks.

@lemonlambda
Copy link
Author

I do not feel like making the required changes as of right now to finish this pull request so I will now close this. (I don't know if other people can reopen pr's but if someone else wants to finish this they can)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out S - api Design and usability
Development

Successfully merging this pull request may close these issues.

6 participants