State of the Windows platform ?

[filnet]

Hi,

As a foreword, I am new to Rust and winit. Also, English is not my native language, so please take what follows with a grain of salt.

My understanding about winit on Windows is that:

• it was recently updated quite heavily (in 0.20.0) to go from multi threaded to single threaded.
• it has some complexity due to how Windows handles resizing and moving windows (switches to a modal mode where event query methods do not return anymore).

I've been looking at the code to address some issues [1][2][3] and it looks like the Windows code has inherited some complexity from the time it was multi threaded (?).

Thing's I have noticed:

• there is some code duplication regarding event handling and runner state changes (in runner::process_event, runner::main_events_cleared and runner::draw_events_cleared)
• state changes are located a bit everywhere and are not always consistent or enforced.
• DeferredNewEvents handling seems brittle in particular when control flow is WaitUntil.
• [nitpick] why the single runner.rs file in the event_loop directory

I am open to be given directions or to discuss ways to improve the situation if it needs to.

[1] #1391
[2] #1427
[3] #1428

PS: Looks like some projects are rolling back their winit 0.20.0 update because of issue [1].
See aclysma/skulpin#39

PPS : I have two PRs open :

• Fix panic at redraw event in non redraw phase #1422
• fix issues in wait_until_time_or_msg function #1423

[filnet]

One concrete example:

pub(crate) struct SubclassInput<T: 'static> {
    pub window_state: Arc<Mutex<WindowState>>,
    pub event_loop_runner: EventLoopRunnerShared<T>,
    pub file_drop_handler: FileDropHandler,
}

Is the Mutex protecting WindowState still needed ?

[filnet]

@Osspial, I believe this recent change has broken the Windows platform in a few ways : 6a330a2#diff-d4d8c8dcf07874a35d08c6a70e0a6daf

It most probably introduced this regression: #1427
The change introduced a borrow() that can fail and fails. Similar borrows done right after it use try_borrow() and handle the case when it fails.

I am also pretty sure it introduced #1391.

And I found a suspect code change:

               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;

Here the test on msg after the PeekMessageW() call is pointless as we are in the case where there is no message.

I will try to revert the fix and reproduce the missing RedrawRequested issue.
Were there any specifics needed to reproduce the issue?

[filnet]

PS: there is an open PR for issue #1391 : #1422

But I am not too happy about it...

[filnet]

I reverted 6a330a2 and was able to reproduce an issue that looks similar to the one addressed by that fix.

If the user calls window.request_redraw() while handling the MainEventsCleared event, then the RedrawRequested event is sent on the next loop iteration and not the current iteration.
Calling window.request_redraw() while handling the other events works fine.

I am now trying to fix the issue in a different way...

[filnet]

#1427 was closed as a duplicate of #1400

[filnet]

For reference:

    /// Emits a `WindowEvent::RedrawRequested` event in the associated event loop after all OS
    /// events have been processed by the event loop.
    ///
    /// This is the **strongly encouraged** method of redrawing windows, as it can integrate with
    /// OS-requested redraws (e.g. when a window gets resized).
    ///
    /// This function can cause `RedrawRequested` events to be emitted after `Event::MainEventsCleared`
    /// but before `Event::NewEvents` if called in the following circumstances:
    /// * While processing `MainEventsCleared`.
    /// * While processing a `RedrawRequested` event that was sent during `MainEventsCleared` or any
    ///   directly subsequent `RedrawRequested` event.
    ///
    /// ## Platform-specific
    ///
    /// - **iOS:** Can only be called on the main thread.
    #[inline]
    pub fn request_redraw(&self) {
        self.window.request_redraw()
    }

My understanding is that calling request_redraw should cause an RedrawRequested event to be sent during the current loop iteration not matter when this is called (when handling a MainEventsCleared event or even a RedrawRequested (possible infinite loop here?)

[filnet]

request_redraw in 0.21.0

    pub fn request_redraw(&self) {
        unsafe {
            winuser::RedrawWindow(
                self.window.0,
                ptr::null(),
                ptr::null_mut(),
                winuser::RDW_INTERNALPAINT,
            );
        }
    }

request_redraw in 0.20.0-alpha1

   #[inline]
    pub fn request_redraw(&self) {
        unsafe {
            if self.thread_executor.trigger_newevents_on_redraw() {
                winuser::RedrawWindow(
                    self.window.0,
                    ptr::null(),
                    ptr::null_mut(),
                    winuser::RDW_INTERNALPAINT,
                );
            } else {
                winuser::PostMessageW(self.window.0, *REQUEST_REDRAW_NO_NEWEVENTS_MSG_ID, 0, 0);
            }
        }
    }

[filnet]

What I am seeing after reverting the commit is confirmed by the associated PR description (which I overlooked) : #1366

Continuing my effort to understand and fix request_redraw on Windows, I did a bit of forensic and looked at how other platforms handle it.

On Windows, the current approach is to post a WM_PAINT message.
Older versions (0.20.0-alpha) also posted a REQUEST_REDRAW_NO_NEWEVENTS_MSG_ID message to distinguish request_redraw called after or before events were cleared. Not sure why.
These approaches are tricky because you need to dispatch these messages after clearing the main events.

Other platforms use a different approach: they raise a flag or buffer the redraw internally when request_redraw is called.

X11:

    pub fn request_redraw(&self) {
        self.pending_redraws
            .lock()
            .unwrap()
            .insert(WindowId(self.xwindow));
    }

Wayland:

    pub fn request_redraw(&self) {
        *self.need_refresh.lock().unwrap() = true;
    }

These approaches seem much easier to handle. Problem is that on Windows, the window struct does not have access to the event_loop or runner.

@Osspial any thoughts on that ?

[Osspial]

Hey, thanks for opening an issue to keep track of all this. I'll write up a more thorough response within the next couple days when I've got the time to look into this more. Sorry I didn't get back on this sooner - I was fairly stressed over the past month and wasn't keeping tabs on Winit to help manage that, but I'm feeling quite a bit better now and should be able to respond more promptly for the foreseeable future.

These approaches seem much easier to handle. Problem is that on Windows, the window struct does not have access to the event_loop or runner.

@Osspial any thoughts on that ?

The rationale behind using WM_PAINT is that that's the best way to get the OS to merge user-requested and OS-requested redraw events. We could theoretically keep track of that information ourselves and emit RedrawRequested events ourselves when the event loop has been drained, but Windows strongly prefers that you execute your redrawing code in the WM_PAINT handler rather than outside of it. That creates a tricky balance for Winit, since it's difficult to both satisfy its external API and winapi's preferences at the same time. #1366 was my initial attempt to address that, which... clearly didn't work out all that great. I've got ideas on how to handle that better, but it's been a bit since I've looked at this code closely so they aren't coming to mind at this particular moment.

[filnet]

Hi @Osspial,

Thanks for getting back!

I have looked at the request_redraw vs WM_PAINT issue quite a bit these last couple of weeks.
I looked at older 0.20.0 alpha versions and at your recent attempt at fixing related issues.

I also looked at how other platforms handle it and came up with this approach: #1461. Please take a look.

We can discuss it further here or in the PR.

[filnet]

If redrawing must occur only when receiving a WM_PAINT message then my PR does not comply.

I am now looking into another approach using UpdateWindow instead. It looks promising.
Calling UpdateWindow from request_redraw will cause the WM_PAINT message to be sent immediately. This removes the need to Peek/Get/Dispatch messages after calling main_events_cleared() in the event the user calls request_redraw when handling the MainEventsCleared event (which is the root of the problems).

Looks like RedrawWindow can work in that mode too if the RDW_UPDATENOW flag is set.