-
Notifications
You must be signed in to change notification settings - Fork 902
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
Event Loop 2.0 API and Windows implementation #638
Event Loop 2.0 API and Windows implementation #638
Conversation
c6627af
to
7fea558
Compare
Who would be up for reviewing the changes made here? I don't have a good idea of who has access to a Windows machine, and I'd rather this didn't wait around in the backlog forever. |
Hello, I've tried some experiments with this branch, and run into an issue where I get a panic if I try to spawn a win32 context menu on handling a right-click event using the TrackPopupMenuEx winapi function. It's pretty easy to replicate, but let me know if you'd like a code snippet. I also note that this is already a great improvement to the current master winit event loop on win32 which has the second thread; since TrackPopupMenuEx needs to be called on the thread owning the Window, it just silently fails to create the context menu, and would require some inter-thread message passing shenanigans to work at all. Overall I find the API design of this branch much nicer for my use-cases. Cheers |
@hardiesoft Thanks! The reason that's happening is because, as you said, The best way I can think of to solve this without changing the API would be to buffer up events if new events are generating during the closure then send them all off when the closure returns, although that would pause user event processing while the It may also be worth adding a function to let event_loop = EventLoop::new();
let window = WindowBuilder::new()
.with_title("A fantastic window!")
.build(&event_loop)
.unwrap();
event_loop.run(move |event, event_loop, control_flow| {
match event {
Event::WindowEvent {
event: WindowEvent::CloseRequested,
..
} => *control_flow = ControlFlow::Exit,
Event::WindowEvent {
event: WindowEvent::MouseInput {
state: winit::event::ElementState::Released,
button: winit::event::MouseButton::Right,
..
},
..
} => {
event_loop.queue_function(|| unsafe {
use std::ptr;
use winapi::um::winuser::{self, *};
let menu = winuser::CreatePopupMenu();
winuser::InsertMenuA(menu, 0, MF_BYPOSITION | MF_STRING, 0, "Hello\0".as_ptr() as _);
winuser::TrackPopupMenuEx(
menu,
0,
512,
512,
window.get_hwnd() as _,
ptr::null_mut()
);
winuser::DestroyMenu(menu);
});
}
_ => *control_flow = ControlFlow::Wait,
}
}); And |
Hi @Osspial I do like your idea of queuing a function to be run after the main loop closure; for full flexibility ideally I don't want to stop other user event processing while a context menu (or OS file dialog etc) is open. The main issue I see here is just one of documenting this properly if this is the direction you decide to go in - perhaps providing an example that addresses this use-case? Would the proposed API be useful outside of Windows, or would it essentially be a platform specific thing to work around this issue? If you decide it's not worth the specialised API to handle this I'd be okay with it just buffering the events (or even throwing them away) and blocking. If I care about things like animations continuing to run in my window I'd probably have my render loop off the main thread, and I generally don't expect to receive input to my window while a context menu or other OS UI element has focus. IIRC, macos will just block the calling thread in this case too, so it's sort of a lowest common denominator behaviour. |
661f6af
to
b332591
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not in any position to review windows-specific code, so I only looked at the public API.
Overall the new API looks clean to me, I don't have much to say, as most of it was already discussed beforehand anyway. 👍
I've just left a few comments regarding points were the documentation could need some clarification.
Regarding the next steps, the plan is to merge this into the eventloop-2.0
branch, and then use it as a basis to implement the other platform on the same branch in following PRs, am I correct?
This model doesn't seem to be compatible with SendMessage, which has legitimate uses. Is there any way we can make event handlers reentrant? |
@caitp The only way I can think of doing that would be to make the run functions take a |
How is continuous rendering meant to be handled with the new API? I'm trying to do my rendering in response to |
I've also managed to get winit to hit unreachable code inside |
@dylanede Also, could I have an example of the code that causes |
So, with I will boil down a minimal example to reproduce the triggering of |
The following code hits the unreachable code for me as soon as I move the mouse after starting the application. extern crate winit;
use winit::platform::desktop::EventLoopExtDesktop;
fn main() {
let mut event_loop = winit::event_loop::EventLoop::new();
let window = winit::window::WindowBuilder::new()
.build(&event_loop)
.expect("Failed to create window");
event_loop.run_return(move |event, event_loop, control_flow| {
use winit::event::WindowEvent;
use winit::event::Event;
use winit::event_loop::ControlFlow;
*control_flow = ControlFlow::Wait;
match event {
Event::EventsCleared => {
window.request_redraw();
}
Event::WindowEvent { event: WindowEvent::CloseRequested, .. } => {
*control_flow = ControlFlow::Exit;
}
Event::WindowEvent { event: WindowEvent::RedrawRequested, .. } => {
*control_flow = ControlFlow::Poll;
println!("Draw");
}
_ => {}
}
});
} |
I just ran all of the examples, and tried to break them in order to provide some useful feedback. This is what I have discovered. Errors in examples:
The other examples seemed to work correctly. I'm excited to see this PR progress! 🎉 |
Using a proxy in combination with |
Thanks for the reports! I've pushed fixes that seem to fix all of those issues. I haven't tested the |
I also still hit that unreachable code, but only in --release mode.
|
@Osspial I think the problem I mentioned with |
Hmm... I think this may be triggering undefined behavior somewhere, since removing the |
3c99691
to
9c15f03
Compare
@hardiesoft Your example should be fixed in the latest commit. @dylanede, can you run your |
@Osspial well done! That's stopped the crashes for me. |
Thanks @Osspial that seems to have done the trick for me too! |
What is the status of this? Can it be merged into |
@vberger AFAIK the main thing this is waiting on is a review of the code. @francesca64 volunteered to do it, but it doesn't seem like she's found the time to make that happen. |
Also, this is now merged against the latest master! It was a whole lot less of a PITA than rebasing, but it still takes time so I don't want to do that sort of thing yet again. (pro tip: if you've got a massive PR like this use |
Is there anything blocking the merge of this in the Or has this part of the initial plan been completely dropped? |
Nope, there isn't. I'll go ahead and merge this now. |
macOS is indeed complete (aside from some minor correctness issues that I know how to fix but haven't gotten around to yet), but I haven't done any work on X11 aside from the rough one I did during the API discussion. An X11 implementation should be easy, though.
That's reasonable. I'll definitely be handling the Android implementation, but it's not the priority right now.
I'm honestly not sure how many people will want to use/maintain Emscripten after #589 anyway, so I'm fine with this too. |
Just want to say: I’m delighted with this, because my Win32 EdgeHTML experimentation (Windows.Web.UI.Interop.WebViewControl, which incidentally requires a single-threaded apartment) had not been working, in a very opaque manner, and I finally today tried minimising and translating a known-good C++ implementation, and that worked; the key difference then was the use of winit, which I had erroneously thought a completely harmless component. I began to get suspicious of its event loop implementation (though I still don’t have any sort of grasp on why it makes an IAsyncOperation seem to be working but never complete, even when polling on the original thread). So I came to the issue tracker, and found recent discussion about all this and this very-recently-merged work; and guess what? It all works now. So with this work I will finally be able to resume work on a good web view control (MSHTML is disqualified from that classification, naturally). So: thanks for sorting this out! |
I just realized that, instead of aborting the entire process at the end of |
@francesca64 Do you have any sort of timeline for when you think the MacOS and X11 backends will be done by? I really don't want to be pushy, but these changes have been in progress for the better part of a year I'd like to move on from this. There's a bunch of stuff I'd like to make, both within Winit and without, that are either waiting on this to get going or would be significantly improved from having these overhauls on crates.io. I'm tired of having to choose between either writing Winit stuff that will make EL2.0 harder to merge, or writing stuff against the EL2.0 branch and not knowing when that code will see the light of day. |
If there have been any personal or work issues that have made it hard to work on this, that's okay. Stuff happens. But (and I'll admit that I absolutely need to get better on this too) going radio silent doesn't really work. |
I may be able to work on the x11 migration, with two caveats:
So if someone feels more competent and/or has more time, don't hesitate. :) |
winit Events Loop 2.0 with wasm32-unknown-unknown seems relevant to the |
@francesca64 If you don't have the time to finish the macOS implementation up, could you post a link to your current work and whatever remaining issues it has so that someone else could potentially pick it up and finish it? |
For the record, I'm in the process of porting the X11 backend to eventloop 2.0. I'm finished with the "fixing imports and moving stuff around" part, and I'm down to the hard part: implementing |
I intend to port the MacOS via to use of |
@zegentzy I have a Mac but not much time on my hands right now, but if you ping me during development I'll be able to build and run your branch within a day or two. |
@zegentzy I finally managed to find @francesca64's WIP implementation! This branch should provide a good start: https://github.com/francesca64/winit/tree/macos-ev2 She's said that there are issues with that implementation, but I don't know what those issues are. We need to test it to figure out what exactly isn't working, since she's never commented on what exactly those issues were. |
@@ -323,24 +323,80 @@ impl SeatManager { | |||
|
|||
fn remove_seat(&mut self, id: u32) { | |||
use self::wl_seat::RequestsTrait as SeatRequests; | |||
<<<<<<< HEAD:src/platform_impl/linux/wayland/event_loop.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an othersight that was fixed in #790
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I just wonder how CI didn't find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI didn't pass at all for the other platforms than windows, which were not ported yet in this PR.
Is there a reason for why extension traits were renamed to include the name of the operating system? This isn't usually the standard convention (and feels awkward imo) so I'm curious as to the reasoning for this. Also, this change wasn't documented in |
* Rename EventsLoop and associated types to EventLoop * Rename WindowEvent::Refresh to WindowEvent::Redraw * Remove second thread from win32 backend * Update run_forever to hijack thread * Replace windows Mutex with parking_lot Mutex * Implement new ControlFlow and associated events * Add StartCause::Init support, timer example * Add ability to send custom user events * Fully invert windows control flow so win32 calls into winit's callback * Add request_redraw * Rename platform to platform_impl * Rename os to platform, add Ext trait postfixes * Add platform::desktop module with EventLoopExt::run_return * Re-organize into module structure * Improve documentation * Small changes to examples * Improve docs for run and run_return * Change instances of "events_loop" to "event_loop" * Rename MonitorId to MonitorHandle * Add CHANGELOG entry * Improve WaitUntil timer precision * When SendEvent is called during event closure, buffer events * Fix resize lag when waiting in some situations * Update send test and errors that broke some examples/APIs * Improve clarity/fix typos in docs * Fix unreachable panic after setting ControlFlow to Poll during some RedrawRequested events. * Fix crash when running in release mode * Remove crossbeam dependency and make drop events work again * Remove serde implementations from ControlFlow * Fix 1.24.1 build * Fix freeze when setting decorations * Replace &EventLoop in callback with &EventLoopWindowTarget * Document and implement Debug for EventLoopWindowTarget * Fix some deadlocks that could occur when changing window state * Fix thread executor not executing closure when called from non-loop thread * Fix buffered events not getting dispatched * Fix crash with runner refcell not getting dropped * Address review feedback * Fix CHANGELOG typo * Catch panics in user callback
Changes winit's API as discussed in #459 and implements said changes on Windows.
This PR does not provide implementations for the new API on any non-Windows platform. Said platforms have not been tested, and there should be no expectations that they even compile. However, it does provide a working version of the API on Windows as well as API docs.
Before this is merged, we should thoroughly review the code changes and make sure all the documentation is correct/includes working links.
Changelog entry is included below for convenience and potential further discussion:
EventsLoop
toEventLoop
.event
,event_loop
,monitor
, andwindow
modules.os
module changes:platform
.desktop
module on Windows, Mac, and Linux.EventLoopProxy::wakeup
has been removed in favor ofsend_event
.run
method drives winit event loop.!
to ensure API behaves identically across all supported platforms.emscripten
implementation to work without lying about the API.ControlFlow
's variants have been replaced withWait
,WaitUntil(Instant)
,Poll
, andExit
.EventsCleared
is processed.Wait
waits until new events are available.WaitUntil
waits until either new events are available or the provided time has been reached.Poll
instantly resumes the event loop.Exit
aborts the event loop.'static + FnMut(Event<T>, &EventLoop<T>, &mut ControlFlow)
.&EventLoop<T>
is provided to allow newWindow
s to be created.platform::desktop
module exposesEventLoopExtDesktop
trait withrun_return
method.run
, but returns control flow to the calling context can take non-'static
closures.EventLoop
'spoll_events
andrun_forever
methods have been removed in favor ofrun
andrun_return
.Event::Awakened
in favor ofEvent::UserEvent(T)
.EventLoopProxy::send_event
.WindowEvent::Refresh
toWindowEvent::RedrawRequested
.RedrawRequested
can be sent by the user with theWindow::request_redraw
method.EventLoop
,EventLoopProxy
, andEvent
are now generic overT
, for use inUserEvent
.NewEvents(StartCause)
,EventsCleared
, andLoopDestroyed
variants toEvent
.NewEvents
is emitted when new events are ready to be processed by event loop.StartCause
describes why new events are available, withResumeTimeReached
,Poll
,WaitCancelled
, andInit
(sent once at start of loop).EventsCleared
is emitted when all available events have been processed.LoopDestroyed
is emitted when therun
orrun_return
method is about to exit.MonitorId
toMonitorHandle
.