Repository: microsoft/terminal
File: src/cascadia/TerminalApp/ContentManager.cpp (C++)

Code before:

#include "../../types/inc/utils.hpp"

using namespace winrt::Windows::ApplicationModel;
using namespace winrt::Windows::ApplicationModel::DataTransfer;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Windows::UI::Core;
using namespace winrt::Windows::System;
using namespace winrt::Microsoft::Terminal;
using namespace winrt::Microsoft::Terminal::Control;
using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt
{
    namespace MUX = Microsoft::UI::Xaml;
    using IInspectable = Windows::Foundation::IInspectable;
}
namespace winrt::TerminalApp::implementation
{
    ControlInteractivity ContentManager::CreateCore(Microsoft::Terminal::Control::IControlSettings settings,
                                                    IControlAppearance unfocusedAppearance,
                                                    TerminalConnection::ITerminalConnection connection)
    {
        auto content = ControlInteractivity{ settings, unfocusedAppearance, connection };
        content.Closed({ this, &ContentManager::_closedHandler });
        _content.Insert(content.Id(), content);
        return content;
    }

    ControlInteractivity ContentManager::LookupCore(winrt::guid id)
    {
        return _content.TryLookup(id);
    }

    void ContentManager::_closedHandler(winrt::Windows::Foundation::IInspectable sender,
                                        winrt::Windows::Foundation::IInspectable e)
    {
        if (const auto& content{ sender.try_as<winrt::Microsoft::Terminal::Control::ControlInteractivity>() })
        {
            const auto& contentGuid{ content.Id() };
            _content.TryRemove(contentGuid);
        }
    }
}


Review comment (bug):
Given that this function returns the `ControlInteractivity` instance it's not immediately obvious whether it's guaranteed that `ContentManager` lives longer than every single `ControlInteractivity` it manages. If any async coroutine is holding onto a `ControlInteractivity` instance, while `ContentManager` is already deallocated, could it cause a crash? In any case, since the `Closed` event is very rare, I think a `get_weak()` won't hurt in either case.

Code after:

#include "../../types/inc/utils.hpp"

using namespace winrt::Windows::ApplicationModel;
using namespace winrt::Windows::ApplicationModel::DataTransfer;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Windows::UI::Core;
using namespace winrt::Windows::System;
using namespace winrt::Microsoft::Terminal;
using namespace winrt::Microsoft::Terminal::Control;
using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::TerminalApp::implementation
{
    ControlInteractivity ContentManager::CreateCore(const Microsoft::Terminal::Control::IControlSettings& settings,
                                                    const IControlAppearance& unfocusedAppearance,
                                                    const TerminalConnection::ITerminalConnection& connection)
    {
        ControlInteractivity content{ settings, unfocusedAppearance, connection };
        content.Closed({ get_weak(), &ContentManager::_closedHandler });

        _content.emplace(content.Id(), content);

        return content;
    }

    ControlInteractivity ContentManager::TryLookupCore(uint64_t id)
    {
        const auto it = _content.find(id);
        return it != _content.end() ? it->second : ControlInteractivity{ nullptr };
    }

    void ContentManager::_closedHandler(winrt::Windows::Foundation::IInspectable sender,
                                        winrt::Windows::Foundation::IInspectable e)
    {
        if (const auto& content{ sender.try_as<winrt::Microsoft::Terminal::Control::ControlInteractivity>() })
        {
            const auto& contentId{ content.Id() };
            _content.erase(contentId);
        }
    }
}