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

Code before:
    TerminalConnection::ITerminalConnection ControlCore::Connection()
    {
        return _connection;
    }

    // Method Description:
    // - Setup our event handlers for this connection. If we've currently got a
    //   connection, then this'll revoke the existing connection's handlers.
    // - This will not call Start on the incoming connection. The caller should do that.
    // - 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)
    {
        const bool replacing = _connection != nullptr;
        TerminalConnection::ConnectionState oldConnectionState{ TerminalConnection::ConnectionState::NotConnected };

        if (replacing)
        {
            _connectionOutputEventRevoker.revoke();
            oldConnectionState = _connection.State();
        }
        // Bail early if we're not actually giving the Terminal a new connection
        if (newConnection == nullptr)
        {
            if (replacing)
            {
                _connectionStateChangedRevoker.revoke();
                _ConnectionStateChangedHandlers(*this, nullptr);
                _connection = nullptr;
            }
            return;
        }

        // 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>() })
        {

Review comment (refactor):
this has the same problem actually. We call the handlers before nulling the connection, so that when someone calls in they get the state of the connection that they just overwrote with null.


in general, I don't totally get why this method flows the way it does. early returns, roundabout ownership, etc.. It feels like it was refactored three times - it might be an opportunity to scour it completely and start anew!

could you do this instead?

```c++
void Connection(C newConn)
{
    auto oldState = ConnectionState(); // rely on ControlCore's automatic null handling
    // revoke ALL old handlers immediately

    _connection = newConn;
    if (_connection) {
        // do all hookups now, add ALL new handlers immediately, set window etc
    }

    if (oldState != ConnectionState()) { // rely on the null handling again
        // send the notification
    }
}
```

Code after:
    TerminalConnection::ITerminalConnection ControlCore::Connection()
    {
        return _connection;
    }

    // Method Description:
    // - Setup our event handlers for this connection. If we've currently got a
    //   connection, then this'll revoke the existing connection's handlers.
    // - This will not call Start on the incoming connection. The caller should do that.
    // - 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