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.

Sign up for GitHub

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

Jump to bottom

Add EventLoopBuilder #2137

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Add EventLoopBuilder #2137

merged 1 commit into from
Feb 16, 2022

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jan 5, 2022 •
edited
Loading

The beginning of #2120.

  • Tested on all platforms changed
    • macOS
    • X11
    • Wayland
    • Windows
  • 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

@madsmtm madsmtm added S - api Design and usability C - in progress Implementation is proceeding smoothly labels Jan 5, 2022
@madsmtm madsmtm force-pushed the add-eventloopbuilder branch 5 times, most recently from 009619a to fbeebc2 Compare January 5, 2022 17:09
@madsmtm madsmtm added DS - macos DS - wayland DS - windows DS - x11 C - waiting on maintainer A maintainer must review this code and removed C - in progress Implementation is proceeding smoothly labels Jan 5, 2022
@madsmtm madsmtm marked this pull request as ready for review January 5, 2022 17:37
kchibisov
kchibisov requested changes Jan 5, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I'm in favor of adding something like that to give ability to opt-out form specific handling winit is performing on Wayland, like pointer grabs, extra motions, etc, etc., since those are mostly required for games, but certain clients don't need those, so opt-ting out when building event loop would be nice.

However I don't think that X11 and Wayland share the same options, since those are completely different backends and you'd end up with _wayland methods and Wayland related structs if you'd continue to use unified struct...

src/platform/unix.rs Show resolved Hide resolved
maroider
maroider reviewed Jan 9, 2022
View reviewed changes
Copy link
Member

@maroider maroider left a comment

Choose a reason for hiding this comment

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

I'm not a massive fan of reflowing (doc) comments to fit what appears to be 80 columns, but that's not a hill I want to die on either. The changelog entries also look weird to me.

Should we mark the changelog entries as **Breaking: **?

Otherwise, this looks good.

CHANGELOG.md Outdated Show resolved Hide resolved
@maroider maroider mentioned this pull request Jan 9, 2022
Closed
5 tasks
@madsmtm
Copy link
Member Author

madsmtm commented Jan 10, 2022

Sorry about the reflow, I've changed it back, same with the changelog entries. In lack of this style choice being automatic (rust-lang/rustfmt#3347) I just did it on instinct 🙄.

I've marked the entries as breaking, well spotted!

@madsmtm madsmtm mentioned this pull request Jan 13, 2022
Merged
5 tasks
ArturKovacs
ArturKovacs reviewed Jan 18, 2022
View reviewed changes
Copy link
Contributor

@ArturKovacs ArturKovacs left a comment

Choose a reason for hiding this comment

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

I read the macos implementation and I only had the following remark.

src/platform_impl/macos/event_loop.rs Show resolved Hide resolved
msiglreith
msiglreith reviewed Jan 18, 2022
View reviewed changes
Copy link
Member

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Fine from my side!
The extension methods not using with_ prefixes feels a bit inconsistent to me with the rest of the existing builder API in winit.

@madsmtm
Copy link
Member Author

madsmtm commented Jan 18, 2022

The extension methods not using with_ prefixes feels a bit inconsistent to me with the rest of the existing builder API in winit.

You're right, I've changed these now

filnet
filnet reviewed Jan 19, 2022
View reviewed changes
src/platform/windows.rs Outdated Show resolved Hide resolved
filnet
filnet reviewed Jan 19, 2022
View reviewed changes
src/platform/unix.rs Outdated Show resolved Hide resolved
filnet
filnet reviewed Jan 19, 2022
View reviewed changes
src/platform/macos.rs Outdated Show resolved Hide resolved
filnet
filnet reviewed Jan 19, 2022
View reviewed changes
src/event_loop.rs Outdated Show resolved Hide resolved
@madsmtm
Copy link
Member Author

madsmtm commented Jan 19, 2022

Thanks for the review @filnet!

@madsmtm madsmtm mentioned this pull request Jan 31, 2022
Closed
5 tasks
@madsmtm @kchibisov
Add EventLoopBuilder struct
c71d69a 
This commit adds an `EventLoopBuilder` structure to simplify event loop
customization and providing options to it upon creation. It also
deprecates the use of `EventLoop::with_user_event` in favor of the same
method on new builder, and replaces old platforms specific extension
traits with the new ones on the `EventLoopBuilder`.
@kchibisov
Copy link
Member

The build failure is unrelated and present on master.

@kchibisov kchibisov merged commit f3f6f10 into rust-windowing:master Feb 16, 2022
@madsmtm madsmtm deleted the add-eventloopbuilder branch February 17, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maroider maroider maroider left review comments

@filnet filnet filnet left review comments

@msiglreith msiglreith msiglreith left review comments

@kchibisov kchibisov kchibisov approved these changes

@ArturKovacs ArturKovacs ArturKovacs approved these changes

Assignees
No one assigned
Labels
C - waiting on maintainer A maintainer must review this code DS - macos DS - wayland DS - windows DS - x11 S - api Design and usability
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@madsmtm @kchibisov @msiglreith @filnet @ArturKovacs @maroider