Skip to content
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

Experiment with some conpty windowing fixes #16014

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zadjii-msft
Copy link
Member

  1. This break needs to be there. It's unclear what happens without it.
  2. Experimentally, try different window styles for the conpty window. These are locked to Dev and Canary builds (so we can try them and back them out if they don't work). This might affect

Comment on lines +334 to +336
// We're doing WS_MINIMIZEBOX | WS_SYSMENU to get a "minimize
// button" for the window, but not the rest of WS_OVERLAPPED, so
// that we don't have an actual border, caption, nothing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a minimize button? Does minimizing a window only work if there's a minimize button?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YUP

WS_OVERLAPPED | (WS_MINIMIZEBOX | WS_SYSMENU) | WS_POPUP :
WS_OVERLAPPEDWINDOW | WS_POPUP;
const auto exStyles = Feature_ConPtyHwndStyles::IsEnabled() ?
WS_EX_TRANSPARENT | WS_EX_LAYERED | WS_EX_NOACTIVATE :
Copy link
Member

@lhecker lhecker Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are any of these styles necessary? We should probably try creating a window without extended flags first, since regular message-only windows don't define any extended flags either.

That said, why don't we create a standard message-only window ala https://learn.microsoft.com/en-us/windows/win32/winmsg/window-features#message-only-windows ?
I.e. set the parent to HWND_MESSAGE instead of owner to create a HWND as lean as possible (= no gray squares hopefully) and then use GWLP_HWNDPARENT to set the actual parent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the pseudoconsole window to support programmatic minimize (which console application do. Those jerks.) it needs to be an actual minimizable window. It cannot be a message HWND. I think there's some details in Michael's original PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye

[Wednesday 2:52 PM] Evan Koschik

Hmm, overlappedwindow adds ws_maximizebox and minbox, which says that the window can be minimized. So I think just overlapped would break someone using the fake window to minimize the console.

Copy link
Member

@lhecker lhecker Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this and cannot reproduce WS_OVERLAPPEDWINDOW working any different from either WS_OVERLAPPED or just a basic message-only window. Here's the code I've used:

#include <Windows.h>

#include <cstdio>
#include <thread>

static LRESULT CALLBACK window_callback(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) noexcept
{
    char buffer[64];
    const char* name = &buffer[0];
    
    switch (message)
    {
    case WM_CREATE:
        name = "WM_CREATE           "; break;
    case WM_MOVE:
        name = "WM_MOVE             "; break;
    case WM_SIZE:
        name = "WM_SIZE             "; break;
    case WM_GETMINMAXINFO:
        name = "WM_GETMINMAXINFO    "; break;
    case WM_WINDOWPOSCHANGING:
        name = "WM_WINDOWPOSCHANGING"; break;
    case WM_WINDOWPOSCHANGED:
        name = "WM_WINDOWPOSCHANGED "; break;
    case WM_GETICON:
        name = "WM_GETICON          "; break;
    case WM_NCCREATE:
        name = "WM_NCCREATE         "; break;
    case WM_NCCALCSIZE:
        name = "WM_NCCALCSIZE       "; break;
    default:
        sprintf_s(buffer, "%04x                ", message); break;
    }

    printf("%s %20lld %20lld\n", name, wparam, lparam);
    return DefWindowProcW(hwnd, message, wparam, lparam);
}

int main()
{
    WNDCLASSEXW wcex{
        .cbSize = sizeof(WNDCLASSEX),
        .lpfnWndProc = window_callback,
        .lpszClassName = L"test",
    };
    RegisterClassExW(&wcex);

    const auto hwnd = CreateWindowExW(
        0,
        L"test",
        L"test",
        0,            // you can set this to WS_OVERLAPPED instead and...
        0,
        0,
        0,
        0,
        HWND_MESSAGE, // ...set this to nullptr
        nullptr,
        nullptr,
        nullptr
    );

    std::thread t([hwnd]() {
        Sleep(1000);

        printf("--- ShowWindow(hwnd, SW_MINIMIZE) ---\n");
        fflush(stdout);

        ShowWindow(hwnd, SW_MINIMIZE);

        Sleep(100);
        exit(0);
    });

    MSG msg;
    while (GetMessageW(&msg, hwnd, 0, 0)) {
        DispatchMessageW(&msg);
    }

    return 0;
}

Am I doing something wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that there are weird edge cases for things like restoring a window, that don't apply to message only windows. Like, WM_SIZE or SYSCOMMAND or something weird like that. IIRC, user32 doesn't track the actual window state for message-only windows, so trying to minimize/restore/maximize a message only window doesn't work exactly right. That's why we needed a more real window than an actual message-only one.

I may be mistaken. It's been a while since this code was written.

Copy link
Member

@lhecker lhecker May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: The discussion in the other issue.

Like, message windows have different state that's tracked by user32. So if you query a message window after a SW_MINIMIZE, user32 doesn't actually think that window's minimized.

I believe that's correct. My understanding is also that message-only windows don't take part in any of the user32 machinery. In particular, Raymond writes:

This system-managed common parent window [HWND_MESSAGE] is permanently invisible, which results in message-only windows being permanently invisible. And that’s also how message-only windows are invisible to enumeration and broadcasts: Enumeration and broadcasting is done to top-level windows, but message-only windows are internally treated as child windows of HWND_MESSAGE and therefore are not considered top-level.

However, that doesn't mean they don't store state or attributes. All the relevant Win32 functions I can think of (well, primarily IsIconic) still fully work. I don't think there's anything relevant that a message-only window can't do that a regular window can. This is what a minimized message-only window looks like:
image

I think a HWND_MESSAGE window is technically optimal for us, but personally I'm concerned about two things:

  • It's not owned by Windows Terminal. I'm not sure whether that's something we need to care about though.
    I do know we have the GetAncestor(pseudoHwnd, GA_ROOTOWNER) code, but that code can just be replaced with directly using the owner HWND the PseudoConsoleWindow was created with. (Not sure why I didn't realize that any earlier...)
  • We'd probably have to check with the people who recently seem to have implemented foreground boosting for WT, to ensure it still works with a HWND_MESSAGE.

Alternatively, I think we should modify our flags to just be this:

const auto windowStyle = WS_MINIMIZEBOX | WS_POPUP;
const auto exStyles = WS_EX_NOACTIVATE;

WS_EX_LAYERED allocates a DWM surface which we shouldn't use unless needed I think. WS_EX_TRANSPARENT enables click-through which should not be relevant for a 0x0 pixel window. Neither of those are needed though if WS_POPUP because then the window has no frame or titlebar and so it'll not be visible anyway. WS_OVERLAPPED is 0 and can be dropped and I don't think WS_SYSMENU works for a WS_POPUP window since it has no titlebar. Using only these 3 flags results in a small but noticeable reduction in memory usage which I take as an indication that it does less things which is probably good for us.

However, in the end nothing beats a HWND_MESSAGE, which saves us an easy 10% of CPU time during ConPTY startup. And it's probably way way more robust against these issues, precisely because it circumvents all of the shell32 machinery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants