Repository: microsoft/terminal
File: src/cascadia/TerminalControl/ControlCore.cpp (C++)

Code before:
    {
        if (_connection)
        {
            _connectionOutputEventRevoker.revoke();
        }

        // Subscribe to the connection's disconnected event and call our connection closed handlers.
        _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) {
            _ConnectionStateChangedHandlers(*this, nullptr);
        });

        // Fire off a connection state changed notification, to let our hosting
        // app know that we're in a different state now.
        if (newConnection.State() != _connection.State())
        {
            _ConnectionStateChangedHandlers(*this, nullptr);
        }

        // Get our current size in rows/cols, and hook them up to
        // this connection too.
        {
            const auto vp = _terminal->GetViewport();
            const auto width = vp.Width();
            const auto height = vp.Height();

            newConnection.Resize(height, width);
        }
        // Window owner too.
        if (auto conpty{ newConnection.try_as<TerminalConnection::ConptyConnection>() })
        {
            conpty.ReparentWindow(_owningHwnd);
        }
        const bool replacing = _connection != nullptr;
        if (replacing)
        {
            _connection.Close();
        }
        _connection = newConnection;
        // This event is explicitly revoked in the destructor: does not need weak_ref
        _connectionOutputEventRevoker = _connection.TerminalOutput(winrt::auto_revoke, { this, &ControlCore::_connectionOutputHandler });
        if (replacing)
        {
            _connection.Start();
        }
    }

    bool ControlCore::Initialize(const float actualWidth,
                                 const float actualHeight,
                                 const float compositionScale)
    {
        assert(_settings);

Review comment (bug):
Some of these things are new code, like this block. I think it's fine to be a little overzealous when calling such functions, but the implementations of these functions in turn are already overzealous themselves, which causes this call to end up being effectively an RPC message into OpenConsole. Either we should try to avoid doing more than necessary here, or `Resize` should check for `_rows != rows`.

Put differently: I like the way this code works - It's very plain, and simple to follow. But I don't think the current architecture of `ConptyConnection` helps this code be simple and safe. To continue the above example, we construct it with a `ValueSet` that contains the row/cols, but the code that should be authority over the size of the connection (= this function (and class), because it owns the `_terminal` which owns the `_mainBuffer`) is not the one that constructs the `ValueSet`. That's done in `TerminalPage`, even though `TerminalPage` doesn't have access to the `TextBuffer` that contains the authoritative buffer size? And so, we got two places that determine the rows/cols and they aren't in the same file. That makes this function bug-prone, be it because the two places may be out-of-sync, or because they accidentally cause RPCs like here.

BTW there's also an option to be a bit repetitive in this function and just wrap all of it in a single `if (_connection) {} else {}` statement, if it makes things simpler.

Code after:
    // - If the caller doesn't want the old connection to be closed, then they
    //   should grab a reference to it before calling this (so that it doesn't
    //   destruct, and close) during this call.
    void ControlCore::Connection(const TerminalConnection::ITerminalConnection& newConnection)
    {
        auto oldState = ConnectionState(); // rely on ControlCore's automatic null handling
        // revoke ALL old handlers immediately

        _connectionOutputEventRevoker.revoke();
        _connectionStateChangedRevoker.revoke();

        _connection = newConnection;
        if (_connection)
        {
            // Subscribe to the connection's disconnected event and call our connection closed handlers.
            _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) {
                _ConnectionStateChangedHandlers(*this, nullptr);
            });

            // Get our current size in rows/cols, and hook them up to
            // this connection too.
            {
                const auto vp = _terminal->GetViewport();
                const auto width = vp.Width();
                const auto height = vp.Height();

                newConnection.Resize(height, width);
            }
            // Window owner too.
            if (auto conpty{ newConnection.try_as<TerminalConnection::ConptyConnection>() })
            {
                conpty.ReparentWindow(_owningHwnd);
            }

            // This event is explicitly revoked in the destructor: does not need weak_ref
            _connectionOutputEventRevoker = _connection.TerminalOutput(winrt::auto_revoke, { this, &ControlCore::_connectionOutputHandler });
        }

        // Fire off a connection state changed notification, to let our hosting
        // app know that we're in a different state now.
        if (oldState != ConnectionState())
        { // rely on the null handling again
            // send the notification
            _ConnectionStateChangedHandlers(*this, nullptr);
        }
    }

    bool ControlCore::Initialize(const float actualWidth,
                                 const float actualHeight,
                                 const float compositionScale)
    {