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

Rework event handling to be based on a trait #3432

Open
13 tasks
madsmtm opened this issue Jan 27, 2024 · 8 comments
Open
13 tasks

Rework event handling to be based on a trait #3432

madsmtm opened this issue Jan 27, 2024 · 8 comments
Labels
C - needs discussion Direction must be ironed out S - api Design and usability

Comments

@madsmtm
Copy link
Member

madsmtm commented Jan 27, 2024

Part of #3367, opening to discuss separately.

Winit needs some way for the user to synchronously respond to events with a value.

Prominent examples include:

To do this, we propose allowing the user to implement a trait, where each method is a callback that will be called when a certain event happens. Roughly:

// API
pub trait ApplicationHandler {
    fn new_events(&mut self, event_loop: ActiveEventLoop<'_>, start_cause: StartCause);

    fn resized(
        &mut self,
        event_loop: ActiveEventLoop<'_>,
        window_id: WindowId,
        size: PhysicalSize<u32>
    ) -> bool;

    // ... Further events
}

// User code
struct App {
    window: Option<Window>,
}
impl Application for App { ... }

fn main() {
    event_loop.run(App { window: None })?;
}

Feel free to update this code-block once we narrow down the actual API.


Identified problems to which we need some sort of solution:

Implementation plan:

@madsmtm madsmtm added S - api Design and usability C - needs discussion Direction must be ironed out labels Jan 27, 2024
@madsmtm madsmtm added this to the Version 0.30.0 milestone Jan 27, 2024
@madsmtm
Copy link
Member Author

madsmtm commented Jan 27, 2024

Re mini-batching problem:

@daxpedda suggested a design similar to tracing-subscriber, something like an extra crate winit-subscriber, linking his in here, but he ultimately realized it isn't going to work, so we need some other way to solve it.

We discussed a potential solution at our last meeting, I'll try to write a bit more about that in the coming days.

@madsmtm madsmtm pinned this issue Jan 27, 2024
@kchibisov
Copy link
Member

I'd like to add that for 0.30.0 we agreed that we do what the current design is doing and that tracing subscriber approach is more for 0.31.0 version. The resolution for 0.30.0 was to have a method that can will be called before each event.

@madsmtm
Copy link
Member Author

madsmtm commented Feb 2, 2024

We discussed the mini-batching problem and the platform event trait problem today, meeting notes here.

One of us still needs to write up some more details on this, but we think it should be possible.

@madsmtm madsmtm unpinned this issue Feb 9, 2024
@nicoburns
Copy link

Regarding platform-specific events, why not define the methods on all platforms but just not call them on irrelevant platform(s)? (or alternatively have them as conditionally compiled trait methods). So long as the methods had a default implementation then it would be quite usable I think.

@kchibisov
Copy link
Member

The said issue is solved for 0.31, adding subpar solution for that into 0.30 is not appealing to me. In general IDETs/vtable + as_any solves that really good, but you need to go hard on traits, as long as you still have enum Event it won't really work.

kchibisov added a commit that referenced this issue Feb 23, 2024
Winit is moving towards trait based end user application structure, thus
add a simple `ApplicationHandler` trait which follows the `Event<T>`. To
make use of this trait the new `run_app` group of APIs were added with
the old `run` APIs being deprecated.

Part-of: #3432
kchibisov added a commit that referenced this issue Feb 23, 2024
Winit is moving towards trait based end user application structure, thus
add a simple `ApplicationHandler` trait which follows the `Event<T>`. To
make use of this trait the new `run_app` group of APIs were added with
the old `run` APIs being deprecated.

Part-of: #3432
kchibisov added a commit that referenced this issue Feb 23, 2024
Winit is moving towards trait based end user application structure, thus
add a simple `ApplicationHandler` trait which follows the `Event<T>`. To
make use of this trait the new `run_app` group of APIs were added with
the old `run` APIs being deprecated.

Part-of: #3432
kchibisov added a commit that referenced this issue Feb 28, 2024
Winit is moving towards trait based end user application structure, thus
add a simple `ApplicationHandler` trait which follows the `Event<T>`. To
make use of this trait the new `run_app` group of APIs were added with
the old `run` APIs being deprecated.

Part-of: #3432
kchibisov added a commit that referenced this issue Mar 1, 2024
Winit is moving towards trait based end user application structure, thus
add a simple `ApplicationHandler` trait which follows the `Event<T>`. To
make use of this trait the new `run_app` group of APIs were added with
the old `run` APIs being deprecated.

Part-of: #3432
kchibisov added a commit that referenced this issue Mar 5, 2024
Add a simple `ApplicationHandler` trait since winit is moving towards
trait based API. Add `run_app` group of APIs to accept `&mut impl
ApplicationHandler` deprecating the old `run` APIs.

Part-of: #3432
kchibisov added a commit that referenced this issue Mar 5, 2024
Add a simple `ApplicationHandler` trait since winit is moving towards
trait based API. Add `run_app` group of APIs to accept `&mut impl
ApplicationHandler` deprecating the old `run` APIs.

Part-of: #3432
kchibisov added a commit that referenced this issue Mar 5, 2024
Add a simple `ApplicationHandler` trait since winit is moving towards
trait based API. Add `run_app` group of APIs to accept `&mut impl
ApplicationHandler` deprecating the old `run` APIs.

Part-of: #3432
notgull pushed a commit that referenced this issue Mar 7, 2024
Add a simple `ApplicationHandler` trait since winit is moving towards
trait based API. Add `run_app` group of APIs to accept `&mut impl
ApplicationHandler` deprecating the old `run` APIs.

Part-of: #3432
@est31
Copy link
Contributor

est31 commented Aug 1, 2024

As a downstream user of winit (glium), I want to file my opposition to the deprecation of the run function on event loops. It's okay if you add a second trait based API, but it increases the overhead in writing small programs that use winit. The main problem is that trait impls require you to repeat all parameters of all implemented functions, together with their types.

This causes overhead, as now you have to type out the parameters, and potentially also import the types. It makes it harder to use winit.

If you want to ignore a certain event, you can hide it in a => () match arm right now already.

@daxpedda
Copy link
Member

daxpedda commented Aug 1, 2024

For feedback please post in #3626.

That said, we have wrote quite a lot about the motivation to switch to traits, the simplest one is return values.
I encourage any feedback to address the shortcoming of the old event loop; without an alternative solution, we are really left without a choice.

@kchibisov
Copy link
Member

I don't understand this argument, traits will be all with default impls, so you don't have to implement anything unless you want something, and once you want something, yes, you'd have to write the method down, but it's not really an issue.

Every program should implement methods/traits it needs, so in the end of the day you'll have less code to compile and less methods.

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

No branches or pull requests

5 participants