Repository: microsoft/terminal
File: src/host/PtySignalInputThread.cpp (C++)

Code before:
        //
        // Calling SetParent on a window that is WS_VISIBLE will cause the OS to
        // hide the window, make it a _child_ window, then call SW_SHOW on the
        // window to re-show it. SW_SHOW, however, will cause the OS to also set
        // that window as the _foreground_ window, which would result in the
        // pty's hwnd stealing the foreground away from the owning terminal
        // window. That's bad.
        //
        // SetWindowLongPtr seems to do the job of changing who the window owner
        // is, without all the other side effects of reparenting the window.
        // See #13066
        ::SetWindowLongPtr(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
    }
}

// Method Description:
// - Retrieves bytes from the file stream and exits or throws errors should the pipe state
//   be compromised.
// Arguments:
// - pBuffer - Buffer to fill with data.
// - cbBuffer - Count of bytes in the given buffer.
// Return Value:
// - True if data was retrieved successfully. False otherwise.
bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer,
                                    const DWORD cbBuffer)
{
    if (!_hFile)
    {
        return false;
    }

    DWORD dwRead = 0;
    if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr))
    {
        _hFile.reset();
        return false;
    }

    if (dwRead != cbBuffer)
    {
        return false;
    }

    return true;
}

// Method Description:
// - Starts the PTY Signal input thread.
[[nodiscard]] HRESULT PtySignalInputThread::Start() noexcept
{
    RETURN_LAST_ERROR_IF(!_hFile);

Review comment (bug):
Regarding...
```cpp
        auto lastError = GetLastError();
        if (lastError == ERROR_BROKEN_PIPE)
        {
            _Shutdown();
        }
        else
        {
            THROW_WIN32(lastError);
        }
```
Looks like we never really handled the thrown error, right? So I guess it's fine that we removed it. But should we at least collect and log it?

Code after:
    {
        // DO NOT USE SetParent HERE!
        //
        // Calling SetParent on a window that is WS_VISIBLE will cause the OS to
        // hide the window, make it a _child_ window, then call SW_SHOW on the
        // window to re-show it. SW_SHOW, however, will cause the OS to also set
        // that window as the _foreground_ window, which would result in the
        // pty's hwnd stealing the foreground away from the owning terminal
        // window. That's bad.
        //
        // SetWindowLongPtr seems to do the job of changing who the window owner
        // is, without all the other side effects of reparenting the window.
        // See #13066
        ::SetWindowLongPtr(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
    }
}

// Method Description:
// - Retrieves bytes from the file stream and exits or throws errors should the pipe state
//   be compromised.
// Arguments:
// - pBuffer - Buffer to fill with data.
// - cbBuffer - Count of bytes in the given buffer.
// Return Value:
// - True if data was retrieved successfully. False otherwise.
[[nodiscard]] bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer)
{
    if (!_hFile)
    {
        return false;
    }

    DWORD dwRead = 0;
    if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr))
    {
        if (const auto err = GetLastError(); err != ERROR_BROKEN_PIPE)
        {
            LOG_WIN32(err);
        }
        _hFile.reset();
        return false;
    }

    if (dwRead != cbBuffer)
    {
        return false;
    }

    return true;
}