From de54072ad85f68f5fd2532b03faa1865322eabd1 Mon Sep 17 00:00:00 2001 From: Philippe Renon Date: Thu, 13 Feb 2020 01:07:26 +0100 Subject: [PATCH] on Windows, use a pending redraws set to keep track of request_redraw simplifies the message loops and event handling considerably strongly inspired from the X11 platform partially reverts https://github.com/rust-windowing/winit/commit/6a330a2894873d29fbbfdeebfc1a215577213996 fixes https://github.com/rust-windowing/winit/issues/1400 fixes https://github.com/rust-windowing/winit/issues/1391 --- CHANGELOG.md | 1 + src/platform_impl/windows/event_loop.rs | 123 ++++----- .../windows/event_loop/runner.rs | 244 +++++------------- src/platform_impl/windows/window.rs | 13 +- src/platform_impl/windows/window_state.rs | 4 - 5 files changed, 122 insertions(+), 263 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3bcf516de2..cb99209bbbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased +- On Windows, rework handling of request_redraw() to address panics. - On macOS, fix `set_simple_screen` to remember frame excluding title bar. - On Wayland, fix coordinates in touch events when scale factor isn't 1. - On Wayland, fix color from `close_button_icon_color` not applying. diff --git a/src/platform_impl/windows/event_loop.rs b/src/platform_impl/windows/event_loop.rs index 0049b09c898..526c2385971 100644 --- a/src/platform_impl/windows/event_loop.rs +++ b/src/platform_impl/windows/event_loop.rs @@ -227,56 +227,29 @@ impl EventLoop { break; } } - if msg.message == winuser::WM_PAINT { - unread_message_exists = true; - break; - } winuser::TranslateMessage(&mut msg); winuser::DispatchMessageW(&mut msg); unread_message_exists = false; - } - runner.main_events_cleared(); - loop { - if !unread_message_exists { - if 0 == winuser::PeekMessageW( - &mut msg, - ptr::null_mut(), - winuser::WM_PAINT, - winuser::WM_PAINT, - 1, - ) { - break; - } - } - winuser::TranslateMessage(&mut msg); - winuser::DispatchMessageW(&mut msg); - - unread_message_exists = false; - } - if runner.redraw_events_cleared().events_buffered() { - if runner.control_flow() == ControlFlow::Exit { - break 'main; + if msg.message == winuser::WM_PAINT { + break; } - continue; } - - if !unread_message_exists { - let control_flow = runner.control_flow(); - match control_flow { - ControlFlow::Exit => break 'main, - ControlFlow::Wait => { - if 0 == winuser::GetMessageW(&mut msg, ptr::null_mut(), 0, 0) { - break 'main; - } - unread_message_exists = true; + runner.events_cleared(); + match runner.control_flow() { + ControlFlow::Exit => break 'main, + ControlFlow::Wait => { + assert!(!unread_message_exists); + if 0 == winuser::GetMessageW(&mut msg, ptr::null_mut(), 0, 0) { + break 'main; } - ControlFlow::WaitUntil(resume_time) => { - wait_until_time_or_msg(resume_time); - } - ControlFlow::Poll => (), + unread_message_exists = true; + } + ControlFlow::WaitUntil(resume_time) => { + wait_until_time_or_msg(resume_time); } + ControlFlow::Poll => (), } } } @@ -676,7 +649,6 @@ unsafe extern "system" fn public_window_callback( } winuser::WM_PAINT => { - subclass_input.event_loop_runner.main_events_cleared(); subclass_input.send_event(Event::RedrawRequested(RootWindowId(WindowId(window)))); commctrl::DefSubclassProc(window, msg, wparam, lparam) } @@ -1721,49 +1693,44 @@ unsafe extern "system" fn thread_event_target_callback( let in_modal_loop = subclass_input.event_loop_runner.in_modal_loop(); if in_modal_loop { let mut msg = mem::zeroed(); - if 0 == winuser::PeekMessageW(&mut msg, ptr::null_mut(), 0, 0, 0) { - if msg.message != 0 && msg.message != winuser::WM_PAINT { - queue_call_again(); - return 0; + loop { + if 0 == winuser::PeekMessageW(&mut msg, ptr::null_mut(), 0, 0, 0) { + break; } - - subclass_input.event_loop_runner.main_events_cleared(); - loop { - if 0 == winuser::PeekMessageW( - &mut msg, - ptr::null_mut(), - winuser::WM_PAINT, - winuser::WM_PAINT, - 1, - ) { - break; + // Clear all paint/timer messages from the queue before sending the events cleared message. + match msg.message { + // Flush the event queue of WM_PAINT messages. + winuser::WM_PAINT | winuser::WM_TIMER => { + // Remove the message from the message queue. + winuser::PeekMessageW(&mut msg, ptr::null_mut(), 0, 0, 1); + + if msg.hwnd != window { + winuser::TranslateMessage(&mut msg); + winuser::DispatchMessageW(&mut msg); + } } - - if msg.hwnd != window { - winuser::TranslateMessage(&mut msg); - winuser::DispatchMessageW(&mut msg); + // If the message isn't one of those three, it may be handled by the modal + // loop so we should return control flow to it. + _ => { + queue_call_again(); + return 0; } } } - // we don't borrow until here because TODO SAFETY let runner = &subclass_input.event_loop_runner; - if runner.redraw_events_cleared().events_buffered() { - queue_call_again(); - runner.new_events(); - } else { - match runner.control_flow() { - // Waiting is handled by the modal loop. - ControlFlow::Exit | ControlFlow::Wait => runner.new_events(), - ControlFlow::WaitUntil(resume_time) => { - wait_until_time_or_msg(resume_time); - runner.new_events(); - queue_call_again(); - } - ControlFlow::Poll => { - runner.new_events(); - queue_call_again(); - } + runner.events_cleared(); + match runner.control_flow() { + // Waiting is handled by the modal loop. + ControlFlow::Exit | ControlFlow::Wait => runner.new_events(), + ControlFlow::WaitUntil(resume_time) => { + wait_until_time_or_msg(resume_time); + runner.new_events(); + queue_call_again(); + } + ControlFlow::Poll => { + runner.new_events(); + queue_call_again(); } } } diff --git a/src/platform_impl/windows/event_loop/runner.rs b/src/platform_impl/windows/event_loop/runner.rs index 44a044d43a1..f71e8ad6883 100644 --- a/src/platform_impl/windows/event_loop/runner.rs +++ b/src/platform_impl/windows/event_loop/runner.rs @@ -1,4 +1,13 @@ -use std::{any::Any, cell::RefCell, collections::VecDeque, mem, panic, ptr, rc::Rc, time::Instant}; +use parking_lot::Mutex; +use std::{ + any::Any, + cell::RefCell, + collections::{HashSet, VecDeque}, + mem, panic, ptr, + rc::Rc, + sync::Arc, + time::Instant, +}; use winapi::{shared::windef::HWND, um::winuser}; @@ -6,7 +15,7 @@ use crate::{ dpi::PhysicalSize, event::{Event, StartCause, WindowEvent}, event_loop::ControlFlow, - platform_impl::platform::{event_loop::EventLoop, util}, + platform_impl::platform::event_loop::{util, EventLoop, WindowId as PlatformWindowId}, window::WindowId, }; @@ -14,8 +23,9 @@ pub(crate) type EventLoopRunnerShared = Rc>; pub(crate) struct ELRShared { runner: RefCell>>, buffer: RefCell>>, - redraw_buffer: Rc>>, + pub pending_redraws: Arc>>, } + struct EventLoopRunner { control_flow: ControlFlow, runner_state: RunnerState, @@ -23,8 +33,9 @@ struct EventLoopRunner { in_modal_loop: bool, event_handler: Box, &mut ControlFlow)>, panic_error: Option, - redraw_buffer: Rc>>, + pending_redraws: Arc>>, } + pub type PanicError = Box; pub enum BufferedEvent { @@ -32,22 +43,6 @@ pub enum BufferedEvent { ScaleFactorChanged(WindowId, f64, PhysicalSize), } -#[must_use] -#[derive(Debug, Clone, Copy)] -pub enum AreEventsBuffered { - EventsBuffered, - ReadyToSleep, -} - -impl AreEventsBuffered { - pub fn events_buffered(&self) -> bool { - match self { - Self::EventsBuffered => true, - Self::ReadyToSleep => false, - } - } -} - impl BufferedEvent { pub fn from_event(event: Event<'_, T>) -> BufferedEvent { match event { @@ -89,7 +84,7 @@ impl ELRShared { ELRShared { runner: RefCell::new(None), buffer: RefCell::new(VecDeque::new()), - redraw_buffer: Default::default(), + pending_redraws: Default::default(), } } @@ -97,17 +92,14 @@ impl ELRShared { where F: FnMut(Event<'_, T>, &mut ControlFlow), { - let mut runner = EventLoopRunner::new(event_loop, self.redraw_buffer.clone(), f); + let runner = EventLoopRunner::new(event_loop, self.pending_redraws.clone(), f); { let mut runner_ref = self.runner.borrow_mut(); - loop { - let event = self.buffer.borrow_mut().pop_front(); - match event { - Some(e) => e.dispatch_event(|e| runner.process_event(e)), - None => break, - } - } *runner_ref = Some(runner); + if let Some(ref mut runner) = *runner_ref { + // Dispatch any events that were buffered. + self.dispatch_buffered_events(runner); + } } } @@ -119,39 +111,18 @@ impl ELRShared { let mut runner_ref = self.runner.borrow_mut(); if let Some(ref mut runner) = *runner_ref { runner.new_events(); - loop { - let buffered_event_opt = self.buffer.borrow_mut().pop_front(); - match buffered_event_opt { - Some(e) => e.dispatch_event(|e| runner.process_event(e)), - None => break, - } - } + // Dispatch any events that were buffered during the call to `new_events`. + self.dispatch_buffered_events(runner); } } pub(crate) unsafe fn send_event(&self, event: Event<'_, T>) { - let handling_redraw = self - .runner - .borrow() - .as_ref() - .map(|r| RunnerState::HandlingRedraw == r.runner_state) - .unwrap_or(false); - let mut send = None; - if handling_redraw { - if let Event::RedrawRequested(_) = event { - send = Some(event); - } else { - self.buffer_event(event); - } - } else { - send = Some(event); - } - if let Some(event) = send { - if let Err(event) = self.send_event_unbuffered(event) { - // If the runner is already borrowed, we're in the middle of an event loop invocation. Add - // the event to a buffer to be processed later. - self.buffer_event(event); - } + if let Err(event) = self.send_event_unbuffered(event) { + // If the runner is already borrowed, we're in the middle of an event loop invocation. Add + // the event to a buffer to be processed later. + self.buffer + .borrow_mut() + .push_back(BufferedEvent::from_event(event)); } } @@ -159,28 +130,8 @@ impl ELRShared { if let Ok(mut runner_ref) = self.runner.try_borrow_mut() { if let Some(ref mut runner) = *runner_ref { runner.process_event(event); - - let handling_redraw = if let RunnerState::HandlingRedraw = runner.runner_state { - true - } else { - false - }; - - if !handling_redraw { - // Dispatch any events that were buffered during the call to `process_event`. - loop { - // We do this instead of using a `while let` loop because if we use a `while let` - // loop the reference returned `borrow_mut()` doesn't get dropped until the end - // of the loop's body and attempts to add events to the event buffer while in - // `process_event` will fail. - let buffered_event_opt = self.buffer.borrow_mut().pop_front(); - match buffered_event_opt { - Some(e) => e.dispatch_event(|e| runner.process_event(e)), - None => break, - } - } - } - + // Dispatch any events that were buffered during the call to `process_event`. + self.dispatch_buffered_events(runner); return Ok(()); } } @@ -188,6 +139,20 @@ impl ELRShared { Err(event) } + fn dispatch_buffered_events(&self, runner: &mut EventLoopRunner) { + // We do this instead of using a `while let` loop because if we use a `while let` + // loop the reference returned `borrow_mut()` doesn't get dropped until the end + // of the loop's body and attempts to add events to the event buffer while in + // `process_event` will fail. + loop { + let buffered_event_opt = self.buffer.borrow_mut().pop_front(); + match buffered_event_opt { + Some(e) => e.dispatch_event(|e| runner.process_event(e)), + None => break, + } + } + } + pub(crate) unsafe fn call_event_handler(&self, event: Event<'static, T>) { if let Ok(mut runner_ref) = self.runner.try_borrow_mut() { if let Some(ref mut runner) = *runner_ref { @@ -197,21 +162,10 @@ impl ELRShared { } } - pub(crate) fn main_events_cleared(&self) { + pub(crate) fn events_cleared(&self) { let mut runner_ref = self.runner.borrow_mut(); if let Some(ref mut runner) = *runner_ref { - runner.main_events_cleared(); - } - } - - pub(crate) fn redraw_events_cleared(&self) -> AreEventsBuffered { - let mut runner_ref = self.runner.borrow_mut(); - if let Some(ref mut runner) = *runner_ref { - runner.redraw_events_cleared(); - } - match self.buffer.borrow().len() { - 0 => AreEventsBuffered::ReadyToSleep, - _ => AreEventsBuffered::EventsBuffered, + runner.events_cleared(); } } @@ -248,18 +202,6 @@ impl ELRShared { ControlFlow::Exit } } - - fn buffer_event(&self, event: Event<'_, T>) { - match event { - Event::RedrawRequested(window_id) => { - self.redraw_buffer.borrow_mut().push_back(window_id) - } - _ => self - .buffer - .borrow_mut() - .push_back(BufferedEvent::from_event(event)), - } - } } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -275,13 +217,15 @@ enum RunnerState { /// The event loop is handling the OS's events and sending them to the user's callback. /// `NewEvents` has been sent, and `MainEventsCleared` hasn't. HandlingEvents, + /// The event loop is handling the redraw events and sending them to the user's callback. + /// `MainEventsCleared` has been sent, and `RedrawEventsCleared` hasn't. HandlingRedraw, } impl EventLoopRunner { unsafe fn new( event_loop: &EventLoop, - redraw_buffer: Rc>>, + pending_redraws: Arc>>, f: F, ) -> EventLoopRunner where @@ -297,7 +241,7 @@ impl EventLoopRunner { Box, &mut ControlFlow)>, >(Box::new(f)), panic_error: None, - redraw_buffer, + pending_redraws, } } @@ -323,6 +267,8 @@ impl EventLoopRunner { } // When `NewEvents` gets sent after an idle depends on the control flow... + // Some `NewEvents` are deferred because not all Windows messages trigger an event_loop event. + // So we defer the `NewEvents` to when we actually process an event. RunnerState::Idle(wait_start) => { match self.control_flow { // If we're polling, send `NewEvents` and immediately move into event processing. @@ -414,23 +360,19 @@ impl EventLoopRunner { } match (self.runner_state, &event) { - (RunnerState::HandlingRedraw, Event::RedrawRequested(_)) => { - self.call_event_handler(event) + (RunnerState::HandlingRedraw, Event::RedrawRequested(window_id)) => { + self.pending_redraws.lock().insert(window_id.0); } - (RunnerState::New, Event::RedrawRequested(_)) - | (RunnerState::Idle(..), Event::RedrawRequested(_)) => { - self.new_events(); - self.main_events_cleared(); - self.call_event_handler(event); - } - (_, Event::RedrawRequested(_)) => { - panic!("redraw event in non-redraw phase"); + (_, Event::RedrawRequested(window_id)) => { + self.call_event_handler(Event::MainEventsCleared); + self.runner_state = RunnerState::HandlingRedraw; + self.pending_redraws.lock().insert(window_id.0); } (RunnerState::HandlingRedraw, _) => { - panic!( - "Non-redraw event dispatched durning redraw phase: {:?}", - event.map_nonuser_event::<()>().ok() - ); + warn!("non-redraw event in redraw phase"); + self.events_cleared(); + self.new_events(); + self.call_event_handler(event); } (_, _) => { self.runner_state = RunnerState::HandlingEvents; @@ -440,61 +382,13 @@ impl EventLoopRunner { } fn flush_redraws(&mut self) { - loop { - let redraw_window_opt = self.redraw_buffer.borrow_mut().pop_front(); - match redraw_window_opt { - Some(window_id) => self.process_event(Event::RedrawRequested(window_id)), - None => break, - } - } - } - - fn main_events_cleared(&mut self) { - match self.runner_state { - // If we were handling events, send the MainEventsCleared message. - RunnerState::HandlingEvents => { - self.call_event_handler(Event::MainEventsCleared); - self.runner_state = RunnerState::HandlingRedraw; - } - - RunnerState::HandlingRedraw => (), - - // If we *weren't* handling events, we don't have to do anything. - RunnerState::New | RunnerState::Idle(..) => (), - - // Some control flows require a NewEvents call even if no events were received. This - // branch handles those. - RunnerState::DeferredNewEvents(wait_start) => { - match self.control_flow { - // If we had deferred a Poll, send the Poll NewEvents and MainEventsCleared. - ControlFlow::Poll => { - self.call_event_handler(Event::NewEvents(StartCause::Poll)); - self.call_event_handler(Event::MainEventsCleared); - self.runner_state = RunnerState::HandlingRedraw; - } - // If we had deferred a WaitUntil and the resume time has since been reached, - // send the resume notification and MainEventsCleared event. - ControlFlow::WaitUntil(resume_time) => { - if Instant::now() >= resume_time { - self.call_event_handler(Event::NewEvents( - StartCause::ResumeTimeReached { - start: wait_start, - requested_resume: resume_time, - }, - )); - self.call_event_handler(Event::MainEventsCleared); - self.runner_state = RunnerState::HandlingRedraw; - } - } - // If we deferred a wait and no events were received, the user doesn't have to - // get an event. - ControlFlow::Wait | ControlFlow::Exit => (), - } - } + let windows: Vec<_> = self.pending_redraws.lock().drain().collect(); + for wid in windows { + self.process_event(Event::RedrawRequested(WindowId(wid))); } } - fn redraw_events_cleared(&mut self) { + fn events_cleared(&mut self) { match self.runner_state { // If we were handling events, send the MainEventsCleared message. RunnerState::HandlingEvents => { @@ -521,7 +415,9 @@ impl EventLoopRunner { // If we had deferred a Poll, send the Poll NewEvents and MainEventsCleared. ControlFlow::Poll => { self.call_event_handler(Event::NewEvents(StartCause::Poll)); + self.runner_state = RunnerState::HandlingEvents; self.call_event_handler(Event::MainEventsCleared); + self.runner_state = RunnerState::HandlingRedraw; self.flush_redraws(); self.call_event_handler(Event::RedrawEventsCleared); } @@ -535,7 +431,9 @@ impl EventLoopRunner { requested_resume: resume_time, }, )); + self.runner_state = RunnerState::HandlingEvents; self.call_event_handler(Event::MainEventsCleared); + self.runner_state = RunnerState::HandlingRedraw; self.flush_redraws(); self.call_event_handler(Event::RedrawEventsCleared); } diff --git a/src/platform_impl/windows/window.rs b/src/platform_impl/windows/window.rs index f3e7851a126..6779827073a 100644 --- a/src/platform_impl/windows/window.rs +++ b/src/platform_impl/windows/window.rs @@ -4,6 +4,7 @@ use parking_lot::Mutex; use raw_window_handle::{windows::WindowsHandle, RawWindowHandle}; use std::{ cell::Cell, + collections::HashSet, ffi::OsStr, io, mem, os::windows::ffi::OsStrExt, @@ -56,6 +57,8 @@ pub struct Window { // The events loop proxy. thread_executor: event_loop::EventLoopThreadExecutor, + + pending_redraws: Arc>>, } impl Window { @@ -136,14 +139,7 @@ impl Window { #[inline] pub fn request_redraw(&self) { - unsafe { - winuser::RedrawWindow( - self.window.0, - ptr::null(), - ptr::null_mut(), - winuser::RDW_INTERNALPAINT, - ); - } + self.pending_redraws.lock().insert(self.id()); } #[inline] @@ -770,6 +766,7 @@ unsafe fn init( window: real_window, window_state, thread_executor: event_loop.create_thread_executor(), + pending_redraws: event_loop.runner_shared.pending_redraws.clone(), }; let dimensions = attributes diff --git a/src/platform_impl/windows/window_state.rs b/src/platform_impl/windows/window_state.rs index 8150d1199a9..293fcc31cfa 100644 --- a/src/platform_impl/windows/window_state.rs +++ b/src/platform_impl/windows/window_state.rs @@ -29,9 +29,6 @@ pub struct WindowState { pub dpi_factor: f64, pub fullscreen: Option, - /// Used to supress duplicate redraw attempts when calling `request_redraw` multiple - /// times in `MainEventsCleared`. - pub queued_out_of_band_redraw: bool, pub is_dark_mode: bool, pub high_surrogate: Option, window_flags: WindowFlags, @@ -118,7 +115,6 @@ impl WindowState { dpi_factor, fullscreen: None, - queued_out_of_band_redraw: false, is_dark_mode, high_surrogate: None, window_flags: WindowFlags::empty(),