ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/TabBase.cpp (C++)

Code before:
        _updateIsClosable();
    }

    void TabBase::_updateIsClosable()
    {
        if (ReadOnly())
        {
            TabViewItem().IsClosable(false);
        }
        else
        {
            switch (_closeButtonVisibility)
            {
            case TabCloseButtonVisibility::Never:
                TabViewItem().IsClosable(false);
                break;
            case TabCloseButtonVisibility::Hover:
                TabViewItem().IsClosable(true);
                break;
            case TabCloseButtonVisibility::ActiveOnly:
            {
                TabViewItem().IsClosable(_focused());
                break;
            }
            default:
                TabViewItem().IsClosable(true);
                break;
            }
        }
    }

    bool TabBase::_focused() const noexcept
    {
        return _focusState != FocusState::Unfocused;
    }

}


Review comment (suggestion):
We will never leave this function without calling `TabViewItem().IsClosable()` at least once. Can you factor out the common subexpression? No point in emitting 10 function calls when 2 will do :smile:

Code after:
    // - This is called every time the active tab changes. That way, the changes
    //   in focused tab can be reflected for the "ActiveOnly" state.
    void TabBase::CloseButtonVisibility(TabCloseButtonVisibility visibility)
    {
        _closeButtonVisibility = visibility;
        _updateIsClosable();
    }

    // Method Description:
    // - Update our close button's visibility, to reflect both the ReadOnly
    //   state of the tab content, and also if if we were told to have a visible
    //   close button at all.
    //   - the tab being read-only takes precedence. That will always suppress
    //     the close button.
    //   - Otherwise we'll use the state set in CloseButtonVisibility to control
    //     the tab's visibility.
    void TabBase::_updateIsClosable()
    {
        bool isClosable = true;

        if (ReadOnly())
        {
            isClosable = false;
        }
        else
        {
            switch (_closeButtonVisibility)
            {
            case TabCloseButtonVisibility::Never:
                isClosable = false;
                break;
            case TabCloseButtonVisibility::Hover:
                isClosable = true;
                break;
            case TabCloseButtonVisibility::ActiveOnly:
                isClosable = _focused();
                break;
            default:
                isClosable = true;
                break;
            }
        }
        TabViewItem().IsClosable(isClosable);
    }

    bool TabBase::_focused() const noexcept
    {
        return _focusState != FocusState::Unfocused;
    }

}
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/AppActionHandlers.cpp (C++)

Code before:
        args.Handled(true);
    }

    void TerminalPage::_HandleShowContextMenu(const IInspectable& /*sender*/,
                                              const ActionEventArgs& args)
    {
        if (const auto& control{ _GetActiveControl() })
        {
            control.ShowContextMenu();
        }
        args.Handled(true);
    }

    void TerminalPage::_HandleOpenScratchpad(const IInspectable& /*sender*/,
                                             const ActionEventArgs& args)
    {
        if (Feature_ScratchpadPane::IsEnabled())
        {
            auto scratchPane{ winrt::make_self<ScratchpadContent>() };

            // This is maybe a little wacky - add our key event handler to the pane
            // we made. So that we can get actions for keys that the content didn't
            // handle.
            scratchPane->GetRoot().KeyDown({ this, &TerminalPage::_KeyDownHandler });

            auto resultPane = std::make_shared<Pane>(*scratchPane);
            _SplitPane(_GetFocusedTabImpl(), SplitDirection::Automatic, 0.5f, resultPane);
            args.Handled(true);
        }
    }

    void TerminalPage::_HandleOpenAbout(const IInspectable& /*sender*/,
                                        const ActionEventArgs& args)
    {
        _ShowAboutDialog();
        args.Handled(true);
    }
}


Review comment (suggestion):
```suggestion
            const auto resultPane = std::make_shared<Pane>(*scratchPane);
```

Code after:
    void TerminalPage::_HandleShowContextMenu(const IInspectable& /*sender*/,
                                              const ActionEventArgs& args)
    {
        if (const auto& control{ _GetActiveControl() })
        {
            control.ShowContextMenu();
        }
        args.Handled(true);
    }

    void TerminalPage::_HandleOpenScratchpad(const IInspectable& /*sender*/,
                                             const ActionEventArgs& args)
    {
        if (Feature_ScratchpadPane::IsEnabled())
        {
            const auto& scratchPane{ winrt::make_self<ScratchpadContent>() };

            // This is maybe a little wacky - add our key event handler to the pane
            // we made. So that we can get actions for keys that the content didn't
            // handle.
            scratchPane->GetRoot().KeyDown({ this, &TerminalPage::_KeyDownHandler });

            const auto resultPane = std::make_shared<Pane>(*scratchPane);
            _SplitPane(_GetFocusedTabImpl(), SplitDirection::Automatic, 0.5f, resultPane);
            args.Handled(true);
        }
    }

    void TerminalPage::_HandleOpenAbout(const IInspectable& /*sender*/,
                                        const ActionEventArgs& args)
    {
        _ShowAboutDialog();
        args.Handled(true);
    }
}
Repository: microsoft/terminal
File: src/buffer/out/search.cpp (C++)

Code before:

    if (const auto s = GetCurrent())
    {
        // Convert buffer selection offsets into the equivalent screen coordinates
        // required by SelectNewRegion, taking line renditions into account.
        const auto& textBuffer = _renderData->GetTextBuffer();
        const auto selStart = textBuffer.BufferToScreenPosition(s->start);
        const auto selEnd = textBuffer.BufferToScreenPosition(s->end);

        _renderData->SelectNewRegion(selStart, selEnd);

        for (const auto& r : _results)
        {
            const auto rbStart = textBuffer.BufferToScreenPosition(r.start);
            const auto rbEnd = textBuffer.BufferToScreenPosition(r.end);

            til::inclusive_rect re;
            re.top = rbStart.y;
            re.bottom = rbEnd.y;
            re.left = rbStart.x;
            re.right = rbEnd.x;

            toSelect.emplace_back(re);
        }

        _renderData->SelectSearchRegions(toSelect);

        return true;
    }

    return false;
}

const std::vector<til::point_span>& Search::Results() const noexcept
{
    return _results;
}

ptrdiff_t Search::CurrentMatch() const noexcept
{
    return _index;
}


Review comment (bug):
This is the wrong place to put it, because conhost (aka OpenConsole) doesn't use this information. Additionally, this method runs every time you click "next" or "previous" so this would be a waste of compute since the `_results` array doesn't change.

I would put this here: https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalControl/ControlCore.cpp#L1605C9-L1609

FYI never forget to `std::move` container classes whenever you pass them by value. Otherwise you make a copy just to throw away the original soon after.

Code after:
    {
        const auto rbStart = textBuffer.BufferToScreenPosition(r.start);
        const auto rbEnd = textBuffer.BufferToScreenPosition(r.end);

        til::inclusive_rect re;
        re.top = rbStart.y;
        re.bottom = rbEnd.y;
        re.left = rbStart.x;
        re.right = rbEnd.x;

        toSelect.emplace_back(re);
    }

    _renderData->SelectSearchRegions(std::move(toSelect));
}

// Routine Description:
// - Takes the found word and selects it in the screen buffer

bool Search::SelectCurrent() const
{
    if (const auto s = GetCurrent())
    {
        // Convert buffer selection offsets into the equivalent screen coordinates
        // required by SelectNewRegion, taking line renditions into account.
        const auto& textBuffer = _renderData->GetTextBuffer();
        const auto selStart = textBuffer.BufferToScreenPosition(s->start);
        const auto selEnd = textBuffer.BufferToScreenPosition(s->end);
        _renderData->SelectNewRegion(selStart, selEnd);
        return true;
    }

    _renderData->ClearSelection();
    return false;
}

const std::vector<til::point_span>& Search::Results() const noexcept
{
    return _results;
}

ptrdiff_t Search::CurrentMatch() const noexcept
{
    return _index;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/ColorHelper.h (C/C++)

Code before:
#pragma once
#include "pch.h"

#include <winrt/windows.ui.core.h>

namespace winrt::TerminalApp
{
    class HSL
    {
    public:
        float H;
        float S;
        float L;
    };

    class ColorHelper
    {
    public:
        static bool IsBrightColor(const Windows::UI::Color& color);
        static HSL RgbToHsl(const Windows::UI::Color& color);
        static Windows::UI::Color HslToRgb(const HSL& color);
        static Windows::UI::Color Lighten(const Windows::UI::Color& color, float amount = 10.f);
        static Windows::UI::Color Darken(const Windows::UI::Color& color, float amount = 10.f);
        static Windows::UI::Color GetAccentColor(const Windows::UI::Color& color);
        static float GetLuminance(const Windows::UI::Color& color);
        static float GetReadability(const Windows::UI::Color& first, const Windows::UI::Color& second);
        static float GetReadability(const HSL& first, const HSL& second);


Review comment (suggestion):
jeepers I don't love this, presumably this should be in the pch for both this project and the tests, but also those tests really haven't had love in years. So, this is fine for now. (I can only hope that it _also_ being in the pch.h for TerminalAppLib means it won't _actually_ get re-parsed here)

Code after:
#pragma once

#include <winrt/Windows.UI.h>

namespace winrt::TerminalApp
{
    class HSL
    {
    public:
        float H;
        float S;
        float L;
    };

    class ColorHelper
    {
    public:
        static bool IsBrightColor(const Windows::UI::Color& color);
        static HSL RgbToHsl(const Windows::UI::Color& color);
        static Windows::UI::Color HslToRgb(const HSL& color);
        static Windows::UI::Color Lighten(const Windows::UI::Color& color, float amount = 10.f);
        static Windows::UI::Color Darken(const Windows::UI::Color& color, float amount = 10.f);
        static Windows::UI::Color GetAccentColor(const Windows::UI::Color& color);
        static float GetLuminance(const Windows::UI::Color& color);
        static float GetReadability(const Windows::UI::Color& first, const Windows::UI::Color& second);
        static float GetReadability(const HSL& first, const HSL& second);

    private:
Repository: microsoft/terminal
File: src/cascadia/TerminalControl/TermControl.cpp (C++)

Code before:

        if (!self)
        {
            co_return;
        }

        const auto core = winrt::get_self<ControlCore>(_core);
        if (!core)
        {
            co_return;
        }

        try
        {
            core->RestoreFromPath(path.c_str());
        }
        CATCH_LOG();

        try
        {
            if (const auto connection = core->Connection())
            {
                connection.Start();
            }
        }
        CATCH_LOG();
    }

    void TermControl::_CharacterHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
                                        const Input::CharacterReceivedRoutedEventArgs& e)
    {
        if (_IsClosing())
        {
            return;
        }

        HidePointerCursor.raise(*this, nullptr);

        const auto ch = e.Character();
        const auto keyStatus = e.KeyStatus();
        const auto scanCode = gsl::narrow_cast<WORD>(keyStatus.ScanCode);
        auto modifiers = _GetPressedModifierKeys();

        if (keyStatus.IsExtendedKey)
        {
            modifiers |= ControlKeyStates::EnhancedKey;
        }

        // Broadcast the character to all listeners
        // only broadcast if there's an actual listener. Saves the overhead of some object creation.
        if (CharSent)

Review comment (suggestion):
1% worried about starting the connection on the BG thread instead of the window thread, but I don't actually see anything in `ConptyConnection::Start` that looks like it needs the main thread. 

Code after:
        {
            co_await winrt::resume_background();

            const auto self = weakSelf.get();
            if (!self)
            {
                co_return;
            }

            winrt::get_self<ControlCore>(_core)->RestoreFromPath(path.c_str());
        }
        CATCH_LOG();

        try
        {
            co_await uiThread;

            const auto self = weakSelf.get();
            if (!self)
            {
                co_return;
            }

            if (const auto connection = _core.Connection())
            {
                connection.Start();
            }
        }
        CATCH_LOG();
    }

    void TermControl::_CharacterHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
                                        const Input::CharacterReceivedRoutedEventArgs& e)
    {
        if (_IsClosing())
        {
            return;
        }

        HidePointerCursor.raise(*this, nullptr);

        const auto ch = e.Character();
        const auto keyStatus = e.KeyStatus();
        const auto scanCode = gsl::narrow_cast<WORD>(keyStatus.ScanCode);
        auto modifiers = _GetPressedModifierKeys();

        if (keyStatus.IsExtendedKey)
        {
            modifiers |= ControlKeyStates::EnhancedKey;
        }
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/ActionEntry.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "ActionEntry.h"
#include "JsonUtils.h"

#include "ActionEntry.g.cpp"

using namespace Microsoft::Terminal::Settings::Model;
using namespace winrt::Microsoft::Terminal::Settings::Model::implementation;

static constexpr std::string_view ActionIdKey{ "actionId" };

ActionEntry::ActionEntry() noexcept :
    ActionEntryT<ActionEntry, NewTabMenuEntry>(NewTabMenuEntryType::Action)
{
}

Json::Value ActionEntry::ToJson() const
{
    auto json = NewTabMenuEntry::ToJson();

    JsonUtils::SetValueForKey(json, ActionIdKey, _ActionId);

    return json;
}

winrt::com_ptr<NewTabMenuEntry> ActionEntry::FromJson(const Json::Value& json)
{
    auto entry = winrt::make_self<ActionEntry>();

    JsonUtils::GetValueForKey(json, ActionIdKey, entry->_ActionId);

    return entry;
}


Review comment (suggestion):
qq: why should we stutter?

```
{ "type": "action", "actionId": "Terminal.Quit" }
           ^^^^^^    ^^^^^^
                \---/
                  `they are the same word
```

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "ActionEntry.h"
#include "JsonUtils.h"

#include "ActionEntry.g.cpp"

using namespace Microsoft::Terminal::Settings::Model;
using namespace winrt::Microsoft::Terminal::Settings::Model::implementation;

static constexpr std::string_view ActionIdKey{ "id" };

ActionEntry::ActionEntry() noexcept :
    ActionEntryT<ActionEntry, NewTabMenuEntry>(NewTabMenuEntryType::Action)
{
}

Json::Value ActionEntry::ToJson() const
{
    auto json = NewTabMenuEntry::ToJson();

    JsonUtils::SetValueForKey(json, ActionIdKey, _ActionId);

    return json;
}

winrt::com_ptr<NewTabMenuEntry> ActionEntry::FromJson(const Json::Value& json)
{
    auto entry = winrt::make_self<ActionEntry>();

    JsonUtils::GetValueForKey(json, ActionIdKey, entry->_ActionId);

    return entry;
}
Repository: microsoft/terminal
File: src/buffer/out/search.cpp (C++)

Code before:
        _renderData->SelectNewRegion(selStart, selEnd);
        return true;
    }

    _renderData->ClearSelection();
    return false;
}

const std::vector<til::point_span>& Search::Results() const noexcept
{
    return _results;
}

std::vector<til::point_span>&& Search::ExtractResults() noexcept
{
    return std::move(_results);
}

ptrdiff_t Search::CurrentMatch() const noexcept
{
    return _index;
}

bool Search::IsOk() const noexcept
{
    return _ok;
}


Review comment (suggestion):
```suggestion
bool Search::IsSearchInvalid() const noexcept
```
maybe? Just something a bit more specific would be nice imo, because `search->IsOk()` is kinda ambiguous out-of-context.

Code after:
        const auto selStart = textBuffer.BufferToScreenPosition(s->start);
        const auto selEnd = textBuffer.BufferToScreenPosition(s->end);
        _renderData->SelectNewRegion(selStart, selEnd);
        return true;
    }

    _renderData->ClearSelection();
    return false;
}

const std::vector<til::point_span>& Search::Results() const noexcept
{
    return _results;
}

std::vector<til::point_span>&& Search::ExtractResults() noexcept
{
    return std::move(_results);
}

ptrdiff_t Search::CurrentMatch() const noexcept
{
    return _index;
}

bool Search::IsOk() const noexcept
{
    return _ok;
}
Repository: microsoft/terminal
File: src/cascadia/TerminalApp/AppActionHandlers.cpp (C++)

Code before:
            // This is maybe a little wacky - add our key event handler to the pane
            // we made. So that we can get actions for keys that the content didn't
            // handle.
            scratchPane->GetRoot().KeyDown({ this, &TerminalPage::_KeyDownHandler });

            const auto resultPane = std::make_shared<Pane>(*scratchPane);
            _SplitPane(_senderOrFocusedTab(sender), SplitDirection::Automatic, 0.5f, resultPane);
            args.Handled(true);
        }
    }

    void TerminalPage::_HandleOpenAbout(const IInspectable& /*sender*/,
                                        const ActionEventArgs& args)
    {
        _ShowAboutDialog();
        args.Handled(true);
    }

    void TerminalPage::_HandleQuickFix(const IInspectable& /*sender*/,
                                       const ActionEventArgs& args)
    {
        if (const auto& control{ _GetActiveControl() })
        {
            control.OpenQuickFixMenu();
        }
        args.Handled(true);
    }
}


Review comment (suggestion):
shouldn't we only mark it as handled if there was a menu to open...?

that way, the shortcut can pass through if there isn't a menu.

Code after:
            // This is maybe a little wacky - add our key event handler to the pane
            // we made. So that we can get actions for keys that the content didn't
            // handle.
            scratchPane->GetRoot().KeyDown({ this, &TerminalPage::_KeyDownHandler });

            const auto resultPane = std::make_shared<Pane>(*scratchPane);
            _SplitPane(_senderOrFocusedTab(sender), SplitDirection::Automatic, 0.5f, resultPane);
            args.Handled(true);
        }
    }

    void TerminalPage::_HandleOpenAbout(const IInspectable& /*sender*/,
                                        const ActionEventArgs& args)
    {
        _ShowAboutDialog();
        args.Handled(true);
    }

    void TerminalPage::_HandleQuickFix(const IInspectable& /*sender*/,
                                       const ActionEventArgs& args)
    {
        if (const auto& control{ _GetActiveControl() })
        {
            const auto handled = control.OpenQuickFixMenu();
            args.Handled(handled);
        }
    }
}
Repository: microsoft/terminal
File: src/host/settings.hpp (C++)

Code before:
    bool _fTrimLeadingZeros;
    bool _fEnableColorSelection;
    bool _bLineSelection;
    bool _bWrapText; // whether to use text wrapping when resizing the window
    bool _fCtrlKeyShortcutsDisabled; // disables Ctrl+<something> key intercepts
    BYTE _bWindowAlpha; // describes the opacity of the window

    bool _fFilterOnPaste; // should we filter text when the user pastes? (e.g. remove <tab>)
    std::wstring _LaunchFaceName;
    bool _fAllowAltF4Close;
    DWORD _dwVirtTermLevel;
    SettingsTextMeasurementMode _textMeasurement = SettingsTextMeasurementMode::Graphemes;
    bool _fUseDx;
    bool _fCopyColor;
    bool _fEnableBuiltinGlyphs = true;

    // this is used for the special STARTF_USESIZE mode.
    bool _fUseWindowSizePixels;
    COORD _dwWindowSizePixels;

    CursorType _CursorType;

    bool _fInterceptCopyPaste;

    bool _TerminalScrolling;
    WCHAR _answerbackMessage[32];
    friend class RegistrySerialization;
};


Review comment (suggestion):
I believe you need to initialize the array, as it's only set/initialized when the registry value exists.

Is 32 chars sufficient for most use cases? You don't need to be conservative about memory here.

Code after:
    bool _fTrimLeadingZeros;
    bool _fEnableColorSelection;
    bool _bLineSelection;
    bool _bWrapText; // whether to use text wrapping when resizing the window
    bool _fCtrlKeyShortcutsDisabled; // disables Ctrl+<something> key intercepts
    BYTE _bWindowAlpha; // describes the opacity of the window

    bool _fFilterOnPaste; // should we filter text when the user pastes? (e.g. remove <tab>)
    std::wstring _LaunchFaceName;
    bool _fAllowAltF4Close;
    DWORD _dwVirtTermLevel;
    SettingsTextMeasurementMode _textMeasurement = SettingsTextMeasurementMode::Graphemes;
    bool _fUseDx;
    bool _fCopyColor;
    bool _fEnableBuiltinGlyphs = true;

    // this is used for the special STARTF_USESIZE mode.
    bool _fUseWindowSizePixels;
    COORD _dwWindowSizePixels;

    CursorType _CursorType;

    bool _fInterceptCopyPaste;

    bool _TerminalScrolling;
    WCHAR _answerbackMessage[32] = {};
    friend class RegistrySerialization;
};