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

Begin transition to a trait-based system #3386

Closed
wants to merge 8 commits into from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jan 12, 2024

Part of #3367. Blocked on #3299 #3447, since I want to keep the active handle being passed around as simple as possible.

Add a trait ApplicationHandler<T> which replaces the closure impl FnMut(Event<T>, ActiveEventLoop<'_>) matching on Event<T>s. This trait is fully compatible with the closure, and all backends and most examples still use the old event design. But it should be possible to slowly migrate things from here.

Note that this will not be the final design of the trait, exit will probably be replaced by Drop (you shouldn't need to access the ActiveEventLoop<'_> from there anyhow, and it's more "Rusty"), we'll probably introduce a closure at the beginning instead of init, there are events that should be changed to have return types, and so on.
But we need to keep these APIs around mostly as-is, so that we can convert between the trait and the closure, at least until we've gathered experience, and are ready to remove the closure-based API.

TODO:

  • Gather feedback.
  • Consider extensibility wrt. backend-specific functionality.
  • Finish implementation (I should probably add a WindowHandler in this PR too).
  • Prepare at least one backend to migrate to this, to see that it is possible.
  • Add changelog entry and docs.
  • Update all examples (can probably be done in another PR).
  • Change run function, as discussed previously.

Note: This PR is similar to my earlier #3073, but differs in that it doesn't try to do anything fancy with the event lifecycle yet, and that it has the full two-way compatibility between trait and closure.

nerditation and others added 8 commits December 24, 2023 00:55
the `EventLoopWindowTarget` is needed for window creation. conceptually,
only `EventLoop` and `EventLoopProxy` need to be parameterized, and all
other parts of the backend should be agnostic about the user event type,
parallel to how `Event<T>` is parameterized, but `WindowEvent` is not.

this change removes the dependency on the type of user events from the
`EventLoopWindowTarget` for the Windows backend, but keep a phantom data
to keep the API intact. to achieve this, I moved the `Receiver` end of
the mpsc channel from `ThreadMsgTargetData` into `EventLoop` itself, so
the `UserEvent` is only passed between `EventLoop` and `EventLoopProxy`,
all other part of the backend just use unit type as a placeholder for
user events.

it's similar to the macos backend where an erased `EventHandler` trait
object is used so all component except `EventLoop` and `EventLoopProxy`
need to be parameterized. however `EventLoop` of the Windows backend
already use an `Box<dyn FnMut>` to wrap the user provided event handler
callback, so no need for an dedicated trait object, I just modified the
wrapper to replace the placeholder user event with real value pulled
from the channel. I find this is the approach which need minimum change
to be made to existing code. but it does the job and could serve as a
starting point to future Windows backend re-works.
this field is transitional and her to keep API compatibility only.
the correct variance and such is already ensured by the top-level
`EventLoopWindowTarget`, just use `PhantomData<T>` here.
@madsmtm madsmtm added S - api Design and usability C - needs discussion Direction must be ironed out labels Jan 12, 2024
@madsmtm madsmtm changed the title Begin transition to a trait-based system (ApplicationHandler, WindowHandler) Begin transition to a trait-based system (ApplicationHandler, WindowHandler, ...) Jan 12, 2024
@madsmtm madsmtm mentioned this pull request Jan 12, 2024
@madsmtm madsmtm changed the base branch from master to active-event-loop January 12, 2024 22:44
@madsmtm madsmtm changed the title Begin transition to a trait-based system (ApplicationHandler, WindowHandler, ...) Begin transition to a trait-based system Jan 12, 2024
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 don't see why I should merge dead from the start code...

@madsmtm
Copy link
Member Author

madsmtm commented Jan 13, 2024

Concretely, because we can ship this to users in v0.30.0, and allow them to upgrade to the trait-based approach with the old event-based approach being deprecated, and allow them to give feedback if there's something that didn't work for them.

And then we can work on removing Event<T> and associated helpers.

(But I get that we differ in views on this).

@kchibisov
Copy link
Member

kchibisov commented Jan 13, 2024

I don't want for users to have more work upgrading to incomplete API and then to complete one, I as downstream project maintainer I won't waste my time on such mess.

@madsmtm
Copy link
Member Author

madsmtm commented Jan 13, 2024

I don't see your issue as a downstream user? ApplicationHandler matches the Application trait which you've outlined in winit-next's application.rs, which is one of the primary API changes that users are going to have to deal with?

Regarding ApplicationWindow, I will add that to this PR as well, though the only thing that would really be different in this initial phase is that close_requested won't return anything yet (and that should be a minor thing to upgrade to).

@kchibisov
Copy link
Member

The thing about my API is that it's not complete because I was asked to wait, more things will change, like surely you can port some of that work, but I can't really deal with return values on Wayland because the don't make any sense to me since everything is queued, so the main motivation to return values is completely lost.

And the whole return is blocked for me because Window is Send + Sync, so I have API users can't use and I can't use, if I can't use it and it solves nothing why should I bother to upgrade Wayland backend and alacritty to it.

@kchibisov
Copy link
Member

The same issue with neovide since I can't solve their sync issues unless I just do the full design and &dyn stuff.

@madsmtm madsmtm force-pushed the active-event-loop branch 7 times, most recently from 2e1dc24 to 5f4430e Compare January 15, 2024 16:18
@daxpedda daxpedda added this to the Version 0.30.0 milestone Jan 17, 2024
@kchibisov
Copy link
Member

Superseded by #3517.

@kchibisov kchibisov closed this Feb 23, 2024
@madsmtm madsmtm deleted the application-handler-transition branch May 27, 2024 08:55
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.

4 participants