ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/terminal
File: src/terminal/adapter/DispatchTypes.hpp (C++)

Code before:
        ASCII = VTID("B")
    };

    enum CodingSystem : uint64_t
    {
        ISO2022 = VTID("@"),
        UTF8 = VTID("G")
    };

    enum TabClearType : unsigned short
    {
        ClearCurrentColumn = 0,
        ClearAllColumns = 3
    };

    enum WindowManipulationType : unsigned int
    {
        Invalid = 0,
        RefreshWindow = 7,
        ResizeWindowInCharacters = 8,
    };

    enum class CursorStyle : unsigned int
    {
        BlinkingBlock = 0, // Implemented as "restore cursor to user default".
        BlinkingBlockDefault = 1,
        SteadyBlock = 2,
        BlinkingUnderline = 3,
        SteadyUnderline = 4,
        BlinkingBar = 5,
        SteadyBar = 6
    };

    enum class LineFeedType : unsigned int
    {
        WithReturn,
        WithoutReturn,
        DependsOnMode
    };

    constexpr short s_sDECCOLMSetColumns = 132;
    constexpr short s_sDECCOLMResetColumns = 80;

}


Review comment (suggestion):
Instead of adding a comment here, couldn't we just rename the enum value to something like `UserDefault`, and then also rename `BlinkingBlockDefault` to `BlinkingBlock`. That seems like it would be a better indication of what these values actually represent.

Code after:
        ASCII = VTID("B")
    };

    enum CodingSystem : uint64_t
    {
        ISO2022 = VTID("@"),
        UTF8 = VTID("G")
    };

    enum TabClearType : unsigned short
    {
        ClearCurrentColumn = 0,
        ClearAllColumns = 3
    };

    enum WindowManipulationType : unsigned int
    {
        Invalid = 0,
        RefreshWindow = 7,
        ResizeWindowInCharacters = 8,
    };

    enum class CursorStyle : unsigned int
    {
        UserDefault = 0, // Implemented as "restore cursor to user default".
        BlinkingBlock = 1,
        SteadyBlock = 2,
        BlinkingUnderline = 3,
        SteadyUnderline = 4,
        BlinkingBar = 5,
        SteadyBar = 6
    };

    enum class LineFeedType : unsigned int
    {
        WithReturn,
        WithoutReturn,
        DependsOnMode
    };

    constexpr short s_sDECCOLMSetColumns = 132;
    constexpr short s_sDECCOLMResetColumns = 80;

}
Repository: microsoft/terminal
File: src/cascadia/TerminalControl/TermControl.cpp (C++)

Code before:
            uiButton = WM_RBUTTONUP;
            break;
        default:
            uiButton = WM_MOUSEMOVE;
        }

        return uiButton;
    }

    void TermControl::_coreWarningBell(const IInspectable& /*sender*/, const IInspectable& /*args*/)
    {
        _playWarningBell->Run();
    }

    Windows::UI::Xaml::DependencyProperty VisualBellLight::m_isTargetProperty =
        Windows::UI::Xaml::DependencyProperty::RegisterAttached(
            L"IsTarget",
            winrt::xaml_typename<bool>(),
            winrt::xaml_typename<winrt::Microsoft::Terminal::Control::VisualBellLight>(),
            Windows::UI::Xaml::PropertyMetadata{ winrt::box_value(false), Windows::UI::Xaml::PropertyChangedCallback{ &VisualBellLight::OnIsTargetChanged } });

    void VisualBellLight::OnConnected(Windows::UI::Xaml::UIElement const& /* newElement */)
    {
        if (!CompositionLight())
        {
            // OnConnected is called when the first target UIElement is shown on the screen. This enables delaying composition object creation until it's actually necessary.
            auto spotLight2{ Windows::UI::Xaml::Window::Current().Compositor().CreateAmbientLight() };
            spotLight2.Color(Windows::UI::Colors::WhiteSmoke());
            spotLight2.Intensity(static_cast<float>(1.5));
            CompositionLight(spotLight2);
        }
    }

    void VisualBellLight::OnDisconnected(Windows::UI::Xaml::UIElement const& /* oldElement */)
    {
        // OnDisconnected is called when there are no more target UIElements on the screen.
        // Dispose of composition resources when no longer in use.
        if (CompositionLight())
        {
            CompositionLight(nullptr);
        }
    }

    winrt::hstring VisualBellLight::GetId()
    {
        return VisualBellLight::GetIdStatic();
    }

    void VisualBellLight::OnIsTargetChanged(Windows::UI::Xaml::DependencyObject const& d, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const& e)
    {
        auto uielem{ d.try_as<Windows::UI::Xaml::UIElement>() };

Review comment (suggestion):
nit: you could make this a `// Method Description` comment instead

Code after:
            uiButton = WM_LBUTTONUP;
            break;
        case winrt::Windows::UI::Input::PointerUpdateKind::MiddleButtonPressed:
            uiButton = WM_MBUTTONDOWN;
            break;
        case winrt::Windows::UI::Input::PointerUpdateKind::MiddleButtonReleased:
            uiButton = WM_MBUTTONUP;
            break;
        case winrt::Windows::UI::Input::PointerUpdateKind::RightButtonPressed:
            uiButton = WM_RBUTTONDOWN;
            break;
        case winrt::Windows::UI::Input::PointerUpdateKind::RightButtonReleased:
            uiButton = WM_RBUTTONUP;
            break;
        default:
            uiButton = WM_MOUSEMOVE;
        }

        return uiButton;
    }

    void TermControl::_coreWarningBell(const IInspectable& /*sender*/, const IInspectable& /*args*/)
    {
        _playWarningBell->Run();
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/AddProfile.h (C/C++)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "AddProfile.g.h"
#include "AddProfilePageNavigationState.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    struct AddProfilePageNavigationState : AddProfilePageNavigationStateT<AddProfilePageNavigationState>
    {
    public:
        AddProfilePageNavigationState(const Model::CascadiaSettings& settings) :
            _Settings{ settings } {}

        void RequestAddNew()
        {
            _AddNewHandlers(nullptr, nullptr);
        }

        void RequestDuplicate(GUID profile)
        {
            _AddNewHandlers(nullptr, winrt::box_value(profile));
        }


Review comment (refactor):
nit: add an abstract and plop your name on it.

I know some of the other pages don't have it. They should. The reason they don't is because we were rushing to get this thing out there haha.

Code after:
﻿/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- AddProfile.h

Abstract:
- This creates the 'add new profile' page in the settings UI and handles
  user interaction with it, raising events to the main page as necessary


Author(s):
- Pankaj Bhojwani - March 2021

--*/

#pragma once

#include "AddProfile.g.h"
#include "AddProfilePageNavigationState.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    struct AddProfilePageNavigationState : AddProfilePageNavigationStateT<AddProfilePageNavigationState>
    {
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/AddProfile.h (C/C++)

Code before:
            _AddNewHandlers(nullptr, nullptr);
        }

        void RequestDuplicate(GUID profile)
        {
            _AddNewHandlers(nullptr, winrt::box_value(profile));
        }

        GETSET_PROPERTY(Model::CascadiaSettings, Settings, nullptr)
        TYPED_EVENT(AddNew, Windows::Foundation::IInspectable, Windows::Foundation::IInspectable);
    };

    struct AddProfile : AddProfileT<AddProfile>
    {
    public:
        AddProfile();

        void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);

        GETSET_PROPERTY(Editor::AddProfilePageNavigationState, State, nullptr);

    private:
        friend struct AddProfileT<AddProfile>; // for Xaml to bind events

        void _AddNewClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
        void _DuplicateClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
    };
}

namespace winrt::Microsoft::Terminal::Settings::Editor::factory_implementation
{
    BASIC_FACTORY(AddProfile);
}


Review comment (refactor):
nit: you could also just make `AddNewClick` and `DuplicateClick` public. Then you don't need to make the friend struct.

Code after:
#include "AddProfile.g.h"
#include "AddProfilePageNavigationState.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    struct AddProfilePageNavigationState : AddProfilePageNavigationStateT<AddProfilePageNavigationState>
    {
    public:
        AddProfilePageNavigationState(const Model::CascadiaSettings& settings) :
            _Settings{ settings } {}

        void RequestAddNew()
        {
            _AddNewHandlers(winrt::guid{});
        }

        void RequestDuplicate(GUID profile)
        {
            _AddNewHandlers(profile);
        }

        WINRT_PROPERTY(Model::CascadiaSettings, Settings, nullptr)
        WINRT_CALLBACK(AddNew, AddNewArgs);
    };

    struct AddProfile : AddProfileT<AddProfile>
    {
    public:
        AddProfile();

        void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);

        void AddNewClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
        void DuplicateClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);

        WINRT_PROPERTY(Editor::AddProfilePageNavigationState, State, nullptr);
    };
}

namespace winrt::Microsoft::Terminal::Settings::Editor::factory_implementation
{
    BASIC_FACTORY(AddProfile);
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/AddProfile.h (C/C++)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "AddProfile.g.h"
#include "AddProfilePageNavigationState.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    struct AddProfilePageNavigationState : AddProfilePageNavigationStateT<AddProfilePageNavigationState>
    {
    public:
        AddProfilePageNavigationState(const Model::CascadiaSettings& settings) :
            _Settings{ settings } {}

        void RequestAddNew()
        {
            _AddNewHandlers(GUID{});
        }

        void RequestDuplicate(GUID profile)
        {
            _AddNewHandlers(profile);
        }

        WINRT_PROPERTY(Model::CascadiaSettings, Settings, nullptr)
        WINRT_CALLBACK(AddNew, AddNewArgs);
    };

    struct AddProfile : AddProfileT<AddProfile>
    {
    public:
        AddProfile();

        void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);

        void AddNewClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
        void DuplicateClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);

        WINRT_PROPERTY(Editor::AddProfilePageNavigationState, State, nullptr);
    };
}


Review comment (suggestion):
```suggestion
            _AddNewHandlers(winrt::guid{});
```
nit: we should be using `guid` instead of `GUID` in general since we're working with WinRT stuff.

Code after:
﻿/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.

Module Name:
- AddProfile.h

Abstract:
- This creates the 'add new profile' page in the settings UI and handles
  user interaction with it, raising events to the main page as necessary


Author(s):
- Pankaj Bhojwani - March 2021

--*/

#pragma once

#include "AddProfile.g.h"
#include "AddProfilePageNavigationState.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    struct AddProfilePageNavigationState : AddProfilePageNavigationStateT<AddProfilePageNavigationState>
    {
    public:
        AddProfilePageNavigationState(const Model::CascadiaSettings& settings) :
            _Settings{ settings } {}

        void RequestAddNew()
        {
            _AddNewHandlers(winrt::guid{});
        }

        void RequestDuplicate(GUID profile)
        {
            _AddNewHandlers(profile);
        }

        WINRT_PROPERTY(Model::CascadiaSettings, Settings, nullptr)
        WINRT_CALLBACK(AddNew, AddNewArgs);
    };
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsModel/ActionArgs.cpp (C++)

Code before:
        {
            return RS_(L"NewWindowCommandKey");
        }
        return winrt::hstring{
            fmt::format(L"{}, {}", RS_(L"NewWindowCommandKey"), newTerminalArgsStr)
        };
    }

    winrt::hstring SwitchToAdjacentTabArgs::GenerateName() const
    {
        if (_SwitcherMode && _SwitcherMode.Value() == TabSwitcherMode::MostRecentlyUsed)
        {
            return L"most recently used";
        }
        return L"in order";
    }

    winrt::hstring PrevTabArgs::GenerateName() const
    {
        const auto switchToAdjacentTabStr = SwitchToAdjacentTabArgs::GenerateName();
        if (switchToAdjacentTabStr.empty())
        {
            return RS_(L"PrevTabCommandKey");
        }

        return winrt::hstring(fmt::format(L"{}, {}", RS_(L"PrevTabCommandKey"), switchToAdjacentTabStr));
    }

    winrt::hstring NextTabArgs::GenerateName() const
    {
        const auto switchToAdjacentTabStr = SwitchToAdjacentTabArgs::GenerateName();
        if (switchToAdjacentTabStr.empty())
        {
            return RS_(L"NextTabCommandKey");
        }

        return winrt::hstring(fmt::format(L"{}, {}", RS_(L"NextTabCommandKey"), switchToAdjacentTabStr));
    }
}


Review comment (suggestion):
So these end up being evaluated as 
* "Next/Previous Tab, most recently used" 
* "Next/Previous Tab, in order"

I think that's probably fine. The "most recently used" and "in order" are kinda like the profile, commandline, etc args for NewTerminalArgs, and we don't localize those 

Code after:
            return RS_(L"NewWindowCommandKey");
        }
        return winrt::hstring{
            fmt::format(L"{}, {}", RS_(L"NewWindowCommandKey"), newTerminalArgsStr)
        };
    }

    winrt::hstring PrevTabArgs::GenerateName() const
    {
        if (!_SwitcherMode)
        {
            return RS_(L"PrevTabCommandKey");
        }

        const auto mode = _SwitcherMode.Value() == TabSwitcherMode::MostRecentlyUsed ? L"most recently used" : L"in order";
        return winrt::hstring(fmt::format(L"{}, {}", RS_(L"PrevTabCommandKey"), mode));
    }

    winrt::hstring NextTabArgs::GenerateName() const
    {
        if (!_SwitcherMode)
        {
            return RS_(L"NextTabCommandKey");
        }

        const auto mode = _SwitcherMode.Value() == TabSwitcherMode::MostRecentlyUsed ? L"most recently used" : L"in order";
        return winrt::hstring(fmt::format(L"{}, {}", RS_(L"NextTabCommandKey"), mode));
    }
}
Repository: microsoft/terminal
File: src/cascadia/TerminalSettingsEditor/PreviewConnection.cpp (C++)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "PreviewConnection.h"
#include <LibraryResources.h>

using namespace ::winrt::Microsoft::Terminal::TerminalConnection;
using namespace ::winrt::Windows::Foundation;

static constexpr std::wstring_view PreviewText{ L"Windows Terminal\r\nCopyright (c) Microsoft Corporation.\r\n\nC:\\Windows\\Terminal> " };

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    PreviewConnection::PreviewConnection() noexcept
    {
    }

    void PreviewConnection::Start() noexcept
    {
        // First send a sequence to disable cursor blinking
        _TerminalOutputHandlers(L"\x1b[?12l");
        // Send the preview text
        _TerminalOutputHandlers(PreviewText);
    }

    void PreviewConnection::WriteInput(hstring const& /*data*/)
    {
    }

    void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept
    {
    }

    void PreviewConnection::Close() noexcept
    {

Review comment (suggestion):
```suggestion
static constexpr std::wstring_view PreviewText{ L"Windows Terminal\r\nCopyright (c) Microsoft Corporation\r\n\nC:\\Windows\\Terminal> " };
```

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "PreviewConnection.h"
#include <LibraryResources.h>

using namespace ::winrt::Microsoft::Terminal::TerminalConnection;
using namespace ::winrt::Windows::Foundation;

static constexpr std::wstring_view PreviewText{ L"Windows Terminal\r\nCopyright (c) Microsoft Corporation\r\n\nC:\\Windows\\Terminal> " };

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
    PreviewConnection::PreviewConnection() noexcept
    {
    }

    void PreviewConnection::Start() noexcept
    {
        // First send a sequence to disable cursor blinking
        _TerminalOutputHandlers(L"\x1b[?12l");
        // Send the preview text
        _TerminalOutputHandlers(PreviewText);
    }

    void PreviewConnection::WriteInput(hstring const& /*data*/)
    {
    }

    void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept
    {
    }

    void PreviewConnection::Close() noexcept
    {
Repository: microsoft/terminal
File: src/cascadia/TerminalCore/TerminalDispatch.cpp (C++)

Code before:
    // of what needs to be done.

    bool success = true;

    // // If in the alt buffer, switch back to main before doing anything else.
    // if (_usingAltBuffer)
    // {
    //     success = _pConApi->PrivateUseMainScreenBuffer();
    //     _usingAltBuffer = !success;
    // }

    // Sets the SGR state to normal - this must be done before EraseInDisplay
    //      to ensure that it clears with the default background color.
    success = SoftReset() && success;

    // Clears the screen - Needs to be done in two operations.
    success = EraseInDisplay(DispatchTypes::EraseType::All) && success;
    success = EraseInDisplay(DispatchTypes::EraseType::Scrollback) && success;

    // Set the DECSCNM screen mode back to normal.
    success = SetScreenMode(false) && success;

    // Cursor to 1,1 - the Soft Reset guarantees this is absolute
    success = CursorPosition(1, 1) && success;

    // // Delete all current tab stops and reapply
    _ResetTabStops();

    return success;
}


Review comment (suggestion):
```suggestion
    // Delete all current tab stops and reapply
```

Code after:
    // of what needs to be done.

    bool success = true;

    // // If in the alt buffer, switch back to main before doing anything else.
    // if (_usingAltBuffer)
    // {
    //     success = _pConApi->PrivateUseMainScreenBuffer();
    //     _usingAltBuffer = !success;
    // }

    // Sets the SGR state to normal - this must be done before EraseInDisplay
    //      to ensure that it clears with the default background color.
    success = SoftReset() && success;

    // Clears the screen - Needs to be done in two operations.
    success = EraseInDisplay(DispatchTypes::EraseType::All) && success;
    success = EraseInDisplay(DispatchTypes::EraseType::Scrollback) && success;

    // Set the DECSCNM screen mode back to normal.
    success = SetScreenMode(false) && success;

    // Cursor to 1,1 - the Soft Reset guarantees this is absolute
    success = CursorPosition(1, 1) && success;

    // Delete all current tab stops and reapply
    _ResetTabStops();

    return success;
}
Repository: microsoft/terminal
File: src/inc/DefaultSettings.h (C/C++)

Code before:
/*++
Copyright (c) Microsoft Corporation.
Licensed under the MIT license.

Module Name:
- DefaultSettings.h

Abstract:
- A header with a bunch of default values used for settings, especially for
        terminal settings used by Cascadia
Author(s):
- Mike Griese - March 2019

--*/
#pragma once

constexpr COLORREF COLOR_WHITE = 0x00ffffff;
constexpr COLORREF COLOR_BLACK = 0x00000000;
constexpr COLORREF OPACITY_OPAQUE = 0xff000000;

constexpr COLORREF DEFAULT_FOREGROUND = COLOR_WHITE;
constexpr COLORREF DEFAULT_FOREGROUND_WITH_ALPHA = OPACITY_OPAQUE | DEFAULT_FOREGROUND;
constexpr COLORREF DEFAULT_BACKGROUND = COLOR_BLACK;
constexpr COLORREF DEFAULT_BACKGROUND_WITH_ALPHA = OPACITY_OPAQUE | DEFAULT_BACKGROUND;

constexpr short DEFAULT_HISTORY_SIZE = 9001;

#pragma warning(push)
#pragma warning(disable : 26426)
// TODO GH 2674, don't disable this warning, move to std::wstring_view or something like that.
const std::wstring DEFAULT_FONT_FACE{ L"Cascadia Mono" };
constexpr int DEFAULT_FONT_SIZE = 12;
constexpr uint16_t DEFAULT_FONT_WEIGHT = 400; // normal

constexpr int DEFAULT_ROWS = 30;
constexpr int DEFAULT_COLS = 120;

const std::wstring DEFAULT_PADDING{ L"8, 8, 8, 8" };
const std::wstring DEFAULT_STARTING_DIRECTORY{ L"%USERPROFILE%" };

constexpr COLORREF DEFAULT_CURSOR_COLOR = COLOR_WHITE;
constexpr COLORREF DEFAULT_CURSOR_HEIGHT = 25;

Review comment (suggestion):
the 3-arg version of the til::color constructor sets alpha to 0xff. Alpha is only used if necessary. When converted to a COLORREF, it is ignored. Since we pushed til::colors all the way through, and retained the "with_alpha" calls inside TControl, it is safe to get rid of the WITH_ALPHA versions.

Code after:
/*++
Copyright (c) Microsoft Corporation.
Licensed under the MIT license.

Module Name:
- DefaultSettings.h

Abstract:
- A header with a bunch of default values used for settings, especially for
        terminal settings used by Cascadia
Author(s):
- Mike Griese - March 2019

--*/
#pragma once

constexpr til::color COLOR_WHITE{ 0xff, 0xff, 0xff };
constexpr til::color COLOR_BLACK{ 0, 0, 0 };
constexpr COLORREF OPACITY_OPAQUE = 0xff000000;

constexpr auto DEFAULT_FOREGROUND = COLOR_WHITE;
constexpr auto DEFAULT_BACKGROUND = COLOR_BLACK;

constexpr short DEFAULT_HISTORY_SIZE = 9001;

#pragma warning(push)
#pragma warning(disable : 26426)
// TODO GH 2674, don't disable this warning, move to std::wstring_view or something like that.
const std::wstring DEFAULT_FONT_FACE{ L"Cascadia Mono" };
constexpr int DEFAULT_FONT_SIZE = 12;
constexpr uint16_t DEFAULT_FONT_WEIGHT = 400; // normal

constexpr int DEFAULT_ROWS = 30;
constexpr int DEFAULT_COLS = 120;

const std::wstring DEFAULT_PADDING{ L"8, 8, 8, 8" };
const std::wstring DEFAULT_STARTING_DIRECTORY{ L"%USERPROFILE%" };

constexpr auto DEFAULT_CURSOR_COLOR = COLOR_WHITE;
constexpr COLORREF DEFAULT_CURSOR_HEIGHT = 25;

const std::wstring DEFAULT_WORD_DELIMITERS{ L" ./\\()\"'-:,.;<>~!@#$%^&*|+=[]{}~?\u2502" };