Fix resizing issues, remove multi-threaded message handling from WinGUI variant

[clangen]

Overview

Some time ago a change was merged into PDCursesMod to offload Window message handling to a background thread to make resizing happen more fluidly: dd59e4c

Unfortunately, this caused some issues with my apps. Specifically, I augment existing PDCursesMod functionality to do things like change the app icon and support minimizing to the notification tray. To do this, I get a reference to the main Window Handle (HWND), and call various Win32 API methods against it. Unfortunately, I am making these calls from the main app thread, not the thread running the message pump, so it leads to weird/undefined behavior that usually manifests as the app freezing at random times during operation, and very often during startup and shutdown when I'm making these API calls.

Why does PDCursesMod use a non-main Window thread?

This had me scratching my head for a while, but after a couple hours of debugging I figured it out. When getch() or one of its variants are called, pending key events are returned to the caller, but only after all pending Windows messages have been processed by the internal message pump.

The main message pump in PDCursesMod (and most Win32) apps looks like this, in PDC_napms:

while( PeekMessage(&msg, NULL, 0, 0, PM_REMOVE) )
{
    TranslateMessage(&msg);
    DispatchMessage(&msg);
}

Basically: it runs a tight loop until the message queue is completely drained.

However, when a resize event starts (i.e the user clicks the resize handle), the message pump remains active, as the Windows runtime is using it to monitor and process mouse moves, converting them to WM_SIZE and WM_SIZING messages. That means that the curses KEY_RESIZE event is not returned to the caller until the message pump is drained, and all sizing has finished. So, while the native window itself updates and changes size, the client app does not have an opportunity to update its curses WINDOW and/or PANEL instances until it's too late (i.e. sizing has finished).

Offloading the message processing to a separate thread, along with some careful synchronization via CRITICAL_SECTION addresses this immediate issue, but it introduces other problems as described above (i.e. whenever the user needs to do something with the main HWND directly).

A potential solution

On UNIX platforms we can handle the SIGWINCH signal to be notified when the controlling terminal changes size, then update our WINDOW and PANEL instances accordingly. Unfortunately we don't have this luxury on Windows.

However, we could add a WinGui specific callback to the PDC API that fires during WM_SIZE, whenever the Window has changed dimensions. The calling app could then use this the same way it handles SIGWINCH on UNIX platforms to update layout, and then we're good to go.

Clearly the downside is this introduces a non-standard, bespoke API for only WinGui, but it would do away with the goofy CRITICAL_SECTION management stuff, and also generally make things more stable.

Code changes

Unfortunately, I realize that the introduction of a custom API just for WinGui is undesirable and will likely result in this change not being merged. However, I wanted to share my findings and a sample implementation, as it may be useful for coming up with a more robust solution.

This proof-of-concept implementation spans 4 commits; it's probably easiest to view this commit-by-commit with the following commentary:

1. clangen@dd59e4c: reverted the original commit that introduced multi-threading.
2. clangen@451b213: noticed some weird background painting issues while resizing, so I simplified the code to ensure it always just paints a black background before anything else.
3. clangen@26d39f4: added the bespoke API described above that will fire a callback whenever the Window size changes. Also fixed a bit of mouse handling code in the WndProc, as it had been changed upstream (for the better)
4. clangen@2be5a26: added double buffering to the WM_PAINT handling to ensure there is no/minimal flicker when aggressively resizing. Windows isn't very good about batching draw commands to an on-screen DC, so we accumulate all updates to an offscreen buffer, then blit it back to the screen all at once. (This is an old Win32 trick to deal with this sort of issue).

Demo

Here's what it looks like, said and done:

2022-07-24.17-22-52.mov

[GitMensch]

That is most reasonable for me. Just out of interest @clangen:

• Can you show the same demo with an unpatched WinGUI?
• What is the result before/after with the wincon port (and ideally vt)? Do they share similar issues on Win32?

[Bill-Gray]

Unfortunately, I realize that the introduction of a custom API just for WinGui is undesirable and will likely result in this change not being merged

Not sure I'd jump to that conclusion. The dual-threaded solution has been a real annoyance, and really solves only two minor problems : (1) while a window is resized, you can't process anything; (2) the background of the window can get mangled during the resizing (but becomes okay once you stop resizing and the program gets a KEY_RESIZE and can redraw.) We lived quite happily for nine years with both issues, and I'd be content to do so again.

As you note, the two-thread solution causes problems in accessing the main window. It also makes the code a tangled mess. I've made fixes (see commit b84cf0c, issue #197 and issue #212) where mysterious bugs cropped up. They were the stereotypical sort of bugs you get with multithead solutions : hard to reproduce, hard to diagnose. I was extremely pleased when William made the X11 port run in one process, for similar reasons.

Forgive me if I'm overlooking something here, but from your description and the video, it looks as if the callback function enables us to evade both of the above issues. True, it's WinGUI specific. But you can ignore it, and the only consequence would be that when resizing in WinGUI, you''d see problems (1) and (2) above. Is that a fair summary?

If so, the WinGUI specific nature of your fix is not particularly troubling to me. I expect that 99% of people will ignore it. Frankly, we had (1) and (2) for nine years without complaint. But your solution would mean that in those 1% of cases where resizing must look good, you can set up a callback function and be okay.

[Bill-Gray]

...And in the Department of Really Minor Issues : in wingui/pdcscrn.c, you've got

static void PrepareBackBuffer(HDC hdc, RECT rect)
{
    memset(&backBuffer, 0, sizeof(backBuffer));
    int width = rect.right - rect.left;
    int height = rect.bottom - rect.top;
    backBuffer.rect = rect;

causing compilation failures on OpenWATCOM, which is a ISO C89 (maybe C90?) compiler... anyway, it uses the C standard in which the memset() will have to go under the declarations/assignments of width and height.

[clangen]

That is most reasonable for me. Just out of interest @clangen:

• Can you show the same demo with an unpatched WinGUI?
• What is the result before/after with the wincon port (and ideally vt)? Do they share similar issues on Win32?

Sure, here's what it looks like with the current multi-threaded solution:

resize_with_multi-threaded_code.mov

The wincon and vt ports are completely unaffected by this change, so they will continue to work however they do today.

Forgive me if I'm overlooking something here, but from your description and the video, it looks as if the callback function enables us to evade both of the above issues. True, it's WinGUI specific. But you can ignore it, and the only consequence would be that when resizing in WinGUI, you''d see problems (1) and (2) above. Is that a fair summary?

Yep, that's correct -- you're given an escape hatch to perform computations if necessary while the callback runs, including performing layout operations/transformations with your WINDOW and PANEL instances.

causing compilation failures on OpenWATCOM, which is a ISO C89 (maybe C90?) compiler... anyway, it uses the C standard in which the memset() will have to go under the declarations/assignments of width and height.

Ah, right. That should be easy to fix. Thanks for pointing it out!

I also found another issue where the cursor doesn't seem to update properly during resize operations after my change. It just sort of stays anchored in its original place until the drag operation completes, then it snaps into position. I'm going to play around with it a bit more to see if I can figure out what's going on there. Never mind, this was just a bug in my app code.

[clangen]

Alright, made a couple additional changes and CI looks happy. Let me know if there are any other changes you'd like me to take a look at. :)

[clangen]

One additional thing that may be nice: also double buffer the repaint from doupdate().

However, that doesn't seem to be easy to cleanly because the implementation lives in the shared refresh.c, and we'd need to figure out how to take a custom code path. I could #ifdef around the PDC_transform_line to disable it for WinGUI, then simply send a WM_PAINT message to the main HWND in the WinGUI version of PDC_doupdate in wingui/pdcdisp.c, but that feels pretty gross.

Definitely not strictly a requirement, as I don't observe any flicker from doupdate() most of the time, but it would be cool to have a single way to repaint the window when multiple lines change, taking advantage of double buffering wherever possible.

[Bill-Gray]

I've been sidetracked by day-job stuff, but this is looking quite good. I see that adding the callback function costs us about ten lines of entirely readable and understandable code. Can't argue with that.

I gave a try to a single-thread version on my machine, running on Wine in Linux, nothing except the reversion and updating, and... everything looked good. Then I remembered that we never had the problem under Wine to begin with; Wine, oddly enough, handles these resizing issues "correctly", with no blocking. Unlike, say, Microsoft's product. (Really, the ideal fix here would be for MS to handle resizing without locking everything up. But there are probably programs out there that rely on that mis-feature.)

I do still have a "real" Win1 0 machine, and will give it a go on that... with luck, I'll get a shot at it this weekend, then merge it.

Agreed on the double buffering involving uglier code than we'd really like to have. I'd suggest holding off on that for at least a while; maybe something cleaner will occur to us.

[Bill-Gray]

@clangen - I've now tried this out on both Linux/Wine and Win1 0, and it all looks good to me. Shall I click on 'Merge pull request' now, or do you still have tweaks in mind?

The only regression I saw was that in Wine, the removal of fix_up_edges() causes artifacts at the right and bottom edges. (A problem avoided in "real" Windows by botching the way sizing is handled.) I may, or may not, put it back in after pulling this patch. The problem is a small one affecting the small group using Wine. Then again, we've got the code and may as well enjoy it... just mention this in case you see fix_up_edges() and wonder why it's come back to life.

[clangen]

I don't have any more tweaks in mind, and can even add the fix_up_edges() code back if you'd like. Let me know if you'd like me to go ahead and do that; otherwise, merge at will! :)