{"action":"created","issue":{"url":"https://api.github.com/repos/tomaka/winit/issues/459","repository_url":"https://api.github.com/repos/tomaka/winit","labels_url":"https://api.github.com/repos/tomaka/winit/issues/459/labels{/name}","comments_url":"https://api.github.com/repos/tomaka/winit/issues/459/comments","events_url":"https://api.github.com/repos/tomaka/winit/issues/459/events","html_url":"https://github.com/tomaka/winit/issues/459","id":313948740,"number":459,"title":"API Change Proposal: Events Loop 2.0","user":{"login":"Osspial","id":16296823,"avatar_url":"https://avatars1.githubusercontent.com/u/16296823?v=4","gravatar_id":"","url":"https://api.github.com/users/Osspial","html_url":"https://github.com/Osspial","followers_url":"https://api.github.com/users/Osspial/followers","following_url":"https://api.github.com/users/Osspial/following{/other_user}","gists_url":"https://api.github.com/users/Osspial/gists{/gist_id}","starred_url":"https://api.github.com/users/Osspial/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Osspial/subscriptions","organizations_url":"https://api.github.com/users/Osspial/orgs","repos_url":"https://api.github.com/users/Osspial/repos","events_url":"https://api.github.com/users/Osspial/events{/privacy}","received_events_url":"https://api.github.com/users/Osspial/received_events","type":"User","site_admin":false},"labels":[{"id":474350557,"url":"https://api.github.com/repos/tomaka/winit/labels/A-api","name":"A-api","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-04-13T02:28:53Z","updated_at":"2018-04-18T14:27:01Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"This issue proposes a series of changes to winit's `EventsLoop` API, based around the experiences I've had with creating a GUI library that uses winit as a window creation backend. The main goal is to give winit complete control over the event loop, both for event polling and event waiting. These changes should let us fix various implementation bugs on the win32 backend, as well as fix what I believe are design bugs in the current API.\r\n\r\n## Technical Motivation:\r\n\r\nRight now, the `win32` backend creates a background thread, in which the events loop resides. The biggest problem with this is that it introduces various unexpected synchronization bugs that require a good amount of effort to fix and increase the complexity of the backend (see #415 and #391). However, it's necessary to cleanly handle `win32`'s various modal event loops.\r\n\r\nSo, what are these modal loops, and why do they matter? On Windows, when a user begins resizing a window, Windows suspends the program's main event loop and begins its own loop, inside of the resize event's handler. Unfortunately, this freezes any processing that's going on outside the Windows event loop, which is currently worked around by having the loop exist in its own thread, introducing quite a few bugs as mentioned above. Inside the loop, Windows does a bunch of additional processing, most notably handling window snapping (although it probably does other stuff too that isn't obvious).\r\n\r\nWindows doesn't publicly expose the APIs used to handle snapping, so re-implementing the modal loops ourselves so that they don't suspend the main event loop isn't really an option. However, Windows should still process events we push to the Windows events queue, letting us implement event polling inside the Windows event loop with custom events ([using the same mechanisms used for wakeup messages](https://github.com/tomaka/winit/blob/bdc01fee1a54470b07a897592927c26d1af60341/src/platform/windows/events_loop.rs#L312-L328)), as long as winit's API lets it assume complete control over program control flow. Right now, it doesn't, but this change would let us remove the second thread and simplify much of the implementation.\r\n\r\nActually removing the second thread isn't that difficult (see #445), but these bugs present pretty major blockers that should be resolved before that's merged.\r\n\r\n\r\n## Design Details\r\n\r\n* Depricate the `poll_events` function, in favor of `run_forever` with the expanded `ControlFlow` enum.\r\n* Change the definition of `ControlFlow` to the following:\r\n    ```rust\r\n    pub enum ControlFlow {\r\n        /// Equivalent to the current `Continue`. Suspends the thread until another OS event\r\n        /// arrives.\r\n        Wait,\r\n        /// Suspends the thread until either another event arrives, or the timeout expires.\r\n        WaitTimeout(Duration),\r\n        /// Replaces the `poll_events` method's functionality. After all the OS's events have\r\n        /// been processed, the callback is repeatedly called with `Event::Poll`, until the\r\n        /// next event arrives.\r\n        Poll,\r\n        /// Breaks out of the event loop.\r\n        Break\r\n    }\r\n    ```\r\n* Add the following variants to the `Event` enum:\r\n    ```rust\r\n    /// Sent if the time specified by `ControlFlow::WaitTimeout` has been elapsed. Contains the\r\n    /// moment the timeout was requested, and the requested duration of the timeout.\r\n    TimeoutExpired {\r\n        start: Instant,\r\n        duration: Duration\r\n    },\r\n    /// Sent if the OS has new events to send to the window, after a wait was requested. Contains\r\n    /// the moment the wait was requested, and if a wait timout was requested, its duration.\r\n    WaitCancelled {\r\n        start: Instant,\r\n        requested_duration: Option<Duration>\r\n    },\r\n    /// Sent after `ControlFlow::Poll` was returned by the callback, and all the OS events have\r\n    /// been exhausted.\r\n    Poll\r\n    ```\r\n* Change the event loop's callback definition, so that it takes a new type: `EventsLoopWindows`\r\n    * Change `Window::new()` and `WindowBuilder::build()` to take this new type.\r\n    * Implement `Deref<Target=EventsLoopWindows>` for `EventsLoop`.\r\n    * Motivation: Currently, a programmer can't create new windows while inside of `run_forever`, as the function mutably borrows `EventsLoop`. Adding this new type would allow them to do that.\r\n* Increase the importance of `WindowEvent::Refresh`:\r\n    * Make handling this event the *officially recommended* method of redrawing a window.\r\n    * Add the following function to `Window`:\r\n    ```rust\r\n    /// Makes the events loop send a `WindowEvent::Refresh` event, after all OS events have been\r\n    /// processed by the event loop (`WindowEvent`, `DeviceEvent`, `Awakened`, and `Suspended`), but before any\r\n    /// winit events have been processed (`TimeoutExpred`, `WaitCancelled`, and `Poll`).\r\n    fn queue_refresh(&self) {...}\r\n    ```\r\n    * Motivations:\r\n        * Integrates better with OS-generated redraw events, i.e. when a window is resized (on Windows, this is through the `WM_PAINT` message). By using this, we can handle window redrawing entirely within the OS callback, which is where the OS expects this to happen. This should improve the smoothness of window resizes, and will hopefully fix the current issue where resizing the window leaves some sort of blank space at the edges of the window, before the program has a chance to draw content to fill it.\r\n        * Makes it much easier for the program to redraw only when necessary, especially when waiting for events. Currently, if a program uses `run_forever` and wants to only redraws when user input is received, the simplest method is to redraw after every event is received, like so:\r\n\r\n        ```rust\r\n        events_loop.run_forever(|event, _| {\r\n            match event {\r\n                Event::WindowEvent{..} => {\r\n                    /* Update program state */\r\n                },\r\n                _ => ()\r\n            }\r\n\r\n            /* Redraw the window */\r\n\r\n            ControlFlow::Continue\r\n        });\r\n        ```\r\n\r\n        *This is the wrong approach*: it isn't uncommon for the OS to send multiple events in the duration of a single frame, and redrawing after every event wastes time rendering an image the program's user will never see, and can slow down the program. Right now, the only way I can see to do this properly is to do the following:\r\n\r\n        ```rust\r\n        loop {\r\n            let update_state = |event| {/* Updates program state */};\r\n            // Wait for the first OS event to be received, and then exit the event loop.\r\n            events_loop.run_forever(|event| {update_state(event); ControlFlow::Break});\r\n            // Handle the rest of the frame's events.\r\n            events_loop.poll_events(update_state);\r\n\r\n            /* Redraw the window */\r\n        }\r\n        ```\r\n\r\n        I don't think that's the kind of design we want to be encouraging, and neither the issue nor the fix is obvious to a programmer using winit. If `run_forever` is being used, the program should spend pretty much its entire lifetime within the loop, and right now that's discouraged. Using this API would let us write the event loop as the following:\r\n\r\n        ```rust\r\n        events_loop.run_forever(|event, _| {\r\n            match event {\r\n                Event::WindowEvent{..} => {\r\n                    /* Update program state */\r\n\r\n                    // Tells winit to queue a window refresh. Importantly, the refresh doesn't happen right\r\n                    // when this function is called, letting winit delay drawing until after we've processed\r\n                    // all the user input events and the program's state has stopped changing at a rate faster\r\n                    // than the user can see.\r\n                    window.queue_refresh();\r\n                },\r\n                Event::Refresh => {\r\n                    /* Redraw the window */\r\n                },\r\n                _ => ()\r\n            }\r\n\r\n            ControlFlow::Wait\r\n        });\r\n        ```\r\n\r\n        Which is both cleaner and a more direct way to have the program behave the right way.\r\n"},"comment":{"url":"https://api.github.com/repos/tomaka/winit/issues/comments/382405917","html_url":"https://github.com/tomaka/winit/issues/459#issuecomment-382405917","issue_url":"https://api.github.com/repos/tomaka/winit/issues/459","id":382405917,"user":{"login":"willglynn","id":118362,"avatar_url":"https://avatars3.githubusercontent.com/u/118362?v=4","gravatar_id":"","url":"https://api.github.com/users/willglynn","html_url":"https://github.com/willglynn","followers_url":"https://api.github.com/users/willglynn/followers","following_url":"https://api.github.com/users/willglynn/following{/other_user}","gists_url":"https://api.github.com/users/willglynn/gists{/gist_id}","starred_url":"https://api.github.com/users/willglynn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/willglynn/subscriptions","organizations_url":"https://api.github.com/users/willglynn/orgs","repos_url":"https://api.github.com/users/willglynn/repos","events_url":"https://api.github.com/users/willglynn/events{/privacy}","received_events_url":"https://api.github.com/users/willglynn/received_events","type":"User","site_admin":false},"created_at":"2018-04-18T14:27:01Z","updated_at":"2018-04-18T14:27:01Z","author_association":"NONE","body":"I think a breaking API change is necessary, but it must require the user to [entirely surrender their thread](https://github.com/tomaka/winit/issues/231#issuecomment-360254946). A callback-only API would fix Windows, it would fix MacOS (which has nearly identical resizing gotchas described in #219), it would fix the web (which needs to `return` to the browser to deliver more events), it would fix iOS and Android (which likewise expect to run the event loop), etc.\r\n\r\nGoing callback-only is a massive breaking change, but it's the right move. Most platforms expect to call user code from the UI thread when things happen, while `winit` tells its users that they can keep the UI thread and occasionally call into the platform. _That_ is why this is hard.\r\n\r\nDropping `run_forever()` in favor of a new API that takes over your thread would be a massive breaking change, but it is possible to glue both models back together again using the pattern from #237. We could make a `winit-compat` crate that provides `run_forever()` on top of a new callback-only API via coroutines, and it could work on the major platforms.\r\n\r\nHaving said all that, while I am convinced that this is the right way forward, I am not in a position to be the champion of this approach. I no longer use `winit` and can't justify investing the required time and effort into making this happen."}}
{"actor":{"display_login":"willglynn"}}